net: usb: pegasus: Request Ethernet FCS from hardware

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@mirix.org>
---
 drivers/net/usb/pegasus.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

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@mirix.org>
>> ---
>>  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

On 21-12-26 17:12:24, Matthias-Christian Ott wrote:
> 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@mirix.org>
> >> ---
> >>  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.

The fix that commit 1a8deec09d12 introduces is real (the commit message makes
sense) and i don't feel confident to revert it so lightly.  I think i have all
relevant datasheets somewhere, along with a couple of old "pegasus I" devices,
which i could use for testing. Not at home right now, the aforementioned testing
will have to wait a couple of days.

> 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.

Like i said, i don't want to hurry up and revert something that looks like a
valid fix.  Especially after five years worth of kernel releases and no
complaints related to 1a8deec09d12.  This should mean two things: a) the driver
isn't used anymore, or b) this commit fixes a real problem.

However, if it turn out that your fix is the right one, it goes in without fuss.
So lets see what it is...


cheers,
Petko

On 26/12/2021 23:12, Petko Manolov wrote:
> On 21-12-26 17:12:24, Matthias-Christian Ott wrote:
>> 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@mirix.org>
>>>> ---
>>>>  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.
> 
> The fix that commit 1a8deec09d12 introduces is real (the commit message makes
> sense) and i don't feel confident to revert it so lightly.  I think i have all
> relevant datasheets somewhere, along with a couple of old "pegasus I" devices,
> which i could use for testing. Not at home right now, the aforementioned testing
> will have to wait a couple of days.
> 
>> 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.
> 
> Like i said, i don't want to hurry up and revert something that looks like a
> valid fix.  Especially after five years worth of kernel releases and no
> complaints related to 1a8deec09d12.  This should mean two things: a) the driver
> isn't used anymore, or b) this commit fixes a real problem.
> 
> However, if it turn out that your fix is the right one, it goes in without fuss.
> So lets see what it is...

I agree. It is not my intention to break something. Take your time to
test it when you find the time and let me know of the results. We are
not in a hurry. I have my private fork of the driver for the longterm
kernel.

Kind regards,
Matthias-Christian Ott

On 26/12/2021 23:17, Matthias-Christian Ott wrote:
> On 26/12/2021 23:12, Petko Manolov wrote:
>> On 21-12-26 17:12:24, Matthias-Christian Ott wrote:
>>> 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@mirix.org>
>>>>> ---
>>>>>   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.
>>
>> The fix that commit 1a8deec09d12 introduces is real (the commit message makes
>> sense) and i don't feel confident to revert it so lightly.  I think i have all
>> relevant datasheets somewhere, along with a couple of old "pegasus I" devices,
>> which i could use for testing. Not at home right now, the aforementioned testing
>> will have to wait a couple of days.
>>
>>> 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.
>>
>> Like i said, i don't want to hurry up and revert something that looks like a
>> valid fix.  Especially after five years worth of kernel releases and no
>> complaints related to 1a8deec09d12.  This should mean two things: a) the driver
>> isn't used anymore, or b) this commit fixes a real problem.
>>
>> However, if it turn out that your fix is the right one, it goes in without fuss.
>> So lets see what it is...
> 
> I agree. It is not my intention to break something. Take your time to
> test it when you find the time and let me know of the results. We are
> not in a hurry. I have my private fork of the driver for the longterm
> kernel.

I imported a LinkSys EtherFast 10/100 USB Network Adapter with model 
number USB100TX, FCC ID MQ4UFF1KA and revision B1 from the USA. 
According to the FCC photos and the source code of the driver, it is a 
pegasus I device. It also includes the FCS.

I can provide Ethernet and USB captures in private correspondence if 
someone is interested.

Kind regards,
Matthias-Christian Ott