On 26/12/2021 17:02, Petko Manolov wrote:
> On 21-12-26 14:25:02, Matthias-Christian Ott wrote:
>> Commit 1a8deec09d12 ("pegasus: fixes reported packet length") tried to
>> configure the hardware to not include the FCS/CRC of Ethernet frames.
>> Unfortunately, this does not work with the D-Link DSB-650TX (USB IDs
>> 2001:4002 and 2001:400b): the transferred "packets" (in the terminology
>> of the hardware) still contain 4 additional octets. For IP packets in
>> Ethernet this is not a problem as IP packets contain their own lengths
>> fields but other protocols also see Ethernet frames that include the FCS
>> in the payload which might be a problem for some protocols.
>>
>> I was not able to open the D-Link DSB-650TX as the case is a very tight
>> press fit and opening it would likely destroy it. However, according to
>> the source code the earlier revision of the D-Link DSB-650TX (USB ID
>> 2001:4002) is a Pegasus (possibly AN986) and not Pegasus II (AN8511)
>> device. I also tried it with the later revision of the D-Link DSB-650TX
>> (USB ID 2001:400b) which is a Pegasus II device according to the source
>> code and had the same results. Therefore, I'm not sure whether the RXCS
>> (rx_crc_sent) field of the EC0 (Ethernet control_0) register has any
>> effect or in which revision of the hardware it is usable and has an
>> effect. As a result, it seems best to me to revert commit
>> 1a8deec09d12 ("pegasus: fixes reported packet length") and to set the
>> RXCS (rx_crc_sent) field of the EC0 (Ethernet control_0) register so
>> that the FCS/CRC is always included.
>>
>> Fixes: 1a8deec09d12 ("pegasus: fixes reported packet length")
>> Signed-off-by: Matthias-Christian Ott <ott@xxxxxxxxx>
>> ---
>> drivers/net/usb/pegasus.c | 15 ++++++++++++++-
>> 1 file changed, 14 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/usb/pegasus.c b/drivers/net/usb/pegasus.c
>> index c4cd40b090fd..140d11ae6688 100644
>> --- a/drivers/net/usb/pegasus.c
>> +++ b/drivers/net/usb/pegasus.c
>> @@ -422,7 +422,13 @@ static int enable_net_traffic(struct net_device *dev, struct usb_device *usb)
>> ret = read_mii_word(pegasus, pegasus->phy, MII_LPA, &linkpart);
>> if (ret < 0)
>> goto fail;
>> - data[0] = 0xc8; /* TX & RX enable, append status, no CRC */
>> + /* At least two hardware revisions of the D-Link DSB-650TX (USB IDs
>> + * 2001:4002 and 2001:400b) include the Ethernet FCS in the packets,
>> + * even if RXCS is set to 0 in the EC0 register and the hardware is
>> + * instructed to not include the Ethernet FCS in the packet.Therefore,
>> + * it seems best to set RXCS to 1 and later ignore the Ethernet FCS.
>> + */
>> + data[0] = 0xc9; /* TX & RX enable, append status, CRC */
>> data[1] = 0;
>> if (linkpart & (ADVERTISE_100FULL | ADVERTISE_10FULL))
>> data[1] |= 0x20; /* set full duplex */
>> @@ -513,6 +519,13 @@ static void read_bulk_callback(struct urb *urb)
>> pkt_len = buf[count - 3] << 8;
>> pkt_len += buf[count - 4];
>> pkt_len &= 0xfff;
>> + /* The FCS at the end of the packet is ignored. So subtract
>> + * its length to ignore it.
>> + */
>> + pkt_len -= ETH_FCS_LEN;
>> + /* Subtract the length of the received status at the end of the
>> + * packet as it is not part of the Ethernet frame.
>> + */
>> pkt_len -= 4;
>> }
>
> Nice catch. However, changing these constants for all devices isn't such a good
> idea. I'd rather use vendor and device IDs to distinguish these two cases in
> the above code.
I don't think that it would hurt to include the FCS for all devices. I
only have the datasheets for the ADM8511/X and the ADM8513 but it seems
that all devices that are supported by the driver also include the RXCS
field in EC0. This was also the previous behaviour before commit
1a8deec09d12 and seemed to have worked. It also only adds four octet
that have to be transferred and it seems to avoid exceptions for
different devices which seems to be a good idea, in particular, because
it is not easy to acquire all of the supported devices as they are no
longer sold or manufactured.
That being said, if you are going to veto this change otherwise, I can
of course just add the FCS back for the two USB IDs, even though it
likely affects other devices as well.
Kind regards,
Matthias-Christian Ott
