Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1524067 > unrolled thread

RE: [PATCH net 1/2] r8152: fix the sw rx checksum is unavailable

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  RE: [PATCH net 1/2] r8152: fix the sw rx checksum is unavailable Hayes Wang <hayeswang@realtek.com> - 2016-11-17 04:40 +0100
    Re: [PATCH net 1/2] r8152: fix the sw rx checksum is unavailable Mark Lord <mlord@pobox.com> - 2016-11-17 15:30 +0100
    Re: [PATCH net 1/2] r8152: fix the sw rx checksum is unavailable Mark Lord <mlord@pobox.com> - 2016-11-17 15:40 +0100
    RE: [PATCH net 1/2] r8152: fix the sw rx checksum is unavailable Hayes Wang <hayeswang@realtek.com> - 2016-11-18 09:00 +0100
      Re: [PATCH net 1/2] r8152: fix the sw rx checksum is unavailable Mark Lord <mlord@pobox.com> - 2016-11-18 13:10 +0100
        Re: [PATCH net 1/2] r8152: fix the sw rx checksum is unavailable Mark Lord <mlord@pobox.com> - 2016-11-22 14:20 +0100
        RE: [PATCH net 1/2] r8152: fix the sw rx checksum is unavailable Hayes Wang <hayeswang@realtek.com> - 2016-11-23 05:00 +0100
          Re: [PATCH net 1/2] r8152: fix the sw rx checksum is unavailable Mark Lord <mlord@pobox.com> - 2016-11-23 14:50 +0100
            RE: [PATCH net 1/2] r8152: fix the sw rx checksum is unavailable Hayes Wang <hayeswang@realtek.com> - 2016-11-23 16:20 +0100
              Re: [PATCH net 1/2] r8152: fix the sw rx checksum is unavailable Mark Lord <mlord@pobox.com> - 2016-11-23 20:30 +0100
                RE: [PATCH net 1/2] r8152: fix the sw rx checksum is unavailable Hayes Wang <hayeswang@realtek.com> - 2016-11-24 04:30 +0100
                Re: [PATCH net 1/2] r8152: fix the sw rx checksum is unavailable Mark Lord <mlord@pobox.com> - 2016-11-24 13:40 +0100
                  RE: [PATCH net 1/2] r8152: fix the sw rx checksum is unavailable Hayes Wang <hayeswang@realtek.com> - 2016-11-24 14:30 +0100
                    Re: [PATCH net 1/2] r8152: fix the sw rx checksum is unavailable David Miller <davem@davemloft.net> - 2016-11-24 17:30 +0100
                      Re: [PATCH net 1/2] r8152: fix the sw rx checksum is unavailable Mark Lord <mlord@pobox.com> - 2016-11-24 17:50 +0100
                        Re: [PATCH net 1/2] r8152: fix the sw rx checksum is unavailable Mark Lord <mlord@pobox.com> - 2016-11-24 18:10 +0100
                          Re: [PATCH net 1/2] r8152: fix the sw rx checksum is unavailable David Miller <davem@davemloft.net> - 2016-11-24 18:20 +0100
                        Re: [PATCH net 1/2] r8152: fix the sw rx checksum is unavailable David Miller <davem@davemloft.net> - 2016-11-24 18:20 +0100
                          Re: [PATCH net 1/2] r8152: fix the sw rx checksum is unavailable Mark Lord <mlord@pobox.com> - 2016-11-24 19:40 +0100
                            Re: [PATCH net 1/2] r8152: fix the sw rx checksum is unavailable Mark Lord <mlord@pobox.com> - 2016-11-24 19:50 +0100
                            Re: [PATCH net 1/2] r8152: fix the sw rx checksum is unavailable Greg KH <greg@kroah.com> - 2016-11-24 20:10 +0100
                              Re: [PATCH net 1/2] r8152: fix the sw rx checksum is unavailable Greg KH <greg@kroah.com> - 2016-11-24 20:20 +0100
                              Re: [PATCH net 1/2] r8152: fix the sw rx checksum is unavailable Mark Lord <mlord@pobox.com> - 2016-11-24 20:20 +0100
                            Re: [PATCH net 1/2] r8152: fix the sw rx checksum is unavailable Francois Romieu <romieu@fr.zoreil.com> - 2016-11-25 01:30 +0100
                              Re: [PATCH net 1/2] r8152: fix the sw rx checksum is unavailable Mark Lord <mlord@pobox.com> - 2016-11-25 04:50 +0100
                        Re: [PATCH net 1/2] r8152: fix the sw rx checksum is unavailable Greg KH <gregkh@linuxfoundation.org> - 2016-11-24 19:50 +0100
                          Re: [PATCH net 1/2] r8152: fix the sw rx checksum is unavailable Mark Lord <mlord@pobox.com> - 2016-11-24 20:00 +0100
                        RE: [PATCH net 1/2] r8152: fix the sw rx checksum is unavailable Hayes Wang <hayeswang@realtek.com> - 2016-11-25 07:40 +0100
                          RE: [PATCH net 1/2] r8152: fix the sw rx checksum is unavailable Hayes Wang <hayeswang@realtek.com> - 2016-11-25 08:00 +0100
                    RE: [PATCH net 1/2] r8152: fix the sw rx checksum is unavailable Hayes Wang <hayeswang@realtek.com> - 2016-11-25 07:20 +0100
                  Re: [PATCH net 1/2] r8152: fix the sw rx checksum is unavailable David Miller <davem@davemloft.net> - 2016-11-24 17:30 +0100
            RE: [PATCH net 1/2] r8152: fix the sw rx checksum is unavailable Hayes Wang <hayeswang@realtek.com> - 2016-11-24 13:40 +0100

Page 1 of 2  [1] 2  Next page →

#1524067 — RE: [PATCH net 1/2] r8152: fix the sw rx checksum is unavailable

[...]
> Fix the hw rx checksum is always enabled, and the user couldn't switch
> it to sw rx checksum.
> 
> Note that the RTL_VER_01 only supports sw rx checksum only. Besides,
> the hw rx checksum for RTL_VER_02 is disabled after
> commit b9a321b48af4 ("r8152: Fix broken RX checksums."). Re-enable it.

Excuse me. If I want to re-send this one patch, should I let
RTL_VER_02 use rx hw checksum?

Best Regards,
Hayes

[toc] | [next] | [standalone]

#1524447

On 16-11-17 09:14 AM, Mark Lord wrote:
..
> Using coherent buffers (non-cacheable, allocated with usb_alloc_coherent),

Note that the same behaviour also happens with the original kmalloc'd buffers.

> I can get it to fail extremely regularly by simply reducing the buffer size
> (agg_buf_sz) from 16KB down to 4KB.   This makes reproducing the issue
> much much easier -- the same problems do happen with the larger 16KB size,
> but much less often than with smaller sizes.

Increasing the buffer size to 64KB makes the problem much less frequent,
as one might expect.  Thus far I haven't seen it happen at all, but a longer
run (1-3 days) is needed to make sure.  This however is NOT a "fix".

> So.. with a 4KB URB transfer_buffer size, along with a ton of added error-checking,
> I see this behaviour every 10 (rx) URBs or so:
>
> First URB (number 593):
> [   34.260667] r8152_rx_bottom: 593 corrupted urb: head=bf014000 urb_offset=2856/4096 pkt_len(1518) exceeds remainder(1216)
> [   34.271931] r8152_dump_rx_desc: 044805ee 40080000 006005dc 06020000 00000000 00000000 rx_len=1518
>
> Next URB (number 594):
> [   34.281172] r8152_check_rx_desc: rx_desc looks bad.
> [   34.286228] r8152_rx_bottom: 594 corrupted urb. head=bf018000 urb_offset=0/304 len_used=24
> [   34.294774] r8152_dump_rx_desc: 00008300 00008400 00008500 00008600 00008700 00008800 rx_len=768
>
> What the above sample shows, is the URB transfer buffer ran out of space in the middle
> of a packet, and the hardware then tried to just continue that same packet in the next URB,
> without an rx_desc header inserted.  The r8152.c driver always assumes the URB buffer begins
> with an rx_desc, so of course this behaviour produces really weird effects, and system crashes, etc..
>
> So until that driver bug is addressed, I would advise disabling hardware RX checksums
> for all chip versions, not only for version 02.
>
> It is not clear to me how the chip decides when to forward an rx URB to the host.
> If you could describe how that part works for us, then it would help in further
> understanding why fast systems (eg. a PC) don't generally notice the issue,
> while much slower embedded systems do see the issue regularly.

That last part is critical to understanding things:
How does the chip decide that a URB is "full enough" before sending it to the host?
Why does a really fast host see fewer packets jammed together into a single URB than a slower host?

The answers will help understand if there are more bugs to be found/fixed,
or if everything is explained by what has been observed thus far.

To recap:  the hardware sometimes fills a URB to the very end, and then continues the
current packet at the first byte of the following URB.  The r8152.c driver does NOT
handle this situation; instead it always interprets the first 24 bytes of every URB
as an "rx_desc" structure, without any kind of sanity/validation.  This results in
buffer overruns (it trusts the packet length field, even though the URB is too small
to hold such a packet), and other semi-random behaviour.

Using software rx checksums prevents Bad Things(tm) happening from most of this,
but even that is not perfect given the severity of the bug.

Cheers

[toc] | [prev] | [next] | [standalone]

#1524454

(resending.. not sure if the original had mailer errors)

On 16-11-16 10:36 PM, Hayes Wang wrote:
 > [...]
 >> Fix the hw rx checksum is always enabled, and the user couldn't switch
 >> it to sw rx checksum.
 >>
 >> Note that the RTL_VER_01 only supports sw rx checksum only. Besides,
 >> the hw rx checksum for RTL_VER_02 is disabled after
 >> commit b9a321b48af4 ("r8152: Fix broken RX checksums."). Re-enable it.
 >
 > Excuse me. If I want to re-send this one patch, should I let
 > RTL_VER_02 use rx hw checksum?

Definitely NOT.

I am still doing low-level tracing through the driver as time permits,
and just now found some really interesting evidence.

Using coherent buffers (non-cacheable, allocated with usb_alloc_coherent),
I can get it to fail extremely regularly by simply reducing the buffer size
(agg_buf_sz) from 16KB down to 4KB.   This makes reproducing the issue
much much easier -- the same problems do happen with the larger 16KB size,
but much less often than with smaller sizes.

So.. with a 4KB URB transfer_buffer size, along with a ton of added error-checking,
I see this behaviour every 10 (rx) URBs or so:

First URB (number 593):
[   34.260667] r8152_rx_bottom: 593 corrupted urb: head=bf014000 urb_offset=2856/4096 pkt_len(1518) exceeds remainder(1216)
[   34.271931] r8152_dump_rx_desc: 044805ee 40080000 006005dc 06020000 00000000 00000000 rx_len=1518

Next URB (number 594):
[   34.281172] r8152_check_rx_desc: rx_desc looks bad.
[   34.286228] r8152_rx_bottom: 594 corrupted urb. head=bf018000 urb_offset=0/304 len_used=24
[   34.294774] r8152_dump_rx_desc: 00008300 00008400 00008500 00008600 00008700 00008800 rx_len=768

What the above sample shows, is the URB transfer buffer ran out of space in the middle
of a packet, and the hardware then tried to just continue that same packet in the next URB,
without an rx_desc header inserted.  The r8152.c driver always assumes the URB buffer begins
with an rx_desc, so of course this behaviour produces really weird effects, and system crashes, etc..

So until that driver bug is addressed, I would advise disabling hardware RX checksums
for all chip versions, not only for version 02.

It is not clear to me how the chip decides when to forward an rx URB to the host.
If you could describe how that part works for us, then it would help in further
understanding why fast systems (eg. a PC) don't generally notice the issue,
while much slower embedded systems do see the issue regularly.

Thanks
Mark

[toc] | [prev] | [next] | [standalone]

#1525059

Mark Lord [mailto:mlord@pobox.com]
> Sent: Thursday, November 17, 2016 9:42 PM
[...]
> What the above sample shows, is the URB transfer buffer ran out of space in the
> middle
> of a packet, and the hardware then tried to just continue that same packet in the
> next URB,
> without an rx_desc header inserted.  The r8152.c driver always assumes the URB
> buffer begins
> with an rx_desc, so of course this behaviour produces really weird effects, and
> system crashes, etc..

The USB device wouldn't know the address and size of buffer. Only
the USB host controller knows. Therefore, the device sends the
data to host, and the host fills the memory. According to your
description, it seems the host splits the data from the device
into two different buffers (or URB transfers). I wonder if it would
occur. As far as I know, the host wouldn't allow the buffer size
less than the data length.

Our hw engineers need the log from the USB analyzer to confirm
what the device sends to the host. However, I don't think you
have USB analyzer to do this. I would try to reproduce the issue.
But, I am busy, so I don't think I would response quickly.

Besides, the maximum data length which the RTL8152 would send to
the host is 16KB. That is, if the agg_buf_sz is 16KB, the host
wouldn't split it. However, you still see problems for it.

[...]
> It is not clear to me how the chip decides when to forward an rx URB to the host.
> If you could describe how that part works for us, then it would help in further
> understanding why fast systems (eg. a PC) don't generally notice the issue,
> while much slower embedded systems do see the issue regularly.

The driver expects the rx buffer would be

	rx_desc + a packet + padding to 8 alignment + 
	rx_desc + a packet + padding to 8 alignment + ...
	
Therefore, when a urb transfer is completed, the driver parsers
the buffer by this way. After the buffer is handled, it would
be submitted to the host, until the transfer is completed again.
If the submitting fail, the driver would try again later. The
urb->actual_length means how much data the host fills. The drive
uses it to check the end of the data. The urb->status mean if
the transfer is successful. The driver submits the urb to the
host directly if the status is not successful.

Best Regards,
Hayes

[toc] | [prev] | [next] | [standalone]

#1525239

On 16-11-18 02:57 AM, Hayes Wang wrote:
..
> Besides, the maximum data length which the RTL8152 would send to
> the host is 16KB. That is, if the agg_buf_sz is 16KB, the host
> wouldn't split it. However, you still see problems for it.

How does the RTL8152 know that the limit is 16KB,
rather than some other number?  Is this a hardwired number
in the hardware, or is it a parameter that the software
sends to the chip during initialization?

I have a USB analyzer, but it is difficult to figure out how
to program an appropriate trigger point for the capture,
since the problem (with 16KB URBs) takes minutes to hours
or even days to trigger.

And the output from the analyzer is in some proprietary format.
The in-kernel software analzer could be useful, but I have never
figured out how to use it.  :)

Since my earlier email, I have figured out another piece of the
puzzle with this dongle.

The first issue is that a packet sometimes begins in one URB,
and completes in the next URB, without an rx_desc at the start
of the second URB.  This I have already reported earlier.

But the driver, as written, sometimes accesses bytes outside
of the 16KB URB buffer, because it trusts the non-existent
rx_desc in these cases, and also because it accesses bytes
from the rx_desc without first checking whether there is
sufficient remaining space in the URB to hold an rx_desc.

These incorrect accesses sometimes touch memory outside
of the URB buffer.  Since the driver allocates all of its
rx URB buffers at once, they are highly likely to be
physically (and therefore virtually) adjacent in memory.

So mistakenly accessing beyond the end of one buffer will
often result in a read from memory of the next URB buffer.
Which causes a portion of it to be loaded in the the D-cache.

When that URB is subsequently filled by DMA, there then exists
a data-consistency issue:  the D-cache contains stale information
from before the latest DMA cycle.

So this explains the strange memory behaviour observed earlier on.
When I add a call to invalidate_dcache_range() to the driver
just before it begins examining a new rx URB, the problems go away.
So this confirms the observations.

Using non-cacheable RAM also makes the problem go away.
But neither is a fix for the real buffer overrun accesses in the driver.

Fix the "packet spans URBs" bug, and fix the driver to ALWAYS
test lengths/ranges before accessing the actual buffer,
and everything should begin working reliably.

Cheers
-- 
Mark Lord
Real-Time Remedies Inc.
mlord@pobox.com

[toc] | [prev] | [next] | [standalone]

#1527502

On 16-11-18 07:03 AM, Mark Lord wrote:
> On 16-11-18 02:57 AM, Hayes Wang wrote:
> ..
>> Besides, the maximum data length which the RTL8152 would send to
>> the host is 16KB. That is, if the agg_buf_sz is 16KB, the host
>> wouldn't split it. However, you still see problems for it.
>
> How does the RTL8152 know that the limit is 16KB,
> rather than some other number?  Is this a hardwired number
> in the hardware, or is it a parameter that the software
> sends to the chip during initialization?
..
> The first issue is that a packet sometimes begins in one URB,
> and completes in the next URB, without an rx_desc at the start
> of the second URB.  This I have already reported earlier.

Long run tests over the weekend, with the invalidate_dcache_range() call
before the inner loop of r8152_rx_bottom(), turned up a few instances
where packets were truncated inside a 16384 byte URB buffer, without filling the URB.

[10.293228] r8152_rx_bottom: 4278 corrupted urb: head=9d210000 urb_offset=2856/3376 pkt_len(1518) exceeds remainder(496)
[10.304523] r8152_dump_rx_desc: 044805ee 40080000 006005dc 06020000 00000000 00000000 rx_len=1518
..
[   16.660431] r8152_rx_bottom: 7802 corrupted urb: head=9d1f8000 urb_offset=1544/2064 pkt_len(1518) exceeds remainder(496)
[   16.671719] r8152_dump_rx_desc: 044805ee 40480000 004005dc 46020006 00000000 00000000 rx_len=1518

The r8152.c driver attempted to build skb's for the entire packet size,
even though the 1518-byte packets had only 496-bytes of data in the URB.
It is not clear what the chip did with the rest of the packets in question,
but the next URBs in each case began with a new/real rx_desc and new packet.

There were also unconnected events during the test runs where the
test code noticed totally invalid rx_desc structs in the middles of URBs.
The stock driver would again have attempted to treat those as "valid" (ugh).

..
[   10.273906] r8152_check_rx_desc: rx_desc looks bad.
[   10.279012] r8152_rx_bottom: 4338 corrupted urb. head=9d210000 urb_offset=2856/3376 len_used=2880
[   10.288196] r8152_dump_rx_desc: 312e3239 382e3836 0a20382e 3d435253 3034336d 202f3a30 rx_len=12857

..
[    7.184565] r8152_check_rx_desc: rx_desc looks bad.
[    7.189657] r8152_rx_bottom: 1678 corrupted urb. head=9d210000 urb_offset=2856/3376 len_used=2880
[    7.198852] r8152_dump_rx_desc: a1388402 803c9001 84380810 a67c5c4c a77c782b c64c782b rx_len=1026
..
[   10.351251] r8152_check_rx_desc: rx_desc looks bad.
[   10.356356] r8152_rx_bottom: 4397 corrupted urb. head=9d20c000 urb_offset=4400/7984 len_used=4424
[   10.365543] r8152_dump_rx_desc: 312e3239 382e3836 0a20382e 3d435253 3034336d 202f3a30 rx_len=12857
..
[   10.518119] r8152_check_rx_desc: rx_desc looks bad.
[   10.523204] r8152_rx_bottom: 4458 corrupted urb. head=9d210000 urb_offset=4400/7984 len_used=4424
[   10.532416] r8152_dump_rx_desc: 54544120 6e3d5352 636f6c6f 65762c6b 343d7372 6464612c rx_len=16672
..

> But the driver, as written, sometimes accesses bytes outside
> of the 16KB URB buffer, because it trusts the non-existent
> rx_desc in these cases, and also because it accesses bytes
> from the rx_desc without first checking whether there is
> sufficient remaining space in the URB to hold an rx_desc.
>
> These incorrect accesses sometimes touch memory outside
> of the URB buffer.  Since the driver allocates all of its
> rx URB buffers at once, they are highly likely to be
> physically (and therefore virtually) adjacent in memory.
>
> So mistakenly accessing beyond the end of one buffer will
> often result in a read from memory of the next URB buffer.
> Which causes a portion of it to be loaded in the the D-cache.
>
> When that URB is subsequently filled by DMA, there then exists
> a data-consistency issue:  the D-cache contains stale information
> from before the latest DMA cycle.
>
> So this explains the strange memory behaviour observed earlier on.
> When I add a call to invalidate_dcache_range() to the driver
> just before it begins examining a new rx URB, the problems go away.
> So this confirms the observations.
>
> Using non-cacheable RAM also makes the problem go away.
> But neither is a fix for the real buffer overrun accesses in the driver.
>
> Fix the "packet spans URBs" bug, and fix the driver to ALWAYS
> test lengths/ranges before accessing the actual buffer,
> and everything should begin working reliably.

[toc] | [prev] | [next] | [standalone]

#1528118

Mark Lord [mailto:mlord@pobox.com]
> Sent: Friday, November 18, 2016 8:03 PM
[..]
> How does the RTL8152 know that the limit is 16KB,
> rather than some other number?  Is this a hardwired number
> in the hardware, or is it a parameter that the software
> sends to the chip during initialization?

It is the limitation of the hardware.

> I have a USB analyzer, but it is difficult to figure out how
> to program an appropriate trigger point for the capture,
> since the problem (with 16KB URBs) takes minutes to hours
> or even days to trigger.

It is good. Our hw engineers real want it. Maybe you could send
a specific packet, and trigger it. You could allocate a skb and
fill the data which you prefer, and call

	skb_queue_tail(&tp->tx_queue, skb);

[...]
> The first issue is that a packet sometimes begins in one URB,
> and completes in the next URB, without an rx_desc at the start
> of the second URB.  This I have already reported earlier.

However, our hw engineer says it wouldn't happen. Our hw always
sends rx_desc + packet + padding. The hw wouldn't split it to
two or more transmission. That is why I wonder who does it.

> But the driver, as written, sometimes accesses bytes outside
> of the 16KB URB buffer, because it trusts the non-existent
> rx_desc in these cases, and also because it accesses bytes
> from the rx_desc without first checking whether there is
> sufficient remaining space in the URB to hold an rx_desc.

I think I check them. According to the followning code,

	list_for_each_safe(cursor, next, &rx_queue) {
		struct rx_desc *rx_desc;
		struct rx_agg *agg;
		int len_used = 0;
		struct urb *urb;
		u8 *rx_data;

		...

		rx_desc = agg->head;
		rx_data = agg->head;
		len_used += sizeof(struct rx_desc); //<-- add the size of next rx_desc

		while (urb->actual_length > len_used) {
			struct net_device *netdev = tp->netdev;
			struct net_device_stats *stats = &netdev->stats;
			unsigned int pkt_len;
			struct sk_buff *skb;

			pkt_len = le32_to_cpu(rx_desc->opts1) & RX_LEN_MASK;
			if (pkt_len < ETH_ZLEN)
				break;

			len_used += pkt_len;
			if (urb->actual_length < len_used)
				break;

			pkt_len -= CRC_SIZE;
			rx_data += sizeof(struct rx_desc);

			...

find_next_rx:
			rx_data = rx_agg_align(rx_data + pkt_len + CRC_SIZE);
			rx_desc = (struct rx_desc *)rx_data;
			len_used = (int)(rx_data - (u8 *)agg->head);
			len_used += sizeof(struct rx_desc); //<-- add the size of next rx_desc
		}

submit:
		...
	}

The while loop would check if the next rx_desc is inside the urb
buffer, because the len_used includes the size of the next rx_desc.
Then, in the while loop, the len_used adds the packet size and check
with urb->actual_length again. These make sure the rx_desc and the
packet are inside the urb buffer. Except the urb->actual_length
is more than agg_buf_sz. However, I don't think it would happen.

Best Regards,
Hayes

[toc] | [prev] | [next] | [standalone]

#1528426

What does this code do:

>static void r8153_set_rx_early_size(struct r8152 *tp)
>{
>        u32 mtu = tp->netdev->mtu;
>        u32 ocp_data = (agg_buf_sz - mtu - VLAN_ETH_HLEN - VLAN_HLEN) / 4;
>
>        ocp_write_word(tp, MCU_TYPE_USB, USB_RX_EARLY_SIZE, ocp_data);
>}

How is ocp_data used by the hardware?
Shouldn't the calculation also include sizeof(rx_desc) in there somewhere?

Thanks
-- 
Mark Lord
Real-Time Remedies Inc.
mlord@pobox.com

[toc] | [prev] | [next] | [standalone]

#1528492

Mark Lord [mlord@pobox.com]
[...]
> What does this code do:

> >static void r8153_set_rx_early_size(struct r8152 *tp)
> >{
> >        u32 mtu = tp->netdev->mtu;
> >        u32 ocp_data = (agg_buf_sz - mtu - VLAN_ETH_HLEN - VLAN_HLEN) / 4;
> >
> >        ocp_write_word(tp, MCU_TYPE_USB, USB_RX_EARLY_SIZE, ocp_data);
> >}

This only works for RTL8153. However, what you use is RTL8152.
It is like delay completion. It is used to reduce the loading of CPU
by letting a transfer contain more data to reduce the number of
transfers.

> How is ocp_data used by the hardware?
> Shouldn't the calculation also include sizeof(rx_desc) in there somewhere?

The algorithm is from our hw engineers, and it should be 

   (agg_buf_sz - packet size) / 8

You could refer to commit a59e6d815226 ("r8152: correct the rx early size").

[toc] | [prev] | [next] | [standalone]

#1528707

On 16-11-23 10:12 AM, Hayes Wang wrote:
> Mark Lord [mlord@pobox.com]
> [...]
>> What does this code do:
>
>>> static void r8153_set_rx_early_size(struct r8152 *tp)
>>> {
>>>        u32 mtu = tp->netdev->mtu;
>>>        u32 ocp_data = (agg_buf_sz - mtu - VLAN_ETH_HLEN - VLAN_HLEN) / 4;
>>>
>>>        ocp_write_word(tp, MCU_TYPE_USB, USB_RX_EARLY_SIZE, ocp_data);
>>> }
>
> This only works for RTL8153. However, what you use is RTL8152.
> It is like delay completion. It is used to reduce the loading of CPU
> by letting a transfer contain more data to reduce the number of
> transfers.
>
>> How is ocp_data used by the hardware?
>> Shouldn't the calculation also include sizeof(rx_desc) in there somewhere?
>
> The algorithm is from our hw engineers, and it should be
>
>    (agg_buf_sz - packet size) / 8
>
> You could refer to commit a59e6d815226 ("r8152: correct the rx early size").

Thanks.

Right now I am working quite hard trying to narrow things down exactly.
You are correct that the driver does appear to be careful about accesses
beyond the filled portion of a URB buffer -- for some reason I thought
the original driver had issues there, but looking again it does not seem to.

One idea that is now looking more likely:
Things could be suffering from speculative CPU accesses to RAM
(the system here has non-coherent d-cache/RAM).
This could incorrectly pre-load data from adjacent URB buffers
into the d-cache, creating coherency issues.  I am testing now
with cacheline-sized guard zones between the buffers to see if
that is the issue or not.

Worth repeating: other dongles we have tried, eg. those using the asix driver,
do not cause us any troubles here.  Only the r8152 dongles do.

The other drivers do not use hardware checksums, so even if they did
incur similar bad packets, whatever the reason, those bad packets
would be detected/rejected by the Linux network stack (software checksums).
So everything appears to behave fine with them, as it does with
the r8152 driver when hardware checksums are disabled.

Still trying to understand exactly how these errors are happening.
It takes a very long time to do a conclusive test of anything here,
and I only have the hardware for a day or two a week.
So my apologies if I am slow in getting back to you on stuff.

Cheers

[toc] | [prev] | [next] | [standalone]

#1528940

Mark Lord [mailto:mlord@pobox.com]
> Sent: Thursday, November 24, 2016 3:30 AM
[...]
> Worth repeating: other dongles we have tried, eg. those using the asix driver,
> do not cause us any troubles here.  Only the r8152 dongles do.

I couldn't tell you why you would see the problem. I have tested the
RTL8152 on raspberry pi platform with iperf more than 17 hours. And
I don't see any invalid rx descriptor. I don't think it really is the
issue about our hw.

Best Regards,
Hayes

[toc] | [prev] | [next] | [standalone]

#1529246

On 16-11-23 02:29 PM, Mark Lord wrote:
> On 16-11-23 10:12 AM, Hayes Wang wrote:
>> Mark Lord [mlord@pobox.com]
>> [...]
>>> What does this code do:
>>
>>>> static void r8153_set_rx_early_size(struct r8152 *tp)
>>>> {
>>>>        u32 mtu = tp->netdev->mtu;
>>>>        u32 ocp_data = (agg_buf_sz - mtu - VLAN_ETH_HLEN - VLAN_HLEN) / 4;
>>>>
>>>>        ocp_write_word(tp, MCU_TYPE_USB, USB_RX_EARLY_SIZE, ocp_data);
>>>> }
>>
>> This only works for RTL8153. However, what you use is RTL8152.
>> It is like delay completion. It is used to reduce the loading of CPU
>> by letting a transfer contain more data to reduce the number of
>> transfers.
>>
>>> How is ocp_data used by the hardware?
>>> Shouldn't the calculation also include sizeof(rx_desc) in there somewhere?
>>
>> The algorithm is from our hw engineers, and it should be
>>
>>    (agg_buf_sz - packet size) / 8
>>
>> You could refer to commit a59e6d815226 ("r8152: correct the rx early size").
>
> Thanks.
>
> Right now I am working quite hard trying to narrow things down exactly.
> You are correct that the driver does appear to be careful about accesses
> beyond the filled portion of a URB buffer -- for some reason I thought
> the original driver had issues there, but looking again it does not seem to.
>
> One idea that is now looking more likely:
> Things could be suffering from speculative CPU accesses to RAM
> (the system here has non-coherent d-cache/RAM).
> This could incorrectly pre-load data from adjacent URB buffers
> into the d-cache, creating coherency issues.  I am testing now
> with cacheline-sized guard zones between the buffers to see if
> that is the issue or not.

Nope.  Guard zones did not fix it, so it's probably not a prefetch issue.
Oddly, adding a couple of memory barriers to specific places in the driver
does help, A LOT.  Still not 100%, but it did pass 1800 reboot tests over night
with only three bad rx_desc's reported.

That's a new record here for the driver using kmalloc'd buffers,
and put reliability on par with using non-cacheable buffers.

Any way we look at it though, the chip/driver are simply unreliable,
and relying upon hardware checksums (which fail due to the driver
looking at garbage rather than the checksum bits) leads to data corruption.

Cheers

[toc] | [prev] | [next] | [standalone]

#1529271

Mark Lord [mailto:mlord@pobox.com]
> Sent: Thursday, November 24, 2016 8:31 PM
[...]
> Nope.  Guard zones did not fix it, so it's probably not a prefetch issue.
> Oddly, adding a couple of memory barriers to specific places in the driver
> does help, A LOT.  Still not 100%, but it did pass 1800 reboot tests over night
> with only three bad rx_desc's reported.
> 
> That's a new record here for the driver using kmalloc'd buffers,
> and put reliability on par with using non-cacheable buffers.
> 
> Any way we look at it though, the chip/driver are simply unreliable,
> and relying upon hardware checksums (which fail due to the driver
> looking at garbage rather than the checksum bits) leads to data corruption.

I don't think the garbage results from our driver or device.
If it is the issue about memory, I think the host driver ought
to deal with it, because it handles the DMA.

Besides, it doesn't seem to occur for all platforms. I have
tested the iperf more than 26 hours, and it still works fine.
I think I would get the same result on x86 or x86_64 platform.

Best Regards,
Hayes

[toc] | [prev] | [next] | [standalone]

#1529543

From: Hayes Wang <hayeswang@realtek.com>
Date: Thu, 24 Nov 2016 13:26:55 +0000

> I don't think the garbage results from our driver or device.

This is my impression with what has been presented so far as well.

[toc] | [prev] | [next] | [standalone]

#1529566

On 16-11-24 11:21 AM, David Miller wrote:
> From: Hayes Wang <hayeswang@realtek.com>
> Date: Thu, 24 Nov 2016 13:26:55 +0000
>
>> I don't think the garbage results from our driver or device.
> This is my impression with what has been presented so far as well.

It's not garbage.

The latest run with the debug code I posted here earlier just spat out this below.
Using coherent (guarded, non-cacheable) RX buffers, with mb() calls:

[   15.199157] r8152_check_rx_desc: rx_desc looks bad.
[   15.204270] r8152_rx_bottom: offset=0/3376 bad rx_desc
[   15.209584] r8152_dump_rx_desc: 3d435253 3034336d 202f3a30 47524154 2f3d5445 3034336d rx_len=21075

The bad data in this case is ASCII:

         "SRC=m3400:/ TARGET=/m340"

This data is what is seen in /run/mount/utab, a file that is read/written over NFS on each boot.

         "SRC=m3400:/ TARGET=/m3400 ROOT=/ ATTRS=nolock,addr=192.168.8.1\n"

But how does this ASCII data end up at offset zero of the rx buffer??
Not possible -- this isn't even stale data, because only an rx_desc could
be at that offset in that buffer.

So even if this were a platform memory coherency issue, one should still
never see ASCII data at the beginning of an rx buffer.  The driver NEVER
writes anything to the rx buffers.  Only the USB hardware ever does.

And only the r8152 dongle/driver exhibits this issue.
Other USB dongles do not.  They *might* still have such issues,
but because they use software checksums, the bad packets are caught/rejected.

The r8152 driver, without the debug/error-checking additions, would have tried
to interpret that ASCII data as an "rx_desc", and would have interpreted the
"checksum bits" therein as "valid checksum", and the packet would have passed
through the network stack, corrupting data.

This driver worked without noticeable issues in 3.12.xx.
It hasn't worked since.  Because it now trusts the hardware checksums,
without first checking to see if noise-on-the-line or something else
has corrupted the data before receipt in the rx buffer.

Based on the above capture, I suspect a bug in the chip itself, which perhaps
is only manifest on a very slow CPU.

Nobody here tests with slow CPUs, but they are very prevalent in embedded space.
And very few people use USB network dongles nowadays either, as nearly all "computers"
have built-in networking.  The market for USB network dongles is mostly embedded space.

Ergo.

Cheers

[toc] | [prev] | [next] | [standalone]

#1529578

On 16-11-24 11:43 AM, Mark Lord wrote:
..
> But how does this ASCII data end up at offset zero of the rx buffer??
> Not possible -- this isn't even stale data, because only an rx_desc could
> be at that offset in that buffer.

Answering my own question here, I suspect it ends up there as a result
of overrunning the previous URB.  So I have updated the test copy of the driver
here now to check for that exact situation.  It's running now, but could take
hours or a day for the bug to occur again.

It seems I am being overly helpful here.

Perhaps I should have just stopped with the original regression report
(driver works in 3.12.xx, fails on all newer kernels, as a result of enabling
hardware checksums).

Had I left it there, one might reasonably expect the onus to be on the driver
developer to sort it out, with me providing retests of supplied patches as need be.

But I've gone WAY BEYOND that, even questioning the sanity of the platform on
which it is being used, just to avoid blaming a buggy USB dongle for some other issue.
And this is leading people to suspect that I really think the platform is buggy.

It isn't.   It's been running for years, with a variety of USB hardware attached,
and nary a problem.  Except with this r8152 dongle on kernels > 3.12.

So, yeah, the driver is fixed in our local tree, and has been for some time now.
I just was hoping that perhaps others might be interested in it too,
since the bug (whatever it is) corrupts data on the NFS server.

Cheers

[toc] | [prev] | [next] | [standalone]

#1529584

From: Mark Lord <mlord@pobox.com>
Date: Thu, 24 Nov 2016 12:00:15 -0500

> It seems I am being overly helpful here.

Either you want to cry or you want to keep helping us track down
this problem.  It is your choice, and your choice alone.

Please do not pretend otherwise, everyone else in this thread is
operating with the best intentions and wants to see this through
to a full analysis and a proper solution for the corruptions.

Thank you.

[toc] | [prev] | [next] | [standalone]

#1529580

From: Mark Lord <mlord@pobox.com>
Date: Thu, 24 Nov 2016 11:43:53 -0500

> So even if this were a platform memory coherency issue, one should
> still never see ASCII data at the beginning of an rx buffer.

I'm not so convinced, since this is the kind of random corruption one
would expect to see when dealing with virtual caches that have
aliasing or similar issues.

Writes to address X that show up at address Y or not at all are
precisely the signature of virtual cache aliasing problems.

Is it a case of the chip writing to X but the cpu is still seeing
stale data from a previous CPU store?

For NFS the cpu is writing into the page cache, so we know that
cpu side stores are where the ASCII text is coming from.

Now is the r8152 buffer one that the USB host controller is DMA'ing
into directly, or is it one that SWIOMMU or similar bounce buffering
is copying into?  In the latter case we are doing cpu stores into
the area and the writes aren't coming from the device.

[toc] | [prev] | [next] | [standalone]

#1529614

On 16-11-24 12:11 PM, David Miller wrote:
> From: Mark Lord <mlord@pobox.com>
> Date: Thu, 24 Nov 2016 11:43:53 -0500
> 
>> So even if this were a platform memory coherency issue, one should
>> still never see ASCII data at the beginning of an rx buffer.
> 
> I'm not so convinced, since this is the kind of random corruption one
> would expect to see when dealing with virtual caches that have
> aliasing or similar issues.
> 
> Writes to address X that show up at address Y or not at all are
> precisely the signature of virtual cache aliasing problems.
> 
> Is it a case of the chip writing to X but the cpu is still seeing
> stale data from a previous CPU store?
> 
> For NFS the cpu is writing into the page cache, so we know that
> cpu side stores are where the ASCII text is coming from.
> 
> Now is the r8152 buffer one that the USB host controller is DMA'ing
> into directly, or is it one that SWIOMMU or similar bounce buffering
> is copying into?  In the latter case we are doing cpu stores into
> the area and the writes aren't coming from the device.

From tracing through the powerpc arch code, this is the buffer that
is being directly DMA'd into.  And the USB layer does an invalidate_dcache
on that entire buffer before initiating the DMA (confirmed via printk).

The driver itself NEVER writes anything to that buffer,
and nobody else has a pointer to it other than the USB host controller,
so there's nothing else that can write to it either.

According to the driver writer, the chip should only ever write a fresh
rx_desc struct at the beginning of a buffer, never ASCII data.

So how does that buffer end up containing ASCII data from the NFS transfers?

The only explanation I can see, is if the URB itself contains
the data that we see in the URB buffer.  Which is what one would expect.
So for that to happen, the ethernet chip must be transferring that data.

The thing that is special about the situation here, is that the processor
is very slow (800Mhz 32-bit powerpc), and very busy elsewhere.
So it can easily fall way behind in servicing the ethernet dongle,
something that never happens with most modern faster machines.
So perhaps this results in a FIFO overflow somewhere in the chip.

We can boot/run this same machine from a USB memory stick, and nary a problem.
Ditto for other types of ethernet dongles.
But boot/run from that specific ethernet dongle, and we get regular
random segfaults from corrupted page fetches over NFS.

The only end-to-end data integrity available here is the rx checksum,
when verified by software rather than trusting it to the chip/driver.

One thought:  bulk data streams are byte streams, not packets.
Scheduling on the USB bus can break up larger transfers across
multiple in-kernel buffers.  A "real" URB buffer on USB2 is max 512 bytes.
The driver is providing 16384-byte buffers, and assumes that data will
never spill over from one such buffer to the next.
Yet the observations here consistently show otherwise.

Cheers
-- 
Mark Lord

[toc] | [prev] | [next] | [standalone]

#1529617

On 16-11-24 01:34 PM, Mark Lord wrote:
>From tracing through the powerpc arch code, this is the buffer that
> is being directly DMA'd into.  And the USB layer does an invalidate_dcache
> on that entire buffer before initiating the DMA (confirmed via printk).

Slight correction:  the invalidate_dcache_range() is only done when
using kmalloc'd buffers.  I have converted the driver here
to use usb_alloc_coherent() instead, so that now gets skipped
since the memory is never cached.

[toc] | [prev] | [next] | [standalone]

Page 1 of 2  [1] 2  Next page →

Back to top | Article view | linux.kernel

csiph-web