LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH 1/1] staging: wilc1000: Use common structs to parse ip packets
@ 2018-05-29 19:08 Thibaut Robert
  2018-05-30 11:29 ` Dan Carpenter
  0 siblings, 1 reply; 8+ messages in thread
From: Thibaut Robert @ 2018-05-29 19:08 UTC (permalink / raw)
  To: Aditya Shankar, Ganesh Krishna, Greg Kroah-Hartman
  Cc: linux-wireless, devel, linux-kernel, 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 __be16

Signed-off-by: Thibaut Robert <thibaut.robert@gmail.com>
---
 drivers/staging/wilc1000/wilc_wlan.c | 44 +++++++++++-----------------
 1 file changed, 17 insertions(+), 27 deletions(-)

diff --git a/drivers/staging/wilc1000/wilc_wlan.c b/drivers/staging/wilc1000/wilc_wlan.c
index d4ebbf67e50b..28c93f3f846e 100644
--- a/drivers/staging/wilc1000/wilc_wlan.c
+++ b/drivers/staging/wilc1000/wilc_wlan.c
@@ -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 *)&eth_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;
 
-- 
2.17.0

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 1/1] staging: wilc1000: Use common structs to parse ip packets
  2018-05-29 19:08 [PATCH 1/1] staging: wilc1000: Use common structs to parse ip packets Thibaut Robert
@ 2018-05-30 11:29 ` Dan Carpenter
  2018-06-04  5:25   ` Ajay Singh
  0 siblings, 1 reply; 8+ messages in thread
From: Dan Carpenter @ 2018-05-30 11:29 UTC (permalink / raw)
  To: Thibaut Robert
  Cc: Aditya Shankar, Ganesh Krishna, Greg Kroah-Hartman, devel,
	linux-wireless, linux-kernel

On Tue, May 29, 2018 at 09:08:39PM +0200, Thibaut Robert wrote:
>  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;

No blank line, please.

> -	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 *)&eth_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;


>  
> -		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);

This alignment is a bit unfortunate...  Perhaps?

			tcp_hdr_ptr = buf + ETH_HLEN + IHL;

regards,
dan carpenter

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 1/1] staging: wilc1000: Use common structs to parse ip packets
  2018-05-30 11:29 ` Dan Carpenter
@ 2018-06-04  5:25   ` Ajay Singh
  2018-06-04 19:27     ` Thibaut Robert
  2018-06-19 18:44     ` [PATCH v2] " Thibaut Robert
  0 siblings, 2 replies; 8+ messages in thread
From: Ajay Singh @ 2018-06-04  5:25 UTC (permalink / raw)
  To: Thibaut Robert
  Cc: Dan Carpenter, devel, Greg Kroah-Hartman, linux-wireless,
	linux-kernel, Ganesh Krishna, Aditya Shankar


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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 1/1] staging: wilc1000: Use common structs to parse ip packets
  2018-06-04  5:25   ` Ajay Singh
@ 2018-06-04 19:27     ` Thibaut Robert
  2018-06-05  8:35       ` Dan Carpenter
  2018-06-19 18:44     ` [PATCH v2] " Thibaut Robert
  1 sibling, 1 reply; 8+ messages in thread
From: Thibaut Robert @ 2018-06-04 19:27 UTC (permalink / raw)
  To: Ajay Singh
  Cc: Dan Carpenter, devel, Greg Kroah-Hartman, linux-wireless,
	linux-kernel, Ganesh Krishna, Aditya Shankar

Le lundi 04 juin 2018 à 10:55:49 (+0530), Ajay Singh a écrit :
> 
> 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

> 
> Regards,
> Ajay
> 
> 

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 1/1] staging: wilc1000: Use common structs to parse ip packets
  2018-06-04 19:27     ` Thibaut Robert
@ 2018-06-05  8:35       ` Dan Carpenter
  0 siblings, 0 replies; 8+ messages in thread
From: Dan Carpenter @ 2018-06-05  8:35 UTC (permalink / raw)
  To: Thibaut Robert
  Cc: Ajay Singh, devel, Greg Kroah-Hartman, linux-wireless,
	linux-kernel, Ganesh Krishna, Aditya Shankar

On Mon, Jun 04, 2018 at 09:27:02PM +0200, Thibaut Robert wrote:
> Le lundi 04 juin 2018 à 10:55:49 (+0530), Ajay Singh a écrit :
> > 
> > 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 !

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH v2] staging: wilc1000: Use common structs to parse ip packets
  2018-06-04  5:25   ` Ajay Singh
  2018-06-04 19:27     ` Thibaut Robert
@ 2018-06-19 18:44     ` Thibaut Robert
  2018-06-20  9:36       ` Dan Carpenter
  2018-06-26  7:55       ` Claudiu Beznea
  1 sibling, 2 replies; 8+ messages in thread
From: Thibaut Robert @ 2018-06-19 18:44 UTC (permalink / raw)
  To: Aditya Shankar, Ganesh Krishna, Greg Kroah-Hartman,
	linux-wireless, devel, linux-kernel
  Cc: 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 __be16

Signed-off-by: Thibaut Robert <thibaut.robert@gmail.com>
---
 drivers/staging/wilc1000/wilc_wlan.c | 43 ++++++++++------------------
 1 file changed, 15 insertions(+), 28 deletions(-)

diff --git a/drivers/staging/wilc1000/wilc_wlan.c b/drivers/staging/wilc1000/wilc_wlan.c
index 55755d7fbb30..85af36595e69 100644
--- a/drivers/staging/wilc1000/wilc_wlan.c
+++ b/drivers/staging/wilc1000/wilc_wlan.c
@@ -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 *)&eth_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;
 
-- 
2.17.1


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2] staging: wilc1000: Use common structs to parse ip packets
  2018-06-19 18:44     ` [PATCH v2] " Thibaut Robert
@ 2018-06-20  9:36       ` Dan Carpenter
  2018-06-26  7:55       ` Claudiu Beznea
  1 sibling, 0 replies; 8+ messages in thread
From: Dan Carpenter @ 2018-06-20  9:36 UTC (permalink / raw)
  To: Thibaut Robert
  Cc: Aditya Shankar, Ganesh Krishna, Greg Kroah-Hartman,
	linux-wireless, devel, linux-kernel

Nice...

Reviewed-by: Dan Carpenter <dan.carpenter@oracle.com>

regards,
dan carpenter


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2] staging: wilc1000: Use common structs to parse ip packets
  2018-06-19 18:44     ` [PATCH v2] " Thibaut Robert
  2018-06-20  9:36       ` Dan Carpenter
@ 2018-06-26  7:55       ` Claudiu Beznea
  1 sibling, 0 replies; 8+ messages in thread
From: Claudiu Beznea @ 2018-06-26  7:55 UTC (permalink / raw)
  To: Thibaut Robert, Aditya Shankar, Ganesh Krishna,
	Greg Kroah-Hartman, linux-wireless, devel, linux-kernel

Reviewed-by: Claudiu Beznea <claudiu.beznea@microchip.com>

On 19.06.2018 21:44, Thibaut Robert wrote:
> 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
> 
> Signed-off-by: Thibaut Robert <thibaut.robert@gmail.com>
> ---
>  drivers/staging/wilc1000/wilc_wlan.c | 43 ++++++++++------------------
>  1 file changed, 15 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/staging/wilc1000/wilc_wlan.c b/drivers/staging/wilc1000/wilc_wlan.c
> index 55755d7fbb30..85af36595e69 100644
> --- a/drivers/staging/wilc1000/wilc_wlan.c
> +++ b/drivers/staging/wilc1000/wilc_wlan.c
> @@ -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 *)&eth_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;
>  
> 

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2018-06-26  7:55 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-05-29 19:08 [PATCH 1/1] staging: wilc1000: Use common structs to parse ip packets Thibaut Robert
2018-05-30 11:29 ` Dan Carpenter
2018-06-04  5:25   ` Ajay Singh
2018-06-04 19:27     ` Thibaut Robert
2018-06-05  8:35       ` Dan Carpenter
2018-06-19 18:44     ` [PATCH v2] " Thibaut Robert
2018-06-20  9:36       ` Dan Carpenter
2018-06-26  7:55       ` Claudiu Beznea

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).