Re: [PATCH v2] char: mwave: smapi: Fix signedness of SmapiOK variable

[Greg KH <gregkh@linuxfoundation.org>]

On Thu, Apr 17, 2025 at 03:19:58PM +0530, Purva Yeshi wrote:
> On 17/04/25 15:08, Greg KH wrote:
> > On Thu, Apr 17, 2025 at 02:40:18PM +0530, Purva Yeshi wrote:
> > > Smatch warning:
> > > drivers/char/mwave/smapi.c:69 smapi_request() warn:
> > > assigning (-5) to unsigned variable 'usSmapiOK'
> > > 
> > > Fix Smatch warning caused by assigning -EIO to an unsigned short.
> > > 
> > > Smatch detected a warning due to assigning -EIO (a negative value) to an
> > > unsigned short variable, causing a type mismatch and potential issues.
> > > 
> > > In v1, the type was changed to short, which resolved the warning, but
> > > retained the misleading "us" prefix in the variable name.
> > > 
> > > In v2, update the type to s16 and rename the variable to SmapiOK,
> > > removing the "us" (unsigned short) prefix as per Greg KH suggestion.
> > > 
> > > This change ensures type correctness, avoids confusion, and improves
> > > overall code readability.
> > > 
> > > Signed-off-by: Purva Yeshi <purvayeshi550@gmail.com>
> > > ---
> > > V1 - https://lore.kernel.org/all/20250409211929.213360-1-purvayeshi550@gmail.com/
> > > V2 - Use s16 type and rename variable to remove misleading "us" prefix.
> > > 
> > >   drivers/char/mwave/smapi.c | 10 +++++-----
> > >   1 file changed, 5 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/drivers/char/mwave/smapi.c b/drivers/char/mwave/smapi.c
> > > index f8d79d393b69..65bc7e1ea6cf 100644
> > > --- a/drivers/char/mwave/smapi.c
> > > +++ b/drivers/char/mwave/smapi.c
> > > @@ -66,7 +66,7 @@ static int smapi_request(unsigned short inBX, unsigned short inCX,
> > >   	unsigned short myoutDX = 5, *pmyoutDX = &myoutDX;
> > >   	unsigned short myoutDI = 6, *pmyoutDI = &myoutDI;
> > >   	unsigned short myoutSI = 7, *pmyoutSI = &myoutSI;
> > > -	unsigned short usSmapiOK = -EIO, *pusSmapiOK = &usSmapiOK;
> > > +	s16 SmapiOK = -EIO, *pSmapiOK = &SmapiOK;
> > 
> > Do you think that "SmapiOK" is a valid kernel variable name?  Doesn't
> > look ok to me, what does checkpatch.pl say?  :)
> > 
> > thanks,
> > 
> > greg k-h
> 
> Hi Greg,
> 
> Thank you for the feedback.
> 
> I ran checkpatch.pl on the patch, and it reports 0 errors and 0 warnings, so
> the variable name "SmapiOK" is valid in this context.

kernel variables should not be InterCaps like this.  they should be all
lower case, unless they are describing some hardware thing (like a
register or data field defined outside of Linux's control.)

BUT, if you look at this code, this field is coming directly from
hardware, based on the SMAPI specification, so maybe I'm wrong?  Look up
the spec for that and see how it defines these fields and maybe we just
have to live with the name following that?  If so, document it as such
in the changelog text please.

thanks,

greg k-h