| Message ID | 20250123163304.46034-1-wsa+renesas@sang-engineering.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [RFC] hwmon: (pmbus/core) use the new i2c_client debugfs dir | expand |
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
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
> > 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?
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
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
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(-)