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