| Message ID | 20230627184027.16343-1-eajames@linux.ibm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | hwmon: (pmbus/acbel-fsg032) Add firmware version debugfs attribute | expand |
On 6/27/23 11:40, Eddie James wrote: > Like the IBM CFFPS driver, export the PSU's firmware version to a > debugfs attribute as reported in the manufacturer register. > > Signed-off-by: Eddie James <eajames@linux.ibm.com> > --- > drivers/hwmon/pmbus/acbel-fsg032.c | 48 ++++++++++++++++++++++++++++++ > 1 file changed, 48 insertions(+) > > diff --git a/drivers/hwmon/pmbus/acbel-fsg032.c b/drivers/hwmon/pmbus/acbel-fsg032.c > index 0a0ef4ce3493..4b97f108cfe3 100644 > --- a/drivers/hwmon/pmbus/acbel-fsg032.c > +++ b/drivers/hwmon/pmbus/acbel-fsg032.c > @@ -3,6 +3,7 @@ > * Copyright 2023 IBM Corp. > */ > > +#include <linux/debugfs.h> > #include <linux/device.h> > #include <linux/fs.h> > #include <linux/i2c.h> > @@ -11,6 +12,52 @@ > #include <linux/hwmon-sysfs.h> > #include "pmbus.h" > > +#define ACBEL_MFR_FW_REVISION 0xd9 > + > +static ssize_t acbel_fsg032_debugfs_read(struct file *file, char __user *buf, size_t count, > + loff_t *ppos) > +{ > + struct i2c_client *client = file->private_data; > + char data[I2C_SMBUS_BLOCK_MAX + 2] = { 0 }; > + char out[8]; > + int rc; > + int i; > + > + rc = pmbus_lock_interruptible(client); Is that needed here ? > + if (rc) > + return rc; > + > + rc = i2c_smbus_read_block_data(client, ACBEL_MFR_FW_REVISION, data); > + pmbus_unlock(client); > + if (rc < 0) > + return rc; > + > + for (i = 0; i < rc && i < 3; ++i) > + snprintf(&out[i * 2], 3, "%02X", data[i]); > + > + rc = i * 2; > + out[rc++] = '\n'; > + out[rc++] = 0; Any reason for not using one of the %*ph variants ? Thanks, Guenter > + return simple_read_from_buffer(buf, count, ppos, out, rc); > +} > + > +static const struct file_operations acbel_debugfs_ops = { > + .llseek = noop_llseek, > + .read = acbel_fsg032_debugfs_read, > + .write = NULL, > + .open = simple_open, > +}; > + > +static void acbel_fsg032_init_debugfs(struct i2c_client *client) > +{ > + struct dentry *debugfs = pmbus_get_debugfs_dir(client); > + > + if (!debugfs) > + return; > + > + debugfs_create_file("fw_version", 0444, debugfs, client, &acbel_debugfs_ops); > +} > + > static const struct i2c_device_id acbel_fsg032_id[] = { > { "acbel_fsg032" }, > {} > @@ -59,6 +106,7 @@ static int acbel_fsg032_probe(struct i2c_client *client) > if (rc) > return rc; > > + acbel_fsg032_init_debugfs(client); > return 0; > } >
On 6/27/23 17:19, Guenter Roeck wrote: > On 6/27/23 11:40, Eddie James wrote: >> Like the IBM CFFPS driver, export the PSU's firmware version to a >> debugfs attribute as reported in the manufacturer register. >> >> Signed-off-by: Eddie James <eajames@linux.ibm.com> >> --- >> drivers/hwmon/pmbus/acbel-fsg032.c | 48 ++++++++++++++++++++++++++++++ >> 1 file changed, 48 insertions(+) >> >> diff --git a/drivers/hwmon/pmbus/acbel-fsg032.c >> b/drivers/hwmon/pmbus/acbel-fsg032.c >> index 0a0ef4ce3493..4b97f108cfe3 100644 >> --- a/drivers/hwmon/pmbus/acbel-fsg032.c >> +++ b/drivers/hwmon/pmbus/acbel-fsg032.c >> @@ -3,6 +3,7 @@ >> * Copyright 2023 IBM Corp. >> */ >> +#include <linux/debugfs.h> >> #include <linux/device.h> >> #include <linux/fs.h> >> #include <linux/i2c.h> >> @@ -11,6 +12,52 @@ >> #include <linux/hwmon-sysfs.h> >> #include "pmbus.h" >> +#define ACBEL_MFR_FW_REVISION 0xd9 >> + >> +static ssize_t acbel_fsg032_debugfs_read(struct file *file, char >> __user *buf, size_t count, >> + loff_t *ppos) >> +{ >> + struct i2c_client *client = file->private_data; >> + char data[I2C_SMBUS_BLOCK_MAX + 2] = { 0 }; >> + char out[8]; >> + int rc; >> + int i; >> + >> + rc = pmbus_lock_interruptible(client); > > Is that needed here ? Good point, it's not. > >> + if (rc) >> + return rc; >> + >> + rc = i2c_smbus_read_block_data(client, ACBEL_MFR_FW_REVISION, >> data); >> + pmbus_unlock(client); >> + if (rc < 0) >> + return rc; >> + >> + for (i = 0; i < rc && i < 3; ++i) >> + snprintf(&out[i * 2], 3, "%02X", data[i]); >> + >> + rc = i * 2; >> + out[rc++] = '\n'; >> + out[rc++] = 0; > > Any reason for not using one of the %*ph variants ? Nope, that will work great, thanks. Eddie > > Thanks, > Guenter > >> + return simple_read_from_buffer(buf, count, ppos, out, rc); >> +} >> + >> +static const struct file_operations acbel_debugfs_ops = { >> + .llseek = noop_llseek, >> + .read = acbel_fsg032_debugfs_read, >> + .write = NULL, >> + .open = simple_open, >> +}; >> + >> +static void acbel_fsg032_init_debugfs(struct i2c_client *client) >> +{ >> + struct dentry *debugfs = pmbus_get_debugfs_dir(client); >> + >> + if (!debugfs) >> + return; >> + >> + debugfs_create_file("fw_version", 0444, debugfs, client, >> &acbel_debugfs_ops); >> +} >> + >> static const struct i2c_device_id acbel_fsg032_id[] = { >> { "acbel_fsg032" }, >> {} >> @@ -59,6 +106,7 @@ static int acbel_fsg032_probe(struct i2c_client >> *client) >> if (rc) >> return rc; >> + acbel_fsg032_init_debugfs(client); >> return 0; >> } >
Hi Eddie,
kernel test robot noticed the following build warnings:
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Eddie-James/hwmon-pmbus-acbel-fsg032-Add-firmware-version-debugfs-attribute/20230628-030615
base: https://git.kernel.org/pub/scm/linux/kernel/git/groeck/linux-staging.git hwmon-next
patch link: https://lore.kernel.org/r/20230627184027.16343-1-eajames%40linux.ibm.com
patch subject: [PATCH] hwmon: (pmbus/acbel-fsg032) Add firmware version debugfs attribute
config: x86_64-randconfig-m001-20230627 (https://download.01.org/0day-ci/archive/20230628/202306281205.NFXmu7xb-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce: (https://download.01.org/0day-ci/archive/20230628/202306281205.NFXmu7xb-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
| Closes: https://lore.kernel.org/r/202306281205.NFXmu7xb-lkp@intel.com/
smatch warnings:
drivers/hwmon/pmbus/acbel-fsg032.c:36 acbel_fsg032_debugfs_read() warn: argument 4 to %02X specifier has type 'char'
vim +/char +36 drivers/hwmon/pmbus/acbel-fsg032.c
d2c6444389b625 Eddie James 2023-06-27 17 static ssize_t acbel_fsg032_debugfs_read(struct file *file, char __user *buf, size_t count,
d2c6444389b625 Eddie James 2023-06-27 18 loff_t *ppos)
d2c6444389b625 Eddie James 2023-06-27 19 {
d2c6444389b625 Eddie James 2023-06-27 20 struct i2c_client *client = file->private_data;
d2c6444389b625 Eddie James 2023-06-27 21 char data[I2C_SMBUS_BLOCK_MAX + 2] = { 0 };
data should probably be a u8.
d2c6444389b625 Eddie James 2023-06-27 22 char out[8];
d2c6444389b625 Eddie James 2023-06-27 23 int rc;
d2c6444389b625 Eddie James 2023-06-27 24 int i;
d2c6444389b625 Eddie James 2023-06-27 25
d2c6444389b625 Eddie James 2023-06-27 26 rc = pmbus_lock_interruptible(client);
d2c6444389b625 Eddie James 2023-06-27 27 if (rc)
d2c6444389b625 Eddie James 2023-06-27 28 return rc;
d2c6444389b625 Eddie James 2023-06-27 29
d2c6444389b625 Eddie James 2023-06-27 30 rc = i2c_smbus_read_block_data(client, ACBEL_MFR_FW_REVISION, data);
d2c6444389b625 Eddie James 2023-06-27 31 pmbus_unlock(client);
d2c6444389b625 Eddie James 2023-06-27 32 if (rc < 0)
d2c6444389b625 Eddie James 2023-06-27 33 return rc;
d2c6444389b625 Eddie James 2023-06-27 34
d2c6444389b625 Eddie James 2023-06-27 35 for (i = 0; i < rc && i < 3; ++i)
d2c6444389b625 Eddie James 2023-06-27 @36 snprintf(&out[i * 2], 3, "%02X", data[i]);
If data[i] is negative this will print FFFFFFF1 etc. (This is an x86
config... Did we ever merge that patch to make char signed by default?)
d2c6444389b625 Eddie James 2023-06-27 37
d2c6444389b625 Eddie James 2023-06-27 38 rc = i * 2;
d2c6444389b625 Eddie James 2023-06-27 39 out[rc++] = '\n';
d2c6444389b625 Eddie James 2023-06-27 40 out[rc++] = 0;
d2c6444389b625 Eddie James 2023-06-27 41 return simple_read_from_buffer(buf, count, ppos, out, rc);
d2c6444389b625 Eddie James 2023-06-27 42 }
On 6/28/23 23:53, Dan Carpenter wrote: > Hi Eddie, > > kernel test robot noticed the following build warnings: > > https://git-scm.com/docs/git-format-patch#_base_tree_information] > > url: https://github.com/intel-lab-lkp/linux/commits/Eddie-James/hwmon-pmbus-acbel-fsg032-Add-firmware-version-debugfs-attribute/20230628-030615 > base: https://git.kernel.org/pub/scm/linux/kernel/git/groeck/linux-staging.git hwmon-next > patch link: https://lore.kernel.org/r/20230627184027.16343-1-eajames%40linux.ibm.com > patch subject: [PATCH] hwmon: (pmbus/acbel-fsg032) Add firmware version debugfs attribute > config: x86_64-randconfig-m001-20230627 (https://download.01.org/0day-ci/archive/20230628/202306281205.NFXmu7xb-lkp@intel.com/config) > compiler: gcc-12 (Debian 12.2.0-14) 12.2.0 > reproduce: (https://download.01.org/0day-ci/archive/20230628/202306281205.NFXmu7xb-lkp@intel.com/reproduce) > > If you fix the issue in a separate patch/commit (i.e. not just a new version of > the same patch/commit), kindly add following tags > | Reported-by: kernel test robot <lkp@intel.com> > | Reported-by: Dan Carpenter <dan.carpenter@linaro.org> > | Closes: https://lore.kernel.org/r/202306281205.NFXmu7xb-lkp@intel.com/ > > smatch warnings: > drivers/hwmon/pmbus/acbel-fsg032.c:36 acbel_fsg032_debugfs_read() warn: argument 4 to %02X specifier has type 'char' > > vim +/char +36 drivers/hwmon/pmbus/acbel-fsg032.c > > d2c6444389b625 Eddie James 2023-06-27 17 static ssize_t acbel_fsg032_debugfs_read(struct file *file, char __user *buf, size_t count, > d2c6444389b625 Eddie James 2023-06-27 18 loff_t *ppos) > d2c6444389b625 Eddie James 2023-06-27 19 { > d2c6444389b625 Eddie James 2023-06-27 20 struct i2c_client *client = file->private_data; > d2c6444389b625 Eddie James 2023-06-27 21 char data[I2C_SMBUS_BLOCK_MAX + 2] = { 0 }; > > data should probably be a u8. > > d2c6444389b625 Eddie James 2023-06-27 22 char out[8]; > d2c6444389b625 Eddie James 2023-06-27 23 int rc; > d2c6444389b625 Eddie James 2023-06-27 24 int i; > d2c6444389b625 Eddie James 2023-06-27 25 > d2c6444389b625 Eddie James 2023-06-27 26 rc = pmbus_lock_interruptible(client); > d2c6444389b625 Eddie James 2023-06-27 27 if (rc) > d2c6444389b625 Eddie James 2023-06-27 28 return rc; > d2c6444389b625 Eddie James 2023-06-27 29 > d2c6444389b625 Eddie James 2023-06-27 30 rc = i2c_smbus_read_block_data(client, ACBEL_MFR_FW_REVISION, data); > d2c6444389b625 Eddie James 2023-06-27 31 pmbus_unlock(client); > d2c6444389b625 Eddie James 2023-06-27 32 if (rc < 0) > d2c6444389b625 Eddie James 2023-06-27 33 return rc; > d2c6444389b625 Eddie James 2023-06-27 34 > d2c6444389b625 Eddie James 2023-06-27 35 for (i = 0; i < rc && i < 3; ++i) > d2c6444389b625 Eddie James 2023-06-27 @36 snprintf(&out[i * 2], 3, "%02X", data[i]); > > If data[i] is negative this will print FFFFFFF1 etc. (This is an x86 > config... Did we ever merge that patch to make char signed by default?) > That makes me wonder what "%*phN\n" in the updated patch does, as it only passes 'data' as pointer argument. Either case, no need to resend. I fixed this up when applying it by declaring data[] as u8. I also dropped the unused variable. Thanks, Guenter
On Thu, Jun 29, 2023 at 09:53:16AM +0300, Dan Carpenter wrote: > d2c6444389b625 Eddie James 2023-06-27 22 char out[8]; > d2c6444389b625 Eddie James 2023-06-27 23 int rc; > d2c6444389b625 Eddie James 2023-06-27 24 int i; > d2c6444389b625 Eddie James 2023-06-27 25 > d2c6444389b625 Eddie James 2023-06-27 26 rc = pmbus_lock_interruptible(client); > d2c6444389b625 Eddie James 2023-06-27 27 if (rc) > d2c6444389b625 Eddie James 2023-06-27 28 return rc; > d2c6444389b625 Eddie James 2023-06-27 29 > d2c6444389b625 Eddie James 2023-06-27 30 rc = i2c_smbus_read_block_data(client, ACBEL_MFR_FW_REVISION, data); > d2c6444389b625 Eddie James 2023-06-27 31 pmbus_unlock(client); > d2c6444389b625 Eddie James 2023-06-27 32 if (rc < 0) > d2c6444389b625 Eddie James 2023-06-27 33 return rc; > d2c6444389b625 Eddie James 2023-06-27 34 > d2c6444389b625 Eddie James 2023-06-27 35 for (i = 0; i < rc && i < 3; ++i) > d2c6444389b625 Eddie James 2023-06-27 @36 snprintf(&out[i * 2], 3, "%02X", data[i]); > > If data[i] is negative this will print FFFFFFF1 etc. (This is an x86 > config... Did we ever merge that patch to make char signed by default?) I meant unsigned not signed. But actually we debated both ways... Signed by default would annoy PowerPC devs since they try to really lean into the fact that char is unsigned on that arch. :P https://lwn.net/Articles/911914/ regards, dan carpenter
On 6/29/23 11:59, Dan Carpenter wrote: > On Thu, Jun 29, 2023 at 09:53:16AM +0300, Dan Carpenter wrote: >> d2c6444389b625 Eddie James 2023-06-27 22 char out[8]; >> d2c6444389b625 Eddie James 2023-06-27 23 int rc; >> d2c6444389b625 Eddie James 2023-06-27 24 int i; >> d2c6444389b625 Eddie James 2023-06-27 25 >> d2c6444389b625 Eddie James 2023-06-27 26 rc = pmbus_lock_interruptible(client); >> d2c6444389b625 Eddie James 2023-06-27 27 if (rc) >> d2c6444389b625 Eddie James 2023-06-27 28 return rc; >> d2c6444389b625 Eddie James 2023-06-27 29 >> d2c6444389b625 Eddie James 2023-06-27 30 rc = i2c_smbus_read_block_data(client, ACBEL_MFR_FW_REVISION, data); >> d2c6444389b625 Eddie James 2023-06-27 31 pmbus_unlock(client); >> d2c6444389b625 Eddie James 2023-06-27 32 if (rc < 0) >> d2c6444389b625 Eddie James 2023-06-27 33 return rc; >> d2c6444389b625 Eddie James 2023-06-27 34 >> d2c6444389b625 Eddie James 2023-06-27 35 for (i = 0; i < rc && i < 3; ++i) >> d2c6444389b625 Eddie James 2023-06-27 @36 snprintf(&out[i * 2], 3, "%02X", data[i]); >> >> If data[i] is negative this will print FFFFFFF1 etc. (This is an x86 >> config... Did we ever merge that patch to make char signed by default?) > > I meant unsigned not signed. But actually we debated both ways... > Signed by default would annoy PowerPC devs since they try to really > lean into the fact that char is unsigned on that arch. :P > > https://lwn.net/Articles/911914/ > As if anything would be easy nowadays ;-). Anyway, in this case, the array should be explicitly unsigned, so changing the type to u8 was the right thing to do. Also, the driver should be usable on non-Intel systems, which is another reason to make the type sign-specific (even more so in the context of the above discussion). Thanks, Guenter
On Thu, Jun 29, 2023 at 12:09:17PM -0700, Guenter Roeck wrote: > On 6/29/23 11:59, Dan Carpenter wrote: > > On Thu, Jun 29, 2023 at 09:53:16AM +0300, Dan Carpenter wrote: > > > d2c6444389b625 Eddie James 2023-06-27 22 char out[8]; > > > d2c6444389b625 Eddie James 2023-06-27 23 int rc; > > > d2c6444389b625 Eddie James 2023-06-27 24 int i; > > > d2c6444389b625 Eddie James 2023-06-27 25 > > > d2c6444389b625 Eddie James 2023-06-27 26 rc = pmbus_lock_interruptible(client); > > > d2c6444389b625 Eddie James 2023-06-27 27 if (rc) > > > d2c6444389b625 Eddie James 2023-06-27 28 return rc; > > > d2c6444389b625 Eddie James 2023-06-27 29 > > > d2c6444389b625 Eddie James 2023-06-27 30 rc = i2c_smbus_read_block_data(client, ACBEL_MFR_FW_REVISION, data); > > > d2c6444389b625 Eddie James 2023-06-27 31 pmbus_unlock(client); > > > d2c6444389b625 Eddie James 2023-06-27 32 if (rc < 0) > > > d2c6444389b625 Eddie James 2023-06-27 33 return rc; > > > d2c6444389b625 Eddie James 2023-06-27 34 > > > d2c6444389b625 Eddie James 2023-06-27 35 for (i = 0; i < rc && i < 3; ++i) > > > d2c6444389b625 Eddie James 2023-06-27 @36 snprintf(&out[i * 2], 3, "%02X", data[i]); > > > > > > If data[i] is negative this will print FFFFFFF1 etc. (This is an x86 > > > config... Did we ever merge that patch to make char signed by default?) > > > > I meant unsigned not signed. But actually we debated both ways... > > Signed by default would annoy PowerPC devs since they try to really > > lean into the fact that char is unsigned on that arch. :P > > > > https://lwn.net/Articles/911914/ > > > > As if anything would be easy nowadays ;-). Anyway, in this case, the array > should be explicitly unsigned, so changing the type to u8 was the right > thing to do. Also, the driver should be usable on non-Intel systems, > which is another reason to make the type sign-specific (even more so in > the context of the above discussion). Actually we did make char unsigned. I don't know if I'm super comfortable with code that assumes char is unsigned. It's makes backporting trickier. But this is definitely a false positive so I have silenced the warning in Smatch. regards, dan carpenter --- a/check_kernel_printf.c +++ b/check_kernel_printf.c @@ -827,7 +827,7 @@ hexbyte(const char *fmt, int fmt_len, struct expression *arg, int vaidx, struct sm_warning("could not determine type of argument %d", vaidx); return; } - if (type == &char_ctype || type == &schar_ctype) + if (type_signed(type) && (type == &char_ctype || type == &schar_ctype)) sm_warning("argument %d to %.*s specifier has type '%s'", vaidx, fmt_len, fmt, type_to_str(type)); }
diff --git a/drivers/hwmon/pmbus/acbel-fsg032.c b/drivers/hwmon/pmbus/acbel-fsg032.c index 0a0ef4ce3493..4b97f108cfe3 100644 --- a/drivers/hwmon/pmbus/acbel-fsg032.c +++ b/drivers/hwmon/pmbus/acbel-fsg032.c @@ -3,6 +3,7 @@ * Copyright 2023 IBM Corp. */ +#include <linux/debugfs.h> #include <linux/device.h> #include <linux/fs.h> #include <linux/i2c.h> @@ -11,6 +12,52 @@ #include <linux/hwmon-sysfs.h> #include "pmbus.h" +#define ACBEL_MFR_FW_REVISION 0xd9 + +static ssize_t acbel_fsg032_debugfs_read(struct file *file, char __user *buf, size_t count, + loff_t *ppos) +{ + struct i2c_client *client = file->private_data; + char data[I2C_SMBUS_BLOCK_MAX + 2] = { 0 }; + char out[8]; + int rc; + int i; + + rc = pmbus_lock_interruptible(client); + if (rc) + return rc; + + rc = i2c_smbus_read_block_data(client, ACBEL_MFR_FW_REVISION, data); + pmbus_unlock(client); + if (rc < 0) + return rc; + + for (i = 0; i < rc && i < 3; ++i) + snprintf(&out[i * 2], 3, "%02X", data[i]); + + rc = i * 2; + out[rc++] = '\n'; + out[rc++] = 0; + return simple_read_from_buffer(buf, count, ppos, out, rc); +} + +static const struct file_operations acbel_debugfs_ops = { + .llseek = noop_llseek, + .read = acbel_fsg032_debugfs_read, + .write = NULL, + .open = simple_open, +}; + +static void acbel_fsg032_init_debugfs(struct i2c_client *client) +{ + struct dentry *debugfs = pmbus_get_debugfs_dir(client); + + if (!debugfs) + return; + + debugfs_create_file("fw_version", 0444, debugfs, client, &acbel_debugfs_ops); +} + static const struct i2c_device_id acbel_fsg032_id[] = { { "acbel_fsg032" }, {} @@ -59,6 +106,7 @@ static int acbel_fsg032_probe(struct i2c_client *client) if (rc) return rc; + acbel_fsg032_init_debugfs(client); return 0; }
Like the IBM CFFPS driver, export the PSU's firmware version to a debugfs attribute as reported in the manufacturer register. Signed-off-by: Eddie James <eajames@linux.ibm.com> --- drivers/hwmon/pmbus/acbel-fsg032.c | 48 ++++++++++++++++++++++++++++++ 1 file changed, 48 insertions(+)