char: mwave: smapi: Fix signedness of SmapiOK variable

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

Posted by Purva Yeshi 8 months ago

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;
 	unsigned int inBXCX = (inBX << 16) | inCX;
 	unsigned int inDISI = (inDI << 16) | inSI;
 	int retval = 0;
@@ -102,15 +102,15 @@ static int smapi_request(unsigned short inBX, unsigned short inCX,
 			    "=m"(*(unsigned short *) pmyoutDX),
 			    "=m"(*(unsigned short *) pmyoutDI),
 			    "=m"(*(unsigned short *) pmyoutSI),
-			    "=m"(*(unsigned short *) pusSmapiOK)
+			    "=m"(*(unsigned short *) pSmapiOK)
 			    :"m"(inBXCX), "m"(inDISI), "m"(g_usSmapiPort)
 			    :"%eax", "%ebx", "%ecx", "%edx", "%edi",
 			    "%esi");
 
 	PRINTK_8(TRACE_SMAPI,
-		"myoutAX %x myoutBX %x myoutCX %x myoutDX %x myoutDI %x myoutSI %x usSmapiOK %x\n",
+		"myoutAX %x myoutBX %x myoutCX %x myoutDX %x myoutDI %x myoutSI %x SmapiOK %x\n",
 		myoutAX, myoutBX, myoutCX, myoutDX, myoutDI, myoutSI,
-		usSmapiOK);
+		SmapiOK);
 	*outAX = myoutAX;
 	*outBX = myoutBX;
 	*outCX = myoutCX;
@@ -118,7 +118,7 @@ static int smapi_request(unsigned short inBX, unsigned short inCX,
 	*outDI = myoutDI;
 	*outSI = myoutSI;
 
-	retval = (usSmapiOK == 1) ? 0 : -EIO;
+	retval = (SmapiOK == 1) ? 0 : -EIO;
 	PRINTK_2(TRACE_SMAPI, "smapi::smapi_request exit retval %x\n", retval);
 	return retval;
 }
-- 
2.34.1

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

Posted by Greg KH 8 months ago

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

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

Posted by Purva Yeshi 8 months ago

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.

Regarding the "usSmapiOK" variable, it was used only within the smapi.c 
file. Therefore, I have renamed it consistently throughout the file to 
avoid the misleading "us" prefix.

Let me know if you have any further suggestions.

Best regards,
Purva

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

Posted by Greg KH 8 months ago

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