staging: r8188eu: fix sparse warnings

PATCH In review Aakash Hemadri 4 participants 11 messages Aug 28, 2021 – Sep 3, 2021 Staging

lore

Share & tools

lore

Aakash Hemadri

author

2021-08-28 11:10 UTC #0 lore raw

v5 cover staging: r8188eu: fix sparse warnings

Hi,
This patch series fixes some sparse warnings in rtw_br_ext.c

Changes in v4 -> v5
- Dropped two patches as they have already been fixed by Larry's changes to endian sensitive variables in struct dhcpMessage

Changes in v3 -> v4
- Added this changelog, as requested by Greg's patch bot

Changes in v2 -> v3
- Fixed incorrect usage/removal of endian swaps and checks

Changes in v1 -> v2
- Split patch

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

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

base-commit: 4adb389e08c95fdf91995271932c59250ff0d561

Signature · 1 line

2.32.0

Aakash Hemadri

author

2021-08-28 11:10 UTC #1 lore raw

PATCH v5 1/3 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 be16_to_cpu()

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 62a672243696..aa56cd1a8490 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 = be16_to_cpu(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-28 11:10 UTC #3 lore raw

PATCH v5 2/3 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 be16_to_cpu()

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 aa56cd1a8490..38f1bd591da9 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,
+										be16_to_cpu(iph->payload_len),
 										IPPROTO_ICMPV6,
-										csum_partial((__u8 *)hdr, iph->payload_len, 0));
+										csum_partial((__u8 *)hdr, be16_to_cpu(iph->payload_len), 0));
 					}
 				}
 			}

Signature · 1 line

2.32.0

Aakash Hemadri

author

2021-08-28 11:10 UTC #7 lore raw

PATCH v5 3/3 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 holds __be16 change it's type to __be16

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 38f1bd591da9..bce73e8cbd52 100644
@@ -474,7 +474,7 @@ int nat25_db_handle(struct adapter *priv, struct sk_buff *skb, int method)
 		/*                Handle PPPoE frame                 */
 		/*---------------------------------------------------*/
 		struct pppoe_hdr *ph = (struct pppoe_hdr *)(skb->data + ETH_HLEN);
-		unsigned short *pMagic;
+		__be16 *pMagic;
 
 		switch (method) {
 		case NAT25_CHECK:

@@ -512,7 +512,7 @@ int nat25_db_handle(struct adapter *priv, struct sk_buff *skb, int method)
 						tag->tag_len = htons(MAGIC_CODE_LEN+RTL_RELAY_TAG_LEN+old_tag_len);
 
 						/*  insert the magic_code+client mac in relay tag */
-						pMagic = (unsigned short *)tag->tag_data;
+						pMagic = (__be16 *)tag->tag_data;
 						*pMagic = htons(MAGIC_CODE);
 						memcpy(tag->tag_data+MAGIC_CODE_LEN, skb->data+ETH_ALEN, ETH_ALEN);

Signature · 1 line

2.32.0

Kari Argillander

2021-08-28 11:37 UTC #4 lore raw

Re: [PATCH v5 2/3] staging: r8188eu: incorrect type in csum_ipv6_magic

In reply to Aakash Hemadri

[PATCH v5 2/3] staging: r8188eu: incorrect type in csum_ipv6_magic

Subject should start with imperative mood. Every patch in this series needs this fix.

I actually check linux coding style and did not found mention about this, but if you look kernel log then you see that it is at least unwritten rule. You can check this if you want
https://chris.beams.io/posts/git-commit/#imperative

Quoted 34 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 be16_to_cpu()
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 aa56cd1a8490..38f1bd591da9 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,
+										be16_to_cpu(iph->payload_len),
 										IPPROTO_ICMPV6,
-										csum_partial((__u8 *)hdr, iph->payload_len, 0));
+										csum_partial((__u8 *)hdr, be16_to_cpu(iph->payload_len), 0));
 					}
 				}
 			}
Signature · 1 line
2.32.0

Dan Carpenter

2021-08-30 10:39 UTC #5 lore raw

Re: [PATCH v5 2/3] staging: r8188eu: incorrect type in csum_ipv6_magic

Quoted 9 lines · Kari Argillander

[PATCH v5 2/3] staging: r8188eu: incorrect type in csum_ipv6_magic
Subject should start with imperative mood. Every patch in this series needs this fix.
I actually check linux coding style and did not found mention about this, but if you look kernel log then you see that it is at least unwritten rule. You can check this if you want
https://chris.beams.io/posts/git-commit/#imperative

The imperitive rule is stupid. We're dealing with thousands of people and lots of them don't speak English as a first language and it's hard to write commit messages. Let's not make it harder than it already is.

If you look through the commit messages on this list people are like. "Blah blah blah. Fix it!" I like that approach because it shows how humanity can triumph over pointless beaucrats. But in terms of understanding the commit adding "Fix it!" doesn't add any value.

The only thing which matters if if the commit message has the correct information in an understandable way.

regards,
dan carpenter

Dan Carpenter

Reviewed-by on the v5 series

2021-08-30 10:52 UTC #10 lore raw

In reply to Aakash Hemadri

These patches are fine.

Reviewed-by Dan Carpenter <[email protected]>

regards,
dan carpenter

Greg Kroah-Hartman

maintainer

2021-09-02 09:25 UTC #2 lore raw

Re: [PATCH v5 1/3] staging: r8188eu: restricted __be16 degrades to int

In reply to Aakash Hemadri

Quoted 26 lines · Aakash Hemadri

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 be16_to_cpu()

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 62a672243696..aa56cd1a8490 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 = be16_to_cpu(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

If this change happens, that means the existing driver does not work at all on little-endian machines today? But that seems odd and wrong, are you sure this change is correct?

How did you test this?

thanks,

greg k-h

Greg Kroah-Hartman

maintainer

2021-09-02 09:27 UTC #6 lore raw

Re: [PATCH v5 2/3] staging: r8188eu: incorrect type in csum_ipv6_magic

In reply to Aakash Hemadri

Quoted 34 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 be16_to_cpu()
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 aa56cd1a8490..38f1bd591da9 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,
+										be16_to_cpu(iph->payload_len),
 										IPPROTO_ICMPV6,
-										csum_partial((__u8 *)hdr, iph->payload_len, 0));
+										csum_partial((__u8 *)hdr, be16_to_cpu(iph->payload_len), 0));
 					}
 				}
 			}
Signature · 1 line
2.32.0

Same question here, does this mean the driver is broken on little-endian systems today? This should be tested...

thanks,

greg k-h

Greg Kroah-Hartman

maintainer

2021-09-02 09:28 UTC #8 lore raw

Re: [PATCH v5 3/3] staging: r8188eu: incorrect type in assignment

In reply to Aakash Hemadri

Quoted 7 lines · 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 holds __be16 change it's type to __be16

I do not understand this sentence, what are you trying to say?

Remember, "it's" is short for "it is".

Quoted 19 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 +1 -1

index 38f1bd591da9..bce73e8cbd52 100644
@@ -474,7 +474,7 @@ int nat25_db_handle(struct adapter *priv, struct sk_buff *skb, int method)
 		/*                Handle PPPoE frame                 */
 		/*---------------------------------------------------*/
 		struct pppoe_hdr *ph = (struct pppoe_hdr *)(skb->data + ETH_HLEN);
-		unsigned short *pMagic;
+		__be16 *pMagic;
 
 		switch (method) {
 		case NAT25_CHECK:

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

Quoted 5 lines

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

@@ -512,7 +512,7 @@ int nat25_db_handle(struct adapter *priv, struct sk_buff *skb, int method)
 						tag->tag_len = htons(MAGIC_CODE_LEN+RTL_RELAY_TAG_LEN+old_tag_len);
 
 						/*  insert the magic_code+client mac in relay tag */
-						pMagic = (unsigned short *)tag->tag_data;
+						pMagic = (__be16 *)tag->tag_data;

Is this whole function ever actually called?

Can someone run a ftrace on the driver to see if this function ever runs, and if not, remove it?

thanks,

greg k-h

Dan Carpenter

2021-09-03 08:12 UTC #9 lore raw

Re: [PATCH v5 3/3] staging: r8188eu: incorrect type in assignment

Quoted 4 lines · Greg Kroah-Hartman

Quoted 3 lines · Aakash Hemadri

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

@@ -512,7 +512,7 @@ int nat25_db_handle(struct adapter *priv, struct sk_buff *skb, int method)
 						tag->tag_len = htons(MAGIC_CODE_LEN+RTL_RELAY_TAG_LEN+old_tag_len);

Quoted 9 lines

drivers/staging/r8188eu/core/rtw_br_ext.c int nat25_db_handle(struct adapter *priv, struct sk_buff *skb, int method) · line 515
Quoted 3 lines
drivers/staging/r8188eu/core/rtw_br_ext.c +1 -1
 						/*  insert the magic_code+client mac in relay tag */
-						pMagic = (unsigned short *)tag->tag_data;
+						pMagic = (__be16 *)tag->tag_data;
Is this whole function ever actually called?
Can someone run a ftrace on the driver to see if this function ever runs, and if not, remove it?

Yeah. That was my thinking when reviewing this patchset is that the NAT stuff is probably not well tested. It's only used for WIFI_STATION_STATE or WIFI_ADHOC_STATE.

I don't think it's dead code but having NAT code in the driver is the wrong layer probably?

regards,
dan carpenter