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

Coding style fix. Fix too many leading tabs and line length.

Signed-off-by: Joash Naidoo <redacted>
---
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

On Wed, Sep 28, 2022 at 09:53:24AM +0200, Joash Naidoo wrote:

Very odd formatting, why split that line?

Why are you not checking for this _before_ all of the above
assignments?

You can check for this right after assigning the protocol variable.

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

Greg KH [off-list ref] writes:

On Wed, Sep 28, 2022 at 09:53:24AM +0200, Joash Naidoo wrote:

quoted

Coding style fix. Fix too many leading tabs and line length.

Signed-off-by: Joash Naidoo <redacted>
---
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?

quoted

+	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?

quoted

+
+	if (protocol != htons(ETH_P_IP)) /*  IP */
+		return;

You can check for this right after assigning the protocol variable.

quoted

+
+	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

On Wed, Sep 28, 2022 at 06:20:28PM +0200, Joash Naidoo wrote:

quoted

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

Greg KH [off-list ref] writes:

On Wed, Sep 28, 2022 at 06:20:28PM +0200, Joash Naidoo wrote:

quoted

quoted

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