[PATCH 1/2] raw/cnxk_gpio: support multi-process mode

[PATCH 1/2] raw/cnxk_gpio: support multi-process mode

[Tomasz Duszynski]

GPIO PMD uses a mixture of standard sysfs attributes and custom
ioctl()s to control behaviour of respective GPIOs available in
the system.

This means that each userspace application, either running as a primary
or a secondary, should be able to control a set of distinct GPIOs.

In rare cases where multiple processes need to control the same set of
GPIOs userspace application is responsible for synchronizing accesses.

Signed-off-by: Tomasz Duszynski <tduszynski@marvell.com>
Reviewed-by: Jerin Jacob Kollanukkaran <jerinj@marvell.com>
---
 doc/guides/rawdevs/cnxk_gpio.rst  |   7 ++
 drivers/raw/cnxk_gpio/cnxk_gpio.c | 143 ++++++++++++++++++++----------
 2 files changed, 104 insertions(+), 46 deletions(-)

diff --git a/doc/guides/rawdevs/cnxk_gpio.rst b/doc/guides/rawdevs/cnxk_gpio.rst
index adff535a77..848ad329e7 100644
--- a/doc/guides/rawdevs/cnxk_gpio.rst
+++ b/doc/guides/rawdevs/cnxk_gpio.rst
@@ -21,6 +21,7 @@ Following features are available:
 - set GPIO edge that triggers interrupt
 - set GPIO active low
 - register interrupt handler for specific GPIO
+- multiprocess aware
 
 Requirements
 ------------
@@ -30,6 +31,12 @@ for installing interrupt handlers for low latency signal processing.
 
 Driver is shipped with Marvell SDK.
 
+Limitations
+-----------
+
+In multiprocess mode user-space application must ensure no GPIO sharing across
+processes takes place.
+
 Device Setup
 ------------
 
diff --git a/drivers/raw/cnxk_gpio/cnxk_gpio.c b/drivers/raw/cnxk_gpio/cnxk_gpio.c
index e2907c18b5..dcd646397e 100644
--- a/drivers/raw/cnxk_gpio/cnxk_gpio.c
+++ b/drivers/raw/cnxk_gpio/cnxk_gpio.c
@@ -19,6 +19,12 @@
 
 #define CNXK_GPIO_BUFSZ 128
 #define CNXK_GPIO_CLASS_PATH "/sys/class/gpio"
+#define CNXK_GPIO_PARAMS_MZ_NAME "cnxk_gpio_params_mz"
+
+struct cnxk_gpio_params {
+	char allowlist[CNXK_GPIO_BUFSZ];
+	int num;
+};
 
 static const char *const cnxk_gpio_args[] = {
 #define CNXK_GPIO_ARG_GPIOCHIP "gpiochip"
@@ -28,8 +34,6 @@ static const char *const cnxk_gpio_args[] = {
 	NULL
 };
 
-static char *allowlist;
-
 static void
 cnxk_gpio_format_name(char *name, size_t len)
 {
@@ -44,8 +48,8 @@ cnxk_gpio_filter_gpiochip(const struct dirent *dirent)
 	return !strncmp(dirent->d_name, pattern, strlen(pattern));
 }
 
-static void
-cnxk_gpio_set_defaults(struct cnxk_gpiochip *gpiochip)
+static int
+cnxk_gpio_set_defaults(struct cnxk_gpio_params *params)
 {
 	struct dirent **namelist;
 	int n;
@@ -53,12 +57,14 @@ cnxk_gpio_set_defaults(struct cnxk_gpiochip *gpiochip)
 	n = scandir(CNXK_GPIO_CLASS_PATH, &namelist, cnxk_gpio_filter_gpiochip,
 		    alphasort);
 	if (n < 0 || n == 0)
-		return;
+		return -ENODEV;
 
-	sscanf(namelist[0]->d_name, "gpiochip%d", &gpiochip->num);
+	sscanf(namelist[0]->d_name, "gpiochip%d", &params->num);
 	while (n--)
 		free(namelist[n]);
 	free(namelist);
+
+	return 0;
 }
 
 static int
@@ -78,21 +84,53 @@ cnxk_gpio_parse_arg_gpiochip(const char *key __rte_unused, const char *value,
 }
 
 static int
-cnxk_gpio_parse_arg_allowlist(const char *key __rte_unused, const char *value,
-			      void *extra_args __rte_unused)
+cnxk_gpio_parse_arg_allowlist(const char *key __rte_unused, const char *value, void *extra_args)
 {
-	allowlist = strdup(value);
-	if (!allowlist)
+	char *allowlist = extra_args;
+
+	rte_strlcpy(allowlist, value, sizeof(((struct cnxk_gpio_params *)0)->allowlist));
+
+	return 0;
+}
+
+static int
+cnxk_gpio_params_store(struct cnxk_gpio_params *params)
+{
+	const struct rte_memzone *mz;
+
+	mz = rte_memzone_reserve(CNXK_GPIO_PARAMS_MZ_NAME, sizeof(*params), rte_socket_id(), 0);
+	if (!mz)
 		return -ENOMEM;
 
+	memcpy(mz->addr, params, sizeof(*params));
+
 	return 0;
 }
 
+static void
+cnxk_gpio_params_restore(struct cnxk_gpio_params *params)
+{
+	const struct rte_memzone *mz;
+
+	mz = rte_memzone_lookup(CNXK_GPIO_PARAMS_MZ_NAME);
+	if (!mz)
+		return;
+
+	memcpy(params, mz->addr, sizeof(*params));
+}
+
+static void
+cnxk_gpio_params_release(void)
+{
+	if (rte_eal_process_type() == RTE_PROC_PRIMARY)
+		rte_memzone_free(rte_memzone_lookup(CNXK_GPIO_PARAMS_MZ_NAME));
+}
+
 static int
-cnxk_gpio_parse_args(struct cnxk_gpiochip *gpiochip, const char *args)
+cnxk_gpio_parse_args(struct cnxk_gpio_params *params, const char *args)
 {
 	struct rte_kvargs *kvlist;
-	int ret;
+	int ret, num = 0;
 
 	kvlist = rte_kvargs_parse(args, cnxk_gpio_args);
 	if (!kvlist)
@@ -101,21 +139,21 @@ cnxk_gpio_parse_args(struct cnxk_gpiochip *gpiochip, const char *args)
 	ret = rte_kvargs_count(kvlist, CNXK_GPIO_ARG_GPIOCHIP);
 	if (ret == 1) {
 		ret = rte_kvargs_process(kvlist, CNXK_GPIO_ARG_GPIOCHIP,
-					 cnxk_gpio_parse_arg_gpiochip,
-					 &gpiochip->num);
+					 cnxk_gpio_parse_arg_gpiochip, &params->num);
 		if (ret)
 			goto out;
 	}
+	num++;
 
 	ret = rte_kvargs_count(kvlist, CNXK_GPIO_ARG_ALLOWLIST);
 	if (ret == 1) {
 		ret = rte_kvargs_process(kvlist, CNXK_GPIO_ARG_ALLOWLIST,
-					 cnxk_gpio_parse_arg_allowlist, NULL);
+					 cnxk_gpio_parse_arg_allowlist, params->allowlist);
 		if (ret)
 			goto out;
 	}
 
-	ret = 0;
+	ret = num++;
 out:
 	rte_kvargs_free(kvlist);
 
@@ -123,7 +161,7 @@ cnxk_gpio_parse_args(struct cnxk_gpiochip *gpiochip, const char *args)
 }
 
 static int
-cnxk_gpio_parse_allowlist(struct cnxk_gpiochip *gpiochip)
+cnxk_gpio_parse_allowlist(struct cnxk_gpiochip *gpiochip, char *allowlist)
 {
 	int i, ret, val, queue = 0;
 	char *token;
@@ -133,6 +171,12 @@ cnxk_gpio_parse_allowlist(struct cnxk_gpiochip *gpiochip)
 	if (!list)
 		return -ENOMEM;
 
+	allowlist = strdup(allowlist);
+	if (!allowlist) {
+		ret = -ENOMEM;
+		goto out;
+	}
+
 	/* replace brackets with something meaningless for strtol() */
 	allowlist[0] = ' ';
 	allowlist[strlen(allowlist) - 1] = ' ';
@@ -166,11 +210,13 @@ cnxk_gpio_parse_allowlist(struct cnxk_gpiochip *gpiochip)
 			list[queue++] = val;
 	} while ((token = strtok(NULL, ",")));
 
+	free(allowlist);
 	gpiochip->allowlist = list;
 	gpiochip->num_queues = queue;
 
 	return 0;
 out:
+	free(allowlist);
 	rte_free(list);
 
 	return ret;
@@ -562,8 +608,7 @@ cnxk_gpio_process_buf(struct cnxk_gpio *gpio, struct rte_rawdev_buf *rbuf)
 		*(int *)rsp = val;
 		break;
 	case CNXK_GPIO_MSG_TYPE_REGISTER_IRQ:
-		ret = cnxk_gpio_register_irq(gpio,
-					     (struct cnxk_gpio_irq *)msg->data);
+		ret = cnxk_gpio_register_irq(gpio, (struct cnxk_gpio_irq *)msg->data);
 		break;
 	case CNXK_GPIO_MSG_TYPE_UNREGISTER_IRQ:
 		ret = cnxk_gpio_unregister_irq(gpio);
@@ -658,18 +703,15 @@ static const struct rte_rawdev_ops cnxk_gpio_rawdev_ops = {
 static int
 cnxk_gpio_probe(struct rte_vdev_device *dev)
 {
+	struct cnxk_gpio_params params = { };
 	char name[RTE_RAWDEV_NAME_MAX_LEN];
 	struct cnxk_gpiochip *gpiochip;
 	struct rte_rawdev *rawdev;
 	char buf[CNXK_GPIO_BUFSZ];
 	int ret;
 
-	if (rte_eal_process_type() != RTE_PROC_PRIMARY)
-		return 0;
-
 	cnxk_gpio_format_name(name, sizeof(name));
-	rawdev = rte_rawdev_pmd_allocate(name, sizeof(*gpiochip),
-					 rte_socket_id());
+	rawdev = rte_rawdev_pmd_allocate(name, sizeof(*gpiochip), rte_socket_id());
 	if (!rawdev) {
 		RTE_LOG(ERR, PMD, "failed to allocate %s rawdev", name);
 		return -ENOMEM;
@@ -678,22 +720,37 @@ cnxk_gpio_probe(struct rte_vdev_device *dev)
 	rawdev->dev_ops = &cnxk_gpio_rawdev_ops;
 	rawdev->device = &dev->device;
 	rawdev->driver_name = dev->device.name;
-
 	gpiochip = rawdev->dev_private;
-	cnxk_gpio_set_defaults(gpiochip);
 
-	/* defaults may be overwritten by this call */
-	ret = cnxk_gpio_parse_args(gpiochip, rte_vdev_device_args(dev));
-	if (ret)
-		goto out;
+	ret = cnxk_gpio_set_defaults(&params);
+	if (ret) {
+		RTE_LOG(ERR, PMD, "failed to set defaults\n");
+		return ret;
+	}
+
+	if (rte_eal_process_type() == RTE_PROC_PRIMARY) {
+		ret = cnxk_gpio_parse_args(&params, rte_vdev_device_args(dev));
+		if (ret < 0)
+			goto out;
+		if (ret > 0) {
+			ret = cnxk_gpio_params_store(&params);
+			if (ret) {
+				RTE_LOG(ERR, PMD, "failed to store params\n");
+				goto out;
+			}
+		}
+	} else {
+		cnxk_gpio_params_restore(&params);
+	}
+
+	gpiochip->num = params.num;
 
 	ret = cnxk_gpio_irq_init(gpiochip);
 	if (ret)
 		goto out;
 
 	/* read gpio base */
-	snprintf(buf, sizeof(buf), "%s/gpiochip%d/base", CNXK_GPIO_CLASS_PATH,
-		 gpiochip->num);
+	snprintf(buf, sizeof(buf), "%s/gpiochip%d/base", CNXK_GPIO_CLASS_PATH, gpiochip->num);
 	ret = cnxk_gpio_read_attr_int(buf, &gpiochip->base);
 	if (ret) {
 		RTE_LOG(ERR, PMD, "failed to read %s", buf);
@@ -701,8 +758,7 @@ cnxk_gpio_probe(struct rte_vdev_device *dev)
 	}
 
 	/* read number of available gpios */
-	snprintf(buf, sizeof(buf), "%s/gpiochip%d/ngpio", CNXK_GPIO_CLASS_PATH,
-		 gpiochip->num);
+	snprintf(buf, sizeof(buf), "%s/gpiochip%d/ngpio", CNXK_GPIO_CLASS_PATH, gpiochip->num);
 	ret = cnxk_gpio_read_attr_int(buf, &gpiochip->num_gpios);
 	if (ret) {
 		RTE_LOG(ERR, PMD, "failed to read %s", buf);
@@ -710,16 +766,13 @@ cnxk_gpio_probe(struct rte_vdev_device *dev)
 	}
 	gpiochip->num_queues = gpiochip->num_gpios;
 
-	if (allowlist) {
-		ret = cnxk_gpio_parse_allowlist(gpiochip);
-		free(allowlist);
-		allowlist = NULL;
-		if (ret)
-			goto out;
+	ret = cnxk_gpio_parse_allowlist(gpiochip, params.allowlist);
+	if (ret) {
+		RTE_LOG(ERR, PMD, "failed to parse allowed gpios\n");
+		goto out;
 	}
 
-	gpiochip->gpios = rte_calloc(NULL, gpiochip->num_gpios,
-				     sizeof(struct cnxk_gpio *), 0);
+	gpiochip->gpios = rte_calloc(NULL, gpiochip->num_gpios, sizeof(struct cnxk_gpio *), 0);
 	if (!gpiochip->gpios) {
 		RTE_LOG(ERR, PMD, "failed to allocate gpios memory");
 		ret = -ENOMEM;
@@ -728,8 +781,8 @@ cnxk_gpio_probe(struct rte_vdev_device *dev)
 
 	return 0;
 out:
-	free(allowlist);
 	rte_free(gpiochip->allowlist);
+	cnxk_gpio_params_release();
 	rte_rawdev_pmd_release(rawdev);
 
 	return ret;
@@ -746,9 +799,6 @@ cnxk_gpio_remove(struct rte_vdev_device *dev)
 
 	RTE_SET_USED(dev);
 
-	if (rte_eal_process_type() != RTE_PROC_PRIMARY)
-		return 0;
-
 	cnxk_gpio_format_name(name, sizeof(name));
 	rawdev = rte_rawdev_pmd_get_named_dev(name);
 	if (!rawdev)
@@ -769,6 +819,7 @@ cnxk_gpio_remove(struct rte_vdev_device *dev)
 	rte_free(gpiochip->allowlist);
 	rte_free(gpiochip->gpios);
 	cnxk_gpio_irq_fini();
+	cnxk_gpio_params_release();
 	rte_rawdev_pmd_release(rawdev);
 
 	return 0;
-- 
2.34.1

[PATCH 2/2] raw/cnxk_gpio: add bunch of newlines

[Tomasz Duszynski]

Improve log output by adding some newlines.

Signed-off-by: Tomasz Duszynski <tduszynski@marvell.com>
Reviewed-by: Jerin Jacob Kollanukkaran <jerinj@marvell.com>
Tested-by: Jerin Jacob Kollanukkaran <jerinj@marvell.com>
---
 drivers/raw/cnxk_gpio/cnxk_gpio.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/raw/cnxk_gpio/cnxk_gpio.c b/drivers/raw/cnxk_gpio/cnxk_gpio.c
index dcd646397e..6c4e4f5eae 100644
--- a/drivers/raw/cnxk_gpio/cnxk_gpio.c
+++ b/drivers/raw/cnxk_gpio/cnxk_gpio.c
@@ -713,7 +713,7 @@ cnxk_gpio_probe(struct rte_vdev_device *dev)
 	cnxk_gpio_format_name(name, sizeof(name));
 	rawdev = rte_rawdev_pmd_allocate(name, sizeof(*gpiochip), rte_socket_id());
 	if (!rawdev) {
-		RTE_LOG(ERR, PMD, "failed to allocate %s rawdev", name);
+		RTE_LOG(ERR, PMD, "failed to allocate %s rawdev\n", name);
 		return -ENOMEM;
 	}
 
@@ -753,7 +753,7 @@ cnxk_gpio_probe(struct rte_vdev_device *dev)
 	snprintf(buf, sizeof(buf), "%s/gpiochip%d/base", CNXK_GPIO_CLASS_PATH, gpiochip->num);
 	ret = cnxk_gpio_read_attr_int(buf, &gpiochip->base);
 	if (ret) {
-		RTE_LOG(ERR, PMD, "failed to read %s", buf);
+		RTE_LOG(ERR, PMD, "failed to read %s\n", buf);
 		goto out;
 	}
 
@@ -761,7 +761,7 @@ cnxk_gpio_probe(struct rte_vdev_device *dev)
 	snprintf(buf, sizeof(buf), "%s/gpiochip%d/ngpio", CNXK_GPIO_CLASS_PATH, gpiochip->num);
 	ret = cnxk_gpio_read_attr_int(buf, &gpiochip->num_gpios);
 	if (ret) {
-		RTE_LOG(ERR, PMD, "failed to read %s", buf);
+		RTE_LOG(ERR, PMD, "failed to read %s\n", buf);
 		goto out;
 	}
 	gpiochip->num_queues = gpiochip->num_gpios;
@@ -774,7 +774,7 @@ cnxk_gpio_probe(struct rte_vdev_device *dev)
 
 	gpiochip->gpios = rte_calloc(NULL, gpiochip->num_gpios, sizeof(struct cnxk_gpio *), 0);
 	if (!gpiochip->gpios) {
-		RTE_LOG(ERR, PMD, "failed to allocate gpios memory");
+		RTE_LOG(ERR, PMD, "failed to allocate gpios memory\n");
 		ret = -ENOMEM;
 		goto out;
 	}
-- 
2.34.1

Re: [PATCH 2/2] raw/cnxk_gpio: add bunch of newlines

[Stephen Hemminger]

On Tue, 3 Oct 2023 22:46:03 +0200
Tomasz Duszynski <tduszynski@marvell.com> wrote:

> Improve log output by adding some newlines.
> 
> Signed-off-by: Tomasz Duszynski <tduszynski@marvell.com>
> Reviewed-by: Jerin Jacob Kollanukkaran <jerinj@marvell.com>
> Tested-by: Jerin Jacob Kollanukkaran <jerinj@marvell.com>
> ---
>  drivers/raw/cnxk_gpio/cnxk_gpio.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/raw/cnxk_gpio/cnxk_gpio.c b/drivers/raw/cnxk_gpio/cnxk_gpio.c
> index dcd646397e..6c4e4f5eae 100644
> --- a/drivers/raw/cnxk_gpio/cnxk_gpio.c
> +++ b/drivers/raw/cnxk_gpio/cnxk_gpio.c
> @@ -713,7 +713,7 @@ cnxk_gpio_probe(struct rte_vdev_device *dev)
>  	cnxk_gpio_format_name(name, sizeof(name));
>  	rawdev = rte_rawdev_pmd_allocate(name, sizeof(*gpiochip), rte_socket_id());
>  	if (!rawdev) {
> -		RTE_LOG(ERR, PMD, "failed to allocate %s rawdev", name);
> +		RTE_LOG(ERR, PMD, "failed to allocate %s rawdev\n", name);
>  		return -ENOMEM;
>  	}
>  
> @@ -753,7 +753,7 @@ cnxk_gpio_probe(struct rte_vdev_device *dev)
>  	snprintf(buf, sizeof(buf), "%s/gpiochip%d/base", CNXK_GPIO_CLASS_PATH, gpiochip->num);
>  	ret = cnxk_gpio_read_attr_int(buf, &gpiochip->base);
>  	if (ret) {
> -		RTE_LOG(ERR, PMD, "failed to read %s", buf);
> +		RTE_LOG(ERR, PMD, "failed to read %s\n", buf);
>  		goto out;
>  	}
>  
> @@ -761,7 +761,7 @@ cnxk_gpio_probe(struct rte_vdev_device *dev)
>  	snprintf(buf, sizeof(buf), "%s/gpiochip%d/ngpio", CNXK_GPIO_CLASS_PATH, gpiochip->num);
>  	ret = cnxk_gpio_read_attr_int(buf, &gpiochip->num_gpios);
>  	if (ret) {
> -		RTE_LOG(ERR, PMD, "failed to read %s", buf);
> +		RTE_LOG(ERR, PMD, "failed to read %s\n", buf);
>  		goto out;
>  	}
>  	gpiochip->num_queues = gpiochip->num_gpios;
> @@ -774,7 +774,7 @@ cnxk_gpio_probe(struct rte_vdev_device *dev)
>  
>  	gpiochip->gpios = rte_calloc(NULL, gpiochip->num_gpios, sizeof(struct cnxk_gpio *), 0);
>  	if (!gpiochip->gpios) {
> -		RTE_LOG(ERR, PMD, "failed to allocate gpios memory");
> +		RTE_LOG(ERR, PMD, "failed to allocate gpios memory\n");
>  		ret = -ENOMEM;
>  		goto out;
>  	}


No driver should be using the PMD logtype. It should always be using a dynamically
allocated log type.

Re: [PATCH 1/2] raw/cnxk_gpio: support multi-process mode

[Stephen Hemminger]

On Tue, 3 Oct 2023 22:46:02 +0200
Tomasz Duszynski <tduszynski@marvell.com> wrote:

> +
> +struct cnxk_gpio_params {
> +	char allowlist[CNXK_GPIO_BUFSZ];
> +	int num;
> +};

Should be using unsigned for number of params since can't be negative.
You could also use a flex array to avoid any buf size issues.

RE: [EXT] Re: [PATCH 1/2] raw/cnxk_gpio: support multi-process mode

[Tomasz Duszynski]

>-----Original Message-----
>From: Stephen Hemminger <stephen@networkplumber.org>
>Sent: Wednesday, October 4, 2023 12:29 AM
>To: Tomasz Duszynski <tduszynski@marvell.com>
>Cc: dev@dpdk.org; Jakub Palider <jpalider@marvell.com>; Anatoly Burakov
><anatoly.burakov@intel.com>; Jerin Jacob Kollanukkaran <jerinj@marvell.com>; thomas@monjalon.net
>Subject: [EXT] Re: [PATCH 1/2] raw/cnxk_gpio: support multi-process mode
>
>External Email
>
>----------------------------------------------------------------------
>On Tue, 3 Oct 2023 22:46:02 +0200
>Tomasz Duszynski <tduszynski@marvell.com> wrote:
>
>> +
>> +struct cnxk_gpio_params {
>> +	char allowlist[CNXK_GPIO_BUFSZ];
>> +	int num;
>> +};
>
>Should be using unsigned for number of params since can't be negative.

I don't think that it breaks anything currently i.e in worst case scenario
chip won't be found in sysfs but from correctness pov you are right. Will change that in v2.

>You could also use a flex array to avoid any buf size issues.

Okay, will address that in v2.  

RE: [EXT] Re: [PATCH 2/2] raw/cnxk_gpio: add bunch of newlines

[Tomasz Duszynski]

>-----Original Message-----
>From: Stephen Hemminger <stephen@networkplumber.org>
>Sent: Wednesday, October 4, 2023 12:26 AM
>To: Tomasz Duszynski <tduszynski@marvell.com>
>Cc: dev@dpdk.org; Jakub Palider <jpalider@marvell.com>; Jerin Jacob Kollanukkaran
><jerinj@marvell.com>; thomas@monjalon.net
>Subject: [EXT] Re: [PATCH 2/2] raw/cnxk_gpio: add bunch of newlines
>
>External Email
>
>----------------------------------------------------------------------
>On Tue, 3 Oct 2023 22:46:03 +0200
>Tomasz Duszynski <tduszynski@marvell.com> wrote:
>
>> Improve log output by adding some newlines.
>>
>> Signed-off-by: Tomasz Duszynski <tduszynski@marvell.com>
>> Reviewed-by: Jerin Jacob Kollanukkaran <jerinj@marvell.com>
>> Tested-by: Jerin Jacob Kollanukkaran <jerinj@marvell.com>
>> ---
>>  drivers/raw/cnxk_gpio/cnxk_gpio.c | 8 ++++----
>>  1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/raw/cnxk_gpio/cnxk_gpio.c
>> b/drivers/raw/cnxk_gpio/cnxk_gpio.c
>> index dcd646397e..6c4e4f5eae 100644
>> --- a/drivers/raw/cnxk_gpio/cnxk_gpio.c
>> +++ b/drivers/raw/cnxk_gpio/cnxk_gpio.c
>> @@ -713,7 +713,7 @@ cnxk_gpio_probe(struct rte_vdev_device *dev)
>>  	cnxk_gpio_format_name(name, sizeof(name));
>>  	rawdev = rte_rawdev_pmd_allocate(name, sizeof(*gpiochip), rte_socket_id());
>>  	if (!rawdev) {
>> -		RTE_LOG(ERR, PMD, "failed to allocate %s rawdev", name);
>> +		RTE_LOG(ERR, PMD, "failed to allocate %s rawdev\n", name);
>>  		return -ENOMEM;
>>  	}
>>
>> @@ -753,7 +753,7 @@ cnxk_gpio_probe(struct rte_vdev_device *dev)
>>  	snprintf(buf, sizeof(buf), "%s/gpiochip%d/base", CNXK_GPIO_CLASS_PATH, gpiochip->num);
>>  	ret = cnxk_gpio_read_attr_int(buf, &gpiochip->base);
>>  	if (ret) {
>> -		RTE_LOG(ERR, PMD, "failed to read %s", buf);
>> +		RTE_LOG(ERR, PMD, "failed to read %s\n", buf);
>>  		goto out;
>>  	}
>>
>> @@ -761,7 +761,7 @@ cnxk_gpio_probe(struct rte_vdev_device *dev)
>>  	snprintf(buf, sizeof(buf), "%s/gpiochip%d/ngpio", CNXK_GPIO_CLASS_PATH, gpiochip->num);
>>  	ret = cnxk_gpio_read_attr_int(buf, &gpiochip->num_gpios);
>>  	if (ret) {
>> -		RTE_LOG(ERR, PMD, "failed to read %s", buf);
>> +		RTE_LOG(ERR, PMD, "failed to read %s\n", buf);
>>  		goto out;
>>  	}
>>  	gpiochip->num_queues = gpiochip->num_gpios; @@ -774,7 +774,7 @@
>> cnxk_gpio_probe(struct rte_vdev_device *dev)
>>
>>  	gpiochip->gpios = rte_calloc(NULL, gpiochip->num_gpios, sizeof(struct cnxk_gpio *), 0);
>>  	if (!gpiochip->gpios) {
>> -		RTE_LOG(ERR, PMD, "failed to allocate gpios memory");
>> +		RTE_LOG(ERR, PMD, "failed to allocate gpios memory\n");
>>  		ret = -ENOMEM;
>>  		goto out;
>>  	}
>
>
>No driver should be using the PMD logtype. It should always be using a dynamically allocated log
>type.

Even though it uses old-fashioned logging that change could still
improve output for some users out there. 

Anyway, if you don't want any extra commit noise let's abandon that and these improvements will show up in dynamic logging patch. 

Re: [EXT] Re: [PATCH 2/2] raw/cnxk_gpio: add bunch of newlines

[Stephen Hemminger]

On Wed, 4 Oct 2023 20:42:47 +0000
Tomasz Duszynski <tduszynski@marvell.com> wrote:

> >
> >No driver should be using the PMD logtype. It should always be using a dynamically allocated log
> >type.  
> 
> Even though it uses old-fashioned logging that change could still
> improve output for some users out there. 
> 
> Anyway, if you don't want any extra commit noise let's abandon that and these improvements will show up in dynamic logging patch. 

Keep it for now, but soon RTE_LOGTYPE_PMD will be marked deprecated.

RE: [EXT] Re: [PATCH 2/2] raw/cnxk_gpio: add bunch of newlines

[Tomasz Duszynski]

>-----Original Message-----
>From: Stephen Hemminger <stephen@networkplumber.org>
>Sent: Thursday, October 5, 2023 4:55 AM
>To: Tomasz Duszynski <tduszynski@marvell.com>
>Cc: dev@dpdk.org; Jakub Palider <jpalider@marvell.com>; Jerin Jacob Kollanukkaran
><jerinj@marvell.com>; thomas@monjalon.net
>Subject: Re: [EXT] Re: [PATCH 2/2] raw/cnxk_gpio: add bunch of newlines
>
>On Wed, 4 Oct 2023 20:42:47 +0000
>Tomasz Duszynski <tduszynski@marvell.com> wrote:
>
>> >
>> >No driver should be using the PMD logtype. It should always be using
>> >a dynamically allocated log type.
>>
>> Even though it uses old-fashioned logging that change could still
>> improve output for some users out there.
>>
>> Anyway, if you don't want any extra commit noise let's abandon that and these improvements will
>show up in dynamic logging patch.
>
>Keep it for now, but soon RTE_LOGTYPE_PMD will be marked deprecated.

Okay. Will respin that and send separate patch that uses dynamic logging instead. 

[PATCH v2 1/2] raw/cnxk_gpio: support multi-process mode

[Tomasz Duszynski]

GPIO PMD uses a mixture of standard sysfs attributes and custom
ioctl()s to control behaviour of respective GPIOs available in
the system.

This means that each userspace application, either running as a primary
or a secondary, should be able to control a set of distinct GPIOs.

In rare cases where multiple processes need to control the same set of
GPIOs userspace application is responsible for synchronizing accesses.

Signed-off-by: Tomasz Duszynski <tduszynski@marvell.com>
---
v2:
- make gpiochip number unsigned
- do not hardcode length of PMD cmd line parameters buffer

 doc/guides/rawdevs/cnxk_gpio.rst  |   7 ++
 drivers/raw/cnxk_gpio/cnxk_gpio.c | 184 ++++++++++++++++++++----------
 2 files changed, 131 insertions(+), 60 deletions(-)

diff --git a/doc/guides/rawdevs/cnxk_gpio.rst b/doc/guides/rawdevs/cnxk_gpio.rst
index adff535a77..848ad329e7 100644
--- a/doc/guides/rawdevs/cnxk_gpio.rst
+++ b/doc/guides/rawdevs/cnxk_gpio.rst
@@ -21,6 +21,7 @@ Following features are available:
 - set GPIO edge that triggers interrupt
 - set GPIO active low
 - register interrupt handler for specific GPIO
+- multiprocess aware

 Requirements
 ------------
@@ -30,6 +31,12 @@ for installing interrupt handlers for low latency signal processing.

 Driver is shipped with Marvell SDK.

+Limitations
+-----------
+
+In multiprocess mode user-space application must ensure no GPIO sharing across
+processes takes place.
+
 Device Setup
 ------------

diff --git a/drivers/raw/cnxk_gpio/cnxk_gpio.c b/drivers/raw/cnxk_gpio/cnxk_gpio.c
index e2907c18b5..1e7e110da9 100644
--- a/drivers/raw/cnxk_gpio/cnxk_gpio.c
+++ b/drivers/raw/cnxk_gpio/cnxk_gpio.c
@@ -19,6 +19,12 @@

 #define CNXK_GPIO_BUFSZ 128
 #define CNXK_GPIO_CLASS_PATH "/sys/class/gpio"
+#define CNXK_GPIO_PARAMS_MZ_NAME "cnxk_gpio_params_mz"
+
+struct cnxk_gpio_params {
+	unsigned int num;
+	char allowlist[];
+};

 static const char *const cnxk_gpio_args[] = {
 #define CNXK_GPIO_ARG_GPIOCHIP "gpiochip"
@@ -28,8 +34,6 @@ static const char *const cnxk_gpio_args[] = {
 	NULL
 };

-static char *allowlist;
-
 static void
 cnxk_gpio_format_name(char *name, size_t len)
 {
@@ -44,8 +48,8 @@ cnxk_gpio_filter_gpiochip(const struct dirent *dirent)
 	return !strncmp(dirent->d_name, pattern, strlen(pattern));
 }

-static void
-cnxk_gpio_set_defaults(struct cnxk_gpiochip *gpiochip)
+static int
+cnxk_gpio_set_defaults(struct cnxk_gpio_params *params)
 {
 	struct dirent **namelist;
 	int n;
@@ -53,69 +57,127 @@ cnxk_gpio_set_defaults(struct cnxk_gpiochip *gpiochip)
 	n = scandir(CNXK_GPIO_CLASS_PATH, &namelist, cnxk_gpio_filter_gpiochip,
 		    alphasort);
 	if (n < 0 || n == 0)
-		return;
+		return -ENODEV;

-	sscanf(namelist[0]->d_name, "gpiochip%d", &gpiochip->num);
+	sscanf(namelist[0]->d_name, "gpiochip%d", &params->num);
 	while (n--)
 		free(namelist[n]);
 	free(namelist);
+
+	return 0;
 }

 static int
 cnxk_gpio_parse_arg_gpiochip(const char *key __rte_unused, const char *value,
 			     void *extra_args)
 {
-	long val;
+	unsigned long val;

 	errno = 0;
-	val = strtol(value, NULL, 10);
+	val = strtoul(value, NULL, 10);
 	if (errno)
 		return -errno;

-	*(int *)extra_args = (int)val;
+	*(unsigned int *)extra_args = val;

 	return 0;
 }

 static int
-cnxk_gpio_parse_arg_allowlist(const char *key __rte_unused, const char *value,
-			      void *extra_args __rte_unused)
+cnxk_gpio_parse_arg_allowlist(const char *key __rte_unused, const char *value, void *extra_args)
 {
-	allowlist = strdup(value);
-	if (!allowlist)
-		return -ENOMEM;
+	*(const char **)extra_args = value;

 	return 0;
 }

 static int
-cnxk_gpio_parse_args(struct cnxk_gpiochip *gpiochip, const char *args)
+cnxk_gpio_params_restore(struct cnxk_gpio_params **params)
+{
+	const struct rte_memzone *mz;
+
+	mz = rte_memzone_lookup(CNXK_GPIO_PARAMS_MZ_NAME);
+	if (!mz)
+		return -ENODEV;
+
+	*params = mz->addr;
+
+	return 0;
+}
+
+static struct cnxk_gpio_params *
+cnxk_gpio_params_reserve(size_t len)
+{
+	const struct rte_memzone *mz;
+
+	mz = rte_memzone_reserve(CNXK_GPIO_PARAMS_MZ_NAME, len, rte_socket_id(), 0);
+	if (!mz)
+		return NULL;
+
+	return mz->addr;
+}
+
+static void
+cnxk_gpio_params_release(void)
+{
+	if (rte_eal_process_type() == RTE_PROC_PRIMARY)
+		rte_memzone_free(rte_memzone_lookup(CNXK_GPIO_PARAMS_MZ_NAME));
+}
+
+static int
+cnxk_gpio_parse_arg(struct rte_kvargs *kvlist, const char *arg, arg_handler_t handler, void *data)
 {
+	int ret;
+
+	ret = rte_kvargs_count(kvlist, arg);
+	if (ret == 0)
+		return 0;
+	if (ret > 1)
+		return -EINVAL;
+
+	return rte_kvargs_process(kvlist, arg, handler, data) ? -EIO : 1;
+}
+
+static int
+cnxk_gpio_parse_store_args(struct cnxk_gpio_params **params, const char *args)
+{
+	size_t len = sizeof(**params);
+	const char *allowlist = NULL;
 	struct rte_kvargs *kvlist;
 	int ret;

 	kvlist = rte_kvargs_parse(args, cnxk_gpio_args);
-	if (!kvlist)
-		return 0;
+	if (!kvlist) {
+		*params = cnxk_gpio_params_reserve(len);
+		if (!*params)
+			return -ENOMEM;

-	ret = rte_kvargs_count(kvlist, CNXK_GPIO_ARG_GPIOCHIP);
-	if (ret == 1) {
-		ret = rte_kvargs_process(kvlist, CNXK_GPIO_ARG_GPIOCHIP,
-					 cnxk_gpio_parse_arg_gpiochip,
-					 &gpiochip->num);
+		ret = cnxk_gpio_set_defaults(*params);
 		if (ret)
 			goto out;
-	}

-	ret = rte_kvargs_count(kvlist, CNXK_GPIO_ARG_ALLOWLIST);
-	if (ret == 1) {
-		ret = rte_kvargs_process(kvlist, CNXK_GPIO_ARG_ALLOWLIST,
-					 cnxk_gpio_parse_arg_allowlist, NULL);
-		if (ret)
-			goto out;
+		return 0;
 	}

-	ret = 0;
+	ret = cnxk_gpio_parse_arg(kvlist, CNXK_GPIO_ARG_ALLOWLIST, cnxk_gpio_parse_arg_allowlist,
+				  &allowlist);
+	if (ret < 0)
+		goto out;
+
+	if (allowlist)
+		len += strlen(allowlist) + 1;
+
+	*params = cnxk_gpio_params_reserve(len);
+	if (!(*params))
+		return -ENOMEM;
+
+	strlcpy((*params)->allowlist, allowlist, strlen(allowlist) + 1);
+
+	ret = cnxk_gpio_parse_arg(kvlist, CNXK_GPIO_ARG_GPIOCHIP, cnxk_gpio_parse_arg_gpiochip,
+				  &(*params)->num);
+	if (ret == 0)
+		ret = cnxk_gpio_set_defaults(*params);
+
 out:
 	rte_kvargs_free(kvlist);

@@ -123,7 +185,7 @@ cnxk_gpio_parse_args(struct cnxk_gpiochip *gpiochip, const char *args)
 }

 static int
-cnxk_gpio_parse_allowlist(struct cnxk_gpiochip *gpiochip)
+cnxk_gpio_parse_allowlist(struct cnxk_gpiochip *gpiochip, char *allowlist)
 {
 	int i, ret, val, queue = 0;
 	char *token;
@@ -133,6 +195,12 @@ cnxk_gpio_parse_allowlist(struct cnxk_gpiochip *gpiochip)
 	if (!list)
 		return -ENOMEM;

+	allowlist = strdup(allowlist);
+	if (!allowlist) {
+		ret = -ENOMEM;
+		goto out;
+	}
+
 	/* replace brackets with something meaningless for strtol() */
 	allowlist[0] = ' ';
 	allowlist[strlen(allowlist) - 1] = ' ';
@@ -166,11 +234,13 @@ cnxk_gpio_parse_allowlist(struct cnxk_gpiochip *gpiochip)
 			list[queue++] = val;
 	} while ((token = strtok(NULL, ",")));

+	free(allowlist);
 	gpiochip->allowlist = list;
 	gpiochip->num_queues = queue;

 	return 0;
 out:
+	free(allowlist);
 	rte_free(list);

 	return ret;
@@ -562,8 +632,7 @@ cnxk_gpio_process_buf(struct cnxk_gpio *gpio, struct rte_rawdev_buf *rbuf)
 		*(int *)rsp = val;
 		break;
 	case CNXK_GPIO_MSG_TYPE_REGISTER_IRQ:
-		ret = cnxk_gpio_register_irq(gpio,
-					     (struct cnxk_gpio_irq *)msg->data);
+		ret = cnxk_gpio_register_irq(gpio, (struct cnxk_gpio_irq *)msg->data);
 		break;
 	case CNXK_GPIO_MSG_TYPE_UNREGISTER_IRQ:
 		ret = cnxk_gpio_unregister_irq(gpio);
@@ -659,17 +728,14 @@ static int
 cnxk_gpio_probe(struct rte_vdev_device *dev)
 {
 	char name[RTE_RAWDEV_NAME_MAX_LEN];
+	struct cnxk_gpio_params *params;
 	struct cnxk_gpiochip *gpiochip;
 	struct rte_rawdev *rawdev;
 	char buf[CNXK_GPIO_BUFSZ];
 	int ret;

-	if (rte_eal_process_type() != RTE_PROC_PRIMARY)
-		return 0;
-
 	cnxk_gpio_format_name(name, sizeof(name));
-	rawdev = rte_rawdev_pmd_allocate(name, sizeof(*gpiochip),
-					 rte_socket_id());
+	rawdev = rte_rawdev_pmd_allocate(name, sizeof(*gpiochip), rte_socket_id());
 	if (!rawdev) {
 		RTE_LOG(ERR, PMD, "failed to allocate %s rawdev", name);
 		return -ENOMEM;
@@ -678,22 +744,26 @@ cnxk_gpio_probe(struct rte_vdev_device *dev)
 	rawdev->dev_ops = &cnxk_gpio_rawdev_ops;
 	rawdev->device = &dev->device;
 	rawdev->driver_name = dev->device.name;
-
 	gpiochip = rawdev->dev_private;
-	cnxk_gpio_set_defaults(gpiochip);

-	/* defaults may be overwritten by this call */
-	ret = cnxk_gpio_parse_args(gpiochip, rte_vdev_device_args(dev));
-	if (ret)
-		goto out;
+	if (rte_eal_process_type() == RTE_PROC_PRIMARY) {
+		ret = cnxk_gpio_parse_store_args(&params, rte_vdev_device_args(dev));
+		if (ret < 0)
+			goto out;
+	} else {
+		ret = cnxk_gpio_params_restore(&params);
+		if (ret)
+			goto out;
+	}
+
+	gpiochip->num = params->num;

 	ret = cnxk_gpio_irq_init(gpiochip);
 	if (ret)
 		goto out;

 	/* read gpio base */
-	snprintf(buf, sizeof(buf), "%s/gpiochip%d/base", CNXK_GPIO_CLASS_PATH,
-		 gpiochip->num);
+	snprintf(buf, sizeof(buf), "%s/gpiochip%d/base", CNXK_GPIO_CLASS_PATH, gpiochip->num);
 	ret = cnxk_gpio_read_attr_int(buf, &gpiochip->base);
 	if (ret) {
 		RTE_LOG(ERR, PMD, "failed to read %s", buf);
@@ -701,8 +771,7 @@ cnxk_gpio_probe(struct rte_vdev_device *dev)
 	}

 	/* read number of available gpios */
-	snprintf(buf, sizeof(buf), "%s/gpiochip%d/ngpio", CNXK_GPIO_CLASS_PATH,
-		 gpiochip->num);
+	snprintf(buf, sizeof(buf), "%s/gpiochip%d/ngpio", CNXK_GPIO_CLASS_PATH, gpiochip->num);
 	ret = cnxk_gpio_read_attr_int(buf, &gpiochip->num_gpios);
 	if (ret) {
 		RTE_LOG(ERR, PMD, "failed to read %s", buf);
@@ -710,16 +779,13 @@ cnxk_gpio_probe(struct rte_vdev_device *dev)
 	}
 	gpiochip->num_queues = gpiochip->num_gpios;

-	if (allowlist) {
-		ret = cnxk_gpio_parse_allowlist(gpiochip);
-		free(allowlist);
-		allowlist = NULL;
-		if (ret)
-			goto out;
+	ret = cnxk_gpio_parse_allowlist(gpiochip, params->allowlist);
+	if (ret) {
+		RTE_LOG(ERR, PMD, "failed to parse allowed gpios\n");
+		goto out;
 	}

-	gpiochip->gpios = rte_calloc(NULL, gpiochip->num_gpios,
-				     sizeof(struct cnxk_gpio *), 0);
+	gpiochip->gpios = rte_calloc(NULL, gpiochip->num_gpios, sizeof(struct cnxk_gpio *), 0);
 	if (!gpiochip->gpios) {
 		RTE_LOG(ERR, PMD, "failed to allocate gpios memory");
 		ret = -ENOMEM;
@@ -728,8 +794,8 @@ cnxk_gpio_probe(struct rte_vdev_device *dev)

 	return 0;
 out:
-	free(allowlist);
 	rte_free(gpiochip->allowlist);
+	cnxk_gpio_params_release();
 	rte_rawdev_pmd_release(rawdev);

 	return ret;
@@ -746,9 +812,6 @@ cnxk_gpio_remove(struct rte_vdev_device *dev)

 	RTE_SET_USED(dev);

-	if (rte_eal_process_type() != RTE_PROC_PRIMARY)
-		return 0;
-
 	cnxk_gpio_format_name(name, sizeof(name));
 	rawdev = rte_rawdev_pmd_get_named_dev(name);
 	if (!rawdev)
@@ -769,6 +832,7 @@ cnxk_gpio_remove(struct rte_vdev_device *dev)
 	rte_free(gpiochip->allowlist);
 	rte_free(gpiochip->gpios);
 	cnxk_gpio_irq_fini();
+	cnxk_gpio_params_release();
 	rte_rawdev_pmd_release(rawdev);

 	return 0;
--
2.34.1

[PATCH v2 2/2] raw/cnxk_gpio: add bunch of newlines

[Tomasz Duszynski]

Improve log output by adding some newlines.

Signed-off-by: Tomasz Duszynski <tduszynski@marvell.com>
Reviewed-by: Jerin Jacob Kollanukkaran <jerinj@marvell.com>
Tested-by: Jerin Jacob Kollanukkaran <jerinj@marvell.com>
---
 drivers/raw/cnxk_gpio/cnxk_gpio.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/raw/cnxk_gpio/cnxk_gpio.c b/drivers/raw/cnxk_gpio/cnxk_gpio.c
index 1e7e110da9..e2fbdef886 100644
--- a/drivers/raw/cnxk_gpio/cnxk_gpio.c
+++ b/drivers/raw/cnxk_gpio/cnxk_gpio.c
@@ -737,7 +737,7 @@ cnxk_gpio_probe(struct rte_vdev_device *dev)
 	cnxk_gpio_format_name(name, sizeof(name));
 	rawdev = rte_rawdev_pmd_allocate(name, sizeof(*gpiochip), rte_socket_id());
 	if (!rawdev) {
-		RTE_LOG(ERR, PMD, "failed to allocate %s rawdev", name);
+		RTE_LOG(ERR, PMD, "failed to allocate %s rawdev\n", name);
 		return -ENOMEM;
 	}
 
@@ -766,7 +766,7 @@ cnxk_gpio_probe(struct rte_vdev_device *dev)
 	snprintf(buf, sizeof(buf), "%s/gpiochip%d/base", CNXK_GPIO_CLASS_PATH, gpiochip->num);
 	ret = cnxk_gpio_read_attr_int(buf, &gpiochip->base);
 	if (ret) {
-		RTE_LOG(ERR, PMD, "failed to read %s", buf);
+		RTE_LOG(ERR, PMD, "failed to read %s\n", buf);
 		goto out;
 	}
 
@@ -774,7 +774,7 @@ cnxk_gpio_probe(struct rte_vdev_device *dev)
 	snprintf(buf, sizeof(buf), "%s/gpiochip%d/ngpio", CNXK_GPIO_CLASS_PATH, gpiochip->num);
 	ret = cnxk_gpio_read_attr_int(buf, &gpiochip->num_gpios);
 	if (ret) {
-		RTE_LOG(ERR, PMD, "failed to read %s", buf);
+		RTE_LOG(ERR, PMD, "failed to read %s\n", buf);
 		goto out;
 	}
 	gpiochip->num_queues = gpiochip->num_gpios;
@@ -787,7 +787,7 @@ cnxk_gpio_probe(struct rte_vdev_device *dev)
 
 	gpiochip->gpios = rte_calloc(NULL, gpiochip->num_gpios, sizeof(struct cnxk_gpio *), 0);
 	if (!gpiochip->gpios) {
-		RTE_LOG(ERR, PMD, "failed to allocate gpios memory");
+		RTE_LOG(ERR, PMD, "failed to allocate gpios memory\n");
 		ret = -ENOMEM;
 		goto out;
 	}
-- 
2.34.1

[PATCH v3 1/2] raw/cnxk_gpio: support multi-process mode

[Tomasz Duszynski]

GPIO PMD uses a mixture of standard sysfs attributes and custom
ioctl()s to control behaviour of respective GPIOs available in
the system.

This means that each userspace application, either running as a primary
or a secondary, should be able to control a set of distinct GPIOs.

In rare cases where multiple processes need to control the same set of
GPIOs userspace application is responsible for synchronizing accesses.

Signed-off-by: Tomasz Duszynski <tduszynski@marvell.com>
---
v3:
- make gpiochip number unsigned
- do not hardcode length of PMD cmd line parameters buffer
- fix unchecked sscanf return value warning

 doc/guides/rawdevs/cnxk_gpio.rst  |   7 ++
 drivers/raw/cnxk_gpio/cnxk_gpio.c | 188 ++++++++++++++++++++----------
 2 files changed, 134 insertions(+), 61 deletions(-)

diff --git a/doc/guides/rawdevs/cnxk_gpio.rst b/doc/guides/rawdevs/cnxk_gpio.rst
index adff535a77..848ad329e7 100644
--- a/doc/guides/rawdevs/cnxk_gpio.rst
+++ b/doc/guides/rawdevs/cnxk_gpio.rst
@@ -21,6 +21,7 @@ Following features are available:
 - set GPIO edge that triggers interrupt
 - set GPIO active low
 - register interrupt handler for specific GPIO
+- multiprocess aware

 Requirements
 ------------
@@ -30,6 +31,12 @@ for installing interrupt handlers for low latency signal processing.

 Driver is shipped with Marvell SDK.

+Limitations
+-----------
+
+In multiprocess mode user-space application must ensure no GPIO sharing across
+processes takes place.
+
 Device Setup
 ------------

diff --git a/drivers/raw/cnxk_gpio/cnxk_gpio.c b/drivers/raw/cnxk_gpio/cnxk_gpio.c
index e2907c18b5..c480711942 100644
--- a/drivers/raw/cnxk_gpio/cnxk_gpio.c
+++ b/drivers/raw/cnxk_gpio/cnxk_gpio.c
@@ -19,6 +19,12 @@

 #define CNXK_GPIO_BUFSZ 128
 #define CNXK_GPIO_CLASS_PATH "/sys/class/gpio"
+#define CNXK_GPIO_PARAMS_MZ_NAME "cnxk_gpio_params_mz"
+
+struct cnxk_gpio_params {
+	unsigned int num;
+	char allowlist[];
+};

 static const char *const cnxk_gpio_args[] = {
 #define CNXK_GPIO_ARG_GPIOCHIP "gpiochip"
@@ -28,8 +34,6 @@ static const char *const cnxk_gpio_args[] = {
 	NULL
 };

-static char *allowlist;
-
 static void
 cnxk_gpio_format_name(char *name, size_t len)
 {
@@ -44,78 +48,138 @@ cnxk_gpio_filter_gpiochip(const struct dirent *dirent)
 	return !strncmp(dirent->d_name, pattern, strlen(pattern));
 }

-static void
-cnxk_gpio_set_defaults(struct cnxk_gpiochip *gpiochip)
+static int
+cnxk_gpio_set_defaults(struct cnxk_gpio_params *params)
 {
 	struct dirent **namelist;
-	int n;
+	int ret = 0, n;

 	n = scandir(CNXK_GPIO_CLASS_PATH, &namelist, cnxk_gpio_filter_gpiochip,
 		    alphasort);
 	if (n < 0 || n == 0)
-		return;
+		return -ENODEV;
+
+	if (sscanf(namelist[0]->d_name, "gpiochip%d", &params->num) != 1)
+		ret = -EINVAL;

-	sscanf(namelist[0]->d_name, "gpiochip%d", &gpiochip->num);
 	while (n--)
 		free(namelist[n]);
 	free(namelist);
+
+	return ret;
 }

 static int
 cnxk_gpio_parse_arg_gpiochip(const char *key __rte_unused, const char *value,
 			     void *extra_args)
 {
-	long val;
+	unsigned long val;

 	errno = 0;
-	val = strtol(value, NULL, 10);
+	val = strtoul(value, NULL, 10);
 	if (errno)
 		return -errno;

-	*(int *)extra_args = (int)val;
+	*(unsigned int *)extra_args = val;

 	return 0;
 }

 static int
-cnxk_gpio_parse_arg_allowlist(const char *key __rte_unused, const char *value,
-			      void *extra_args __rte_unused)
+cnxk_gpio_parse_arg_allowlist(const char *key __rte_unused, const char *value, void *extra_args)
 {
-	allowlist = strdup(value);
-	if (!allowlist)
-		return -ENOMEM;
+	*(const char **)extra_args = value;
+
+	return 0;
+}
+
+static int
+cnxk_gpio_params_restore(struct cnxk_gpio_params **params)
+{
+	const struct rte_memzone *mz;
+
+	mz = rte_memzone_lookup(CNXK_GPIO_PARAMS_MZ_NAME);
+	if (!mz)
+		return -ENODEV;
+
+	*params = mz->addr;

 	return 0;
 }

+static struct cnxk_gpio_params *
+cnxk_gpio_params_reserve(size_t len)
+{
+	const struct rte_memzone *mz;
+
+	mz = rte_memzone_reserve(CNXK_GPIO_PARAMS_MZ_NAME, len, rte_socket_id(), 0);
+	if (!mz)
+		return NULL;
+
+	return mz->addr;
+}
+
+static void
+cnxk_gpio_params_release(void)
+{
+	if (rte_eal_process_type() == RTE_PROC_PRIMARY)
+		rte_memzone_free(rte_memzone_lookup(CNXK_GPIO_PARAMS_MZ_NAME));
+}
+
 static int
-cnxk_gpio_parse_args(struct cnxk_gpiochip *gpiochip, const char *args)
+cnxk_gpio_parse_arg(struct rte_kvargs *kvlist, const char *arg, arg_handler_t handler, void *data)
 {
+	int ret;
+
+	ret = rte_kvargs_count(kvlist, arg);
+	if (ret == 0)
+		return 0;
+	if (ret > 1)
+		return -EINVAL;
+
+	return rte_kvargs_process(kvlist, arg, handler, data) ? -EIO : 1;
+}
+
+static int
+cnxk_gpio_parse_store_args(struct cnxk_gpio_params **params, const char *args)
+{
+	size_t len = sizeof(**params);
+	const char *allowlist = NULL;
 	struct rte_kvargs *kvlist;
 	int ret;

 	kvlist = rte_kvargs_parse(args, cnxk_gpio_args);
-	if (!kvlist)
-		return 0;
+	if (!kvlist) {
+		*params = cnxk_gpio_params_reserve(len);
+		if (!*params)
+			return -ENOMEM;

-	ret = rte_kvargs_count(kvlist, CNXK_GPIO_ARG_GPIOCHIP);
-	if (ret == 1) {
-		ret = rte_kvargs_process(kvlist, CNXK_GPIO_ARG_GPIOCHIP,
-					 cnxk_gpio_parse_arg_gpiochip,
-					 &gpiochip->num);
+		ret = cnxk_gpio_set_defaults(*params);
 		if (ret)
 			goto out;
-	}

-	ret = rte_kvargs_count(kvlist, CNXK_GPIO_ARG_ALLOWLIST);
-	if (ret == 1) {
-		ret = rte_kvargs_process(kvlist, CNXK_GPIO_ARG_ALLOWLIST,
-					 cnxk_gpio_parse_arg_allowlist, NULL);
-		if (ret)
-			goto out;
+		return 0;
 	}

-	ret = 0;
+	ret = cnxk_gpio_parse_arg(kvlist, CNXK_GPIO_ARG_ALLOWLIST, cnxk_gpio_parse_arg_allowlist,
+				  &allowlist);
+	if (ret < 0)
+		goto out;
+
+	if (allowlist)
+		len += strlen(allowlist) + 1;
+
+	*params = cnxk_gpio_params_reserve(len);
+	if (!(*params))
+		return -ENOMEM;
+
+	strlcpy((*params)->allowlist, allowlist, strlen(allowlist) + 1);
+
+	ret = cnxk_gpio_parse_arg(kvlist, CNXK_GPIO_ARG_GPIOCHIP, cnxk_gpio_parse_arg_gpiochip,
+				  &(*params)->num);
+	if (ret == 0)
+		ret = cnxk_gpio_set_defaults(*params);
+
 out:
 	rte_kvargs_free(kvlist);

@@ -123,7 +187,7 @@ cnxk_gpio_parse_args(struct cnxk_gpiochip *gpiochip, const char *args)
 }

 static int
-cnxk_gpio_parse_allowlist(struct cnxk_gpiochip *gpiochip)
+cnxk_gpio_parse_allowlist(struct cnxk_gpiochip *gpiochip, char *allowlist)
 {
 	int i, ret, val, queue = 0;
 	char *token;
@@ -133,6 +197,12 @@ cnxk_gpio_parse_allowlist(struct cnxk_gpiochip *gpiochip)
 	if (!list)
 		return -ENOMEM;

+	allowlist = strdup(allowlist);
+	if (!allowlist) {
+		ret = -ENOMEM;
+		goto out;
+	}
+
 	/* replace brackets with something meaningless for strtol() */
 	allowlist[0] = ' ';
 	allowlist[strlen(allowlist) - 1] = ' ';
@@ -166,11 +236,13 @@ cnxk_gpio_parse_allowlist(struct cnxk_gpiochip *gpiochip)
 			list[queue++] = val;
 	} while ((token = strtok(NULL, ",")));

+	free(allowlist);
 	gpiochip->allowlist = list;
 	gpiochip->num_queues = queue;

 	return 0;
 out:
+	free(allowlist);
 	rte_free(list);

 	return ret;
@@ -562,8 +634,7 @@ cnxk_gpio_process_buf(struct cnxk_gpio *gpio, struct rte_rawdev_buf *rbuf)
 		*(int *)rsp = val;
 		break;
 	case CNXK_GPIO_MSG_TYPE_REGISTER_IRQ:
-		ret = cnxk_gpio_register_irq(gpio,
-					     (struct cnxk_gpio_irq *)msg->data);
+		ret = cnxk_gpio_register_irq(gpio, (struct cnxk_gpio_irq *)msg->data);
 		break;
 	case CNXK_GPIO_MSG_TYPE_UNREGISTER_IRQ:
 		ret = cnxk_gpio_unregister_irq(gpio);
@@ -659,17 +730,14 @@ static int
 cnxk_gpio_probe(struct rte_vdev_device *dev)
 {
 	char name[RTE_RAWDEV_NAME_MAX_LEN];
+	struct cnxk_gpio_params *params;
 	struct cnxk_gpiochip *gpiochip;
 	struct rte_rawdev *rawdev;
 	char buf[CNXK_GPIO_BUFSZ];
 	int ret;

-	if (rte_eal_process_type() != RTE_PROC_PRIMARY)
-		return 0;
-
 	cnxk_gpio_format_name(name, sizeof(name));
-	rawdev = rte_rawdev_pmd_allocate(name, sizeof(*gpiochip),
-					 rte_socket_id());
+	rawdev = rte_rawdev_pmd_allocate(name, sizeof(*gpiochip), rte_socket_id());
 	if (!rawdev) {
 		RTE_LOG(ERR, PMD, "failed to allocate %s rawdev", name);
 		return -ENOMEM;
@@ -678,22 +746,26 @@ cnxk_gpio_probe(struct rte_vdev_device *dev)
 	rawdev->dev_ops = &cnxk_gpio_rawdev_ops;
 	rawdev->device = &dev->device;
 	rawdev->driver_name = dev->device.name;
-
 	gpiochip = rawdev->dev_private;
-	cnxk_gpio_set_defaults(gpiochip);

-	/* defaults may be overwritten by this call */
-	ret = cnxk_gpio_parse_args(gpiochip, rte_vdev_device_args(dev));
-	if (ret)
-		goto out;
+	if (rte_eal_process_type() == RTE_PROC_PRIMARY) {
+		ret = cnxk_gpio_parse_store_args(&params, rte_vdev_device_args(dev));
+		if (ret < 0)
+			goto out;
+	} else {
+		ret = cnxk_gpio_params_restore(&params);
+		if (ret)
+			goto out;
+	}
+
+	gpiochip->num = params->num;

 	ret = cnxk_gpio_irq_init(gpiochip);
 	if (ret)
 		goto out;

 	/* read gpio base */
-	snprintf(buf, sizeof(buf), "%s/gpiochip%d/base", CNXK_GPIO_CLASS_PATH,
-		 gpiochip->num);
+	snprintf(buf, sizeof(buf), "%s/gpiochip%d/base", CNXK_GPIO_CLASS_PATH, gpiochip->num);
 	ret = cnxk_gpio_read_attr_int(buf, &gpiochip->base);
 	if (ret) {
 		RTE_LOG(ERR, PMD, "failed to read %s", buf);
@@ -701,8 +773,7 @@ cnxk_gpio_probe(struct rte_vdev_device *dev)
 	}

 	/* read number of available gpios */
-	snprintf(buf, sizeof(buf), "%s/gpiochip%d/ngpio", CNXK_GPIO_CLASS_PATH,
-		 gpiochip->num);
+	snprintf(buf, sizeof(buf), "%s/gpiochip%d/ngpio", CNXK_GPIO_CLASS_PATH, gpiochip->num);
 	ret = cnxk_gpio_read_attr_int(buf, &gpiochip->num_gpios);
 	if (ret) {
 		RTE_LOG(ERR, PMD, "failed to read %s", buf);
@@ -710,16 +781,13 @@ cnxk_gpio_probe(struct rte_vdev_device *dev)
 	}
 	gpiochip->num_queues = gpiochip->num_gpios;

-	if (allowlist) {
-		ret = cnxk_gpio_parse_allowlist(gpiochip);
-		free(allowlist);
-		allowlist = NULL;
-		if (ret)
-			goto out;
+	ret = cnxk_gpio_parse_allowlist(gpiochip, params->allowlist);
+	if (ret) {
+		RTE_LOG(ERR, PMD, "failed to parse allowed gpios\n");
+		goto out;
 	}

-	gpiochip->gpios = rte_calloc(NULL, gpiochip->num_gpios,
-				     sizeof(struct cnxk_gpio *), 0);
+	gpiochip->gpios = rte_calloc(NULL, gpiochip->num_gpios, sizeof(struct cnxk_gpio *), 0);
 	if (!gpiochip->gpios) {
 		RTE_LOG(ERR, PMD, "failed to allocate gpios memory");
 		ret = -ENOMEM;
@@ -728,8 +796,8 @@ cnxk_gpio_probe(struct rte_vdev_device *dev)

 	return 0;
 out:
-	free(allowlist);
 	rte_free(gpiochip->allowlist);
+	cnxk_gpio_params_release();
 	rte_rawdev_pmd_release(rawdev);

 	return ret;
@@ -746,9 +814,6 @@ cnxk_gpio_remove(struct rte_vdev_device *dev)

 	RTE_SET_USED(dev);

-	if (rte_eal_process_type() != RTE_PROC_PRIMARY)
-		return 0;
-
 	cnxk_gpio_format_name(name, sizeof(name));
 	rawdev = rte_rawdev_pmd_get_named_dev(name);
 	if (!rawdev)
@@ -769,6 +834,7 @@ cnxk_gpio_remove(struct rte_vdev_device *dev)
 	rte_free(gpiochip->allowlist);
 	rte_free(gpiochip->gpios);
 	cnxk_gpio_irq_fini();
+	cnxk_gpio_params_release();
 	rte_rawdev_pmd_release(rawdev);

 	return 0;
--
2.34.1

[PATCH v3 2/2] raw/cnxk_gpio: add bunch of newlines

[Tomasz Duszynski]

Improve log output by adding some newlines.

Signed-off-by: Tomasz Duszynski <tduszynski@marvell.com>
Reviewed-by: Jerin Jacob Kollanukkaran <jerinj@marvell.com>
Tested-by: Jerin Jacob Kollanukkaran <jerinj@marvell.com>
---
 drivers/raw/cnxk_gpio/cnxk_gpio.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/raw/cnxk_gpio/cnxk_gpio.c b/drivers/raw/cnxk_gpio/cnxk_gpio.c
index c480711942..29c2506726 100644
--- a/drivers/raw/cnxk_gpio/cnxk_gpio.c
+++ b/drivers/raw/cnxk_gpio/cnxk_gpio.c
@@ -739,7 +739,7 @@ cnxk_gpio_probe(struct rte_vdev_device *dev)
 	cnxk_gpio_format_name(name, sizeof(name));
 	rawdev = rte_rawdev_pmd_allocate(name, sizeof(*gpiochip), rte_socket_id());
 	if (!rawdev) {
-		RTE_LOG(ERR, PMD, "failed to allocate %s rawdev", name);
+		RTE_LOG(ERR, PMD, "failed to allocate %s rawdev\n", name);
 		return -ENOMEM;
 	}
 
@@ -768,7 +768,7 @@ cnxk_gpio_probe(struct rte_vdev_device *dev)
 	snprintf(buf, sizeof(buf), "%s/gpiochip%d/base", CNXK_GPIO_CLASS_PATH, gpiochip->num);
 	ret = cnxk_gpio_read_attr_int(buf, &gpiochip->base);
 	if (ret) {
-		RTE_LOG(ERR, PMD, "failed to read %s", buf);
+		RTE_LOG(ERR, PMD, "failed to read %s\n", buf);
 		goto out;
 	}
 
@@ -776,7 +776,7 @@ cnxk_gpio_probe(struct rte_vdev_device *dev)
 	snprintf(buf, sizeof(buf), "%s/gpiochip%d/ngpio", CNXK_GPIO_CLASS_PATH, gpiochip->num);
 	ret = cnxk_gpio_read_attr_int(buf, &gpiochip->num_gpios);
 	if (ret) {
-		RTE_LOG(ERR, PMD, "failed to read %s", buf);
+		RTE_LOG(ERR, PMD, "failed to read %s\n", buf);
 		goto out;
 	}
 	gpiochip->num_queues = gpiochip->num_gpios;
@@ -789,7 +789,7 @@ cnxk_gpio_probe(struct rte_vdev_device *dev)
 
 	gpiochip->gpios = rte_calloc(NULL, gpiochip->num_gpios, sizeof(struct cnxk_gpio *), 0);
 	if (!gpiochip->gpios) {
-		RTE_LOG(ERR, PMD, "failed to allocate gpios memory");
+		RTE_LOG(ERR, PMD, "failed to allocate gpios memory\n");
 		ret = -ENOMEM;
 		goto out;
 	}
-- 
2.34.1

Re: [PATCH v3 2/2] raw/cnxk_gpio: add bunch of newlines

[Jerin Jacob]

On Tue, Oct 17, 2023 at 3:02 PM Tomasz Duszynski <tduszynski@marvell.com> wrote:
>
> Improve log output by adding some newlines.
>
> Signed-off-by: Tomasz Duszynski <tduszynski@marvell.com>
> Reviewed-by: Jerin Jacob Kollanukkaran <jerinj@marvell.com>
> Tested-by: Jerin Jacob Kollanukkaran <jerinj@marvell.com>
> ---
>  drivers/raw/cnxk_gpio/cnxk_gpio.c | 8 ++++----


Updated the git commit as follows and applied to
dpdk-next-net-mrvl/for-next-net. Thanks


commit 26e04c2f64f80bb8323c2e32a4f1b59ce1cb283a (HEAD -> for-next-net,
origin/for-next-net)
Author: Tomasz Duszynski <tduszynski@marvell.com>
Date:   Mon Oct 16 12:04:22 2023 +0200

    raw/cnxk_gpio: improve log output

    Improve log output by adding some newlines.

    Signed-off-by: Tomasz Duszynski <tduszynski@marvell.com>

commit 31f8e6cd63fb5c4eaa4e689fdabc4bce4a0074d7
Author: Tomasz Duszynski <tduszynski@marvell.com>
Date:   Mon Oct 16 12:04:21 2023 +0200

    raw/cnxk_gpio: support multi-process mode

    GPIO PMD uses a mixture of standard sysfs attributes and custom
    ioctl()s to control behaviour of respective GPIOs available in
    the system.

    This means that each userspace application, either running as a primary
    or a secondary, should be able to control a set of distinct GPIOs.

    In rare cases where multiple processes need to control the same set of
    GPIOs userspace application is responsible for synchronizing accesses.

    Signed-off-by: Tomasz Duszynski <tduszynski@marvell.com>