staging: wilc1000: Use common structs to parse ip packets
PATCH 1/1 staging: wilc1000: Use common structs to parse ip packets
Use structs ethhdr, iphdr and tcphdr instead of manual parsing in tcp_process.
This commit fix handling of ip packets containing options. It also fixes the following sparse warning:
drivers/staging/wilc1000//wilc_wlan.c:201:19: warning: cast to restricted __be16
--- drivers/staging/wilc1000/wilc_wlan.c | 44 +++++++++++----------------- 1 file changed, 17 insertions(+), 27 deletions(-)
drivers/staging/wilc1000/wilc_wlan.c +17 -27
@@ -1,4 +1,6 @@// SPDX-License-Identifier: GPL-2.0 +#include <linux/if_ether.h> +#include <linux/ip.h> #include "wilc_wfi_netdevice.h" #include "wilc_wlan_cfg.h"
@@ -184,9 +186,9 @@ static inline int add_tcp_pending_ack(u32 ack, u32 session_index,static inline void tcp_process (struct net_device *dev,struct txq_entry_t *tqe) { -u8 *eth_hdr_ptr; +const struct ethhdr *eth_hdr_ptr = (const struct ethhdr *)tqe->buffer; +u8 *buffer = tqe->buffer; -unsigned short h_proto;int i;unsigned long flags;struct wilc_vif *vif;
@@ -197,37 +199,25 @@ static inline void tcp_process(struct net_device *dev, struct txq_entry_t *tqe)spin_lock_irqsave (&wilc->txq_spinlock, flags); - eth_hdr_ptr = &buffer[0 ]; - h_proto =ntohs (*((unsigned short *)ð_hdr_ptr[12 ])); -if (h_proto ==ETH_P_IP ) { -u8 *ip_hdr_ptr; -u8 protocol; - - ip_hdr_ptr = &buffer[ETHERNET_HDR_LEN ]; - protocol = ip_hdr_ptr[9 ]; +if (eth_hdr_ptr->h_proto ==htons (ETH_P_IP )) { +const struct iphdr *ip_hdr_ptr = (const struct iphdr *) + (buffer +ETH_HLEN ); -if (protocol ==0x06 ) { -u8 *tcp_hdr_ptr; +if (ip_hdr_ptr->protocol ==IPPROTO_TCP ) { +const struct tcphdr *tcp_hdr_ptr;u32 IHL , total_length, data_offset; - tcp_hdr_ptr = &ip_hdr_ptr[IP_HDR_LEN ]; -IHL = (ip_hdr_ptr[0 ] &0xf ) <<2 ; - total_length = ((u32 )ip_hdr_ptr[2 ] <<8 ) + - (u32 )ip_hdr_ptr[3 ]; - data_offset = ((u32 )tcp_hdr_ptr[12 ] &0xf0 ) >>2 ; +IHL = ip_hdr_ptr->ihl <<2 ; + tcp_hdr_ptr = (const struct tcphdr *) + ((u8 *)ip_hdr_ptr +IHL ); + total_length =ntohs (ip_hdr_ptr->tot_len); + + data_offset = tcp_hdr_ptr->doff <<2 ;if (total_length == (IHL + data_offset)) {u32 seq_no, ack_no; - seq_no = ((u32 )tcp_hdr_ptr[4 ] <<24 ) + - ((u32 )tcp_hdr_ptr[5 ] <<16 ) + - ((u32 )tcp_hdr_ptr[6 ] <<8 ) + - (u32 )tcp_hdr_ptr[7 ]; - - ack_no = ((u32 )tcp_hdr_ptr[8 ] <<24 ) + - ((u32 )tcp_hdr_ptr[9 ] <<16 ) + - ((u32 )tcp_hdr_ptr[10 ] <<8 ) + - (u32 )tcp_hdr_ptr[11 ]; - + seq_no =ntohl (tcp_hdr_ptr->seq); + ack_no =ntohl (tcp_hdr_ptr->ack_seq);for (i =0 ; i < tcp_session; i++) {u32 j = ack_session_info[i].seq_num;
Signature · 1 line
2.17.0
drivers/staging/wilc1000/wilc_wlan.c static inline int add_tcp_pending_ack(u32 ack, u32 session_index, · line 191
Quoted 6 lines · Thibaut Robert
static inline void
tcp_process(struct net_device *dev, struct txq_entry_t *tqe){drivers/staging/wilc1000/wilc_wlan.c +2
-1-u8 *eth_hdr_ptr; +const struct ethhdr *eth_hdr_ptr = (const struct ethhdr *)tqe->buffer; +u8 *buffer = tqe->buffer;
No blank line, please.
Quoted 9 lines
drivers/staging/wilc1000/wilc_wlan.c +0
-2-unsigned short h_proto;int i;unsigned long flags;struct wilc_vif *vif;@@ -197,37 +199,25 @@ static inline void tcp_process(struct net_device *dev, struct txq_entry_t *tqe)spin_lock_irqsave (&wilc->txq_spinlock, flags); - eth_hdr_ptr = &buffer[0 ];
drivers/staging/wilc1000/wilc_wlan.c static inline void tcp_process(struct net_device *dev, struct txq_entry_t *tqe) · line 204
Quoted 10 lines
drivers/staging/wilc1000/wilc_wlan.c +3
-7- h_proto =ntohs (*((unsigned short *)ð_hdr_ptr[12 ])); -if (h_proto ==ETH_P_IP ) { -u8 *ip_hdr_ptr; -u8 protocol; - - ip_hdr_ptr = &buffer[ETHERNET_HDR_LEN ]; - protocol = ip_hdr_ptr[9 ]; +if (eth_hdr_ptr->h_proto ==htons (ETH_P_IP )) { +const struct iphdr *ip_hdr_ptr = (const struct iphdr *) + (buffer +ETH_HLEN );
If you declared buffer as a void pointer you could remove this cast.
const struct iphdr *ip_hdr_ptr = buf + ETH_HLEN;
Quoted 5 lines
drivers/staging/wilc1000/wilc_wlan.c +2
-2-if (protocol ==0x06 ) { -u8 *tcp_hdr_ptr; +if (ip_hdr_ptr->protocol ==IPPROTO_TCP ) { +const struct tcphdr *tcp_hdr_ptr;
drivers/staging/wilc1000/wilc_wlan.c static inline void tcp_process(struct net_device *dev, struct txq_entry_t *tqe) · line 212
Quoted 10 lines
drivers/staging/wilc1000/wilc_wlan.c +3
-5u32 IHL , total_length, data_offset; - tcp_hdr_ptr = &ip_hdr_ptr[IP_HDR_LEN ]; -IHL = (ip_hdr_ptr[0 ] &0xf ) <<2 ; - total_length = ((u32 )ip_hdr_ptr[2 ] <<8 ) + - (u32 )ip_hdr_ptr[3 ]; - data_offset = ((u32 )tcp_hdr_ptr[12 ] &0xf0 ) >>2 ; +IHL = ip_hdr_ptr->ihl <<2 ; + tcp_hdr_ptr = (const struct tcphdr *) + ((u8 *)ip_hdr_ptr +IHL );
This alignment is a bit unfortunate... Perhaps?
tcp_hdr_ptr = buf + ETH_HLEN + IHL;
regards,
dan carpenter
Thank you for submitting the patches.
The modification in this patch looks okay to me.
Please resend this by including changes as suggested by Dan. I can do the modification and resubmit this patch by including review comments, if its okay with you.
Regards,
Ajay
Quoted 8 lines · Ajay Singh
Thank you for submitting the patches.
The modification in this patch looks okay to me.
Please resend this by including changes as suggested by Dan. I can do the modification and resubmit this patch by including review comments, if its okay with you.
Sorry, I would like to address the comments, but still had no time to do it correctly. I am doing this as a hobby and am still learning so forgive me for the delay. If you can wait a little bit more, I'll resend an updated version.
Thanks for your time !
Thibaut
Quoted 5 lines
Regards,
Ajay
Quoted 9 lines · Thibaut Robert
Quoted 8 lines · Ajay Singh
Thank you for submitting the patches.
The modification in this patch looks okay to me.
Please resend this by including changes as suggested by Dan. I can do the modification and resubmit this patch by including review comments, if its okay with you.
Quoted 5 lines
Sorry, I would like to address the comments, but still had no time to do it correctly. I am doing this as a hobby and am still learning so forgive me for the delay. If you can wait a little bit more, I'll resend an updated version.
Thanks for your time !
When you're a kernel dev, you sometimes do get the feeling that everyone is waiting for you. We're not. There is no rush. We read through your email and reply and then it's gone from our mind like dust in the wind. There is never a rush and you will never run out of more work to do.
Plus the merge window is open so Greg won't even look at it for the next several weeks.
Take all the time you need.
I normally write a patch, then wait until morning so I can review it with fresh eyes before sending it. If I wait longer than a day, though, I forget what the issue is and I have to retest it from scratch. After one day in my outbox a patch gets old and boring so those are low quality. If it's a cleanup patch, I'll probably just delete it instead.
regards,
dan carpenter
PATCH v2 1/1 staging: wilc1000: Use common structs to parse ip packets
In reply to Ajay SinghUse structs ethhdr, iphdr and tcphdr instead of manual parsing in tcp_process.
This commit fix handling of ip packets containing options. It also fixes the following sparse warning:
drivers/staging/wilc1000//wilc_wlan.c:201:19: warning: cast to restricted __be16
--- drivers/staging/wilc1000/wilc_wlan.c | 43 ++++++++++------------------ 1 file changed, 15 insertions(+), 28 deletions(-)
drivers/staging/wilc1000/wilc_wlan.c +15 -28
@@ -1,4 +1,6 @@// SPDX-License-Identifier: GPL-2.0 +#include <linux/if_ether.h> +#include <linux/ip.h> #include "wilc_wfi_netdevice.h" #include "wilc_wlan_cfg.h"
@@ -150,9 +152,8 @@ static inline int add_tcp_pending_ack(u32 ack, u32 session_index,static inline void tcp_process (struct net_device *dev,struct txq_entry_t *tqe) { -u8 *eth_hdr_ptr; -u8 *buffer = tqe->buffer; -unsigned short h_proto; +void *buffer = tqe->buffer; +const struct ethhdr *eth_hdr_ptr = buffer;int i;unsigned long flags;struct wilc_vif *vif;
@@ -163,37 +164,23 @@ static inline void tcp_process(struct net_device *dev, struct txq_entry_t *tqe)spin_lock_irqsave (&wilc->txq_spinlock, flags); - eth_hdr_ptr = &buffer[0 ]; - h_proto =ntohs (*((unsigned short *)ð_hdr_ptr[12 ])); -if (h_proto ==ETH_P_IP ) { -u8 *ip_hdr_ptr; -u8 protocol; +if (eth_hdr_ptr->h_proto ==htons (ETH_P_IP )) { +const struct iphdr *ip_hdr_ptr = buffer +ETH_HLEN ; - ip_hdr_ptr = &buffer[ETHERNET_HDR_LEN ]; - protocol = ip_hdr_ptr[9 ]; - -if (protocol ==0x06 ) { -u8 *tcp_hdr_ptr; +if (ip_hdr_ptr->protocol ==IPPROTO_TCP ) { +const struct tcphdr *tcp_hdr_ptr;u32 IHL , total_length, data_offset; - tcp_hdr_ptr = &ip_hdr_ptr[IP_HDR_LEN ]; -IHL = (ip_hdr_ptr[0 ] &0xf ) <<2 ; - total_length = ((u32 )ip_hdr_ptr[2 ] <<8 ) + - (u32 )ip_hdr_ptr[3 ]; - data_offset = ((u32 )tcp_hdr_ptr[12 ] &0xf0 ) >>2 ; +IHL = ip_hdr_ptr->ihl <<2 ; + tcp_hdr_ptr = buffer +ETH_HLEN +IHL ; + total_length =ntohs (ip_hdr_ptr->tot_len); + + data_offset = tcp_hdr_ptr->doff <<2 ;if (total_length == (IHL + data_offset)) {u32 seq_no, ack_no; - seq_no = ((u32 )tcp_hdr_ptr[4 ] <<24 ) + - ((u32 )tcp_hdr_ptr[5 ] <<16 ) + - ((u32 )tcp_hdr_ptr[6 ] <<8 ) + - (u32 )tcp_hdr_ptr[7 ]; - - ack_no = ((u32 )tcp_hdr_ptr[8 ] <<24 ) + - ((u32 )tcp_hdr_ptr[9 ] <<16 ) + - ((u32 )tcp_hdr_ptr[10 ] <<8 ) + - (u32 )tcp_hdr_ptr[11 ]; - + seq_no =ntohl (tcp_hdr_ptr->seq); + ack_no =ntohl (tcp_hdr_ptr->ack_seq);for (i =0 ; i < tcp_session; i++) {u32 j = ack_session_info[i].seq_num;
Signature · 1 line
2.17.1
Quoted 85 lines · Thibaut Robert
Use structs ethhdr, iphdr and tcphdr instead of manual parsing in
tcp_process.
This commit fix handling of ip packets containing options. It also fixes the following sparse warning:drivers/staging/wilc1000//wilc_wlan.c:201:19: warning: cast to restricted
__be16Signed-off-by Thibaut Robert <[email protected]>--- drivers/staging/wilc1000/wilc_wlan.c | 43 ++++++++++------------------ 1 file changed, 15 insertions(+), 28 deletions(-)drivers/staging/wilc1000/wilc_wlan.c +15
-28@@ -1,4 +1,6 @@// SPDX-License-Identifier: GPL-2.0 +#include <linux/if_ether.h> +#include <linux/ip.h> #include "wilc_wfi_netdevice.h" #include "wilc_wlan_cfg.h" @@ -150,9 +152,8 @@ static inline int add_tcp_pending_ack(u32 ack, u32 session_index,static inline void tcp_process (struct net_device *dev,struct txq_entry_t *tqe) { -u8 *eth_hdr_ptr; -u8 *buffer = tqe->buffer; -unsigned short h_proto; +void *buffer = tqe->buffer; +const struct ethhdr *eth_hdr_ptr = buffer;int i;unsigned long flags;struct wilc_vif *vif;@@ -163,37 +164,23 @@ static inline void tcp_process(struct net_device *dev, struct txq_entry_t *tqe)spin_lock_irqsave (&wilc->txq_spinlock, flags); - eth_hdr_ptr = &buffer[0 ]; - h_proto =ntohs (*((unsigned short *)ð_hdr_ptr[12 ])); -if (h_proto ==ETH_P_IP ) { -u8 *ip_hdr_ptr; -u8 protocol; +if (eth_hdr_ptr->h_proto ==htons (ETH_P_IP )) { +const struct iphdr *ip_hdr_ptr = buffer +ETH_HLEN ; - ip_hdr_ptr = &buffer[ETHERNET_HDR_LEN ]; - protocol = ip_hdr_ptr[9 ]; - -if (protocol ==0x06 ) { -u8 *tcp_hdr_ptr; +if (ip_hdr_ptr->protocol ==IPPROTO_TCP ) { +const struct tcphdr *tcp_hdr_ptr;u32 IHL , total_length, data_offset; - tcp_hdr_ptr = &ip_hdr_ptr[IP_HDR_LEN ]; -IHL = (ip_hdr_ptr[0 ] &0xf ) <<2 ; - total_length = ((u32 )ip_hdr_ptr[2 ] <<8 ) + - (u32 )ip_hdr_ptr[3 ]; - data_offset = ((u32 )tcp_hdr_ptr[12 ] &0xf0 ) >>2 ; +IHL = ip_hdr_ptr->ihl <<2 ; + tcp_hdr_ptr = buffer +ETH_HLEN +IHL ; + total_length =ntohs (ip_hdr_ptr->tot_len); + + data_offset = tcp_hdr_ptr->doff <<2 ;if (total_length == (IHL + data_offset)) {u32 seq_no, ack_no; - seq_no = ((u32 )tcp_hdr_ptr[4 ] <<24 ) + - ((u32 )tcp_hdr_ptr[5 ] <<16 ) + - ((u32 )tcp_hdr_ptr[6 ] <<8 ) + - (u32 )tcp_hdr_ptr[7 ]; - - ack_no = ((u32 )tcp_hdr_ptr[8 ] <<24 ) + - ((u32 )tcp_hdr_ptr[9 ] <<16 ) + - ((u32 )tcp_hdr_ptr[10 ] <<8 ) + - (u32 )tcp_hdr_ptr[11 ]; - + seq_no =ntohl (tcp_hdr_ptr->seq); + ack_no =ntohl (tcp_hdr_ptr->ack_seq);for (i =0 ; i < tcp_session; i++) {u32 j = ack_session_info[i].seq_num;