[PATCH RFC] hwmon: (pmbus/core) use the new i2c_client debugfs dir

[PATCH RFC] hwmon: (pmbus/core) use the new i2c_client debugfs dir

[Wolfram Sang]

The I2C core now manages a debugfs dir per i2c_client. PMBus has its own
debugfs hierarchy. Link the two, so a user will be pointed to the pmbus
domain from the i2c domain.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---

@Guenter: I don't have any PMBus device here. Would you be interested to
test this patch? It build tests fine at least.

 drivers/hwmon/pmbus/pmbus_core.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
index 787683e83db6..510b88aed326 100644
--- a/drivers/hwmon/pmbus/pmbus_core.c
+++ b/drivers/hwmon/pmbus/pmbus_core.c
@@ -3517,6 +3517,7 @@ static int pmbus_init_debugfs(struct i2c_client *client,
 	int i, idx = 0;
 	char name[PMBUS_NAME_SIZE];
 	struct pmbus_debugfs_entry *entries;
+	const char *symlink, *hwmon_name = dev_name(data->hwmon_dev);
 
 	if (!pmbus_debugfs_dir)
 		return -ENODEV;
@@ -3525,13 +3526,19 @@ static int pmbus_init_debugfs(struct i2c_client *client,
 	 * Create the debugfs directory for this device. Use the hwmon device
 	 * name to avoid conflicts (hwmon numbers are globally unique).
 	 */
-	data->debugfs = debugfs_create_dir(dev_name(data->hwmon_dev),
-					   pmbus_debugfs_dir);
+	data->debugfs = debugfs_create_dir(hwmon_name, pmbus_debugfs_dir);
 	if (IS_ERR_OR_NULL(data->debugfs)) {
 		data->debugfs = NULL;
 		return -ENODEV;
 	}
 
+	/* The default i2c_client debugfs dir should link to where the data is */
+	symlink = kasprintf(GFP_KERNEL, "../../pmbus/%s", hwmon_name);
+	if (!symlink)
+		return -ENOMEM;
+	debugfs_create_symlink(hwmon_name, client->debugfs, symlink);
+	kfree(symlink);
+
 	/*
 	 * Allocate the max possible entries we need.
 	 * 7 entries device-specific
-- 
2.39.2

Re: [PATCH RFC] hwmon: (pmbus/core) use the new i2c_client debugfs dir

[Guenter Roeck]

On 1/23/25 08:33, Wolfram Sang wrote:
> The I2C core now manages a debugfs dir per i2c_client. PMBus has its own
> debugfs hierarchy. Link the two, so a user will be pointed to the pmbus
> domain from the i2c domain.
> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> ---
> 
> @Guenter: I don't have any PMBus device here. Would you be interested to
> test this patch? It build tests fine at least.
> 
>   drivers/hwmon/pmbus/pmbus_core.c | 11 +++++++++--
>   1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
> index 787683e83db6..510b88aed326 100644
> --- a/drivers/hwmon/pmbus/pmbus_core.c
> +++ b/drivers/hwmon/pmbus/pmbus_core.c
> @@ -3517,6 +3517,7 @@ static int pmbus_init_debugfs(struct i2c_client *client,
>   	int i, idx = 0;
>   	char name[PMBUS_NAME_SIZE];
>   	struct pmbus_debugfs_entry *entries;
> +	const char *symlink, *hwmon_name = dev_name(data->hwmon_dev);
>   
>   	if (!pmbus_debugfs_dir)
>   		return -ENODEV;
> @@ -3525,13 +3526,19 @@ static int pmbus_init_debugfs(struct i2c_client *client,
>   	 * Create the debugfs directory for this device. Use the hwmon device
>   	 * name to avoid conflicts (hwmon numbers are globally unique).
>   	 */
> -	data->debugfs = debugfs_create_dir(dev_name(data->hwmon_dev),
> -					   pmbus_debugfs_dir);
> +	data->debugfs = debugfs_create_dir(hwmon_name, pmbus_debugfs_dir);
>   	if (IS_ERR_OR_NULL(data->debugfs)) {
>   		data->debugfs = NULL;
>   		return -ENODEV;
>   	}
>   
> +	/* The default i2c_client debugfs dir should link to where the data is */
> +	symlink = kasprintf(GFP_KERNEL, "../../pmbus/%s", hwmon_name);

This would have to be "../../../pmbus/".

> +	if (!symlink)
> +		return -ENOMEM;
> +	debugfs_create_symlink(hwmon_name, client->debugfs, symlink);

As mentioned separately, the symlink is not removed if a driver is unloaded.
When it is loaded again, dmesg says something like

	debugfs: File 'hwmon9' in directory '3-0020' already present!

Also, the symlink ends up in, for example,
	/sys/kernel/debug/i2c/i2c-3/3-0020
and looks like
	hwmon9 -> ../../../pmbus/hwmon9

meaning there is an unnecessary "hwmon9" subdirectory in
/sys/kernel/debug/i2c/i2c-3/3-0020

I would prefer to have the actual debugfs files in the i2c debugfs directory
(here /sys/kernel/debug/i2c/i2c-3/3-0020) and create a symlink from
/sys/kernel/debug/pmbus/, such as

	/sys/kernel/debug/pmbus/hwmon9 -> ../i2c/i2c-3/3-0020

I tried to implement it, but right now that doesn't work because the
actual debugfs files are not removed from i2c/i2c-3/3-0020 if a driver
is unloaded and I don't immediately see how to fix that.

Guenter

Re: [PATCH RFC] hwmon: (pmbus/core) use the new i2c_client debugfs dir

[Guenter Roeck]

On 1/25/25 10:24, Guenter Roeck wrote:
> On 1/23/25 08:33, Wolfram Sang wrote:
>> The I2C core now manages a debugfs dir per i2c_client. PMBus has its own
>> debugfs hierarchy. Link the two, so a user will be pointed to the pmbus
>> domain from the i2c domain.
>>
>> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
>> ---
>>
>> @Guenter: I don't have any PMBus device here. Would you be interested to
>> test this patch? It build tests fine at least.
>>
>>   drivers/hwmon/pmbus/pmbus_core.c | 11 +++++++++--
>>   1 file changed, 9 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
>> index 787683e83db6..510b88aed326 100644
>> --- a/drivers/hwmon/pmbus/pmbus_core.c
>> +++ b/drivers/hwmon/pmbus/pmbus_core.c
>> @@ -3517,6 +3517,7 @@ static int pmbus_init_debugfs(struct i2c_client *client,
>>       int i, idx = 0;
>>       char name[PMBUS_NAME_SIZE];
>>       struct pmbus_debugfs_entry *entries;
>> +    const char *symlink, *hwmon_name = dev_name(data->hwmon_dev);
>>       if (!pmbus_debugfs_dir)
>>           return -ENODEV;
>> @@ -3525,13 +3526,19 @@ static int pmbus_init_debugfs(struct i2c_client *client,
>>        * Create the debugfs directory for this device. Use the hwmon device
>>        * name to avoid conflicts (hwmon numbers are globally unique).
>>        */
>> -    data->debugfs = debugfs_create_dir(dev_name(data->hwmon_dev),
>> -                       pmbus_debugfs_dir);
>> +    data->debugfs = debugfs_create_dir(hwmon_name, pmbus_debugfs_dir);
>>       if (IS_ERR_OR_NULL(data->debugfs)) {
>>           data->debugfs = NULL;
>>           return -ENODEV;
>>       }
>> +    /* The default i2c_client debugfs dir should link to where the data is */
>> +    symlink = kasprintf(GFP_KERNEL, "../../pmbus/%s", hwmon_name);
> 
> This would have to be "../../../pmbus/".
> 
>> +    if (!symlink)
>> +        return -ENOMEM;
>> +    debugfs_create_symlink(hwmon_name, client->debugfs, symlink);
> 
> As mentioned separately, the symlink is not removed if a driver is unloaded.
> When it is loaded again, dmesg says something like
> 
>      debugfs: File 'hwmon9' in directory '3-0020' already present!
> 
> Also, the symlink ends up in, for example,
>      /sys/kernel/debug/i2c/i2c-3/3-0020
> and looks like
>      hwmon9 -> ../../../pmbus/hwmon9
> 
> meaning there is an unnecessary "hwmon9" subdirectory in
> /sys/kernel/debug/i2c/i2c-3/3-0020
> 
> I would prefer to have the actual debugfs files in the i2c debugfs directory
> (here /sys/kernel/debug/i2c/i2c-3/3-0020) and create a symlink from
> /sys/kernel/debug/pmbus/, such as
> 
>      /sys/kernel/debug/pmbus/hwmon9 -> ../i2c/i2c-3/3-0020
> 
> I tried to implement it, but right now that doesn't work because the
> actual debugfs files are not removed from i2c/i2c-3/3-0020 if a driver
> is unloaded and I don't immediately see how to fix that.
> 

I was able to implement this after fixing the problem in the i2c code.
It works quite nicely.

root@server:/sys/kernel/debug/pmbus# ls -l
total 0
lrwxrwxrwx 1 root root 0 Jan 25 12:07 hwmon9 -> ../i2c/i2c-5/5-0020
root@server:/sys/kernel/debug/pmbus# cd ../i2c/i2c-5/5-0020
root@server:/sys/kernel/debug/i2c/i2c-5/5-0020# ls
mfr_id  mfr_model  mfr_revision  status0  status0_input  status0_iout  status0_mfr

Guenter

Re: [PATCH RFC] hwmon: (pmbus/core) use the new i2c_client debugfs dir

[Wolfram Sang]

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


> > I tried to implement it, but right now that doesn't work because the
> > actual debugfs files are not removed from i2c/i2c-3/3-0020 if a driver
> > is unloaded and I don't immediately see how to fix that.
> > 
> 
> I was able to implement this after fixing the problem in the i2c code.
> It works quite nicely.

Ok, that means once your I2C fixup patch is applied you will send out
your working version and this one can be discarded, right?


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

Re: [PATCH RFC] hwmon: (pmbus/core) use the new i2c_client debugfs dir

[Guenter Roeck]

On 1/26/25 23:05, Wolfram Sang wrote:
> 
>>> I tried to implement it, but right now that doesn't work because the
>>> actual debugfs files are not removed from i2c/i2c-3/3-0020 if a driver
>>> is unloaded and I don't immediately see how to fix that.
>>>
>>
>> I was able to implement this after fixing the problem in the i2c code.
>> It works quite nicely.
> 
> Ok, that means once your I2C fixup patch is applied you will send out
> your working version and this one can be discarded, right?
> 

Yes, I'll do that.

Thanks,
Guenter