staging: r8188eu: fix sparse warnings

PATCH Superseded Aakash Hemadri 5 participants 20 messages Aug 19, 2021 – Aug 23, 2021 Staging

lore

Share & tools

lore

Aakash Hemadri

author

2021-08-19 08:18 UTC #0 lore raw

v2 cover staging: r8188eu: fix sparse warnings

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

Signature · 1 line

2.32.0

Aakash Hemadri

author

2021-08-19 08:18 UTC #1 lore raw

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

Fix sparse warning:

Quoted 1 line

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 <[email protected]>

---
 drivers/staging/r8188eu/core/rtw_br_ext.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/staging/r8188eu/core/rtw_br_ext.c +1 -1

index ee52f28a1e56..404fa8904e47 100644
@@ -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;

Signature · 1 line

2.32.0

Aakash Hemadri

author

2021-08-19 08:18 UTC #2 lore raw

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

In reply to Aakash Hemadri

Fix sparse warning:

Quoted 1 line

rtw_br_ext.c:836:54: warning: cast to restricted __be32

Unnecessary double cast, remove them.

Signed-off-by Aakash Hemadri <[email protected]>

---
 drivers/staging/r8188eu/core/rtw_br_ext.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/staging/r8188eu/core/rtw_br_ext.c +1 -1

index 404fa8904e47..6a0462ce6230 100644
@@ -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))) {

Signature · 1 line

2.32.0

Aakash Hemadri

author

2021-08-19 08:18 UTC #11 lore raw

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

In reply to Aakash Hemadri

Fix sparse warning:

Quoted 2 lines

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)

Quoted 2 lines

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 <[email protected]>

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

drivers/staging/r8188eu/core/rtw_br_ext.c +2 -2

index 6a0462ce6230..d4acf02ca64f 100644
@@ -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));
 					}
 				}
 			}

Signature · 1 line

2.32.0

Aakash Hemadri

author

2021-08-19 08:18 UTC #15 lore raw

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

In reply to Aakash Hemadri

Fix sparse warning:

Quoted 4 lines

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 <[email protected]>

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

drivers/staging/r8188eu/core/rtw_br_ext.c +2 -2

index d4acf02ca64f..14b2935cab98 100644
@@ -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);

Signature · 1 line

2.32.0

Aakash Hemadri

author

2021-08-19 08:18 UTC #19 lore raw

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

In reply to Aakash Hemadri

Fix sparse warning:

Quoted 1 line

rtw_br_ext.c:516:57: warning: incorrect type in assignment

(different base types)

Quoted 2 lines

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 <[email protected]>

---
 drivers/staging/r8188eu/core/rtw_br_ext.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/staging/r8188eu/core/rtw_br_ext.c +1 -1

index 14b2935cab98..600a38330579 100644
@@ -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 */

Signature · 1 line

2.32.0

Fabio M. De Francesco

2021-08-19 12:58 UTC #3 lore raw

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

In reply to Aakash Hemadri

Quoted 4 lines · CEST Aakash Hemadri

Fix sparse warning:
Quoted 1 line
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

Quoted 24 lines

Signed-off-by Aakash Hemadri <[email protected]>

---
 drivers/staging/r8188eu/core/rtw_br_ext.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/staging/r8188eu/core/rtw_br_ext.c +1 -1

index 404fa8904e47..6a0462ce6230 100644
@@ -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))) {

Signature · 1 line

2.32.0

Greg Kroah-Hartman

maintainer

2021-08-19 17:19 UTC #4 lore raw

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

In reply to Aakash Hemadri

Quoted 15 lines · Aakash Hemadri

Fix sparse warning:
Quoted 1 line
rtw_br_ext.c:836:54: warning: cast to restricted __be32
Unnecessary double cast, remove them.
Signed-off-by Aakash Hemadri <[email protected]>
---
 drivers/staging/r8188eu/core/rtw_br_ext.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/staging/r8188eu/core/rtw_br_ext.c +0 -0
index 404fa8904e47..6a0462ce6230 100644

drivers/staging/r8188eu/core/rtw_br_ext.c void dhcp_flag_bcast(struct adapter *priv, struct sk_buff *skb) · line 674

Quoted 5 lines

drivers/staging/r8188eu/core/rtw_br_ext.c +1 -1

@@ -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

Greg Kroah-Hartman

maintainer

2021-08-19 17:20 UTC #16 lore raw

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

In reply to Aakash Hemadri

Quoted 7 lines · Aakash Hemadri

Fix sparse warning:
Quoted 4 lines
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?

Quoted 11 lines

Signed-off-by Aakash Hemadri <[email protected]>
---
 drivers/staging/r8188eu/core/rtw_br_ext.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/staging/r8188eu/core/rtw_br_ext.c +0 -0
index d4acf02ca64f..14b2935cab98 100644

drivers/staging/r8188eu/core/rtw_br_ext.c void dhcp_flag_bcast(struct adapter *priv, struct sk_buff *skb) · line 677

Quoted 5 lines

drivers/staging/r8188eu/core/rtw_br_ext.c +1 -1

@@ -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 Hemadri

author

2021-08-20 11:40 UTC #5 lore raw

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

In reply to Greg Kroah-Hartman

Quoted 11 lines · Greg Kroah-Hartman

Quoted 10 lines · Aakash Hemadri
Fix sparse warning:
Quoted 1 line
rtw_br_ext.c:836:54: warning: cast to restricted __be32
Unnecessary double cast, remove them.
Signed-off-by Aakash Hemadri <[email protected]>
---
 drivers/staging/r8188eu/core/rtw_br_ext.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Quoted 13 lines

drivers/staging/r8188eu/core/rtw_br_ext.c void dhcp_flag_bcast(struct adapter *priv, struct sk_buff *skb) · line 674
Quoted 10 lines
drivers/staging/r8188eu/core/rtw_br_ext.c +1 -1
index 404fa8904e47..6a0462ce6230 100644
@@ -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

Fabio M. De Francesco

2021-08-20 15:10 UTC #17 lore raw

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

In reply to Greg Kroah-Hartman

Quoted 32 lines · CEST Greg Kroah-Hartman

Quoted 7 lines · Aakash Hemadri
Fix sparse warning:
Quoted 4 lines
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?
Quoted 11 lines
Signed-off-by Aakash Hemadri <[email protected]>
---
 drivers/staging/r8188eu/core/rtw_br_ext.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/staging/r8188eu/core/rtw_br_ext.c +0 -0
index d4acf02ca64f..14b2935cab98 100644
drivers/staging/r8188eu/core/rtw_br_ext.c void dhcp_flag_bcast(struct adapter *priv, struct sk_buff *skb) · line 677
Quoted 5 lines
drivers/staging/r8188eu/core/rtw_br_ext.c +1 -1
@@ -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?

Quoted 6 lines

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

Larry Finger

2021-08-20 21:38 UTC #12 lore raw

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

In reply to Aakash Hemadri

Quoted 21 lines · Aakash Hemadri

Fix sparse warning:
Quoted 2 lines
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)
Quoted 2 lines
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 <[email protected]>
---
  drivers/staging/r8188eu/core/rtw_br_ext.c | 4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)
drivers/staging/r8188eu/core/rtw_br_ext.c +0 -0
index 6a0462ce6230..d4acf02ca64f 100644
@@ -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));

drivers/staging/r8188eu/core/rtw_br_ext.c int nat25_db_handle(struct adapter *priv, struct sk_buff *skb, int method) · line 623

Quoted 10 lines

drivers/staging/r8188eu/core/rtw_br_ext.c +2 -2

  						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

Larry Finger

2021-08-20 21:44 UTC #6 lore raw

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

In reply to Aakash Hemadri

Quoted 16 lines · Aakash Hemadri

Fix sparse warning:
Quoted 1 line
rtw_br_ext.c:836:54: warning: cast to restricted __be32
Unnecessary double cast, remove them.
Signed-off-by Aakash Hemadri <[email protected]>
---
  drivers/staging/r8188eu/core/rtw_br_ext.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)
drivers/staging/r8188eu/core/rtw_br_ext.c +0 -0
index 404fa8904e47..6a0462ce6230 100644

drivers/staging/r8188eu/core/rtw_br_ext.c void dhcp_flag_bcast(struct adapter *priv, struct sk_buff *skb) · line 677

Quoted 8 lines

drivers/staging/r8188eu/core/rtw_br_ext.c +1 -1

@@ -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

Larry Finger

2021-08-20 21:48 UTC #14 lore raw

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

In reply to Aakash Hemadri

Quoted 21 lines · Aakash Hemadri

Fix sparse warning:
Quoted 2 lines
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)
Quoted 2 lines
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 <[email protected]>
---
  drivers/staging/r8188eu/core/rtw_br_ext.c | 4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)
drivers/staging/r8188eu/core/rtw_br_ext.c +0 -0
index 6a0462ce6230..d4acf02ca64f 100644
@@ -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));

drivers/staging/r8188eu/core/rtw_br_ext.c int nat25_db_handle(struct adapter *priv, struct sk_buff *skb, int method) · line 623

Quoted 10 lines

drivers/staging/r8188eu/core/rtw_br_ext.c +2 -2

  						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

Aakash Hemadri

author

2021-08-21 14:18 UTC #18 lore raw

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

In reply to Fabio M. De Francesco

Quoted 20 lines · Fabio M. De Francesco

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).

Quoted 7 lines

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

Aakash Hemadri

author

2021-08-21 14:19 UTC #13 lore raw

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

In reply to Larry Finger

Quoted 2 lines · Larry Finger

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

Aakash Hemadri

author

2021-08-21 14:21 UTC #7 lore raw

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

In reply to Larry Finger

Quoted 17 lines · Larry Finger

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:
(quoted diff) +1 -1
@@ -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.

Quoted 1 line

Larry

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

Thanks,
Aakash Hemadri

David Laight

2021-08-22 21:30 UTC #8 lore raw

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

In reply to Larry Finger

From: Larry Finger

Quoted 24 lines

Sent: 20 August 2021 22:45
Quoted 21 lines · Aakash Hemadri
Fix sparse warning:
Quoted 1 line
rtw_br_ext.c:836:54: warning: cast to restricted __be32
Unnecessary double cast, remove them.
Signed-off-by Aakash Hemadri <[email protected]>
---
  drivers/staging/r8188eu/core/rtw_br_ext.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)
drivers/staging/r8188eu/core/rtw_br_ext.c +1 -1
index 404fa8904e47..6a0462ce6230 100644
@@ -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;
 

Quoted 8 lines

drivers/staging/r8188eu/core/rtw_br_ext.c void dhcp_flag_bcast(struct adapter *priv, struct sk_buff *skb) · line 677
Quoted 3 lines
drivers/staging/r8188eu/core/rtw_br_ext.c +0 -0
  					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)

Aakash Hemadri

author

2021-08-23 08:26 UTC #9 lore raw

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

Quoted 3 lines · David Laight

Modulo anything that really means it should get_unaligned_be32().
David

Thanks for your reviews David!
Will make this change!

Thanks,
Aakash Hemadri

Larry Finger

2021-08-23 14:19 UTC #10 lore raw

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

In reply to David Laight

Quoted 5 lines · David Laight

From: Larry Finger
Quoted 2 lines
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