[PATCH] staging: r8188eu: Fix different base types in assignments and parameters

[PATCH] staging: r8188eu: Fix different base types in assignments and parameters

[Fabio M. De Francesco]

Fix sparse warnings of different base types in assignments
and in passing function parameters.

Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
---
 drivers/staging/r8188eu/core/rtw_br_ext.c | 46 ++++++++++++++++++-----
 1 file changed, 36 insertions(+), 10 deletions(-)

diff --git a/drivers/staging/r8188eu/core/rtw_br_ext.c b/drivers/staging/r8188eu/core/rtw_br_ext.c
index e00302137a60..31ca2e548555 100644
--- a/drivers/staging/r8188eu/core/rtw_br_ext.c
+++ b/drivers/staging/r8188eu/core/rtw_br_ext.c
@@ -71,7 +71,7 @@ static inline int __nat25_add_pppoe_tag(struct sk_buff *skb, struct pppoe_tag *t
 	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;
@@ -134,42 +134,68 @@ static inline void __nat25_generate_ipv4_network_addr(unsigned char *networkAddr
 }
 
 static inline void __nat25_generate_ipx_network_addr_with_node(unsigned char *networkAddr,
-				unsigned int *ipxNetAddr, unsigned char *ipxNodeAddr)
+				__be32 *ipxNetAddr, unsigned char *ipxNodeAddr)
 {
+	union {
+                unsigned int f0;
+                unsigned char f1[IPX_NODE_LEN];
+        } addr;
+
 	memset(networkAddr, 0, MAX_NETWORK_ADDR_LEN);
 
 	networkAddr[0] = NAT25_IPX;
-	memcpy(networkAddr+1, (unsigned char *)ipxNetAddr, 4);
+	addr.f0 = be32_to_cpu(*ipxNetAddr);
+	memcpy(networkAddr+1, addr.f1, 4);
 	memcpy(networkAddr+5, ipxNodeAddr, 6);
 }
 
 static inline void __nat25_generate_ipx_network_addr_with_socket(unsigned char *networkAddr,
-				unsigned int *ipxNetAddr, unsigned short *ipxSocketAddr)
+				__be32 *ipxNetAddr, __be16 *ipxSocketAddr)
 {
+	union {
+		unsigned int f0;
+		unsigned char f1[4];
+	} addr;
+
 	memset(networkAddr, 0, MAX_NETWORK_ADDR_LEN);
 
 	networkAddr[0] = NAT25_IPX;
-	memcpy(networkAddr+1, (unsigned char *)ipxNetAddr, 4);
-	memcpy(networkAddr+5, (unsigned char *)ipxSocketAddr, 2);
+	addr.f0 = be32_to_cpu(*ipxNetAddr);
+	memcpy(networkAddr+1, addr.f1, 4);
+	addr.f0 ^= addr.f0;
+	addr.f0 = be16_to_cpu(*ipxSocketAddr);
+	memcpy(networkAddr+5, addr.f1, 2);
 }
 
 static inline void __nat25_generate_apple_network_addr(unsigned char *networkAddr,
-				unsigned short *network, unsigned char *node)
+				__be16 *network, unsigned char *node)
 {
+	union {
+                unsigned short f0;
+                unsigned char f1[2];
+        } addr;
+
 	memset(networkAddr, 0, MAX_NETWORK_ADDR_LEN);
 
 	networkAddr[0] = NAT25_APPLE;
-	memcpy(networkAddr+1, (unsigned char *)network, 2);
+	addr.f0 = be16_to_cpu(*network);
+	memcpy(networkAddr+1, addr.f1, 2);
 	networkAddr[3] = *node;
 }
 
 static inline void __nat25_generate_pppoe_network_addr(unsigned char *networkAddr,
-				unsigned char *ac_mac, unsigned short *sid)
+				unsigned char *ac_mac, __be16 *sid)
 {
+	union {
+                unsigned short f0;
+                unsigned char f1[2];
+        } addr;
+
 	memset(networkAddr, 0, MAX_NETWORK_ADDR_LEN);
 
 	networkAddr[0] = NAT25_PPPOE;
-	memcpy(networkAddr+1, (unsigned char *)sid, 2);
+	addr.f0 = be16_to_cpu(*sid);
+	memcpy(networkAddr+1, addr.f1, 2);
 	memcpy(networkAddr+3, (unsigned char *)ac_mac, 6);
 }
 
-- 
2.32.0

Re: [PATCH] staging: r8188eu: Fix different base types in assignments and parameters

[Dan Carpenter]

On Fri, Jul 30, 2021 at 08:14:52PM +0200, Fabio M. De Francesco wrote:
> Fix sparse warnings of different base types in assignments
> and in passing function parameters.

This patch fixes some endian bugs but it's not mentioned at all in the
commit message.  Did you send to the correct patch?

> 
> Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
> ---
>  drivers/staging/r8188eu/core/rtw_br_ext.c | 46 ++++++++++++++++++-----
>  1 file changed, 36 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/staging/r8188eu/core/rtw_br_ext.c b/drivers/staging/r8188eu/core/rtw_br_ext.c
> index e00302137a60..31ca2e548555 100644
> --- a/drivers/staging/r8188eu/core/rtw_br_ext.c
> +++ b/drivers/staging/r8188eu/core/rtw_br_ext.c
> @@ -71,7 +71,7 @@ static inline int __nat25_add_pppoe_tag(struct sk_buff *skb, struct pppoe_tag *t
>  	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;
> @@ -134,42 +134,68 @@ static inline void __nat25_generate_ipv4_network_addr(unsigned char *networkAddr
>  }
>  
>  static inline void __nat25_generate_ipx_network_addr_with_node(unsigned char *networkAddr,
> -				unsigned int *ipxNetAddr, unsigned char *ipxNodeAddr)
> +				__be32 *ipxNetAddr, unsigned char *ipxNodeAddr)
>  {
> +	union {
> +                unsigned int f0;
> +                unsigned char f1[IPX_NODE_LEN];

What is going on here??  Why is f1 six bytes?

> +        } addr;
> +
>  	memset(networkAddr, 0, MAX_NETWORK_ADDR_LEN);
>  
>  	networkAddr[0] = NAT25_IPX;
> -	memcpy(networkAddr+1, (unsigned char *)ipxNetAddr, 4);
> +	addr.f0 = be32_to_cpu(*ipxNetAddr);
> +	memcpy(networkAddr+1, addr.f1, 4);

What's the point of a union?  memcpy() doesn't care about endian
anotations.

>  	memcpy(networkAddr+5, ipxNodeAddr, 6);
>  }
>  
>  static inline void __nat25_generate_ipx_network_addr_with_socket(unsigned char *networkAddr,
> -				unsigned int *ipxNetAddr, unsigned short *ipxSocketAddr)
> +				__be32 *ipxNetAddr, __be16 *ipxSocketAddr)
>  {
> +	union {
> +		unsigned int f0;
> +		unsigned char f1[4];
> +	} addr;
> +
>  	memset(networkAddr, 0, MAX_NETWORK_ADDR_LEN);
>  
>  	networkAddr[0] = NAT25_IPX;
> -	memcpy(networkAddr+1, (unsigned char *)ipxNetAddr, 4);
> -	memcpy(networkAddr+5, (unsigned char *)ipxSocketAddr, 2);
> +	addr.f0 = be32_to_cpu(*ipxNetAddr);
> +	memcpy(networkAddr+1, addr.f1, 4);
> +	addr.f0 ^= addr.f0;

What on earth????

> +	addr.f0 = be16_to_cpu(*ipxSocketAddr);

I'm so puzzled.

> +	memcpy(networkAddr+5, addr.f1, 2);

This patch is really weird so I'm done reviewing it.

regards,
dan carpenter

Re: [PATCH] staging: r8188eu: Fix different base types in assignments and parameters

[Fabio M. De Francesco]

On Monday, August 2, 2021 4:05:05 PM CEST Dan Carpenter wrote:
> On Fri, Jul 30, 2021 at 08:14:52PM +0200, Fabio M. De Francesco wrote:
> > Fix sparse warnings of different base types in assignments
> > and in passing function parameters.
> 
> This patch fixes some endian bugs but it's not mentioned at all in the
> commit message.  Did you send to the correct patch?
> 

Too late to change the commit message: Greg K-H has already taken this patch 
as-is (please see commit  56febcc2595e).

> > Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
> > ---
> > 
> >  drivers/staging/r8188eu/core/rtw_br_ext.c | 46 ++++++++++++++++++-----
> >  1 file changed, 36 insertions(+), 10 deletions(-)
> > 
> > diff --git a/drivers/staging/r8188eu/core/rtw_br_ext.c
> > b/drivers/staging/r8188eu/core/rtw_br_ext.c index 
e00302137a60..31ca2e548555 100644
> >
> > [...]
> >
> >  static inline void __nat25_generate_ipx_network_addr_with_node(unsigned 
char
> >  *networkAddr,> 
> > -				unsigned int *ipxNetAddr, 
unsigned char *ipxNodeAddr)
> > +				__be32 *ipxNetAddr, unsigned char 
*ipxNodeAddr)
> > 
> >  {
> > 
> > +	union {
> > +                unsigned int f0;
> > +                unsigned char f1[IPX_NODE_LEN];
> 
> What is going on here??  Why is f1 six bytes?
> 

Please look at the third parameter of the latest memcpy() in this function.

> > +        } addr;
> > +
> > 
> >  	memset(networkAddr, 0, MAX_NETWORK_ADDR_LEN);
> >  	
> >  	networkAddr[0] = NAT25_IPX;
> > 
> > -	memcpy(networkAddr+1, (unsigned char *)ipxNetAddr, 4);
> > +	addr.f0 = be32_to_cpu(*ipxNetAddr);
> > +	memcpy(networkAddr+1, addr.f1, 4);
> 
> What's the point of a union?  memcpy() doesn't care about endian
> anotations.
> 
> >  	memcpy(networkAddr+5, ipxNodeAddr, 6);
			                     ^^^^^

I'm talking about this memcpy().

> >  }
> >  
> >  static inline void __nat25_generate_ipx_network_addr_with_socket(unsigned 
char
> >  *networkAddr,> 
> > -				unsigned int *ipxNetAddr, 
unsigned short *ipxSocketAddr)
> > +				__be32 *ipxNetAddr, __be16 
*ipxSocketAddr)
> > 
> >  {
> > 
> > +	union {
> > +		unsigned int f0;
> > +		unsigned char f1[4];
> > +	} addr;
> > +
> > 
> >  	memset(networkAddr, 0, MAX_NETWORK_ADDR_LEN);
> >  	
> >  	networkAddr[0] = NAT25_IPX;
> > 
> > -	memcpy(networkAddr+1, (unsigned char *)ipxNetAddr, 4);
> > -	memcpy(networkAddr+5, (unsigned char *)ipxSocketAddr, 2);
> > +	addr.f0 = be32_to_cpu(*ipxNetAddr);
> > +	memcpy(networkAddr+1, addr.f1, 4);
> > +	addr.f0 ^= addr.f0;
> 
> What on earth????

I can't see any problem in xor(ing) a field with itself. Perhaps I read too 
much Assembly code :-) . However, am I missing something? 

> > +	addr.f0 = be16_to_cpu(*ipxSocketAddr);
> 
> I'm so puzzled.
> 
> > +	memcpy(networkAddr+5, addr.f1, 2);
> 
> This patch is really weird so I'm done reviewing it.

I'm sorry that you don't like this patch, but Greg already had the last word 
on it.

> regards,
> dan carpenter

Regards,

Fabio

Re: [PATCH] staging: r8188eu: Fix different base types in assignments and parameters

[Fabio M. De Francesco]

On Monday, August 2, 2021 4:26:33 PM CEST Fabio M. De Francesco wrote:
> On Monday, August 2, 2021 4:05:05 PM CEST Dan Carpenter wrote:
> > On Fri, Jul 30, 2021 at 08:14:52PM +0200, Fabio M. De Francesco wrote:
> > > Fix sparse warnings of different base types in assignments
> > > and in passing function parameters.
> > 
> > [...]
> > > 
> > > +	union {
> > > +                unsigned int f0;
> > > +                unsigned char f1[IPX_NODE_LEN];
> > 
> > What is going on here??  Why is f1 six bytes?
> 
> Please look at the third parameter of the latest memcpy() in this function.
> 

No, I'm wrong here. I must have exchanged in my mind the latest and the 
memcpy() before the latest. So I see a '6' in the wrong memcpy().

I'll fix it ASAP.

Thanks,

Fabio

Re: [PATCH] staging: r8188eu: Fix different base types in assignments and parameters

[Fabio M. De Francesco]

On Monday, August 2, 2021 4:05:05 PM CEST Dan Carpenter wrote:
> On Fri, Jul 30, 2021 at 08:14:52PM +0200, Fabio M. De Francesco wrote:
> > Fix sparse warnings of different base types in assignments
> > and in passing function parameters.
> 
> This patch fixes some endian bugs but it's not mentioned at all in the
> commit message.  Did you send to the correct patch?
Dear Greg, Dan,

I just read another message where Greg K-H decided to revert this patch, 
probably also according to the review by Dan C..

In doing so, the sparse warnings of different base types in assignments and 
some endian bugs will appear anew and I think that these problem should be 
addressed in some other way.

I'll try to find some other solutions that result to be more correct and in 
line with Linux conventions. But, before doing the work I need to better 
understand what went wrong the first time. So, please, devote some time to 
respond to my questions...

> > Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
> > ---
> > 
> >  drivers/staging/r8188eu/core/rtw_br_ext.c | 46 ++++++++++++++++++-----
> >  1 file changed, 36 insertions(+), 10 deletions(-)
> > 
> > diff --git a/drivers/staging/r8188eu/core/rtw_br_ext.c
> > b/drivers/staging/r8188eu/core/rtw_br_ext.c index 
e00302137a60..31ca2e548555 100644
> > --- a/drivers/staging/r8188eu/core/rtw_br_ext.c
> > +++ b/drivers/staging/r8188eu/core/rtw_br_ext.c
> > @@ -71,7 +71,7 @@ static inline int __nat25_add_pppoe_tag(struct sk_buff 
*skb, struct
> > pppoe_tag *t> 
> >  	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;
> > 
> > @@ -134,42 +134,68 @@ static inline void 
__nat25_generate_ipv4_network_addr(unsigned
> > char *networkAddr> 
> >  }
> >  
> >  static inline void __nat25_generate_ipx_network_addr_with_node(unsigned 
char
> >  *networkAddr,> 
> > -				unsigned int *ipxNetAddr, 
unsigned char *ipxNodeAddr)
> > +				__be32 *ipxNetAddr, unsigned char 
*ipxNodeAddr)
> > 
> >  {
> > 
> > +	union {
> > +                unsigned int f0;
> > +                unsigned char f1[IPX_NODE_LEN];
> 
> What is going on here??  Why is f1 six bytes?

Here I've been misled by something else. I agree that f1 should be four bytes.
 
> > +        } addr;
> > +
> > 
> >  	memset(networkAddr, 0, MAX_NETWORK_ADDR_LEN);
> >  	
> >  	networkAddr[0] = NAT25_IPX;
> > 
> > -	memcpy(networkAddr+1, (unsigned char *)ipxNetAddr, 4);
> > +	addr.f0 = be32_to_cpu(*ipxNetAddr);
> > +	memcpy(networkAddr+1, addr.f1, 4);
> 
> What's the point of a union?  memcpy() doesn't care about endian
> anotations.

I'm not sure to understand. Please, elaborate a bit. Do you mean that there's 
no need to use be32_to_cpu(*ipxNetAddr) and that we can use ipxNetAddr as-is 
(it is a __be32) as the second parameter of memcpy(), even though the 
networkAddr+1 is the address of an array of unsigned char? Is this that you 
tried to explain to me?

However, there's a huge error in passing the second parameter to memcpy(): it 
should be &addr.f1. Or better, let's make addr to be __be32 and then pass 
&addr (so there's no more need of that union). Do you agree?

> >  	memcpy(networkAddr+5, ipxNodeAddr, 6);
> >  
> >  }
> >  
> >  static inline void __nat25_generate_ipx_network_addr_with_socket(unsigned 
char
> >  *networkAddr,> 
> > -				unsigned int *ipxNetAddr, 
unsigned short *ipxSocketAddr)
> > +				__be32 *ipxNetAddr, __be16 
*ipxSocketAddr)
> > 
> >  {
> > 
> > +	union {
> > +		unsigned int f0;
> > +		unsigned char f1[4];
> > +	} addr;
> > +
> > 
> >  	memset(networkAddr, 0, MAX_NETWORK_ADDR_LEN);
> >  	
> >  	networkAddr[0] = NAT25_IPX;
> > 
> > -	memcpy(networkAddr+1, (unsigned char *)ipxNetAddr, 4);
> > -	memcpy(networkAddr+5, (unsigned char *)ipxSocketAddr, 2);
> > +	addr.f0 = be32_to_cpu(*ipxNetAddr);
> > +	memcpy(networkAddr+1, addr.f1, 4);
> > +	addr.f0 ^= addr.f0;
> 
> What on earth????

According to my understanding I need to wipe all the bits of addr.f0 before 
reusing it in the next assignment. Don't I have to?

If we agree on this, I'd like to explain why I use the xor of itself... I know 
that `addr.f0 = 0;` is more easily readable. But I've always thought that a 
direct assignment of a specific value (0, in this case) should indicate that 
one wants to use that 0 for something that needs that specific number.

For example, in `for(i = 0; i < len; i++)` to indicate that we want to loop 
starting from 0; or in `res = some_func(); if( !res ) return _FAIL;`, and so 
on. In this cases I want to make it clear that 0 has a precise and special 
meaning.

Differently, when one simply wants a variable wiped out of any content, just 
before re-using it for a subsequent "real" assignment, I prefer to xor(ing) 
that variable with itself.

Does it make sense to you?

> > +	addr.f0 = be16_to_cpu(*ipxSocketAddr);
> 
> I'm so puzzled.
>

Again like the assignment above, *ipxSocketAddr is of type __be16 and the 
destination of memcpy() is an array of unsigned char(s). I think I could just 
have a __be16 variable and pass the address of it to memcpy.

Do you agree with the solution above? (Probably I was so distracted that I 
didn't see I should have passed a pointer as the second parameter of 
memcpy()).
 
> > +	memcpy(networkAddr+5, addr.f1, 2);
> 
> This patch is really weird so I'm done reviewing it.
> 
> regards,
> dan carpenter

I'm going to wait a bit for your reply to this message. If I don't receive any 
response I'll wait for the revert and make a new patch and see what happens.
Or, I could fix the patch, if Greg does not revert 56febcc2595e.

Patiently waiting... :-)

Thanks,

Fabio

Re: [PATCH] staging: r8188eu: Fix different base types in assignments and parameters

[Dan Carpenter]

1) I'm pretty sure after a little more review that the patch introduces
   bugs.  The data is supposed to be big endian.

2) The commit message did not clearly describe the "bug" and how the it
   looks like to the user.

3) The way we set variables to zero is we say "foo = 0;"  We do not
   say "foo ^= foo;".  Everyone knows the XOR a variable with itself
   trick but obfuscated code makes us cross.

4) The unions were unnecessary and ugly/bad.

regards,
dan carpenter

Re: [PATCH] staging: r8188eu: Fix different base types in assignments and parameters

[Dan Carpenter]

On Fri, Jul 30, 2021 at 08:14:52PM +0200, Fabio M. De Francesco wrote:
>  static inline void __nat25_generate_ipx_network_addr_with_socket(unsigned char *networkAddr,
> -				unsigned int *ipxNetAddr, unsigned short *ipxSocketAddr)
> +				__be32 *ipxNetAddr, __be16 *ipxSocketAddr)
>  {
> +	union {
> +		unsigned int f0;
> +		unsigned char f1[4];
> +	} addr;
> +
>  	memset(networkAddr, 0, MAX_NETWORK_ADDR_LEN);
>  
>  	networkAddr[0] = NAT25_IPX;
> -	memcpy(networkAddr+1, (unsigned char *)ipxNetAddr, 4);
> -	memcpy(networkAddr+5, (unsigned char *)ipxSocketAddr, 2);
> +	addr.f0 = be32_to_cpu(*ipxNetAddr);
> +	memcpy(networkAddr+1, addr.f1, 4);
> +	addr.f0 ^= addr.f0;
> +	addr.f0 = be16_to_cpu(*ipxSocketAddr);
> +	memcpy(networkAddr+5, addr.f1, 2);

Here is another bug which was obscured/caused by the union.

	addr.f0 = be16_to_cpu(*ipxSocketAddr);

The addr.f0 variable is an int.  On big endian systems only the last two
bytes are set:

	memcpy(networkAddr+5, addr.f1, 2);

So this is the equivalent of:

	memset(networkAddr+5, 0, 2);

regards,
dan carpenter

Re: [PATCH] staging: r8188eu: Fix different base types in assignments and parameters

[Fabio M. De Francesco]

On Wednesday, August 4, 2021 9:59:30 AM CEST Dan Carpenter wrote:
> On Fri, Jul 30, 2021 at 08:14:52PM +0200, Fabio M. De Francesco wrote:
> >  static inline void __nat25_generate_ipx_network_addr_with_socket(unsigned 
char
> >  *networkAddr,> 
> > -				unsigned int *ipxNetAddr, 
unsigned short *ipxSocketAddr)
> >
> > [...]
> >
> Here is another bug which was obscured/caused by the union.
> 
> 	addr.f0 = be16_to_cpu(*ipxSocketAddr);
> 
> The addr.f0 variable is an int.  On big endian systems only the last two
> bytes are set:
> 
> 	memcpy(networkAddr+5, addr.f1, 2);
> 
> So this is the equivalent of:
> 
> 	memset(networkAddr+5, 0, 2);
> 
> regards,
> dan carpenter

Dear Dan,

Thanks, for pointing me to one more bug I introduced with this patch. The most 
of them were due to me forgetting that memcpy() takes pointers. For some 
reason I was thinking it takes values, therefore I put in it a lot of 
unnecessary and faulty complications.

I'd like to make a new patch, a better one (I hope), without unneeded unions 
without the other wrong lines that are in the commit 56febcc2595e.

However, I see that Greg hasn't yet had the time to revert the above commit, 
so I don't know how to make a new patch.

I mean: I could (1) either wait for Greg to revert it and then to fix the 
sparse warnings with a new patch, or (2) I could fix the bugs I made in 
56febcc2595e without having it reverted. I would prefer the solution (2) with 
a "Fixes: 56febcc2595e (...)" and a "Reported-by: Dan Carpenter <...>" tags.

What is the best solution between the two above?

Thanks,

Fabio

Re: [PATCH] staging: r8188eu: Fix different base types in assignments and parameters

[Dan Carpenter]

The patch was based on the faulty premise that the original code was
buggy so I don't think it can be fixed.  It just needs to be reverted.

regards,
dan carpenter

Re: [PATCH] staging: r8188eu: Fix different base types in assignments and parameters

[Fabio M. De Francesco]

On Wednesday, August 4, 2021 11:58:39 AM CEST Dan Carpenter wrote:
> The patch was based on the faulty premise that the original code was
> buggy so I don't think it can be fixed.  It just needs to be reverted.
> 
> regards,
> dan carpenter

With the original code, GCC + sparse emit a dozen of warnings like the 
following ones:

drivers/staging/r8188eu/core/rtw_br_ext.c:693:101: warning: incorrect type in 
argument 2 (different base types)
drivers/staging/r8188eu/core/rtw_br_ext.c:693:101:    expected unsigned int 
*ipxNetAddr
drivers/staging/r8188eu/core/rtw_br_ext.c:693:101:    got restricted __be32 *
drivers/staging/r8188eu/core/rtw_br_ext.c:693:123: warning: incorrect type in 
argument 3 (different base types)
drivers/staging/r8188eu/core/rtw_br_ext.c:693:123:    expected unsigned short 
*ipxSocketAddr
drivers/staging/r8188eu/core/rtw_br_ext.c:693:123:    got restricted __be16 *
drivers/staging/r8188eu/core/rtw_br_ext.c:698:99: warning: incorrect type in 
argument 2 (different base types)

Do we want the above pasted warnings and leave the code as-is?

Regards,

Fabio

Re: [PATCH] staging: r8188eu: Fix different base types in assignments and parameters

[Dan Carpenter]

There is no reason to assume that the Sparse annotations are correct or
complete.

I'm sorry, but you're going to have to figure this one out on your own.
I showed 100% that your patch introduced bugs.  That's the extent of my
job.  I'm not going to answer any more questions.

regards,
dan carpenter