| Message ID | 20200629065008.27620-7-brgl@bgdev.pl (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | devres: provide and use devm_krealloc() | expand |
On Mon, Jun 29, 2020 at 08:50:08AM +0200, Bartosz Golaszewski wrote: > From: Bartosz Golaszewski <bgolaszewski@baylibre.com> > > Shrink pmbus code by using devm_hwmon_device_register_with_groups() > and devm_krealloc() instead of their non-managed variants. > > Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com> > --- > drivers/hwmon/pmbus/pmbus_core.c | 28 +++++++++------------------- > 1 file changed, 9 insertions(+), 19 deletions(-) > > diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c > index a420877ba533..225d0ac162c7 100644 > --- a/drivers/hwmon/pmbus/pmbus_core.c > +++ b/drivers/hwmon/pmbus/pmbus_core.c > @@ -1022,9 +1022,9 @@ static int pmbus_add_attribute(struct pmbus_data *data, struct attribute *attr) > { > if (data->num_attributes >= data->max_attributes - 1) { > int new_max_attrs = data->max_attributes + PMBUS_ATTR_ALLOC_SIZE; > - void *new_attrs = krealloc(data->group.attrs, > - new_max_attrs * sizeof(void *), > - GFP_KERNEL); > + void *new_attrs = devm_krealloc(data->dev, data->group.attrs, > + new_max_attrs * sizeof(void *), > + GFP_KERNEL); dynamic sysfs attributes in a devm-allocated chunk of memory? What could go wrong... Anyway, is this the only in-kernel user that you could find for this function? If so, it feels like it's a lot of extra work for no real gain. thanks, greg k-h
On Thu, Jul 2, 2020 at 2:44 PM Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > > On Mon, Jun 29, 2020 at 08:50:08AM +0200, Bartosz Golaszewski wrote: > > From: Bartosz Golaszewski <bgolaszewski@baylibre.com> > > > > Shrink pmbus code by using devm_hwmon_device_register_with_groups() > > and devm_krealloc() instead of their non-managed variants. > > > > Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com> > > --- > > drivers/hwmon/pmbus/pmbus_core.c | 28 +++++++++------------------- > > 1 file changed, 9 insertions(+), 19 deletions(-) > > > > diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c > > index a420877ba533..225d0ac162c7 100644 > > --- a/drivers/hwmon/pmbus/pmbus_core.c > > +++ b/drivers/hwmon/pmbus/pmbus_core.c > > @@ -1022,9 +1022,9 @@ static int pmbus_add_attribute(struct pmbus_data *data, struct attribute *attr) > > { > > if (data->num_attributes >= data->max_attributes - 1) { > > int new_max_attrs = data->max_attributes + PMBUS_ATTR_ALLOC_SIZE; > > - void *new_attrs = krealloc(data->group.attrs, > > - new_max_attrs * sizeof(void *), > > - GFP_KERNEL); > > + void *new_attrs = devm_krealloc(data->dev, data->group.attrs, > > + new_max_attrs * sizeof(void *), > > + GFP_KERNEL); > > dynamic sysfs attributes in a devm-allocated chunk of memory? What > could go wrong... > So what *can* go wrong, which it couldn't before this patch? The drivers in this directory kfree() this memory anyway on driver detach. Using devm here is equivalent to the previous behavior - only that the memory is freed after remove() not inside it. > Anyway, is this the only in-kernel user that you could find for this > function? If so, it feels like it's a lot of extra work for no real > gain. > No. There are around 100 calls to krealloc() in drivers/. I assume that at least half of these are called with an attached struct device. I chose this driver, because it has a commit in its history that explicitly says that it would use devm_krealloc() if it were available (commit 85cfb3a83536 ("hwmon: (pmbus) Use krealloc to allocate attribute memory"). I didn't want to spend a lot of time on converting other users in case this patch gets rejected. Bartosz
On 7/2/20 5:44 AM, Greg Kroah-Hartman wrote: > On Mon, Jun 29, 2020 at 08:50:08AM +0200, Bartosz Golaszewski wrote: >> From: Bartosz Golaszewski <bgolaszewski@baylibre.com> >> >> Shrink pmbus code by using devm_hwmon_device_register_with_groups() >> and devm_krealloc() instead of their non-managed variants. >> >> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com> >> --- >> drivers/hwmon/pmbus/pmbus_core.c | 28 +++++++++------------------- >> 1 file changed, 9 insertions(+), 19 deletions(-) >> >> diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c >> index a420877ba533..225d0ac162c7 100644 >> --- a/drivers/hwmon/pmbus/pmbus_core.c >> +++ b/drivers/hwmon/pmbus/pmbus_core.c >> @@ -1022,9 +1022,9 @@ static int pmbus_add_attribute(struct pmbus_data *data, struct attribute *attr) >> { >> if (data->num_attributes >= data->max_attributes - 1) { >> int new_max_attrs = data->max_attributes + PMBUS_ATTR_ALLOC_SIZE; >> - void *new_attrs = krealloc(data->group.attrs, >> - new_max_attrs * sizeof(void *), >> - GFP_KERNEL); >> + void *new_attrs = devm_krealloc(data->dev, data->group.attrs, >> + new_max_attrs * sizeof(void *), >> + GFP_KERNEL); > > dynamic sysfs attributes in a devm-allocated chunk of memory? What > could go wrong... > You mean that the memory might be removed before the attributes are removed ? Hmm, but that isn't different to the current implementation. The hwmon device is removed first (removing the sysfs attributes), followed by the kfree. Are you saying this is not safe ? Pretty much all code which allocates memory for struct attribute is doing the same, so that would be a problem throughout the kernel. > Anyway, is this the only in-kernel user that you could find for this > function? If so, it feels like it's a lot of extra work for no real > gain. > And I was so happy that I'd be able to get rid of pmbus_do_remove() subsequently. But then I can also use devm_add_action() and have it call kfree(), with the same result. Given that, if hwmon would really be the only user, we can live without it. Guenter
diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c index a420877ba533..225d0ac162c7 100644 --- a/drivers/hwmon/pmbus/pmbus_core.c +++ b/drivers/hwmon/pmbus/pmbus_core.c @@ -1022,9 +1022,9 @@ static int pmbus_add_attribute(struct pmbus_data *data, struct attribute *attr) { if (data->num_attributes >= data->max_attributes - 1) { int new_max_attrs = data->max_attributes + PMBUS_ATTR_ALLOC_SIZE; - void *new_attrs = krealloc(data->group.attrs, - new_max_attrs * sizeof(void *), - GFP_KERNEL); + void *new_attrs = devm_krealloc(data->dev, data->group.attrs, + new_max_attrs * sizeof(void *), + GFP_KERNEL); if (!new_attrs) return -ENOMEM; data->group.attrs = new_attrs; @@ -2538,7 +2538,7 @@ int pmbus_do_probe(struct i2c_client *client, const struct i2c_device_id *id, ret = pmbus_find_attributes(client, data); if (ret) - goto out_kfree; + return ret; /* * If there are no attributes, something is wrong. @@ -2546,35 +2546,27 @@ int pmbus_do_probe(struct i2c_client *client, const struct i2c_device_id *id, */ if (!data->num_attributes) { dev_err(dev, "No attributes found\n"); - ret = -ENODEV; - goto out_kfree; + return -ENODEV; } data->groups[0] = &data->group; memcpy(data->groups + 1, info->groups, sizeof(void *) * groups_num); - data->hwmon_dev = hwmon_device_register_with_groups(dev, client->name, - data, data->groups); + data->hwmon_dev = devm_hwmon_device_register_with_groups(dev, + client->name, data, data->groups); if (IS_ERR(data->hwmon_dev)) { - ret = PTR_ERR(data->hwmon_dev); dev_err(dev, "Failed to register hwmon device\n"); - goto out_kfree; + return PTR_ERR(data->hwmon_dev); } ret = pmbus_regulator_register(data); if (ret) - goto out_unregister; + return ret; ret = pmbus_init_debugfs(client, data); if (ret) dev_warn(dev, "Failed to register debugfs\n"); return 0; - -out_unregister: - hwmon_device_unregister(data->hwmon_dev); -out_kfree: - kfree(data->group.attrs); - return ret; } EXPORT_SYMBOL_GPL(pmbus_do_probe); @@ -2584,8 +2576,6 @@ int pmbus_do_remove(struct i2c_client *client) debugfs_remove_recursive(data->debugfs); - hwmon_device_unregister(data->hwmon_dev); - kfree(data->group.attrs); return 0; } EXPORT_SYMBOL_GPL(pmbus_do_remove);