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

[Dan Carpenter <error27@gmail.com>]

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