diff mbox series

[v2,net] staging: octeon: Drop on uncorrectable alignment or FCS error

Message ID 20201016101858.11374-2-alexander.sverdlin@nokia.com (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series [v2,net] staging: octeon: Drop on uncorrectable alignment or FCS error | expand

Commit Message

Alexander Sverdlin Oct. 16, 2020, 10:18 a.m. UTC 
From: Alexander Sverdlin <alexander.sverdlin@nokia.com>

Currently in case of alignment or FCS error if the packet cannot be
corrected it's still not dropped. Report the error properly and drop the
packet while making the code around a little bit more readable.

Signed-off-by: Alexander Sverdlin <alexander.sverdlin@nokia.com>
Fixes: 80ff0fd3ab ("Staging: Add octeon-ethernet driver files.")

Change-Id: Ie1fadcc57cb5e221cf3e83c169b53a5533b8edff
---
 drivers/staging/octeon/ethernet-rx.c | 34 +++++++++++++++++++---------------
 1 file changed, 19 insertions(+), 15 deletions(-)

Changes in v2:
- whitespace alignment fix as suggested by Dan Carpenter
- fixed the logic to accept "corrected" packets (preambles 0xd, 0xd5)

Comments

Greg Kroah-Hartman Oct. 16, 2020, 11:17 a.m. UTC | #1 
On Fri, Oct 16, 2020 at 12:18:58PM +0200, Alexander A Sverdlin wrote:
> From: Alexander Sverdlin <alexander.sverdlin@nokia.com>
> 
> Currently in case of alignment or FCS error if the packet cannot be
> corrected it's still not dropped. Report the error properly and drop the
> packet while making the code around a little bit more readable.
> 
> Signed-off-by: Alexander Sverdlin <alexander.sverdlin@nokia.com>
> Fixes: 80ff0fd3ab ("Staging: Add octeon-ethernet driver files.")
> 
> Change-Id: Ie1fadcc57cb5e221cf3e83c169b53a5533b8edff

You didn't run checkpatch :(
Greg Kroah-Hartman Oct. 16, 2020, 1:14 p.m. UTC | #2 
On Fri, Oct 16, 2020 at 12:18:58PM +0200, Alexander A Sverdlin wrote:
> From: Alexander Sverdlin <alexander.sverdlin@nokia.com>
> 
> Currently in case of alignment or FCS error if the packet cannot be
> corrected it's still not dropped. Report the error properly and drop the
> packet while making the code around a little bit more readable.
> 
> Signed-off-by: Alexander Sverdlin <alexander.sverdlin@nokia.com>
> Fixes: 80ff0fd3ab ("Staging: Add octeon-ethernet driver files.")
> 
> Change-Id: Ie1fadcc57cb5e221cf3e83c169b53a5533b8edff

Why is the change-id still here???
Andrew Lunn Oct. 17, 2020, 9:02 p.m. UTC | #3 
> diff --git a/drivers/staging/octeon/ethernet-rx.c b/drivers/staging/octeon/ethernet-rx.c
> index 2c16230..9ebd665 100644
> --- a/drivers/staging/octeon/ethernet-rx.c
> +++ b/drivers/staging/octeon/ethernet-rx.c
> @@ -69,15 +69,17 @@ static inline int cvm_oct_check_rcv_error(struct cvmx_wqe *work)
>  	else
>  		port = work->word1.cn38xx.ipprt;
>  
> -	if ((work->word2.snoip.err_code == 10) && (work->word1.len <= 64)) {
> +	if ((work->word2.snoip.err_code == 10) && (work->word1.len <= 64))

It would be nice to replace all these err_code magic numbers with #defines.

You should also replace 64 with ETH_ZLEN + ETH_FCS_LEN. I also wonder
if <= should be just < ?

>  		/*
>  		 * Ignore length errors on min size packets. Some
>  		 * equipment incorrectly pads packets to 64+4FCS
>  		 * instead of 60+4FCS.  Note these packets still get
>  		 * counted as frame errors.
>  		 */
> -	} else if (work->word2.snoip.err_code == 5 ||
> -		   work->word2.snoip.err_code == 7) {
> +		return 0;
> +
> +	if (work->word2.snoip.err_code == 5 ||
> +	    work->word2.snoip.err_code == 7) {
>  		/*
>  		 * We received a packet with either an alignment error
>  		 * or a FCS error. This may be signalling that we are
> @@ -108,7 +110,10 @@ static inline int cvm_oct_check_rcv_error(struct cvmx_wqe *work)
>  				/* Port received 0xd5 preamble */
>  				work->packet_ptr.s.addr += i + 1;
>  				work->word1.len -= i + 5;
> -			} else if ((*ptr & 0xf) == 0xd) {
> +				return 0;
> +			}
> +
> +			if ((*ptr & 0xf) == 0xd) {

The comments are not so clear what is going on here. Can this
incorrectly match a destination MAC address of xD:XX:XX:XX:XX:XX.

>  				/* Port received 0xd preamble */
>  				work->packet_ptr.s.addr += i;
>  				work->word1.len -= i + 4;

	Andrew
Alexander Sverdlin Oct. 19, 2020, 9:11 a.m. UTC | #4 
Hello Andrew,

thank you for your review!

On 17/10/2020 23:02, Andrew Lunn wrote:
>> diff --git a/drivers/staging/octeon/ethernet-rx.c b/drivers/staging/octeon/ethernet-rx.c
>> index 2c16230..9ebd665 100644
>> --- a/drivers/staging/octeon/ethernet-rx.c
>> +++ b/drivers/staging/octeon/ethernet-rx.c
>> @@ -69,15 +69,17 @@ static inline int cvm_oct_check_rcv_error(struct cvmx_wqe *work)
>>  	else
>>  		port = work->word1.cn38xx.ipprt;
>>  
>> -	if ((work->word2.snoip.err_code == 10) && (work->word1.len <= 64)) {
>> +	if ((work->word2.snoip.err_code == 10) && (work->word1.len <= 64))
> It would be nice to replace all these err_code magic numbers with #defines.
> 
> You should also replace 64 with ETH_ZLEN + ETH_FCS_LEN. I also wonder
> if <= should be just < ?

I think all your comments are valid points, but are rather topics
for separate patches. In this one I've addressed one issue: the structure of
ifs and elses is so deeply nested, that it lead to one logic mistake:
broken packets which cannot be corrected are still not dropped.

Even my patch has two changes in one: error correction and style correction,
but I consider this justified because one was a result of another.

>>  		/*
>>  		 * Ignore length errors on min size packets. Some
>>  		 * equipment incorrectly pads packets to 64+4FCS
>>  		 * instead of 60+4FCS.  Note these packets still get
>>  		 * counted as frame errors.
>>  		 */
>> -	} else if (work->word2.snoip.err_code == 5 ||
>> -		   work->word2.snoip.err_code == 7) {
>> +		return 0;
>> +
>> +	if (work->word2.snoip.err_code == 5 ||
>> +	    work->word2.snoip.err_code == 7) {
>>  		/*
>>  		 * We received a packet with either an alignment error
>>  		 * or a FCS error. This may be signalling that we are
>> @@ -108,7 +110,10 @@ static inline int cvm_oct_check_rcv_error(struct cvmx_wqe *work)
>>  				/* Port received 0xd5 preamble */
>>  				work->packet_ptr.s.addr += i + 1;
>>  				work->word1.len -= i + 5;
>> -			} else if ((*ptr & 0xf) == 0xd) {
>> +				return 0;
>> +			}
>> +
>> +			if ((*ptr & 0xf) == 0xd) {
> The comments are not so clear what is going on here. Can this
> incorrectly match a destination MAC address of xD:XX:XX:XX:XX:XX.
> 
>>  				/* Port received 0xd preamble */
>>  				work->packet_ptr.s.addr += i;
>>  				work->word1.len -= i + 4;
Greg Kroah-Hartman Oct. 25, 2020, 9:22 a.m. UTC | #5 
On Fri, Oct 16, 2020 at 12:18:58PM +0200, Alexander A Sverdlin wrote:
> From: Alexander Sverdlin <alexander.sverdlin@nokia.com>
> 
> Currently in case of alignment or FCS error if the packet cannot be
> corrected it's still not dropped. Report the error properly and drop the
> packet while making the code around a little bit more readable.
> 
> Signed-off-by: Alexander Sverdlin <alexander.sverdlin@nokia.com>
> Fixes: 80ff0fd3ab ("Staging: Add octeon-ethernet driver files.")
> 
> Change-Id: Ie1fadcc57cb5e221cf3e83c169b53a5533b8edff

Please stop using gerrit for patches destined for upstream development
:(
diff mbox series

Patch

diff --git a/drivers/staging/octeon/ethernet-rx.c b/drivers/staging/octeon/ethernet-rx.c
index 2c16230..9ebd665 100644
--- a/drivers/staging/octeon/ethernet-rx.c
+++ b/drivers/staging/octeon/ethernet-rx.c
@@ -69,15 +69,17 @@  static inline int cvm_oct_check_rcv_error(struct cvmx_wqe *work)
 	else
 		port = work->word1.cn38xx.ipprt;
 
-	if ((work->word2.snoip.err_code == 10) && (work->word1.len <= 64)) {
+	if ((work->word2.snoip.err_code == 10) && (work->word1.len <= 64))
 		/*
 		 * Ignore length errors on min size packets. Some
 		 * equipment incorrectly pads packets to 64+4FCS
 		 * instead of 60+4FCS.  Note these packets still get
 		 * counted as frame errors.
 		 */
-	} else if (work->word2.snoip.err_code == 5 ||
-		   work->word2.snoip.err_code == 7) {
+		return 0;
+
+	if (work->word2.snoip.err_code == 5 ||
+	    work->word2.snoip.err_code == 7) {
 		/*
 		 * We received a packet with either an alignment error
 		 * or a FCS error. This may be signalling that we are
@@ -108,7 +110,10 @@  static inline int cvm_oct_check_rcv_error(struct cvmx_wqe *work)
 				/* Port received 0xd5 preamble */
 				work->packet_ptr.s.addr += i + 1;
 				work->word1.len -= i + 5;
-			} else if ((*ptr & 0xf) == 0xd) {
+				return 0;
+			}
+
+			if ((*ptr & 0xf) == 0xd) {
 				/* Port received 0xd preamble */
 				work->packet_ptr.s.addr += i;
 				work->word1.len -= i + 4;
@@ -118,21 +123,20 @@  static inline int cvm_oct_check_rcv_error(struct cvmx_wqe *work)
 					    ((*(ptr + 1) & 0xf) << 4);
 					ptr++;
 				}
-			} else {
-				printk_ratelimited("Port %d unknown preamble, packet dropped\n",
-						   port);
-				cvm_oct_free_work(work);
-				return 1;
+				return 0;
 			}
+
+			printk_ratelimited("Port %d unknown preamble, packet dropped\n",
+					   port);
+			cvm_oct_free_work(work);
+			return 1;
 		}
-	} else {
-		printk_ratelimited("Port %d receive error code %d, packet dropped\n",
-				   port, work->word2.snoip.err_code);
-		cvm_oct_free_work(work);
-		return 1;
 	}
 
-	return 0;
+	printk_ratelimited("Port %d receive error code %d, packet dropped\n",
+			   port, work->word2.snoip.err_code);
+	cvm_oct_free_work(work);
+	return 1;
 }
 
 static void copy_segments_to_skb(struct cvmx_wqe *work, struct sk_buff *skb)