| Message ID | 20201016101858.11374-2-alexander.sverdlin@nokia.com |
|---|---|
| State | Not Applicable |
| Delegated to: | David Miller |
| Headers | show |
| Series | [v2,net] staging: octeon: Drop on uncorrectable alignment or FCS error | expand |
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 :(
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???
> 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
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;
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 --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)