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

[Greg KH <gregkh@linuxfoundation.org>]

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@gmail.com>
> ---
> 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.

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

thanks,

greg k-h