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;
--
Best regards,
Alexander Sverdlin.
_______________________________________________
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
