* [PATCH v4] staging: r8188eu: fix too many leading tabs
@ 2022-09-28 7:53 Joash Naidoo
2022-09-28 8:05 ` Greg KH
0 siblings, 1 reply; 5+ messages in thread
From: Joash Naidoo @ 2022-09-28 7:53 UTC (permalink / raw)
To: Larry.Finger, phil, dan.carpenter, straube.linux, paskripkin
Cc: gregkh, linux-staging, 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
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v4] staging: r8188eu: fix too many leading tabs
2022-09-28 7:53 [PATCH v4] staging: r8188eu: fix too many leading tabs Joash Naidoo
@ 2022-09-28 8:05 ` Greg KH
2022-09-28 16:20 ` Joash Naidoo
0 siblings, 1 reply; 5+ messages in thread
From: Greg KH @ 2022-09-28 8:05 UTC (permalink / raw)
To: Joash Naidoo
Cc: Larry.Finger, phil, dan.carpenter, straube.linux, paskripkin,
linux-staging
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v4] staging: r8188eu: fix too many leading tabs
2022-09-28 8:05 ` Greg KH
@ 2022-09-28 16:20 ` Joash Naidoo
2022-09-28 17:24 ` Greg KH
0 siblings, 1 reply; 5+ messages in thread
From: Joash Naidoo @ 2022-09-28 16:20 UTC (permalink / raw)
To: Greg KH
Cc: Larry.Finger, phil, dan.carpenter, straube.linux, paskripkin,
linux-staging
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v4] staging: r8188eu: fix too many leading tabs
2022-09-28 16:20 ` Joash Naidoo
@ 2022-09-28 17:24 ` Greg KH
2022-10-01 17:48 ` Joash Naidoo
0 siblings, 1 reply; 5+ messages in thread
From: Greg KH @ 2022-09-28 17:24 UTC (permalink / raw)
To: Joash Naidoo
Cc: Larry.Finger, phil, dan.carpenter, straube.linux, paskripkin,
linux-staging
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v4] staging: r8188eu: fix too many leading tabs
2022-09-28 17:24 ` Greg KH
@ 2022-10-01 17:48 ` Joash Naidoo
0 siblings, 0 replies; 5+ messages in thread
From: Joash Naidoo @ 2022-10-01 17:48 UTC (permalink / raw)
To: Greg KH
Cc: Larry.Finger, phil, dan.carpenter, straube.linux, paskripkin,
linux-staging
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
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2022-10-01 19:03 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-09-28 7:53 [PATCH v4] staging: r8188eu: fix too many leading tabs Joash Naidoo
2022-09-28 8:05 ` Greg KH
2022-09-28 16:20 ` Joash Naidoo
2022-09-28 17:24 ` Greg KH
2022-10-01 17:48 ` Joash Naidoo
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.