[PATCH] staging: r8188eu: Used u16 instead of __le16 in rtl8188e_set_FwMediaStatus_cmd()

[PATCH] staging: r8188eu: Used u16 instead of __le16 in rtl8188e_set_FwMediaStatus_cmd()

[Gaurav Pathak]

- Changed 2nd argument from __le16 to u16 to fix sparse warning
  "warning: incorrect type in argument 2 (different base types)"
- Removed le16_to_cpu() in staging/r8188eu/hal/rtl8188e_cmd.c

Signed-off-by: Gaurav Pathak <gauravpathak129@gmail.com>
---
 drivers/staging/r8188eu/hal/rtl8188e_cmd.c     | 4 ++--
 drivers/staging/r8188eu/include/rtl8188e_cmd.h | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/r8188eu/hal/rtl8188e_cmd.c b/drivers/staging/r8188eu/hal/rtl8188e_cmd.c
index 8310d7f53982..a055e71d30ae 100644
--- a/drivers/staging/r8188eu/hal/rtl8188e_cmd.c
+++ b/drivers/staging/r8188eu/hal/rtl8188e_cmd.c
@@ -193,9 +193,9 @@ void rtl8188e_set_FwPwrMode_cmd(struct adapter *adapt, u8 Mode)
 
 }
 
-void rtl8188e_set_FwMediaStatus_cmd(struct adapter *adapt, __le16 mstatus_rpt)
+void rtl8188e_set_FwMediaStatus_cmd(struct adapter *adapt, u16 mstatus_rpt)
 {
-	u16 mst_rpt = le16_to_cpu(mstatus_rpt);
+	u16 mst_rpt = mstatus_rpt;
 
 	FillH2CCmd_88E(adapt, H2C_COM_MEDIA_STATUS_RPT, sizeof(mst_rpt), (u8 *)&mst_rpt);
 }
diff --git a/drivers/staging/r8188eu/include/rtl8188e_cmd.h b/drivers/staging/r8188eu/include/rtl8188e_cmd.h
index 1e01c1662f9a..c785cf8ed683 100644
--- a/drivers/staging/r8188eu/include/rtl8188e_cmd.h
+++ b/drivers/staging/r8188eu/include/rtl8188e_cmd.h
@@ -85,6 +85,6 @@ void rtl8188e_Add_RateATid(struct adapter *padapter, u32 bitmap, u8 arg,
 void rtl8188e_set_p2p_ps_offload_cmd(struct adapter *adapt, u8 p2p_ps_state);
 
 void CheckFwRsvdPageContent(struct adapter *adapt);
-void rtl8188e_set_FwMediaStatus_cmd(struct adapter *adapt, __le16 mstatus_rpt);
+void rtl8188e_set_FwMediaStatus_cmd(struct adapter *adapt, u16 mstatus_rpt);
 
 #endif/* __RTL8188E_CMD_H__ */
-- 
2.39.0

Re: [PATCH] staging: r8188eu: Used u16 instead of __le16 in rtl8188e_set_FwMediaStatus_cmd()

[Dan Carpenter]

On Tue, Jan 10, 2023 at 07:20:58PM +0530, Gaurav Pathak wrote:
> - Changed 2nd argument from __le16 to u16 to fix sparse warning
>   "warning: incorrect type in argument 2 (different base types)"
> - Removed le16_to_cpu() in staging/r8188eu/hal/rtl8188e_cmd.c
> 
> Signed-off-by: Gaurav Pathak <gauravpathak129@gmail.com>
> ---
>  drivers/staging/r8188eu/hal/rtl8188e_cmd.c     | 4 ++--
>  drivers/staging/r8188eu/include/rtl8188e_cmd.h | 2 +-
>  2 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/staging/r8188eu/hal/rtl8188e_cmd.c b/drivers/staging/r8188eu/hal/rtl8188e_cmd.c
> index 8310d7f53982..a055e71d30ae 100644
> --- a/drivers/staging/r8188eu/hal/rtl8188e_cmd.c
> +++ b/drivers/staging/r8188eu/hal/rtl8188e_cmd.c
> @@ -193,9 +193,9 @@ void rtl8188e_set_FwPwrMode_cmd(struct adapter *adapt, u8 Mode)
>  
>  }
>  
> -void rtl8188e_set_FwMediaStatus_cmd(struct adapter *adapt, __le16 mstatus_rpt)
> +void rtl8188e_set_FwMediaStatus_cmd(struct adapter *adapt, u16 mstatus_rpt)
>  {
> -	u16 mst_rpt = le16_to_cpu(mstatus_rpt);
> +	u16 mst_rpt = mstatus_rpt;

You are changing the behavior of the code here for big endian systems.
Either the change is good or bad.  TLDR; I suspect the change is bad but
I don't know.

The mstatus_rpt is CPU endian.  It is the one of two things for connect
or disconnect:

   connect: (psta->mac_id << 8) | 1
disconnect: (psta->mac_id << 8) | 0

So the question is in FillH2CCmd_88E() should the connect/disconnect
come before the mac_id as it currently does or should it only work that
way on little endian systems and be reversed on big endian systems?
My feeling is that the second option makes no sense so this patch is not
correct.

Instead what should happen is:

-void rtl8188e_set_FwMediaStatus_cmd(struct adapter *adapt, __le16 mstatus_rpt)
+void rtl8188e_set_FwMediaStatus_cmd(struct adapter *adapt, u16 mstatus_rpt)
 {
-	u16 mst_rpt = le16_to_cpu(mstatus_rpt);
+	__le16 mst_rpt = cpu_to_le16(mstatus_rpt);

But this is just my intuition and I don't know this hardware.

regards,
dan carpenter

Re: [PATCH] staging: r8188eu: Used u16 instead of __le16 in rtl8188e_set_FwMediaStatus_cmd()

[Larry Finger]

On 1/10/23 08:17, Dan Carpenter wrote:
> On Tue, Jan 10, 2023 at 07:20:58PM +0530, Gaurav Pathak wrote:
>> - Changed 2nd argument from __le16 to u16 to fix sparse warning
>>    "warning: incorrect type in argument 2 (different base types)"
>> - Removed le16_to_cpu() in staging/r8188eu/hal/rtl8188e_cmd.c
>>
>> Signed-off-by: Gaurav Pathak <gauravpathak129@gmail.com>
>> ---
>>   drivers/staging/r8188eu/hal/rtl8188e_cmd.c     | 4 ++--
>>   drivers/staging/r8188eu/include/rtl8188e_cmd.h | 2 +-
>>   2 files changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/staging/r8188eu/hal/rtl8188e_cmd.c b/drivers/staging/r8188eu/hal/rtl8188e_cmd.c
>> index 8310d7f53982..a055e71d30ae 100644
>> --- a/drivers/staging/r8188eu/hal/rtl8188e_cmd.c
>> +++ b/drivers/staging/r8188eu/hal/rtl8188e_cmd.c
>> @@ -193,9 +193,9 @@ void rtl8188e_set_FwPwrMode_cmd(struct adapter *adapt, u8 Mode)
>>   
>>   }
>>   
>> -void rtl8188e_set_FwMediaStatus_cmd(struct adapter *adapt, __le16 mstatus_rpt)
>> +void rtl8188e_set_FwMediaStatus_cmd(struct adapter *adapt, u16 mstatus_rpt)
>>   {
>> -	u16 mst_rpt = le16_to_cpu(mstatus_rpt);
>> +	u16 mst_rpt = mstatus_rpt;
> 
> You are changing the behavior of the code here for big endian systems.
> Either the change is good or bad.  TLDR; I suspect the change is bad but
> I don't know.
> 
> The mstatus_rpt is CPU endian.  It is the one of two things for connect
> or disconnect:
> 
>     connect: (psta->mac_id << 8) | 1
> disconnect: (psta->mac_id << 8) | 0
> 
> So the question is in FillH2CCmd_88E() should the connect/disconnect
> come before the mac_id as it currently does or should it only work that
> way on little endian systems and be reversed on big endian systems?
> My feeling is that the second option makes no sense so this patch is not
> correct.
> 
> Instead what should happen is:
> 
> -void rtl8188e_set_FwMediaStatus_cmd(struct adapter *adapt, __le16 mstatus_rpt)
> +void rtl8188e_set_FwMediaStatus_cmd(struct adapter *adapt, u16 mstatus_rpt)
>   {
> -	u16 mst_rpt = le16_to_cpu(mstatus_rpt);
> +	__le16 mst_rpt = cpu_to_le16(mstatus_rpt);
> 
> But this is just my intuition and I don't know this hardware.

I agree with Dan. This patch was attacking the symptom, not the cause. The 
original Realtek driver confuses cpu_to_le16() with le16_to_cpu() in many 
places. Apparently, I missed this one. On little-endian systems, both are 
no-ops, thus it does not matter.

I have a big-endian system (PowerBook G4) and will test later today.

Larry

Re: [PATCH] staging: r8188eu: Used u16 instead of __le16 in rtl8188e_set_FwMediaStatus_cmd()

[Gaurav Pathak]

Thanks Dan and Larry for looking into it.
I also don't have much understanding of the underlying hardware, please do let me know
of your tests on big-endin machine.

> On little-endian systems, both are no-ops, thus it does not matter
Does it mean that the patch is not required and the original implementation
is okay?

On Tue, Jan 10, 2023 at 11:00:35AM -0600, Larry Finger wrote:
> On 1/10/23 08:17, Dan Carpenter wrote:
> > On Tue, Jan 10, 2023 at 07:20:58PM +0530, Gaurav Pathak wrote:
> > > - Changed 2nd argument from __le16 to u16 to fix sparse warning
> > >    "warning: incorrect type in argument 2 (different base types)"
> > > - Removed le16_to_cpu() in staging/r8188eu/hal/rtl8188e_cmd.c
> > > 
> > > Signed-off-by: Gaurav Pathak <gauravpathak129@gmail.com>
> > > ---
> > >   drivers/staging/r8188eu/hal/rtl8188e_cmd.c     | 4 ++--
> > >   drivers/staging/r8188eu/include/rtl8188e_cmd.h | 2 +-
> > >   2 files changed, 3 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/staging/r8188eu/hal/rtl8188e_cmd.c b/drivers/staging/r8188eu/hal/rtl8188e_cmd.c
> > > index 8310d7f53982..a055e71d30ae 100644
> > > --- a/drivers/staging/r8188eu/hal/rtl8188e_cmd.c
> > > +++ b/drivers/staging/r8188eu/hal/rtl8188e_cmd.c
> > > @@ -193,9 +193,9 @@ void rtl8188e_set_FwPwrMode_cmd(struct adapter *adapt, u8 Mode)
> > >   }
> > > -void rtl8188e_set_FwMediaStatus_cmd(struct adapter *adapt, __le16 mstatus_rpt)
> > > +void rtl8188e_set_FwMediaStatus_cmd(struct adapter *adapt, u16 mstatus_rpt)
> > >   {
> > > -	u16 mst_rpt = le16_to_cpu(mstatus_rpt);
> > > +	u16 mst_rpt = mstatus_rpt;
> > 
> > You are changing the behavior of the code here for big endian systems.
> > Either the change is good or bad.  TLDR; I suspect the change is bad but
> > I don't know.
> > 
> > The mstatus_rpt is CPU endian.  It is the one of two things for connect
> > or disconnect:
> > 
> >     connect: (psta->mac_id << 8) | 1
> > disconnect: (psta->mac_id << 8) | 0
> > 
> > So the question is in FillH2CCmd_88E() should the connect/disconnect
> > come before the mac_id as it currently does or should it only work that
> > way on little endian systems and be reversed on big endian systems?
> > My feeling is that the second option makes no sense so this patch is not
> > correct.
> > 
> > Instead what should happen is:
> > 
> > -void rtl8188e_set_FwMediaStatus_cmd(struct adapter *adapt, __le16 mstatus_rpt)
> > +void rtl8188e_set_FwMediaStatus_cmd(struct adapter *adapt, u16 mstatus_rpt)
> >   {
> > -	u16 mst_rpt = le16_to_cpu(mstatus_rpt);
> > +	__le16 mst_rpt = cpu_to_le16(mstatus_rpt);
> > 
> > But this is just my intuition and I don't know this hardware.
> 
> I agree with Dan. This patch was attacking the symptom, not the cause. The
> original Realtek driver confuses cpu_to_le16() with le16_to_cpu() in many
> places. Apparently, I missed this one. On little-endian systems, both are
> no-ops, thus it does not matter.
> 
> I have a big-endian system (PowerBook G4) and will test later today.
> 
> Larry
> 
> 

Re: [PATCH] staging: r8188eu: Used u16 instead of __le16 in rtl8188e_set_FwMediaStatus_cmd()

[Larry Finger]

[-- Attachment #1: Type: text/plain, Size: 862 bytes --]

On 1/10/23 07:50, Gaurav Pathak wrote:
> - Changed 2nd argument from __le16 to u16 to fix sparse warning
>    "warning: incorrect type in argument 2 (different base types)"
> - Removed le16_to_cpu() in staging/r8188eu/hal/rtl8188e_cmd.c

The first change was correct; however the second was wrong. The 
FillH2CCmd_88E(adapt() call sends information from the host to the computer on 
the card, which must be little endian, but the argument to the routine is in CPU 
order. See the commit message in the attached patch.

I have no interest in submitting that patch upstream, but you might find it an 
interesting read when you prepare V2 of this patch. Of particular note is that 
even though the semantics were wrong, the previous code worked correctly because 
cpu_to_le16() and le16_to_cpu() both byte swap a 16-bit word and their results 
are the same.

Larry

[-- Attachment #2: 0001-staging-r8188eu-Fix-some-endian-problems.patch --]
[-- Type: text/x-patch, Size: 3623 bytes --]

From 1aa7b9e5e6c02b48cc0e2c487b9b791299921af3 Mon Sep 17 00:00:00 2001
From: Larry Finger <Larry.Finger@lwfinger.net>
Date: Thu, 12 Jan 2023 21:38:13 -0600
Subject: [PATCH] staging: r8188eu: Fix some endian problems
To: gregkh@linuxfoundation.org
Cc: phil@philpotter.co.uk

Sparse lists the following warnings:

  CHECK   drivers/staging/r8188eu/core/rtw_mlme.c
drivers/staging/r8188eu/core/rtw_mlme.c:1197:49: warning: incorrect type in
					 argument 2 (different base types)
drivers/staging/r8188eu/core/rtw_mlme.c:1197:49:    expected restricted
					 __le16 [usertype] mstatus_rpt
drivers/staging/r8188eu/core/rtw_mlme.c:1197:49:    got unsigned short
					 [assigned] [usertype] media_status_rpt
drivers/staging/r8188eu/core/rtw_mlme.c:1275:57: warning: incorrect type in
					 argument 2 (different base types)
drivers/staging/r8188eu/core/rtw_mlme.c:1275:57:    expected restricted
					 __le16 [usertype] mstatus_rpt
drivers/staging/r8188eu/core/rtw_mlme.c:1275:57:    got unsigned short
					 [assigned] [usertype] media_status
  CHECK   drivers/staging/r8188eu/core/rtw_mlme_ext.c
drivers/staging/r8188eu/core/rtw_mlme_ext.c:6842:58: warning: incorrect type
					 in argument 2 (different base types)
drivers/staging/r8188eu/core/rtw_mlme_ext.c:6842:58:    expected restricted
					 __le16 [usertype] mstatus_rpt
drivers/staging/r8188eu/core/rtw_mlme_ext.c:6842:58:    got unsigned short
					 [assigned] [usertype] media_status

The second argument of rtl8188e_set_FwMediaStatus_cmd() needs to be in CPU
order, not little-endian; however, when it uses that value to call
FillH2CCmd_88E() the parameter must be in little-endian order as that
value will be sent to the firmware. Note that the conversion from LE to CPU
order was le16_to_cpu() rather than the correct cpu_to_le16.

The definition of FillH2CCmd_88E() is revised, and the proper conversion
routine is used.

Note that the original code performed one byte swap on the secong argument
of FillH2CCmd_88E(), and got the correct answer even though the semantics
were very wrong.

Signed-off-by: Larry Finger <Larry.Finger@lwfinger.net>
---
 drivers/staging/r8188eu/hal/rtl8188e_cmd.c     | 4 ++--
 drivers/staging/r8188eu/include/rtl8188e_cmd.h | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/r8188eu/hal/rtl8188e_cmd.c b/drivers/staging/r8188eu/hal/rtl8188e_cmd.c
index 8310d7f53982..788904d4655c 100644
--- a/drivers/staging/r8188eu/hal/rtl8188e_cmd.c
+++ b/drivers/staging/r8188eu/hal/rtl8188e_cmd.c
@@ -193,9 +193,9 @@ void rtl8188e_set_FwPwrMode_cmd(struct adapter *adapt, u8 Mode)
 
 }
 
-void rtl8188e_set_FwMediaStatus_cmd(struct adapter *adapt, __le16 mstatus_rpt)
+void rtl8188e_set_FwMediaStatus_cmd(struct adapter *adapt, u16 mstatus_rpt)
 {
-	u16 mst_rpt = le16_to_cpu(mstatus_rpt);
+	__le16 mst_rpt = cpu_to_le16(mstatus_rpt);
 
 	FillH2CCmd_88E(adapt, H2C_COM_MEDIA_STATUS_RPT, sizeof(mst_rpt), (u8 *)&mst_rpt);
 }
diff --git a/drivers/staging/r8188eu/include/rtl8188e_cmd.h b/drivers/staging/r8188eu/include/rtl8188e_cmd.h
index 1e01c1662f9a..c785cf8ed683 100644
--- a/drivers/staging/r8188eu/include/rtl8188e_cmd.h
+++ b/drivers/staging/r8188eu/include/rtl8188e_cmd.h
@@ -85,6 +85,6 @@ void rtl8188e_Add_RateATid(struct adapter *padapter, u32 bitmap, u8 arg,
 void rtl8188e_set_p2p_ps_offload_cmd(struct adapter *adapt, u8 p2p_ps_state);
 
 void CheckFwRsvdPageContent(struct adapter *adapt);
-void rtl8188e_set_FwMediaStatus_cmd(struct adapter *adapt, __le16 mstatus_rpt);
+void rtl8188e_set_FwMediaStatus_cmd(struct adapter *adapt, u16 mstatus_rpt);
 
 #endif/* __RTL8188E_CMD_H__ */
-- 
2.39.0