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