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

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

[Joash Naidoo]

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));
+	cookie = be32_to_cpu((__be32)dhcph->cookie);
+
+	if (priv->ethBrExtInfo.dhcp_bcst_disable)
+		return;
+
+	if (protocol != htons(ETH_P_IP)) /*  IP */
+		return;
+
+	if (iph->protocol != IPPROTO_UDP) /*  UDP */
+		return;
+
+	if ((udph->source != __constant_htons(CLIENT_PORT)) ||
+	    (udph->dest != __constant_htons(SERVER_PORT)))
+		return;
+
+	if (cookie != DHCP_MAGIC) /*  match magic word */
+		return;
+
+	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;
 	}
 }
 
-- 
2.35.1

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

[Greg KH]

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

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

[Joash Naidoo]

Greg KH <gregkh@linuxfoundation.org> 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@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.

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

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

[Greg KH]

On Wed, Sep 28, 2022 at 06:20:28PM +0200, Joash Naidoo wrote:
> > 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?

Yes, that would be good to do as a patch series.

thanks,

greg k-h

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

[Joash Naidoo]

Greg KH <gregkh@linuxfoundation.org> writes:

> On Wed, Sep 28, 2022 at 06:20:28PM +0200, Joash Naidoo wrote:
>> > 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?
>
> Yes, that would be good to do as a patch series.
>
> thanks,
>
> greg k-h

Hi all

I want to investigate Greg's suggestion to replace the original 
code's iph cast with ip_hdr(). Looking into the ip_hdr() function 
it seems it effectively casts skb->head + skb->network_header 
property (See [1, 2]) whereas the original code casts skb->data + 
ETH_HLEN [3]. My goal was to use printk statements to see if 
they're equivalent before making any actual changes.

Could anyone advise to a high level action which will trigger the 
dhcp_flag_bcast() function, as seen in my patch. I have limited 
knowledge but I would imagine this function is called when the 
device broadcasts an IP address request from the DHCP server. I am 
using dhcpcd as a client and I have tried to manually trigger this 
action by deleting device's lease file and restarting dhcpcd. 
Among trying other basic things, I have been unsuccessful at 
triggering this function.

Looking further up the code at the rtw_xmit() function [4], I 
learnt the following. Perhaps understanding br_port may also help 
me.

	void *br_port = NULL;
	br_port = rcu_dereference(padapter->pnetdev->rx_handler_data); 
	// <-- this returns 0x00

	if (br_port && check_fwstate(pmlmepriv, WIFI_STATION_STATE | 
	WIFI_ADHOC_STATE)) { // <-- false because of br_port, second 
	condition is true
		res = rtw_br_client_tx(padapter, ppkt); // <-- later calls 
		the dhcp_flag_bcast() See [5]

		if (res == -1) {
			rtw_free_xmitframe(pxmitpriv, pxmitframe);
			return -1;
		}
	}

[1] 
https://elixir.bootlin.com/linux/v6.0-rc5/source/include/linux/ip.h#L19
[2] 
https://elixir.bootlin.com/linux/v6.0-rc5/source/include/linux/skbuff.h#L2819
[3] 
https://elixir.bootlin.com/linux/v6.0-rc5/source/drivers/staging/r8188eu/core/rtw_br_ext.c#L611
[4] 
https://elixir.bootlin.com/linux/v6.0-rc5/source/drivers/staging/r8188eu/core/rtw_xmit.c#L1670
[5] 
https://elixir.bootlin.com/linux/v6.0-rc5/source/drivers/staging/r8188eu/core/rtw_xmit.c#L1598

Thank you all for your time and support.

Kind regards,
Joash