[PATCH v2 0/5] staging: r8188eu: fix sparse warnings

[PATCH v2 0/5] staging: r8188eu: fix sparse warnings

[Aakash Hemadri]

Hi,

This patch series fixes some sparse warnings in rtw_brr_ext.c

Aakash Hemadri (5):
  staging: r8188eu: restricted __be16 degrades to int
  staging: r8188eu: cast to restricted __be32
  staging: r8188eu: incorrect type in csum_ipv6_magic
  staging: r8188eu: restricted __be16 degrades to int
  staging: r8188eu: incorrect type in assignment

 drivers/staging/r8188eu/core/rtw_br_ext.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)


base-commit: 093991aaadf0fbb34184fa37a46e7a157da3f386
-- 
2.32.0

[PATCH v2 1/5] staging: r8188eu: restricted __be16 degrades to int

[Aakash Hemadri]

Fix sparse warning:
> rtw_br_ext.c:73:23: warning: restricted __be16 degrades to integer

Here tag->tag_len is be16, use ntohs()

Signed-off-by: Aakash Hemadri <aakashhemadri123@gmail.com>
---
 drivers/staging/r8188eu/core/rtw_br_ext.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/r8188eu/core/rtw_br_ext.c b/drivers/staging/r8188eu/core/rtw_br_ext.c
index ee52f28a1e56..404fa8904e47 100644
--- a/drivers/staging/r8188eu/core/rtw_br_ext.c
+++ b/drivers/staging/r8188eu/core/rtw_br_ext.c
@@ -70,7 +70,7 @@ static int __nat25_add_pppoe_tag(struct sk_buff *skb, struct pppoe_tag *tag)
 	struct pppoe_hdr *ph = (struct pppoe_hdr *)(skb->data + ETH_HLEN);
 	int data_len;
 
-	data_len = tag->tag_len + TAG_HDR_LEN;
+	data_len = ntohs(tag->tag_len) + TAG_HDR_LEN;
 	if (skb_tailroom(skb) < data_len) {
 		_DEBUG_ERR("skb_tailroom() failed in add SID tag!\n");
 		return -1;
-- 
2.32.0

[PATCH v2 2/5] staging: r8188eu: cast to restricted __be32

[Aakash Hemadri]

Fix sparse warning:
> rtw_br_ext.c:836:54: warning: cast to restricted __be32

Unnecessary double cast, remove them.

Signed-off-by: Aakash Hemadri <aakashhemadri123@gmail.com>
---
 drivers/staging/r8188eu/core/rtw_br_ext.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/r8188eu/core/rtw_br_ext.c b/drivers/staging/r8188eu/core/rtw_br_ext.c
index 404fa8904e47..6a0462ce6230 100644
--- a/drivers/staging/r8188eu/core/rtw_br_ext.c
+++ b/drivers/staging/r8188eu/core/rtw_br_ext.c
@@ -671,7 +671,7 @@ void dhcp_flag_bcast(struct adapter *priv, struct sk_buff *skb)
 				    (udph->dest == __constant_htons(SERVER_PORT))) { /*  DHCP request */
 					struct dhcpMessage *dhcph =
 						(struct dhcpMessage *)((size_t)udph + sizeof(struct udphdr));
-					u32 cookie = be32_to_cpu((__be32)dhcph->cookie);
+					u32 cookie = dhcph->cookie;
 
 					if (cookie == DHCP_MAGIC) { /*  match magic word */
 						if (!(dhcph->flags & htons(BROADCAST_FLAG))) {
-- 
2.32.0

Re: [PATCH v2 2/5] staging: r8188eu: cast to restricted __be32

[Fabio M. De Francesco]

On Thursday, August 19, 2021 10:17:54 AM CEST Aakash Hemadri wrote:
> Fix sparse warning:
> > rtw_br_ext.c:836:54: warning: cast to restricted __be32
> 
> Unnecessary double cast, remove them.

Are you sure that you had a *double* cast?
In the line that you changed I see only a cast and a swap 
(or, better, a potential swap).

Regards,

Fabio

> 
> Signed-off-by: Aakash Hemadri <aakashhemadri123@gmail.com>
> ---
>  drivers/staging/r8188eu/core/rtw_br_ext.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/r8188eu/core/rtw_br_ext.c b/drivers/staging/r8188eu/core/rtw_br_ext.c
> index 404fa8904e47..6a0462ce6230 100644
> --- a/drivers/staging/r8188eu/core/rtw_br_ext.c
> +++ b/drivers/staging/r8188eu/core/rtw_br_ext.c
> @@ -671,7 +671,7 @@ void dhcp_flag_bcast(struct adapter *priv, struct sk_buff *skb)
>  				    (udph->dest == __constant_htons(SERVER_PORT))) { /*  DHCP request */
>  					struct dhcpMessage *dhcph =
>  						(struct dhcpMessage *)((size_t)udph + sizeof(struct udphdr));
> -					u32 cookie = be32_to_cpu((__be32)dhcph->cookie);
> +					u32 cookie = dhcph->cookie;
>  
>  					if (cookie == DHCP_MAGIC) { /*  match magic word */
>  						if (!(dhcph->flags & htons(BROADCAST_FLAG))) {
> -- 
> 2.32.0
> 
> 
> 

Re: [PATCH v2 2/5] staging: r8188eu: cast to restricted __be32

[Greg Kroah-Hartman]

On Thu, Aug 19, 2021 at 01:47:54PM +0530, Aakash Hemadri wrote:
> Fix sparse warning:
> > rtw_br_ext.c:836:54: warning: cast to restricted __be32
> 
> Unnecessary double cast, remove them.
> 
> Signed-off-by: Aakash Hemadri <aakashhemadri123@gmail.com>
> ---
>  drivers/staging/r8188eu/core/rtw_br_ext.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/r8188eu/core/rtw_br_ext.c b/drivers/staging/r8188eu/core/rtw_br_ext.c
> index 404fa8904e47..6a0462ce6230 100644
> --- a/drivers/staging/r8188eu/core/rtw_br_ext.c
> +++ b/drivers/staging/r8188eu/core/rtw_br_ext.c
> @@ -671,7 +671,7 @@ void dhcp_flag_bcast(struct adapter *priv, struct sk_buff *skb)
>  				    (udph->dest == __constant_htons(SERVER_PORT))) { /*  DHCP request */
>  					struct dhcpMessage *dhcph =
>  						(struct dhcpMessage *)((size_t)udph + sizeof(struct udphdr));
> -					u32 cookie = be32_to_cpu((__be32)dhcph->cookie);
> +					u32 cookie = dhcph->cookie;

Wait, what?  The cookie was in big endian format, and now you just
ignore it?  Why is this ok?  This breaks the code, have you tested this?

thanks,

greg k-h

Re: [PATCH v2 2/5] staging: r8188eu: cast to restricted __be32

[Aakash Hemadri]

On 21/08/19 07:19PM, Greg Kroah-Hartman wrote:
> On Thu, Aug 19, 2021 at 01:47:54PM +0530, Aakash Hemadri wrote:
> > Fix sparse warning:
> > > rtw_br_ext.c:836:54: warning: cast to restricted __be32
> > 
> > Unnecessary double cast, remove them.
> > 
> > Signed-off-by: Aakash Hemadri <aakashhemadri123@gmail.com>
> > ---
> >  drivers/staging/r8188eu/core/rtw_br_ext.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/staging/r8188eu/core/rtw_br_ext.c b/drivers/staging/r8188eu/core/rtw_br_ext.c
> > index 404fa8904e47..6a0462ce6230 100644
> > --- a/drivers/staging/r8188eu/core/rtw_br_ext.c
> > +++ b/drivers/staging/r8188eu/core/rtw_br_ext.c
> > @@ -671,7 +671,7 @@ void dhcp_flag_bcast(struct adapter *priv, struct sk_buff *skb)
> >  				    (udph->dest == __constant_htons(SERVER_PORT))) { /*  DHCP request */
> >  					struct dhcpMessage *dhcph =
> >  						(struct dhcpMessage *)((size_t)udph + sizeof(struct udphdr));
> > -					u32 cookie = be32_to_cpu((__be32)dhcph->cookie);
> > +					u32 cookie = dhcph->cookie;
> 
> Wait, what?  The cookie was in big endian format, and now you just
> ignore it?  Why is this ok?  This breaks the code, have you tested this?

Ah, I assumed clearing a sparse warning was enough to make sure my
change was correct. My understanding of endianness is incorrect.
Will redo this.

Thanks,
Aakash Hemadri

Re: [PATCH v2 2/5] staging: r8188eu: cast to restricted __be32

[Larry Finger]

On 8/19/21 3:17 AM, Aakash Hemadri wrote:
> Fix sparse warning:
>> rtw_br_ext.c:836:54: warning: cast to restricted __be32
> 
> Unnecessary double cast, remove them.
> 
> Signed-off-by: Aakash Hemadri <aakashhemadri123@gmail.com>
> ---
>   drivers/staging/r8188eu/core/rtw_br_ext.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/r8188eu/core/rtw_br_ext.c b/drivers/staging/r8188eu/core/rtw_br_ext.c
> index 404fa8904e47..6a0462ce6230 100644
> --- a/drivers/staging/r8188eu/core/rtw_br_ext.c
> +++ b/drivers/staging/r8188eu/core/rtw_br_ext.c
> @@ -671,7 +671,7 @@ void dhcp_flag_bcast(struct adapter *priv, struct sk_buff *skb)
>   				    (udph->dest == __constant_htons(SERVER_PORT))) { /*  DHCP request */
>   					struct dhcpMessage *dhcph =
>   						(struct dhcpMessage *)((size_t)udph + sizeof(struct udphdr));
> -					u32 cookie = be32_to_cpu((__be32)dhcph->cookie);
> +					u32 cookie = dhcph->cookie;
>   
>   					if (cookie == DHCP_MAGIC) { /*  match magic word */
>   						if (!(dhcph->flags & htons(BROADCAST_FLAG))) {
> 

This patch is wrong. All the documentation I could find tells me that the 
multi-byte entries in dhcph are big-endian, thus the new line should read:

					u32 cookie = be32_to_cpu(dhcph->cookie);
combined with:

@@ -649,7 +650,7 @@ struct dhcpMessage {
         u_int8_t chaddr[16];
         u_int8_t sname[64];
         u_int8_t file[128];
-       u_int32_t cookie;
+       __be32 cookie;
         u_int8_t options[308]; /* 312 - cookie */
  };

The old code was, in fact, correct, but not in a way that satisfied Sparse.

Larry

Re: [PATCH v2 2/5] staging: r8188eu: cast to restricted __be32

[Aakash Hemadri]

On 21/08/20 04:44PM, Larry Finger wrote:
> This patch is wrong. All the documentation I could find tells me that the
> multi-byte entries in dhcph are big-endian, thus the new line should read:
> 
> 					u32 cookie = be32_to_cpu(dhcph->cookie);
> combined with:
> 
> @@ -649,7 +650,7 @@ struct dhcpMessage {
>         u_int8_t chaddr[16];
>         u_int8_t sname[64];
>         u_int8_t file[128];
> -       u_int32_t cookie;
> +       __be32 cookie;
>         u_int8_t options[308]; /* 312 - cookie */
>  };
> 
> The old code was, in fact, correct, but not in a way that satisfied Sparse.
> 
> Larry

Thanks for the review Larry. I understand now, will rework and send it
through

Thanks,
Aakash Hemadri

RE: [PATCH v2 2/5] staging: r8188eu: cast to restricted __be32

[David Laight]

From: Larry Finger
> Sent: 20 August 2021 22:45
> 
> On 8/19/21 3:17 AM, Aakash Hemadri wrote:
> > Fix sparse warning:
> >> rtw_br_ext.c:836:54: warning: cast to restricted __be32
> >
> > Unnecessary double cast, remove them.
> >
> > Signed-off-by: Aakash Hemadri <aakashhemadri123@gmail.com>
> > ---
> >   drivers/staging/r8188eu/core/rtw_br_ext.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/staging/r8188eu/core/rtw_br_ext.c b/drivers/staging/r8188eu/core/rtw_br_ext.c
> > index 404fa8904e47..6a0462ce6230 100644
> > --- a/drivers/staging/r8188eu/core/rtw_br_ext.c
> > +++ b/drivers/staging/r8188eu/core/rtw_br_ext.c
> > @@ -671,7 +671,7 @@ void dhcp_flag_bcast(struct adapter *priv, struct sk_buff *skb)
> >   				    (udph->dest == __constant_htons(SERVER_PORT))) { /*  DHCP request */
> >   					struct dhcpMessage *dhcph =
> >   						(struct dhcpMessage *)((size_t)udph + sizeof(struct udphdr));
> > -					u32 cookie = be32_to_cpu((__be32)dhcph->cookie);
> > +					u32 cookie = dhcph->cookie;
> >
> >   					if (cookie == DHCP_MAGIC) { /*  match magic word */
> >   						if (!(dhcph->flags & htons(BROADCAST_FLAG))) {
> >
> 
> This patch is wrong. All the documentation I could find tells me that the
> multi-byte entries in dhcph are big-endian, thus the new line should read:
> 
> 					u32 cookie = be32_to_cpu(dhcph->cookie);

Modulo anything that really means it should get_unaligned_be32().

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [PATCH v2 2/5] staging: r8188eu: cast to restricted __be32

[Aakash Hemadri]

On 21/08/22 09:30PM, David Laight wrote:
> Modulo anything that really means it should get_unaligned_be32().
> 
> 	David

Thanks for your reviews David!
Will make this change!

Thanks,
Aakash Hemadri

Re: [PATCH v2 2/5] staging: r8188eu: cast to restricted __be32

[Larry Finger]

On 8/22/21 4:30 PM, David Laight wrote:
> From: Larry Finger
>> Sent: 20 August 2021 22:45
>> 					u32 cookie = be32_to_cpu(dhcph->cookie);
> 
> Modulo anything that really means it should get_unaligned_be32().

True, but cookie is 32-bit aligned, thus the code is correct.

Larry

[PATCH v2 3/5] staging: r8188eu: incorrect type in csum_ipv6_magic

[Aakash Hemadri]

Fix sparse warning:
> rtw_br_ext.c:771:84:    got restricted __be16 [usertype] payload_len
> rtw_br_ext.c:773:110: warning: incorrect type in argument 2
    (different base types)
> rtw_br_ext.c:773:110:    expected int len
> rtw_br_ext.c:773:110:    got restricted __be16 [usertype] payload_len

csum_ipv6_magic and csum_partial expect int len not __be16, use ntohs()

Signed-off-by: Aakash Hemadri <aakashhemadri123@gmail.com>
---
 drivers/staging/r8188eu/core/rtw_br_ext.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/r8188eu/core/rtw_br_ext.c b/drivers/staging/r8188eu/core/rtw_br_ext.c
index 6a0462ce6230..d4acf02ca64f 100644
--- a/drivers/staging/r8188eu/core/rtw_br_ext.c
+++ b/drivers/staging/r8188eu/core/rtw_br_ext.c
@@ -615,9 +615,9 @@ int nat25_db_handle(struct adapter *priv, struct sk_buff *skb, int method)
 						struct icmp6hdr  *hdr = (struct icmp6hdr *)(skb->data + ETH_HLEN + sizeof(*iph));
 						hdr->icmp6_cksum = 0;
 						hdr->icmp6_cksum = csum_ipv6_magic(&iph->saddr, &iph->daddr,
-										iph->payload_len,
+										ntohs(iph->payload_len),
 										IPPROTO_ICMPV6,
-										csum_partial((__u8 *)hdr, iph->payload_len, 0));
+										csum_partial((__u8 *)hdr, ntohs(iph->payload_len), 0));
 					}
 				}
 			}
-- 
2.32.0

Re: [PATCH v2 3/5] staging: r8188eu: incorrect type in csum_ipv6_magic

[Larry Finger]

On 8/19/21 3:17 AM, Aakash Hemadri wrote:
> Fix sparse warning:
>> rtw_br_ext.c:771:84:    got restricted __be16 [usertype] payload_len
>> rtw_br_ext.c:773:110: warning: incorrect type in argument 2
>      (different base types)
>> rtw_br_ext.c:773:110:    expected int len
>> rtw_br_ext.c:773:110:    got restricted __be16 [usertype] payload_len
> 
> csum_ipv6_magic and csum_partial expect int len not __be16, use ntohs()
> 
> Signed-off-by: Aakash Hemadri <aakashhemadri123@gmail.com>
> ---
>   drivers/staging/r8188eu/core/rtw_br_ext.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/staging/r8188eu/core/rtw_br_ext.c b/drivers/staging/r8188eu/core/rtw_br_ext.c
> index 6a0462ce6230..d4acf02ca64f 100644
> --- a/drivers/staging/r8188eu/core/rtw_br_ext.c
> +++ b/drivers/staging/r8188eu/core/rtw_br_ext.c
> @@ -615,9 +615,9 @@ int nat25_db_handle(struct adapter *priv, struct sk_buff *skb, int method)
>   						struct icmp6hdr  *hdr = (struct icmp6hdr *)(skb->data + ETH_HLEN + sizeof(*iph));
>   						hdr->icmp6_cksum = 0;
>   						hdr->icmp6_cksum = csum_ipv6_magic(&iph->saddr, &iph->daddr,
> -										iph->payload_len,
> +										ntohs(iph->payload_len),
>   										IPPROTO_ICMPV6,
> -										csum_partial((__u8 *)hdr, iph->payload_len, 0));
> +										csum_partial((__u8 *)hdr, ntohs(iph->payload_len), 0));
>   					}
>   				}
>   			}
> 

This patch is correct, but I prefer that you use be16_to_cpu() rather than 
ntohs(). I think it makes the code easier to read.

Larry

Re: [PATCH v2 3/5] staging: r8188eu: incorrect type in csum_ipv6_magic

[Aakash Hemadri]

On 21/08/20 04:38PM, Larry Finger wrote:
> This patch is correct, but I prefer that you use be16_to_cpu() rather than
> ntohs(). I think it makes the code easier to read.

Sure Larry, Will use be16_to_cpu()

Thanks,
Aakash Hemadri

Re: [PATCH v2 3/5] staging: r8188eu: incorrect type in csum_ipv6_magic

[Larry Finger]

On 8/19/21 3:17 AM, Aakash Hemadri wrote:
> Fix sparse warning:
>> rtw_br_ext.c:771:84:    got restricted __be16 [usertype] payload_len
>> rtw_br_ext.c:773:110: warning: incorrect type in argument 2
>      (different base types)
>> rtw_br_ext.c:773:110:    expected int len
>> rtw_br_ext.c:773:110:    got restricted __be16 [usertype] payload_len
> 
> csum_ipv6_magic and csum_partial expect int len not __be16, use ntohs()
> 
> Signed-off-by: Aakash Hemadri <aakashhemadri123@gmail.com>
> ---
>   drivers/staging/r8188eu/core/rtw_br_ext.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/staging/r8188eu/core/rtw_br_ext.c b/drivers/staging/r8188eu/core/rtw_br_ext.c
> index 6a0462ce6230..d4acf02ca64f 100644
> --- a/drivers/staging/r8188eu/core/rtw_br_ext.c
> +++ b/drivers/staging/r8188eu/core/rtw_br_ext.c
> @@ -615,9 +615,9 @@ int nat25_db_handle(struct adapter *priv, struct sk_buff *skb, int method)
>   						struct icmp6hdr  *hdr = (struct icmp6hdr *)(skb->data + ETH_HLEN + sizeof(*iph));
>   						hdr->icmp6_cksum = 0;
>   						hdr->icmp6_cksum = csum_ipv6_magic(&iph->saddr, &iph->daddr,
> -										iph->payload_len,
> +										ntohs(iph->payload_len),
>   										IPPROTO_ICMPV6,
> -										csum_partial((__u8 *)hdr, iph->payload_len, 0));
> +										csum_partial((__u8 *)hdr, ntohs(iph->payload_len), 0));
>   					}
>   				}
>   			}
> 

This patch is correct. Again, I like be16_to_cpu() better than ntohs(), but that 
is not a deal breaker. The kernel is split on the usage.

Larry

[PATCH v2 4/5] staging: r8188eu: restricted __be16 degrades to int

[Aakash Hemadri]

Fix sparse warning:
> rtw_br_ext.c:839:70: warning: restricted __be16 degrades to integer
> rtw_br_ext.c:845:70: warning: invalid assignment: |=
> rtw_br_ext.c:845:70:    left side has type unsigned short
> rtw_br_ext.c:845:70:    right side has type restricted __be16

dhcp->flag is u16, remove htons() as __be16 degrades.

Signed-off-by: Aakash Hemadri <aakashhemadri123@gmail.com>
---
 drivers/staging/r8188eu/core/rtw_br_ext.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/r8188eu/core/rtw_br_ext.c b/drivers/staging/r8188eu/core/rtw_br_ext.c
index d4acf02ca64f..14b2935cab98 100644
--- a/drivers/staging/r8188eu/core/rtw_br_ext.c
+++ b/drivers/staging/r8188eu/core/rtw_br_ext.c
@@ -674,13 +674,13 @@ void dhcp_flag_bcast(struct adapter *priv, struct sk_buff *skb)
 					u32 cookie = dhcph->cookie;
 
 					if (cookie == DHCP_MAGIC) { /*  match magic word */
-						if (!(dhcph->flags & htons(BROADCAST_FLAG))) {
+						if (!(dhcph->flags & BROADCAST_FLAG)) {
 							/*  if not broadcast */
 							register int sum = 0;
 
 							DEBUG_INFO("DHCP: change flag of DHCP request to broadcast.\n");
 							/*  or BROADCAST flag */
-							dhcph->flags |= htons(BROADCAST_FLAG);
+							dhcph->flags |= BROADCAST_FLAG;
 							/*  recalculate checksum */
 							sum = ~(udph->check) & 0xffff;
 							sum += be16_to_cpu(dhcph->flags);
-- 
2.32.0

Re: [PATCH v2 4/5] staging: r8188eu: restricted __be16 degrades to int

[Greg Kroah-Hartman]

On Thu, Aug 19, 2021 at 01:47:56PM +0530, Aakash Hemadri wrote:
> Fix sparse warning:
> > rtw_br_ext.c:839:70: warning: restricted __be16 degrades to integer
> > rtw_br_ext.c:845:70: warning: invalid assignment: |=
> > rtw_br_ext.c:845:70:    left side has type unsigned short
> > rtw_br_ext.c:845:70:    right side has type restricted __be16
> 
> dhcp->flag is u16, remove htons() as __be16 degrades.

Um, are you sure?

> 
> Signed-off-by: Aakash Hemadri <aakashhemadri123@gmail.com>
> ---
>  drivers/staging/r8188eu/core/rtw_br_ext.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/staging/r8188eu/core/rtw_br_ext.c b/drivers/staging/r8188eu/core/rtw_br_ext.c
> index d4acf02ca64f..14b2935cab98 100644
> --- a/drivers/staging/r8188eu/core/rtw_br_ext.c
> +++ b/drivers/staging/r8188eu/core/rtw_br_ext.c
> @@ -674,13 +674,13 @@ void dhcp_flag_bcast(struct adapter *priv, struct sk_buff *skb)
>  					u32 cookie = dhcph->cookie;
>  
>  					if (cookie == DHCP_MAGIC) { /*  match magic word */
> -						if (!(dhcph->flags & htons(BROADCAST_FLAG))) {
> +						if (!(dhcph->flags & BROADCAST_FLAG)) {

So you now just ignore the fact that the code used to properly check
BROADCAST_FLAG being in big endian mode, and now you assume it is native
endian?

Why is this ok?  Did you test this?

thanks,

greg k-h

Re: [PATCH v2 4/5] staging: r8188eu: restricted __be16 degrades to int

[Fabio M. De Francesco]

On Thursday, August 19, 2021 7:20:44 PM CEST Greg Kroah-Hartman wrote:
> On Thu, Aug 19, 2021 at 01:47:56PM +0530, Aakash Hemadri wrote:
> > Fix sparse warning:
> > > rtw_br_ext.c:839:70: warning: restricted __be16 degrades to integer
> > > rtw_br_ext.c:845:70: warning: invalid assignment: |=
> > > rtw_br_ext.c:845:70:    left side has type unsigned short
> > > rtw_br_ext.c:845:70:    right side has type restricted __be16
> > 
> > dhcp->flag is u16, remove htons() as __be16 degrades.
> 
> Um, are you sure?
> 
> > 
> > Signed-off-by: Aakash Hemadri <aakashhemadri123@gmail.com>
> > ---
> >  drivers/staging/r8188eu/core/rtw_br_ext.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/staging/r8188eu/core/rtw_br_ext.c b/drivers/staging/r8188eu/core/rtw_br_ext.c
> > index d4acf02ca64f..14b2935cab98 100644
> > --- a/drivers/staging/r8188eu/core/rtw_br_ext.c
> > +++ b/drivers/staging/r8188eu/core/rtw_br_ext.c
> > @@ -674,13 +674,13 @@ void dhcp_flag_bcast(struct adapter *priv, struct sk_buff *skb)
> >  					u32 cookie = dhcph->cookie;
> >  
> >  					if (cookie == DHCP_MAGIC) { /*  match magic word */
> > -						if (!(dhcph->flags & htons(BROADCAST_FLAG))) {
> > +						if (!(dhcph->flags & BROADCAST_FLAG)) {
> 
> So you now just ignore the fact that the code used to properly check
> BROADCAST_FLAG being in big endian mode, and now you assume it is native
> endian?
> 
> Why is this ok?  Did you test this?
> 
> thanks,
> 
> greg k-h
> 
Aakash,

Building on the objections you had from Greg I suggest that, before attempting 
anew to address problems like these, you get a better understanding of the topics of 
native and network endianness and of the API that (conditionally) swap bytes 
in a variable between little endian and big endian representation.

To start with, please note that the following code leads to tests for "v.vub[0] == 0xDD" 
which is true on little endian architectures while "v.vub[0] == 0xAA" is true on big 
endian ones...

union {
        u32 vud;
        u8 vub[4];
} v;

v.vud = 0xAABBCCDD;

Also note that API like cpu_to_be32(), htonl(), be32_to_cpu(), ntohl, and the likes are 
used to (conditionally) swap bytes (i.e., change the arrangement of the bytes in a 
multi-bytes variable).

Casts have very different purposes and usage patterns and, above all, they cannot 
magically change the endianness of a variable.

Regards,

Fabio

Re: [PATCH v2 4/5] staging: r8188eu: restricted __be16 degrades to int

[Aakash Hemadri]

On 21/08/20 05:10PM, Fabio M. De Francesco wrote:
> Building on the objections you had from Greg I suggest that, before attempting 
> anew to address problems like these, you get a better understanding of the topics of 
> native and network endianness and of the API that (conditionally) swap bytes 
> in a variable between little endian and big endian representation.
> 
> To start with, please note that the following code leads to tests for "v.vub[0] == 0xDD" 
> which is true on little endian architectures while "v.vub[0] == 0xAA" is true on big 
> endian ones...
> 
> union {
>         u32 vud;
>         u8 vub[4];
> } v;
> 
> v.vud = 0xAABBCCDD;
> 
> Also note that API like cpu_to_be32(), htonl(), be32_to_cpu(), ntohl, and the likes are 
> used to (conditionally) swap bytes (i.e., change the arrangement of the bytes in a 
> multi-bytes variable).
> 
> Casts have very different purposes and usage patterns and, above all, they cannot 
> magically change the endianness of a variable.
> 
> Regards,
> 
> Fabio
> 

Thanks for the explanation Fabio!
Will rework and send it through!

Thanks,
Aakash Hemadri

[PATCH v2 5/5] staging: r8188eu: incorrect type in assignment

[Aakash Hemadri]

Fix sparse warning:
> rtw_br_ext.c:516:57: warning: incorrect type in assignment
    (different base types)
> rtw_br_ext.c:516:57:    expected unsigned short
> rtw_br_ext.c:516:57:    got restricted __be16 [usertype]

*pMagic expects unsigned short not __be16, so remove htons and cast
MAGIC_CODE as unsigned short

Signed-off-by: Aakash Hemadri <aakashhemadri123@gmail.com>
---
 drivers/staging/r8188eu/core/rtw_br_ext.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/r8188eu/core/rtw_br_ext.c b/drivers/staging/r8188eu/core/rtw_br_ext.c
index 14b2935cab98..600a38330579 100644
--- a/drivers/staging/r8188eu/core/rtw_br_ext.c
+++ b/drivers/staging/r8188eu/core/rtw_br_ext.c
@@ -513,7 +513,7 @@ int nat25_db_handle(struct adapter *priv, struct sk_buff *skb, int method)
 
 						/*  insert the magic_code+client mac in relay tag */
 						pMagic = (unsigned short *)tag->tag_data;
-						*pMagic = htons(MAGIC_CODE);
+						*pMagic = (unsigned short)MAGIC_CODE;
 						memcpy(tag->tag_data+MAGIC_CODE_LEN, skb->data+ETH_ALEN, ETH_ALEN);
 
 						/* Add relay tag */
-- 
2.32.0