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

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

[Wolfram Sang]

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");
-- 
2.39.2

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

[Guenter Roeck]

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");

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

[Guenter Roeck]

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);
 }
-- 
2.45.2

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

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 397 bytes --]

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...


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Guenter Roeck]

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

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

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 797 bytes --]

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


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Guenter Roeck]

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

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

[Guenter Roeck]

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

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

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 308 bytes --]


> 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.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Guenter Roeck]

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

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

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 140 bytes --]


> You dropped
> 
> 	if (IS_ERR(client->debugfs))
> 		client->debugfs = NULL;

I have no idea how that got lost :( Darn, will fix!


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Guenter Roeck]

On 1/30/25 21:38, Wolfram Sang wrote:
> 
>> You dropped
>>
>> 	if (IS_ERR(client->debugfs))
>> 		client->debugfs = NULL;
> 
> I have no idea how that got lost :( Darn, will fix!
> 

If you didn't do it on purpose, I assume you took the prototype patch
I attached to this exchange instead of the real one. I always find it
a nuisance that patchwork picks up such "attached" patches, but I keep
forgetting about it.

No worries, though, I already added checks for ERR_PTR() into the pmbus code.

Guenter