Re: [PATCH v4] staging: r8188eu: fix too many leading tabs

[Joash Naidoo <joash.n09@xxxxxxxxx>]

Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx> writes:

> On Wed, Sep 28, 2022 at 09:53:24AM +0200, Joash Naidoo wrote:
> > Coding style fix. Fix too many leading tabs and line length.
> >
> > Signed-off-by: Joash Naidoo <joash.n09@xxxxxxxxx>
> > ---
> > Changes in V4:
> >     - Fix compiler warning introduced: mixing declarations and 
> >     code
> > Changes in v3:
> >     - Fix flipped condition mistake
> >     - move skb NULL check before dereferencing it
> > Changes in v2:
> >     - Flip additional nested if conditions and don't reverse 
> >     the last if statement
> >     - Move declarations to start of function
> >     - Separate converting __constant_htons to htons to another 
> >     patch
> > ---
> >  drivers/staging/r8188eu/core/rtw_br_ext.c | 75 
> >  +++++++++++++----------
> >  1 file changed, 42 insertions(+), 33 deletions(-)
> >
> > diff --git a/drivers/staging/r8188eu/core/rtw_br_ext.c 
> > b/drivers/staging/r8188eu/core/rtw_br_ext.c
> > index bca20fe5c..d4bcec152 100644
> > --- a/drivers/staging/r8188eu/core/rtw_br_ext.c
> > +++ b/drivers/staging/r8188eu/core/rtw_br_ext.c
> > @@ -601,42 +601,51 @@ struct dhcpMessage {
> >
> >  void dhcp_flag_bcast(struct adapter *priv, struct sk_buff 
> >  *skb)
> >  {
> > +	__be16 protocol;
> > +	struct iphdr *iph;
> > +	struct udphdr *udph;
> > +	struct dhcpMessage *dhcph;
> > +	u32 cookie;
> > +
> >  	if (!skb)
> >  		return;
> >
> > -	if (!priv->ethBrExtInfo.dhcp_bcst_disable) {
> > -		__be16 protocol = *((__be16 *)(skb->data + 2 * 
> > ETH_ALEN));
> > -
> > -		if (protocol == __constant_htons(ETH_P_IP)) { /*  IP 
> > */
> > -			struct iphdr *iph = (struct iphdr *)(skb->data + 
> > ETH_HLEN);
> > -
> > -			if (iph->protocol == IPPROTO_UDP) { /*  UDP */
> > -				struct udphdr *udph = (struct udphdr 
> > *)((size_t)iph + (iph->ihl << 2));
> > -
> > -				if ((udph->source == 
> > __constant_htons(CLIENT_PORT)) &&
> > -				    (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);
> > -
> > -					if (cookie == DHCP_MAGIC) { /*  match 
> > magic word */
> > -						if (!(dhcph->flags & 
> > htons(BROADCAST_FLAG))) {
> > -							/*  if not broadcast */
> > -							register int sum = 0;
> > -
> > -							/*  or BROADCAST flag */
> > -							dhcph->flags |= 
> > htons(BROADCAST_FLAG);
> > -							/*  recalculate checksum */
> > -							sum = ~(udph->check) & 0xffff;
> > -							sum += be16_to_cpu(dhcph->flags);
> > -							while (sum >> 16)
> > -								sum = (sum & 0xffff) + (sum >> 
> > 16);
> > -							udph->check = ~sum;
> > -						}
> > -					}
> > -				}
> > -			}
> > -		}
> > +	protocol = *((__be16 *)(skb->data + 2 * ETH_ALEN));
> > +	iph = (struct iphdr *)(skb->data + ETH_HLEN);
> > +	udph = (struct udphdr *)((size_t)iph + (iph->ihl << 2));
> > +	/*  DHCP request */
> > +	dhcph =
> > +		(struct dhcpMessage *)((size_t)udph + sizeof(struct 
> > udphdr));
>
> Very odd formatting, why split that line?
>
> > +	cookie = be32_to_cpu((__be32)dhcph->cookie);
> > +
> > +	if (priv->ethBrExtInfo.dhcp_bcst_disable)
> > +		return;
>
> Why are you not checking for this _before_ all of the above
> assignments?
>
>
>
> > +
> > +	if (protocol != htons(ETH_P_IP)) /*  IP */
> > +		return;
>
> You can check for this right after assigning the protocol 
> variable.
>
> > +
> > +	if (iph->protocol != IPPROTO_UDP) /*  UDP */
> > +		return;
>
> You can check for this right after setting iph.

Thank you for your time and feedback. I agree with your comments. 
My rationale for having the way it is stems from my inclination to 
stuck to the original code as close as possible. In hindsight, the 
ordering could be better. I will submit another revision.

> But also, shouldn't you just be using the ip_hdr() call instead 
> of doing
> the crazy cast above?

I would need to investigate and test first before commenting. If a 
change had to be made, I believe it will go in a separate patch?

> thanks,
>
> greg k-h