| Message ID | 20250123160347.44635-1-wsa+renesas@sang-engineering.com (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | [RESEND] hwmon: (isl28022) Use per-client debugfs entry | expand |
Hi Wolfram, when playing with this, I noticed that the i2c debugfs directory and with it the files located within are only removed when i2c_unregister_device() is called. Unfortunately, that function is not [necessarily] called when a driver is unloaded (for example by executing "modprobe -r"), leaving the debugfs files in place. If the driver is then loaded again, the old debugfs files still exist, referencing the previous instance of the driver. I don't know if this happens all the time, but it does happen if a driver which was instantiated using the new_device method is unloaded with modprobe -r. Right now that means that the driver has to delete each individual debugfs file it created when exiting, but I think that defeats the purpose of the entire exercise since it would make drivers more complicated. Do you have an idea how to handle this ? Thanks, Guenter On 1/23/25 08:03, Wolfram Sang wrote: > The I2C core now offers a debugfs-directory per client. Use it and > remove the custom handling. > > Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com> > Reviewed-by: Guenter Roeck <linux@roeck-us.net> > --- > > All dependencies are now in Linus' tree. Thisapatch can be applied now. > > drivers/hwmon/isl28022.c | 44 ++-------------------------------------- > 1 file changed, 2 insertions(+), 42 deletions(-) > > diff --git a/drivers/hwmon/isl28022.c b/drivers/hwmon/isl28022.c > index 3f9b4520b53e..1fb9864635db 100644 > --- a/drivers/hwmon/isl28022.c > +++ b/drivers/hwmon/isl28022.c > @@ -324,26 +324,6 @@ static int shunt_voltage_show(struct seq_file *seqf, void *unused) > } > DEFINE_SHOW_ATTRIBUTE(shunt_voltage); > > -static struct dentry *isl28022_debugfs_root; > - > -static void isl28022_debugfs_remove(void *res) > -{ > - debugfs_remove_recursive(res); > -} > - > -static void isl28022_debugfs_init(struct i2c_client *client, struct isl28022_data *data) > -{ > - char name[16]; > - struct dentry *debugfs; > - > - scnprintf(name, sizeof(name), "%d-%04hx", client->adapter->nr, client->addr); > - > - debugfs = debugfs_create_dir(name, isl28022_debugfs_root); > - debugfs_create_file("shunt_voltage", 0444, debugfs, data, &shunt_voltage_fops); > - > - devm_add_action_or_reset(&client->dev, isl28022_debugfs_remove, debugfs); > -} > - > /* > * read property values and make consistency checks. > * > @@ -475,7 +455,7 @@ static int isl28022_probe(struct i2c_client *client) > if (err) > return err; > > - isl28022_debugfs_init(client, data); > + debugfs_create_file("shunt_voltage", 0444, client->debugfs, data, &shunt_voltage_fops); > > hwmon_dev = devm_hwmon_device_register_with_info(dev, client->name, > data, &isl28022_chip_info, NULL); > @@ -505,27 +485,7 @@ static struct i2c_driver isl28022_driver = { > .probe = isl28022_probe, > .id_table = isl28022_ids, > }; > - > -static int __init isl28022_init(void) > -{ > - int err; > - > - isl28022_debugfs_root = debugfs_create_dir("isl28022", NULL); > - err = i2c_add_driver(&isl28022_driver); > - if (!err) > - return 0; > - > - debugfs_remove_recursive(isl28022_debugfs_root); > - return err; > -} > -module_init(isl28022_init); > - > -static void __exit isl28022_exit(void) > -{ > - i2c_del_driver(&isl28022_driver); > - debugfs_remove_recursive(isl28022_debugfs_root); > -} > -module_exit(isl28022_exit); > +module_i2c_driver(isl28022_driver); > > MODULE_AUTHOR("Carsten Spieß <mail@carsten-spiess.de>"); > MODULE_DESCRIPTION("ISL28022 driver");
Hi again, On Sat, Jan 25, 2025 at 09:42:29AM -0800, Guenter Roeck wrote: > Hi Wolfram, > > when playing with this, I noticed that the i2c debugfs directory and with it > the files located within are only removed when i2c_unregister_device() is called. > Unfortunately, that function is not [necessarily] called when a driver is unloaded > (for example by executing "modprobe -r"), leaving the debugfs files in place. > If the driver is then loaded again, the old debugfs files still exist, referencing > the previous instance of the driver. > > I don't know if this happens all the time, but it does happen if a driver > which was instantiated using the new_device method is unloaded with modprobe -r. > > Right now that means that the driver has to delete each individual debugfs file > it created when exiting, but I think that defeats the purpose of the entire exercise > since it would make drivers more complicated. > > Do you have an idea how to handle this ? > I don't know if my rationale is correct, but the attached patch fixes the problem for me. Guenter --- From c6ac6a1d153cedcc2caf7052929b381be7bfb795 Mon Sep 17 00:00:00 2001 From: Guenter Roeck <linux@roeck-us.net> Date: Sat, 25 Jan 2025 11:15:25 -0800 Subject: [PATCH] i2c: Fix core-managed per-client debugfs handling The debugfs directory should be created when a device is probed, not when it is registered. It should be removed when the device is removed, not when it is unregistered. Fixes: d06905d68610 ("i2c: add core-managed per-client directory in debugfs") Signed-off-by: Guenter Roeck <linux@roeck-us.net> --- drivers/i2c/i2c-core-base.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c index 85de36013835..4bd05eb1b501 100644 --- a/drivers/i2c/i2c-core-base.c +++ b/drivers/i2c/i2c-core-base.c @@ -583,6 +583,9 @@ static int i2c_device_probe(struct device *dev) goto err_detach_pm_domain; } + client->debugfs = debugfs_create_dir(dev_name(&client->dev), + client->adapter->debugfs); + if (driver->probe) status = driver->probe(client); else @@ -602,6 +605,7 @@ static int i2c_device_probe(struct device *dev) return 0; err_release_driver_resources: + debugfs_remove_recursive(client->debugfs); devres_release_group(&client->dev, client->devres_group_id); err_detach_pm_domain: dev_pm_domain_detach(&client->dev, do_power_on); @@ -627,6 +631,8 @@ static void i2c_device_remove(struct device *dev) driver->remove(client); } + debugfs_remove_recursive(client->debugfs); + devres_release_group(&client->dev, client->devres_group_id); dev_pm_domain_detach(&client->dev, true); @@ -1015,8 +1021,6 @@ i2c_new_client_device(struct i2c_adapter *adap, struct i2c_board_info const *inf if (status) goto out_remove_swnode; - client->debugfs = debugfs_create_dir(dev_name(&client->dev), adap->debugfs); - dev_dbg(&adap->dev, "client [%s] registered with bus id %s\n", client->name, dev_name(&client->dev)); @@ -1061,7 +1065,6 @@ void i2c_unregister_device(struct i2c_client *client) if (ACPI_COMPANION(&client->dev)) acpi_device_clear_enumerated(ACPI_COMPANION(&client->dev)); - debugfs_remove_recursive(client->debugfs); device_remove_software_node(&client->dev); device_unregister(&client->dev); }
Hi Guenter, > I don't know if my rationale is correct, but the attached patch fixes > the problem for me. Ah, yes. I didn't think twice when seeing that the debugfs entry for adapters is created in i2c_register_adapter(). But, of course, this function is called from the probe() function of the parent device. Your patch is definitely one solution, but give me a day to think more about it...
On 1/26/25 03:46, Wolfram Sang wrote: > Hi Guenter, > >> I don't know if my rationale is correct, but the attached patch fixes >> the problem for me. > > Ah, yes. I didn't think twice when seeing that the debugfs entry for > adapters is created in i2c_register_adapter(). But, of course, this > function is called from the probe() function of the parent device. > > Your patch is definitely one solution, but give me a day to think more > about it... > Sure. In this context, it would be great if client->debugfs was cleared if creating the debugfs directory failed. While debugfs functions check if the dentry pointer is valid, that is not the case for other kernel functions. Knowing that the pointer is either NULL or valid would simplify drivers (such as the PMBus code) which need to make that check. Thanks, Guenter
Hi Guenter, > > Your patch is definitely one solution, but give me a day to think more > > about it... > > Sure. So, the results of my thinking further and looking at some other debugfs handling resulted in: I think your patch is fine :) > In this context, it would be great if client->debugfs was cleared if creating > the debugfs directory failed. While debugfs functions check if the dentry > pointer is valid, that is not the case for other kernel functions. Knowing > that the pointer is either NULL or valid would simplify drivers (such as the > PMBus code) which need to make that check. Sure, we can do that. Can you add this to your patch and send it to the lists? If possible I would like to apply it this merge-window. Thanks for your efforts! Wolfram
On 1/26/25 23:03, Wolfram Sang wrote: > Hi Guenter, > >>> Your patch is definitely one solution, but give me a day to think more >>> about it... >> >> Sure. > > So, the results of my thinking further and looking at some other debugfs > handling resulted in: I think your patch is fine :) > >> In this context, it would be great if client->debugfs was cleared if creating >> the debugfs directory failed. While debugfs functions check if the dentry >> pointer is valid, that is not the case for other kernel functions. Knowing >> that the pointer is either NULL or valid would simplify drivers (such as the >> PMBus code) which need to make that check. > > Sure, we can do that. Can you add this to your patch and send it to the > lists? If possible I would like to apply it this merge-window. > Sure, I'll do that. Thanks, Guenter
On Thu, Jan 23, 2025 at 05:03:47PM +0100, Wolfram Sang wrote: > The I2C core now offers a debugfs-directory per client. Use it and > remove the custom handling. > > Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com> > Reviewed-by: Guenter Roeck <linux@roeck-us.net> > Signed-off-by: Guenter Roeck <linux@roeck-us.net> Applied to hwmon-next. Note that the branch will only be pushed to linux-next after the commit window closed. I expect that 'i2c: Fix core-managed per-client debugfs handling' will be applied in the mainline kernel by the end of the commit window. If not, I'll drop this patch and re-apply it after the problem in the upstream kernel has been fixed. Thanks, Guenter
> I expect that 'i2c: Fix core-managed per-client debugfs handling' will > be applied in the mainline kernel by the end of the commit window. If > not, I'll drop this patch and re-apply it after the problem in the > upstream kernel has been fixed. Rightfully so. Just waiting for buildbot success report.
On 1/30/25 00:55, Wolfram Sang wrote: > >> I expect that 'i2c: Fix core-managed per-client debugfs handling' will >> be applied in the mainline kernel by the end of the commit window. If >> not, I'll drop this patch and re-apply it after the problem in the >> upstream kernel has been fixed. > > Rightfully so. Just waiting for buildbot success report. > You dropped if (IS_ERR(client->debugfs)) client->debugfs = NULL; from my patch. That will make the pmbus code more complicated since it will to check for IS_ERR_OR_NULL() there. No problem, I'll handle that, but I don't really understand why you dropped that part of the patch. Guenter
> You dropped > > if (IS_ERR(client->debugfs)) > client->debugfs = NULL; I have no idea how that got lost :( Darn, will fix!
diff --git a/drivers/hwmon/isl28022.c b/drivers/hwmon/isl28022.c index 3f9b4520b53e..1fb9864635db 100644 --- a/drivers/hwmon/isl28022.c +++ b/drivers/hwmon/isl28022.c @@ -324,26 +324,6 @@ static int shunt_voltage_show(struct seq_file *seqf, void *unused) } DEFINE_SHOW_ATTRIBUTE(shunt_voltage); -static struct dentry *isl28022_debugfs_root; - -static void isl28022_debugfs_remove(void *res) -{ - debugfs_remove_recursive(res); -} - -static void isl28022_debugfs_init(struct i2c_client *client, struct isl28022_data *data) -{ - char name[16]; - struct dentry *debugfs; - - scnprintf(name, sizeof(name), "%d-%04hx", client->adapter->nr, client->addr); - - debugfs = debugfs_create_dir(name, isl28022_debugfs_root); - debugfs_create_file("shunt_voltage", 0444, debugfs, data, &shunt_voltage_fops); - - devm_add_action_or_reset(&client->dev, isl28022_debugfs_remove, debugfs); -} - /* * read property values and make consistency checks. * @@ -475,7 +455,7 @@ static int isl28022_probe(struct i2c_client *client) if (err) return err; - isl28022_debugfs_init(client, data); + debugfs_create_file("shunt_voltage", 0444, client->debugfs, data, &shunt_voltage_fops); hwmon_dev = devm_hwmon_device_register_with_info(dev, client->name, data, &isl28022_chip_info, NULL); @@ -505,27 +485,7 @@ static struct i2c_driver isl28022_driver = { .probe = isl28022_probe, .id_table = isl28022_ids, }; - -static int __init isl28022_init(void) -{ - int err; - - isl28022_debugfs_root = debugfs_create_dir("isl28022", NULL); - err = i2c_add_driver(&isl28022_driver); - if (!err) - return 0; - - debugfs_remove_recursive(isl28022_debugfs_root); - return err; -} -module_init(isl28022_init); - -static void __exit isl28022_exit(void) -{ - i2c_del_driver(&isl28022_driver); - debugfs_remove_recursive(isl28022_debugfs_root); -} -module_exit(isl28022_exit); +module_i2c_driver(isl28022_driver); MODULE_AUTHOR("Carsten Spieß <mail@carsten-spiess.de>"); MODULE_DESCRIPTION("ISL28022 driver");