Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Fri, 30 Jun 2023 09:18:48 +0300
From:   Dan Carpenter <dan.carpenter@linaro.org>
To:     Guenter Roeck <linux@roeck-us.net>
Cc:     oe-kbuild@lists.linux.dev, Eddie James <eajames@linux.ibm.com>,
        linux-hwmon@vger.kernel.org, lkp@intel.com,
        oe-kbuild-all@lists.linux.dev, linux-kernel@vger.kernel.org,
        jdelvare@suse.com, lakshmiy@us.ibm.com
Subject: Re: [PATCH] hwmon: (pmbus/acbel-fsg032) Add firmware version debugfs
 attribute
Message-ID: <07dac6f4-5b40-4a62-b1a9-257ec13938ed@kadam.mountain>
References: <20230627184027.16343-1-eajames@linux.ibm.com>
 <b12935a2-a8b7-4d70-8c7d-6fd1e92037c2@kadam.mountain>
 <45a29025-4c06-4f88-b82f-a8002c25c376@kadam.mountain>
 <1a27469a-4cdb-8cf0-c539-05103f2e0b8f@roeck-us.net>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1a27469a-4cdb-8cf0-c539-05103f2e0b8f@roeck-us.net>
Precedence: bulk

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