Re: [PATCH RESEND] hwmon: (isl28022) Use per-client debugfs entry

[Guenter Roeck <linux@xxxxxxxxxxxx>]

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@xxxxxxxxxxxxxxxxxxxx>
> Reviewed-by: Guenter Roeck <linux@xxxxxxxxxxxx>
> ---
>
> 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@xxxxxxxxxxxxxxxxx>");
>   MODULE_DESCRIPTION("ISL28022 driver");