| Message ID | 20250315164123.1855142-4-koichiro.den@canonical.com |
|---|---|
| State | New |
| Headers | show |
| Series | None | expand |
On Sat, Mar 15, 2025 at 5:41 PM Koichiro Den <koichiro.den@canonical.com> wrote: > > Prepare for the upcoming configfs interface. These functions will be > used by both the existing sysfs interface and the new configfs > interface, reducing code duplication. > > No functional change. > > Signed-off-by: Koichiro Den <koichiro.den@canonical.com> > --- > drivers/gpio/gpio-aggregator.c | 58 +++++++++++++++++++++------------- > 1 file changed, 36 insertions(+), 22 deletions(-) > > diff --git a/drivers/gpio/gpio-aggregator.c b/drivers/gpio/gpio-aggregator.c > index e026deb4ac64..2692a31e01ac 100644 > --- a/drivers/gpio/gpio-aggregator.c > +++ b/drivers/gpio/gpio-aggregator.c > @@ -36,12 +36,41 @@ > struct gpio_aggregator { > struct gpiod_lookup_table *lookups; > struct platform_device *pdev; > + int id; > char args[]; > }; > > static DEFINE_MUTEX(gpio_aggregator_lock); /* protects idr */ > static DEFINE_IDR(gpio_aggregator_idr); > > +static int aggr_alloc(struct gpio_aggregator **aggr, size_t arg_size) > +{ > + struct gpio_aggregator *new __free(kfree) = NULL; > + int ret; > + > + new = kzalloc(sizeof(*new) + arg_size, GFP_KERNEL); Please prefer declaring the auto variable and initializing it at the same time. Should be: struct gpio_aggregator *new __free(kfree) = kzalloc(...); > + if (!new) > + return -ENOMEM; > + > + mutex_lock(&gpio_aggregator_lock); If adding new code, please use lock guards. Bart
On Thu, Mar 20, 2025 at 04:51:04PM GMT, Bartosz Golaszewski wrote: > On Sat, Mar 15, 2025 at 5:41 PM Koichiro Den <koichiro.den@canonical.com> wrote: > > > > Prepare for the upcoming configfs interface. These functions will be > > used by both the existing sysfs interface and the new configfs > > interface, reducing code duplication. > > > > No functional change. > > > > Signed-off-by: Koichiro Den <koichiro.den@canonical.com> > > --- > > drivers/gpio/gpio-aggregator.c | 58 +++++++++++++++++++++------------- > > 1 file changed, 36 insertions(+), 22 deletions(-) > > > > diff --git a/drivers/gpio/gpio-aggregator.c b/drivers/gpio/gpio-aggregator.c > > index e026deb4ac64..2692a31e01ac 100644 > > --- a/drivers/gpio/gpio-aggregator.c > > +++ b/drivers/gpio/gpio-aggregator.c > > @@ -36,12 +36,41 @@ > > struct gpio_aggregator { > > struct gpiod_lookup_table *lookups; > > struct platform_device *pdev; > > + int id; > > char args[]; > > }; > > > > static DEFINE_MUTEX(gpio_aggregator_lock); /* protects idr */ > > static DEFINE_IDR(gpio_aggregator_idr); > > > > +static int aggr_alloc(struct gpio_aggregator **aggr, size_t arg_size) > > +{ > > + struct gpio_aggregator *new __free(kfree) = NULL; > > + int ret; > > + > > + new = kzalloc(sizeof(*new) + arg_size, GFP_KERNEL); > > Please prefer declaring the auto variable and initializing it at the > same time. Should be: > > struct gpio_aggregator *new __free(kfree) = kzalloc(...); Thanks for the review. Should I send v7 for this change? Koichiro > > > + if (!new) > > + return -ENOMEM; > > + > > + mutex_lock(&gpio_aggregator_lock); > > If adding new code, please use lock guards. > > Bart
On Fri, Mar 21, 2025 at 3:37 AM Koichiro Den <koichiro.den@canonical.com> wrote: > > On Thu, Mar 20, 2025 at 04:51:04PM GMT, Bartosz Golaszewski wrote: > > On Sat, Mar 15, 2025 at 5:41 PM Koichiro Den <koichiro.den@canonical.com> wrote: > > > > > > Prepare for the upcoming configfs interface. These functions will be > > > used by both the existing sysfs interface and the new configfs > > > interface, reducing code duplication. > > > > > > No functional change. > > > > > > Signed-off-by: Koichiro Den <koichiro.den@canonical.com> > > > --- > > > > > > +static int aggr_alloc(struct gpio_aggregator **aggr, size_t arg_size) > > > +{ > > > + struct gpio_aggregator *new __free(kfree) = NULL; > > > + int ret; > > > + > > > + new = kzalloc(sizeof(*new) + arg_size, GFP_KERNEL); > > > > Please prefer declaring the auto variable and initializing it at the > > same time. Should be: > > > > struct gpio_aggregator *new __free(kfree) = kzalloc(...); > > Thanks for the review. Should I send v7 for this change? > You should send one anyway once v6.15-rc1 is tagged. Bartosz
On Fri, Mar 21, 2025 at 10:32:33AM GMT, Bartosz Golaszewski wrote: > On Fri, Mar 21, 2025 at 3:37 AM Koichiro Den <koichiro.den@canonical.com> wrote: > > > > On Thu, Mar 20, 2025 at 04:51:04PM GMT, Bartosz Golaszewski wrote: > > > On Sat, Mar 15, 2025 at 5:41 PM Koichiro Den <koichiro.den@canonical.com> wrote: > > > > > > > > Prepare for the upcoming configfs interface. These functions will be > > > > used by both the existing sysfs interface and the new configfs > > > > interface, reducing code duplication. > > > > > > > > No functional change. > > > > > > > > Signed-off-by: Koichiro Den <koichiro.den@canonical.com> > > > > --- > > > > > > > > +static int aggr_alloc(struct gpio_aggregator **aggr, size_t arg_size) > > > > +{ > > > > + struct gpio_aggregator *new __free(kfree) = NULL; > > > > + int ret; > > > > + > > > > + new = kzalloc(sizeof(*new) + arg_size, GFP_KERNEL); > > > > > > Please prefer declaring the auto variable and initializing it at the > > > same time. Should be: > > > > > > struct gpio_aggregator *new __free(kfree) = kzalloc(...); > > > > Thanks for the review. Should I send v7 for this change? > > > > You should send one anyway once v6.15-rc1 is tagged. Alright. Please let me confirm: - After gpio-updates-for-v6.15-rc1, will something like gpio-updates-for-v6.15-rc2 follow? - If yes, after v6.15-rc1 is tagged, I'll _quickly_ send v7 rebased onto gpio-updates-for-v6.15-rc1, right? Thanks, Koichiro > > Bartosz
On Fri, Mar 21, 2025 at 1:41 PM Koichiro Den <koichiro.den@canonical.com> wrote: > > > > > > > Thanks for the review. Should I send v7 for this change? > > > > > > > You should send one anyway once v6.15-rc1 is tagged. > > Alright. Please let me confirm: > - After gpio-updates-for-v6.15-rc1, will something like > gpio-updates-for-v6.15-rc2 follow? No. I'm not sure if I made myself clear. This series *will not* make v6.15. The merge window for v6.15 starts next week. I'll send my PR tagged as gpio-updates-for-v6.15-rc1 during the merge window. Once it's closed, Linus will tag v6.15-rc1 and we'll start a new development cycle gathering patches for v6.16 in my gpio/for-next branch. This is where your series will go and I'll send it upstream for v6.16. Only send v7 in three weeks, after Linus tags RC1. > - If yes, after v6.15-rc1 is tagged, I'll _quickly_ send v7 rebased onto > gpio-updates-for-v6.15-rc1, right? > No, you'll send your series rebases on top of v6.15-rc1 tag from Torvalds' master branch. Bart
On Fri, Mar 21, 2025 at 01:45:45PM GMT, Bartosz Golaszewski wrote: > On Fri, Mar 21, 2025 at 1:41 PM Koichiro Den <koichiro.den@canonical.com> wrote: > > > > > > > > > > Thanks for the review. Should I send v7 for this change? > > > > > > > > > > You should send one anyway once v6.15-rc1 is tagged. > > > > Alright. Please let me confirm: > > - After gpio-updates-for-v6.15-rc1, will something like > > gpio-updates-for-v6.15-rc2 follow? > > No. I'm not sure if I made myself clear. This series *will not* make > v6.15. The merge window for v6.15 starts next week. I'll send my PR > tagged as gpio-updates-for-v6.15-rc1 during the merge window. Once > it's closed, Linus will tag v6.15-rc1 and we'll start a new > development cycle gathering patches for v6.16 in my gpio/for-next > branch. This is where your series will go and I'll send it upstream > for v6.16. Alright, that makes sense. > > Only send v7 in three weeks, after Linus tags RC1. > > > - If yes, after v6.15-rc1 is tagged, I'll _quickly_ send v7 rebased onto > > gpio-updates-for-v6.15-rc1, right? > > > > No, you'll send your series rebases on top of v6.15-rc1 tag from > Torvalds' master branch. Alright, thank you! Koichiro > > Bart
diff --git a/drivers/gpio/gpio-aggregator.c b/drivers/gpio/gpio-aggregator.c index e026deb4ac64..2692a31e01ac 100644 --- a/drivers/gpio/gpio-aggregator.c +++ b/drivers/gpio/gpio-aggregator.c @@ -36,12 +36,41 @@ struct gpio_aggregator { struct gpiod_lookup_table *lookups; struct platform_device *pdev; + int id; char args[]; }; static DEFINE_MUTEX(gpio_aggregator_lock); /* protects idr */ static DEFINE_IDR(gpio_aggregator_idr); +static int aggr_alloc(struct gpio_aggregator **aggr, size_t arg_size) +{ + struct gpio_aggregator *new __free(kfree) = NULL; + int ret; + + new = kzalloc(sizeof(*new) + arg_size, GFP_KERNEL); + if (!new) + return -ENOMEM; + + mutex_lock(&gpio_aggregator_lock); + ret = idr_alloc(&gpio_aggregator_idr, new, 0, 0, GFP_KERNEL); + mutex_unlock(&gpio_aggregator_lock); + if (ret < 0) + return ret; + + new->id = ret; + *aggr = no_free_ptr(new); + return 0; +} + +static void aggr_free(struct gpio_aggregator *aggr) +{ + mutex_lock(&gpio_aggregator_lock); + idr_remove(&gpio_aggregator_idr, aggr->id); + mutex_unlock(&gpio_aggregator_lock); + kfree(aggr); +} + static int aggr_add_gpio(struct gpio_aggregator *aggr, const char *key, int hwnum, unsigned int *n) { @@ -454,17 +483,15 @@ static ssize_t new_device_store(struct device_driver *driver, const char *buf, { struct gpio_aggregator *aggr; struct platform_device *pdev; - int res, id; + int res; if (!try_module_get(THIS_MODULE)) return -ENOENT; /* kernfs guarantees string termination, so count + 1 is safe */ - aggr = kzalloc(sizeof(*aggr) + count + 1, GFP_KERNEL); - if (!aggr) { - res = -ENOMEM; + res = aggr_alloc(&aggr, count + 1); + if (res) goto put_module; - } memcpy(aggr->args, buf, count + 1); @@ -475,19 +502,10 @@ static ssize_t new_device_store(struct device_driver *driver, const char *buf, goto free_ga; } - mutex_lock(&gpio_aggregator_lock); - id = idr_alloc(&gpio_aggregator_idr, aggr, 0, 0, GFP_KERNEL); - mutex_unlock(&gpio_aggregator_lock); - - if (id < 0) { - res = id; - goto free_table; - } - - aggr->lookups->dev_id = kasprintf(GFP_KERNEL, "%s.%d", DRV_NAME, id); + aggr->lookups->dev_id = kasprintf(GFP_KERNEL, "%s.%d", DRV_NAME, aggr->id); if (!aggr->lookups->dev_id) { res = -ENOMEM; - goto remove_idr; + goto free_table; } res = aggr_parse(aggr); @@ -496,7 +514,7 @@ static ssize_t new_device_store(struct device_driver *driver, const char *buf, gpiod_add_lookup_table(aggr->lookups); - pdev = platform_device_register_simple(DRV_NAME, id, NULL, 0); + pdev = platform_device_register_simple(DRV_NAME, aggr->id, NULL, 0); if (IS_ERR(pdev)) { res = PTR_ERR(pdev); goto remove_table; @@ -510,14 +528,10 @@ static ssize_t new_device_store(struct device_driver *driver, const char *buf, gpiod_remove_lookup_table(aggr->lookups); free_dev_id: kfree(aggr->lookups->dev_id); -remove_idr: - mutex_lock(&gpio_aggregator_lock); - idr_remove(&gpio_aggregator_idr, id); - mutex_unlock(&gpio_aggregator_lock); free_table: kfree(aggr->lookups); free_ga: - kfree(aggr); + aggr_free(aggr); put_module: module_put(THIS_MODULE); return res;
Prepare for the upcoming configfs interface. These functions will be used by both the existing sysfs interface and the new configfs interface, reducing code duplication. No functional change. Signed-off-by: Koichiro Den <koichiro.den@canonical.com> --- drivers/gpio/gpio-aggregator.c | 58 +++++++++++++++++++++------------- 1 file changed, 36 insertions(+), 22 deletions(-)