[PATCH v5 0/3] staging: r8188eu: fix sparse warnings

[PATCH v5 0/3] staging: r8188eu: fix sparse warnings

[Aakash Hemadri]

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

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

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 62a672243696..aa56cd1a8490 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 = 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;
-- 
2.32.0

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

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 aa56cd1a8490..38f1bd591da9 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,
+										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));
 					}
 				}
 			}
-- 
2.32.0

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

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 38f1bd591da9..bce73e8cbd52 100644
--- a/drivers/staging/r8188eu/core/rtw_br_ext.c
+++ b/drivers/staging/r8188eu/core/rtw_br_ext.c
@@ -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);
 
-- 
2.32.0

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

[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

On Sat, Aug 28, 2021 at 04:40:45PM +0530, 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
> be16_to_cpu()
> 
> 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 aa56cd1a8490..38f1bd591da9 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,
> +										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));
>  					}
>  				}
>  			}
> -- 
> 2.32.0
> 

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

[Dan Carpenter]

On Sat, Aug 28, 2021 at 02:37:26PM +0300, Kari Argillander wrote:
> [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

Re: [PATCH v5 0/3] staging: r8188eu: fix sparse warnings

[Dan Carpenter]

These patches are fine.

Reviewed-by: Dan Carpenter <dan.carpenter@oracle.com>

regards,
dan carpenter

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

[Greg Kroah-Hartman]

On Sat, Aug 28, 2021 at 04:40:44PM +0530, Aakash Hemadri wrote:
> Fix sparse warning:
> > 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 <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 62a672243696..aa56cd1a8490 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 = 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;
> -- 
> 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

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

[Greg Kroah-Hartman]

On Sat, Aug 28, 2021 at 04:40:45PM +0530, 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
> be16_to_cpu()
> 
> 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 aa56cd1a8490..38f1bd591da9 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,
> +										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));
>  					}
>  				}
>  			}
> -- 
> 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

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

[Greg Kroah-Hartman]

On Sat, Aug 28, 2021 at 04:40:46PM +0530, Aakash Hemadri wrote:
> 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 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".

> 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 38f1bd591da9..bce73e8cbd52 100644
> --- a/drivers/staging/r8188eu/core/rtw_br_ext.c
> +++ b/drivers/staging/r8188eu/core/rtw_br_ext.c
> @@ -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;

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

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

[Dan Carpenter]

On Thu, Sep 02, 2021 at 11:28:45AM +0200, Greg Kroah-Hartman wrote:
> On Sat, Aug 28, 2021 at 04:40:46PM +0530, Aakash Hemadri wrote:
> > @@ -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?
> 

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