Re: [PATCH] Bluetooth: hci_ldisc: Allow sleeping while proto locks are held.
Hi Ronald,
Sorry for my delay in replying to you.
On 18/10/17 03:00, Life is hard, and then you die wrote:
In fact, hci_uart_tty_close() is really a bit of a mess because it
progressively removes resources. It is is based on old code which has been
patched over the many years. Therefore it has kept code which is probably
obsolete or duplicated. Ideally, hci_uart_tty_close() should be rewritten.
For example, there is a call
cancel_work_sync(&hu->write_work);
in hci_uart_tty_close() which at first glance seems to be helpful but it is
flawed because hci_uart_tx_wakeup() can schedule a new work item AFTER
cancel_work_sync(&hu->write_work) runs. Therefore, locking is needed to
prevent hci_uart_tx_wakeup() from being scheduled whilst
HCI_UART_PROTO_READY is being cleared in hci_uart_tty_close().
Actually, I think there's still problem: in hci_uart_tty_close()
cancel_work_sync() is called before the HCI_UART_PROTO_READY flag is
cleared, opening the following race:
P1P2
cancel_work_sync()
hci_uart_tx_wakeup()
clear_bit(HCI_UART_PROTO_READY)
clean up
hci_uart_write_work()
Yes, this looks bad. There is some protection in hci_uart_write_work()
because hci_uart_dequeue(hu) checks HCI_UART_PROTO_READY but it will not
be foolproof due to resources being removed by hci_uart_tty_close().
So AFAICT cancel_work_sync() needs to be done after clearing the flag:
if (test_bit(HCI_UART_PROTO_READY, &hu->flags)) {
write_lock_irqsave(&hu->proto_lock, flags);
clear_bit(HCI_UART_PROTO_READY, &hu->flags);
write_unlock_irqrestore(&hu->proto_lock, flags);
cancel_work_sync(&hu->write_work); // <---
I agree with this solution. I was going to suggest this but you beat me
to it ;-)
if (hdev) {
(if HCI_UART_PROTO_READY is already clear, then no work can be pending,
so no need to cancel work in that case).
I agree with your statement.
Regards,
Dean
--
Dean Jenkins
Embedded Software Engineer
Linux Transportation Solutions
Mentor Embedded Software Division
Mentor Graphics (UK) Ltd.
Re: [PATCH] Bluetooth: hci_ldisc: Allow sleeping while proto locks are held.
Hi Ronald,
On 15/10/17 11:40, =?UTF-8?q?Ronald=20Tschal=C3=A4r?= wrote:
Lastly, the read-lock in the hci_uart_tx_wakeup() callback now needed
to be removed because that function is called from an IRQ context. But
since it doesn't actually call any proto functions, instead just
queueing the work, and the other operations are atomic bit operations,
this is ok.
You are discussing my commits so I'll try to answer your queries. I am a
bit busy but I'll attempt to answer you in a timely manner.
Sorry, I know nothing about bcm, I will explain using BCSP scenarios.
I use SMP ARM based embedded systems in a commercial environment. These
systems can suffer heavy CPU loading which can expose race conditions.
The crashes are very rare in a PC environment.
I will try to explain why the locking is needed in hci_uart_tx_wakeup().
For reference, I am using the HEAD of Linux 4.14-rc4
There is at least 1 race condition to consider. The atomic variables do
not help because the codeblock as a whole is not atomic. Hence locking
is needed as follows.
The issue is that hci_uart_tty_close() runs asynchronously to
hci_uart_tx_wakeup() which is shown below (assuming a SMP system).
int hci_uart_tx_wakeup(struct hci_uart *hu)
{
read_lock(&hu->proto_lock);
In parallel, hci_uart_tty_close() can be running here:
if (!test_bit(HCI_UART_PROTO_READY, &hu->flags))
goto no_schedule;
In parallel, hci_uart_tty_close() can be running here:
if (test_and_set_bit(HCI_UART_SENDING, &hu->tx_state)) {
In parallel, hci_uart_tty_close() can be running here:
set_bit(HCI_UART_TX_WAKEUP, &hu->tx_state);
goto no_schedule;
}
BT_DBG("");
In parallel, hci_uart_tty_close() can be running here:
schedule_work(&hu->write_work);
In parallel, hci_uart_tty_close() can be running here:
no_schedule:
read_unlock(&hu->proto_lock);
return 0;
}
hci_uart_tty_close() progressively removes the resources needed by the
scheduled work queue hu->write_work which runs hci_uart_write_work().
Also there is a delay between the scheduling and hci_uart_write_work
actually running. This means hci_uart_write_work() can race with
hci_uart_tty_close(), sometimes causing hci_uart_tty_close() to crash,
for example due to the write buffer no longer being there.
static void hci_uart_write_work(struct work_struct *work)
{
set_bit(TTY_DO_WRITE_WAKEUP, &tty->flags);
len = tty->ops->write(tty, skb->data, skb->len); <== can
crash inside the write function pointer as the write buffer is no longer
valid
hdev->stat.byte_tx += len; <== can crash here as hdev now
invalid
}
Therefore, a robust solution is to lock out hci_uart_tty_close() when
hci_uart_tx_wakeup() runs, that is the reason why
read_lock(&hu->proto_lock) is used in hci_uart_write_work(). The atomic
flag HCI_UART_PROTO_READY is prevented from being cleared whilst
hci_uart_tx_wakeup() runs.
In fact, hci_uart_tty_close() is really a bit of a mess because it
progressively removes resources. It is is based on old code which has
been patched over the many years. Therefore it has kept code which is
probably obsolete or duplicated. Ideally, hci_uart_tty_close() should be
rewritten.
For example, there is a call
cancel_work_sync(&hu->write_work);
in hci_uart_tty_close() which at first glance seems to be helpful but
it is flawed because hci_uart_tx_wakeup() can schedule a new work item
AFTER cancel_work_sync(&hu->write_work) runs. Therefore, locking is
needed to prevent hci_uart_tx_wakeup() from being scheduled whilst
HCI_UART_PROTO_READY is being cleared in hci_uart_tty_close().
In the case of BCSP, hci_bscp.c calls hci_uart_tx_wakeup() to schedule
the transmission of BCSP frames. This includes BCSP retransmissions.
Therefore, the scenario where a BCSP retransmission is being scheduled
whilst hci_uart_tty_close() runs needs to be protected to avoid crashes.
BCSP has a re-transmission timer which can expire whilst
hci_uart_tty_close() runs.
In older kernels without my commits, when BCSP attempts to transmit
during the execution of hci_uart_tty_close(), it can result in crashes
as outlined above. The flag HCI_UART_PROTO_READY now prevents
transmission and error messages can sometimes be seen from
hci_uart_send_frame() because transmission is prevented.
It is possible that my commits might have been too robust so perhaps
some simplification is possible.
Feel free to send me further questions about my commits. I will try to
answer your queries within a couple of days each time.
Regards,
Dean
--
Dean Jenkins
Embedded Software Engineer
Linux Transportation Solutions
Mentor Embedded Software Division
Mentor Graphics (UK) Ltd.
Re: [PATCH] asix: Fix offset calculation in asix_rx_fixup() causing slow transmissions
Hi John,
Thanks for your patch. I think the patch has already been applied.
The git commit subject of
"asix: Fix offset calculation in asix_rx_fixup() causing slow transmissions"
I think is a bit misleading as the bug relates to reception and not
transmission.
I guess that your intent was to say that "the through-put of
communications was low" due to the bug.
Personally, I would of used a git subject like
"asix: Fix asix_rx_fixup_interval() offset calculation for spanned frames"
But anyway, I have no real issue with the patch.
On 17/05/16 04:36, John Stultz wrote:
In testing with HiKey, we found that since
commit 3f30b158eba5 ("asix: On RX avoid creating bad Ethernet
frames"),
we're seeing lots of noise during network transfers:
[ 239.027993] asix 1-1.1:1.0 eth0: asix_rx_fixup() Data Header synchronisation
was lost, remaining 988
[ 239.037310] asix 1-1.1:1.0 eth0: asix_rx_fixup() Bad Header Length
0x54ebb5ec, offset 4
[ 239.045519] asix 1-1.1:1.0 eth0: asix_rx_fixup() Bad Header Length
0xcdffe7a2, offset 4
[ 239.275044] asix 1-1.1:1.0 eth0: asix_rx_fixup() Data Header synchronisation
was lost, remaining 988
[ 239.284355] asix 1-1.1:1.0 eth0: asix_rx_fixup() Bad Header Length
0x1d36f59d, offset 4
[ 239.292541] asix 1-1.1:1.0 eth0: asix_rx_fixup() Bad Header Length
0xaef3c1e9, offset 4
[ 239.518996] asix 1-1.1:1.0 eth0: asix_rx_fixup() Data Header synchronisation
was lost, remaining 988
[ 239.528300] asix 1-1.1:1.0 eth0: asix_rx_fixup() Bad Header Length
0x2881912, offset 4
[ 239.536413] asix 1-1.1:1.0 eth0: asix_rx_fixup() Bad Header Length
0x5638f7e2, offset 4
And network throughput ends up being pretty bursty and slow with
a overall throughput of at best ~30kB/s (where as previously we
got 1.1MB/s with the slower USB1.1 "full speed" host).
We found the issue also was reproducible on a x86_64 system,
using a "high-speed" USB2.0 port but the throughput did not
measurably drop (possibly due to the scp transfer being cpu
bound on my slow test hardware).
After lots of debugging, I found the check added in the
problematic commit seems to be calculating the offset
incorrectly.
In the normal case, in the main loop of the function, we do:
(where offset is zero, or set to "offset += (copy_length + 1) &
0xfffe" in the previous loop)
rx->header = get_unaligned_le32(skb->data +
offset);
offset += sizeof(u32);
But the problematic patch calculates:
offset = ((rx->remaining + 1) & 0xfffe) + sizeof(u32);
rx->header = get_unaligned_le32(skb->data + offset);
Adding some debug logic to check those offset calculation used
to find rx->header, the one in problematic code is always too
large by sizeof(u32).
Thus, this patch removes the incorrect " + sizeof(u32)" addition
in the problematic calculation, and resolves the issue.
Cc: Dean Jenkins
Cc: "David B. Robins"
Cc: Mark Craske
Cc: Emil Goode
Cc: "David S. Miller"
Cc: YongQin Liu
Cc: Guodong Xu
Cc: Ivan Vecera
Cc: linux-...@vger.kernel.org
Cc: net...@vger.kernel.org
Cc: stable #4.4+
Reported-by: Yongqin Liu
Signed-off-by: John Stultz
---
drivers/net/usb/asix_common.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/usb/asix_common.c b/drivers/net/usb/asix_common.c
index 0c5c22b..7de5ab5 100644
--- a/drivers/net/usb/asix_common.c
+++ b/drivers/net/usb/asix_common.c
@@ -66,7 +66,7 @@ int asix_rx_fixup_internal(struct usbnet *dev, struct sk_buff
*skb,
* buffer.
*/
if (rx->remaining && (rx->remaining + sizeof(u32) <= skb->len)) {
- offset = ((rx->remaining + 1) & 0xfffe) + sizeof(u32);
+ offset = ((rx->remaining + 1) & 0xfffe);
I have verified that this fixes my ARM board. Thanks for finding the
mistake.
Note that the outer set of brackets could of been removed as they are
redundant.
rx->header = get_unaligned_le32(skb->data + offset);
offset = 0;
Thanks,
Best regards,
Dean
--
Dean Jenkins
Embedded Software Engineer
Linux Transportation Solutions
Mentor Embedded Software Division
Mentor Graphics (UK) Ltd.
Re: [REGRESSION] asix: Lots of asix_rx_fixup() errors and slow transmissions
= 118) Ethernet frame This could be due to IP fragmentation. ie: [ 106.946473] JDB set remaining to 1514 (skblen: 1518) [ 106.946525] JDB set remaining to 1514 (skblen: 1640) [ 106.946546] JDB set remaining to 118 (skblen: 1640) Yes, that 118 confirms my reasoning above. [ 106.946586] JDB set remaining to 1514 (skblen: 1518) So yea.. maybe that will help clue things in a bit? I'm still a bit lost. :) Your observations are consistent with missing URBs from the USB host controller. Here is a summary of what I think is happening in your case: Good case: URB #1: 1514 octets of 1514 Ethernet frame (A) URB #2: 1514 octets of 1514 Ethernet frame (B) + 526 octets of 1514 Ethernet frame (C) URB #3: 988 octets of 1514 Ethernet frame (C) URB #4: 1514 octets of 1514 Ethernet frame (D) Therefore, Ethernet frame (C) is spanning URBs #2 and #3. Bad case, URB #3 is lost: URB #1: 1514 octets of 1514 Ethernet frame (A) URB #2: 1514 octets of 1514 Ethernet frame (B) + 526 octets of 1514 Ethernet frame (C) Remaining is 988 URB #4: 1514 octets of 1514 Ethernet frame (D) But when URB #4 is analysed the 32-bit Header word is not found after 988 octets in the URB buffer so "sync lost". The end of Ethernet frame (C) is missing so drop the Ethernet frame. Now look at the start of the URB #4 buffer and find a 32-bit header word so Ethernet frame (D) can be consumed. So I think the commit is acting as intended and you are suffering from lost URBs. So perhaps you have a URB memory allocation issue for the USB host controller ? However, this would also mean x86 has the same issue. So perhaps it is a bug in common USB kernel code ? You might find it helpful to get USB analyser logs or use the software utility usbmon to grab the USB host controller data. You might then be able to compare the data from the USB log with the URB buffer data. This might confirm that URBs are going missing. On the other hand, it was suggested that removing my commit improved through-put. Is that really true ? Perhaps you can try an experiment where you add test code to add a counter to use my "sync lost" detector for 1000 URBs then by-pass the "sync lost" detector for 1000 URBs and repeat it cyclically. 1000 is a guess, you might need a higher value. Then run some tests to see whether the through-put periodically goes up and down in sympathy with the "sync lost" detector being used or not being used. Also track the RX error counter from ifconfig to see how many bad Ethernet frames got to the IP stack. Also try using wireshark to see whether you are generating IP fragmented frames. Unfortunately, using iperf with IPv6 with defaults will generate IPv6 fragmentation causing twice as many Ethernet frames to be sent as needed so is inefficient. To fix that, in iperf reduce the data length so that the MTU size of 1500 is not exceeded. The commit only runs when Ethernet frames span URBs which typically occurs when IP fragmentation happens or when there is a very high rate of Ethernet frames per second. At least, it is now clear why you see "988 remaining", that is because you are using maximum length Ethernet frames and 2 of those frames are written into a single URB which means 988 octets of the 2nd Ethernet frame goes into the 2nd URB buffer. So that makes sense and is correct. Regards, Dean thanks -john -- Dean Jenkins Embedded Software Engineer Linux Transportation Solutions Mentor Embedded Software Division Mentor Graphics (UK) Ltd.
Re: [REGRESSION] asix: Lots of asix_rx_fixup() errors and slow transmissions
On 06/05/16 16:27, Andrew Lunn wrote:
In other words, the full-speed hub is restricting the USB to
Ethernet Adaptor to a 12Mbps (half-duplex) bandwidth to support
Ethernet 100Mbps (full-duplex) traffic. That is not going to work
very well because Ethernet frames (perhaps partial Ethernet frames)
need to be discarded within the USB link.
If that really is true, the design is broken. I would expect the
adaptor to reliably transfer whole frames over USB, and drop whole
frames from its receive queue when the USB is congested. TCP is also
going to see the USB bottleneck as just like any bottleneck in the
network and back off. So TCP streams should not cause major congestion
on the USB link.
The host's USB host controller polls the USB to Ethernet adaptor for
more data. The USB to Ethernet adaptor cannot predict when the next poll
request comes. The AX88772B can span Ethernet frames across multiple
poll requests. This means it is possible get a partial Ethernet frame
received in the USB host controller on one poll and it is assumed that
the next poll (sometime in the near future) will get the remaining part
of the Ethernet frame.
However, the USB to Ethernet adaptor does not contain an infinitely
sized RX Ethernet buffer for the incoming Ethernet frames. I believe the
USB to Ethernet adaptor is just a pipe and does not directly implement
flow control for Ethernet frames so the RX buffer is going to overflow
causing loss of whole Ethernet frames. I suspect the IP stack in the
host computer implements flow control for Ethernet frames.
Because the AX88772B can span Ethernet frames across multiple poll
requests there is a risk that the designers of the device could of
implemented a solution to discard the remaining part of the Ethernet
frame before the next poll arrives due to the RX buffer overflowing. I
don't know the algorithm used in the AX88772B but there will be loss of
data due to the mismatch in bandwidths. I agree that dropping whole
Ethernet frames would be preferable to dropping partial Ethernet frames
which would corrupt the data stream.
My suspicion is that the URB buffers are containing discontinues in the
data stream because of lost data due to insufficient bandwidth on the
USB link.
Going over a 12Mbps USB link should be no different
to hitting an old Ethernet hub which can only do 10/Half.
Not exactly, because USB is a transport link which is agnostic to the
type of data that is flowing. It is up to the layers above USB to manage
the data content.
In other words, the USB speed needs to be higher than the Ethernet speed
to avoid mismatches in bandwidth.
Therefore please retest with a working high-speed USB hub or remove
the full-speed USB hub from the test environment and directly
connect the USB to Ethernet Adaptor to the root hub of the USB port.
Then repeat the tests to see whether anything improved.
In other words, you need to eliminate the dmesg messages saying "not
running at top speed; connect to a high speed hub".
I would also suggest testing with the Ethernet at 10/half. You should
be able to use Ethtool to set that up. Your USB and Ethernet bandwidth
become more equal. If you still see errors, it suggests a protocol
implementation error somewhere.
I agree with the suggestion but I hope USB high speed (480Mbps)
operation was the intended environment rather than the useless USB full
speed (12Mbps) operation.
Let's hope that not using the USB hub improves things.
Regards,
Dean
Andrew
--
Dean Jenkins
Embedded Software Engineer
Linux Transportation Solutions
Mentor Embedded Software Division
Mentor Graphics (UK) Ltd.
Re: [REGRESSION] asix: Lots of asix_rx_fixup() errors and slow transmissions
not found in the expected location at the start of the URB buffer. [ 41.938370] asix 1-1.2:1.0 eth0: asix_rx_fixup() Bad Header Length 0x11400040, offset 4 This evidence is suggesting either the data stream is garbled such as by many dropped URBs or lost partial Ethernet frames, or the ASIX protocol for the AX88772B chipset differs from the AX88772 chipset so the ASIX driver is looking in the wrong place for the 32-bit header word. I suspect data is lost due to the restricted 12Mbps bandwidth. My conclusion is that your USB to Ethernet Adaptor is not running at high speed (480Mbps) mode which is causing a partial loss (corruption) of Ethernet frames across the USB link. A USB Protocol Analyser or software tool usbmon could be used to confirm this scenario. Therefore please retest with a working high-speed USB hub or remove the full-speed USB hub from the test environment and directly connect the USB to Ethernet Adaptor to the root hub of the USB port. Then repeat the tests to see whether anything improved. In other words, you need to eliminate the dmesg messages saying "not running at top speed; connect to a high speed hub". Best regards, Dean -Guodong Xu -- Dean Jenkins Embedded Software Engineer Linux Transportation Solutions Mentor Embedded Software Division Mentor Graphics (UK) Ltd.
Re: [REGRESSION] asix: Lots of asix_rx_fixup() errors and slow transmissions
On 05/05/16 00:45, John Stultz wrote: On Tue, May 3, 2016 at 3:54 AM, Dean Jenkins wrote: On 03/05/16 11:04, Guodong Xu wrote: did you test on ARM 64-bit system or ARM 32-bit? I ask because HiKey is an ARM 64-bit system. I suggest we should be careful on that. I saw similar issues when transferring to a 64-bit system in other net drivers. We used 32-bit ARM and never tested on 64-bit ARM so I suggest that the commits need to be reviewed with 64-bit OS in mind. Do you have any suggestion on this regard? Try testing on a Linux PC x86 32-bit OS which has has a kernel containing my ASIX commits. This will help to confirm whether the failure is related to 32-bit or 64-bit OS. Then try with Linux PC x86 64-bit OS, this should fail otherwise it points to something specific in your ARM 64-bit platform. Just as a sample point, I have managed to reproduce exactly this issue on an x86_64 system by simply scp'ing a large file. Please tell us the x86_64 kernel version number that you used and which Linux Distribution it was ? This allows other people a chance to reproduce your observations. [ 417.819276] asix 1-5:1.0 eth1: asix_rx_fixup() Data Header synchronisation was lost, remaining 988 It is interesting that the reported "remaining" value is 988. Is 988 always shown ? I mean that do you see any other "remaining" values for the "Data Header synchronisation was lost" error message ? [ 417.823415] asix 1-5:1.0 eth1: asix_rx_fixup() Bad Header Length 0xef830347, offset 4 The gap in the timestamps shows 417.823415 - 417.819276 = 0.004139 = 4ms which is a large gap in terms of USB 2.0 high speed communications. This gap is expected to be in the 100us range for consecutive URBs. So 4ms is strange. The expectation is that the "Data Header synchronisation was lost" error message resets the 32-bit header word synchronisation to the start of the next URB buffer. The "Bad Header Length, offset 4" is the expected outcome for the next URB because it is unlikely the 32-bit header word is at the start of URB buffer due to Ethernet frames spanning across URBs. [ 417.827502] asix 1-5:1.0 eth1: asix_rx_fixup() Bad Header Length 0x31e2b348, offset 4 Timestamps show the gap to be 4ms which is strange for USB 2.0 high speed, are you sure high speed mode is being used ? [ 417.843779] asix 1-5:1.0 eth1: asix_rx_fixup() Data Header synchronisation was lost, remaining 988 [ 417.847921] asix 1-5:1.0 eth1: asix_rx_fixup() Bad Header Length 0x8af91035, offset 4 [ 417.852004] asix 1-5:1.0 eth1: asix_rx_fixup() Bad Header Length 0x8521fa03, offset 4 [ 418.273394] asix 1-5:1.0 eth1: asix_rx_fixup() Data Header synchronisation was lost, remaining 988 [ 418.277532] asix 1-5:1.0 eth1: asix_rx_fixup() Bad Header Length 0x33cd9c7c, offset 4 [ 418.281683] asix 1-5:1.0 eth1: asix_rx_fixup() Bad Header Length 0x3d850896, offset 4 [ 418.286227] asix 1-5:1.0 eth1: asix_rx_fixup() Bad Header Length 0x86443357, offset 4 [ 418.290319] asix 1-5:1.0 eth1: asix_rx_fixup() Bad Header Length 0xee6c81d1, offset 4 I don't have any 32bit x86 installs around so I'm not sure I can easly test there, but its clear its not arm64 specific. I agree the issue is not specific to your ARM 64 bit platform. Please can you supply the output of ifconfig for the USB to Ethernet adaptor, your example above shows eth1 as the device. Please show the output of ifconfig eth1 before and after the issue is seen. This will show us whether the kernel logs any network errors and how many bytes have been transferred. After the issue is seen, please can you show us the output of "dmesg | grep asix" so that we can see status messages from the ASIX driver that the USB to Ethernet adaptor is using. In particular we need to check that USB high speed operation (480Mbps) is being used and not full speed operation (12Mbps). Thanks, Regards, Dean thanks -john -- Dean Jenkins Embedded Software Engineer Linux Transportation Solutions Mentor Embedded Software Division Mentor Graphics (UK) Ltd.
Re: [REGRESSION] asix: Lots of asix_rx_fixup() errors and slow transmissions
On 04/05/16 01:28, David B. Robins wrote:
Here is the code snippet from the patch with my annotations between #
#, I will try to explain my intentions. Feel free to point out any
flaws:
if (rx->remaining && (rx->remaining + sizeof(u32) <= skb->len)) {
# Only runs when rx->remaining !=0 and the end of the Ethernet
frame + next 32-bit header word is within the URB buffer. #
# Therefore, this code does not run when the end of an
Ethernet frame has been reached in the previous URB #
# or when the end of the Ethernet frame + next 32-bit header
word will be in a later URB buffer #
It may well be. I don't have the setup with me now, but I can try
tomorrow to reproduce an environment where I can add some more
detailed logging.
Since the URB length has to be >= than the remaining data plus a u32,
the devices that John Stultz and I are using (AX88772B in my case) may
be adding some additional data/padding after an Ethernet frame,
expecting it to be discarded, and running into this check and its
consequences. This may mean the device is badly behaved, if it is
specified not to send anything extra; in any case, a well-intentioned
error correction has gone badly, but I better understand the intent
now. I am curious to know how often the device you are using benefits
from this block of code.
The issue is that the driver should be robust to cope with missing URBs.
Whilst testing with D-Link DUB-E100 C1 AX88772 USB to Ethernet adaptor
in our ARM embedded system which runs in hostile environments, it was
noticed that URBs could be lost (probably due to a bug elsewhere or low
memory issue). Without this patch, a missing URB causes bad Ethernet
frames to be passed up to the IP stack because rx->remaining spans
multiple URBs.
In the good case of an Ethernet frame spanning 2 URBs, the 1st URB is
processed and copies the 1st part of the Ethernet frame into the netdev
buffer, for the 2nd URB the remaining part of the Ethernet frame is
copied into the same netdev buffer to complete the Ethernet frame. The
netdev buffer is then sent up to the IP stack.
In the case of a missing URB, a bad Ethernet frame is created as follows:
The 1st URB is processed and copies the 1st part of the Ethernet frame
into the netdev buffer, the 2nd URB is lost (somehow), the 3rd URB is
processed and blindly copies what it thinks is the remaining part of the
Ethernet frame in the same netdev buffer which corrupts the Ethernet
frame. The netdev buffer is then sent up to the IP stack. The 3rd URB
and subsequent URBs are processed but synchronisation has been lost so
can misread data as a 32-bit header word. It is likely that some good
Ethernet frames get discarded whilst trying to resynchronise.
A recovery strategy for regaining lock with the 32-bit header word is
necessary otherwise the driver will have difficulty in recovering from a
lost URB.
In the "olden days", the 32-bit header word was always at the start of
the URB buffer so previous URBs did not influence the current URB. So no
recovery strategy was needed at that time. But now we have to remember
what happened in the previous URB and a lost URB can cause a
discontinuity in the data stream because the data is not always aligned
to the start of the URB buffer.
I agree that your environment may never suffer from lost URBs so removal
of the patch would work OK.
I will try to find some time to setup a test environment.
Regards,
Dean
--
Dean Jenkins
Embedded Software Engineer
Linux Transportation Solutions
Mentor Embedded Software Division
Mentor Graphics (UK) Ltd.
Re: [REGRESSION] asix: Lots of asix_rx_fixup() errors and slow transmissions
s provided by John. This does not suggest a random failure unless there are other examples of a non 988 remaining value error message. 988 is well within a Ethernet frame length so seems to be valid. I think a good step would be to add some debug to print the rx->remaining value at entry to asix_rx_fixup_internal(). This would generate a lot of debug but a pattern of the values might emerge. A good test would be to run "ping -c 1 -s $packet_length $ip_address" inside a script which has a loop with an increasing payload length $packet_length with a small delay between ping calls. This will show whether particular packet sizes trigger the failures. Then try with "ping -f -c 200 -s $packet_length $ip_address" to load up the USB link. Seems that I need kernel v4.4 or later to get a kernel with my patch in. This will take me a few days to find time to rig something up to test... Regards, Dean -- Dean Jenkins Embedded Software Engineer Linux Transportation Solutions Mentor Embedded Software Division Mentor Graphics (UK) Ltd.
Re: [REGRESSION] asix: Lots of asix_rx_fixup() errors and slow transmissions
On 03/05/16 11:04, Guodong Xu wrote: On 3 May 2016 at 17:23, Dean Jenkins wrote: On 03/05/16 05:55, John Stultz wrote: In testing with HiKey, we found that since commit 3f30b158eba5c60 (asix: On RX avoid creating bad Ethernet frames), we're seeing lots of noise during network transfers: [ 239.027993] asix 1-1.1:1.0 eth0: asix_rx_fixup() Data Header synchronisation was lost, remaining 988 [ 239.037310] asix 1-1.1:1.0 eth0: asix_rx_fixup() Bad Header Length 0x54ebb5ec, offset 4 [ 239.045519] asix 1-1.1:1.0 eth0: asix_rx_fixup() Bad Header Length 0xcdffe7a2, offset 4 [ 239.275044] asix 1-1.1:1.0 eth0: asix_rx_fixup() Data Header synchronisation was lost, remaining 988 [ 239.284355] asix 1-1.1:1.0 eth0: asix_rx_fixup() Bad Header Length 0x1d36f59d, offset 4 [ 239.292541] asix 1-1.1:1.0 eth0: asix_rx_fixup() Bad Header Length 0xaef3c1e9, offset 4 [ 239.518996] asix 1-1.1:1.0 eth0: asix_rx_fixup() Data Header synchronisation was lost, remaining 988 [ 239.528300] asix 1-1.1:1.0 eth0: asix_rx_fixup() Bad Header Length 0x2881912, offset 4 [ 239.536413] asix 1-1.1:1.0 eth0: asix_rx_fixup() Bad Header Length 0x5638f7e2, offset 4 And network throughput ends up being pretty bursty and slow with a overall throughput of at best ~30kB/s. Looking through the commits since the v4.1 kernel where we didn't see this, I narrowed the regression down, and reverting the following two commits seems to avoid the problem: 6a570814cd430fa5ef4f278e8046dcf12ee63f13 asix: Continue processing URB if no RX netdev buffer 3f30b158eba5c604b6e0870027eef5d19fc9271d asix: On RX avoid creating bad Ethernet frames With these reverted, we don't see all the error messages, and we see better ~1.1MB/s throughput (I've got a mouse plugged in, so I think the usb host is only running at "full-speed" mode here). This worries me some, as the patches seem to describe trying to fix the issue they seem to cause, so I suspect a revert isn't the correct solution, but am not sure why we're having such trouble and the patch authors did not. I'd be happy to do further testing of patches if folks have any ideas. Originally Reported-by: Yongqin Liu thanks -john Hi John, Some ASIX chipsets span the Ethernet frame over consecutive URBs which requires successful transfer of 2 URBs. This means states of a previous URB influences the processing of the next URB including a dropped URB (causes a discontinuity in the data stream). In other words synchronisation of the in-band 32-bit header word needs to be tracked between URBs. Some ASIX chipsets allow the in-band 32-bit header word to be no longer fixed to the start of the URB buffer so it moves to any position within the URB buffer. I understand your point of suggesting it is a "regression" for your device but the driver was broken for DUB-E100 C1 (small black USB device). So you cannot revert the commits as this would break DUB-E100 C1 (small black USB device). 6a570814cd430fa5ef4f278e8046dcf12ee63f13 asix: Continue processing URB if no RX netdev buffer This commit is necessary because it avoids a crash when netdev buffer failed to be allocated for the 1st URB and the 2nd URB containing a spanned Ethernet frame is processed. The crash happens because the 2nd URB assumed that the netdev buffer had been allocated. 3f30b158eba5c604b6e0870027eef5d19fc9271d asix: On RX avoid creating bad Ethernet frames This commit is necessary to avoid sending bad Ethernet frames into the IP stack during loss of synchronisation and to dropping good Ethernet frames. This commit improves the synchronisation recovery mechanism of the in-band 32-bit header word. The ASIX USB to Ethernet devices these commits were tested on where DUB-E100 C1 (small black USB device). Embedded ARM based systems were used where memory resources can run out. I don't have the chance to look into detail yet. But just a caution, did you test on ARM 64-bit system or ARM 32-bit? I ask because HiKey is an ARM 64-bit system. I suggest we should be careful on that. I saw similar issues when transferring to a 64-bit system in other net drivers. We used 32-bit ARM and never tested on 64-bit ARM so I suggest that the commits need to be reviewed with 64-bit OS in mind. Do you have any suggestion on this regard? Try testing on a Linux PC x86 32-bit OS which has has a kernel containing my ASIX commits. This will help to confirm whether the failure is related to 32-bit or 64-bit OS. Then try with Linux PC x86 64-bit OS, this should fail otherwise it points to something specific in your ARM 64-bit platform. It could be that for your USB to Ethernet device that the wrong configuration settings have been used. In other words the ASIX driver is flexible to support various variants of the ASIX chipsets. For example, does your device support Ethernet frames spanning multiple URBs (multiple USB transfers) ? Would you please suggest how to find out this information? How can I c
Re: [REGRESSION] asix: Lots of asix_rx_fixup() errors and slow transmissions
On 03/05/16 05:55, John Stultz wrote: In testing with HiKey, we found that since commit 3f30b158eba5c60 (asix: On RX avoid creating bad Ethernet frames), we're seeing lots of noise during network transfers: [ 239.027993] asix 1-1.1:1.0 eth0: asix_rx_fixup() Data Header synchronisation was lost, remaining 988 [ 239.037310] asix 1-1.1:1.0 eth0: asix_rx_fixup() Bad Header Length 0x54ebb5ec, offset 4 [ 239.045519] asix 1-1.1:1.0 eth0: asix_rx_fixup() Bad Header Length 0xcdffe7a2, offset 4 [ 239.275044] asix 1-1.1:1.0 eth0: asix_rx_fixup() Data Header synchronisation was lost, remaining 988 [ 239.284355] asix 1-1.1:1.0 eth0: asix_rx_fixup() Bad Header Length 0x1d36f59d, offset 4 [ 239.292541] asix 1-1.1:1.0 eth0: asix_rx_fixup() Bad Header Length 0xaef3c1e9, offset 4 [ 239.518996] asix 1-1.1:1.0 eth0: asix_rx_fixup() Data Header synchronisation was lost, remaining 988 [ 239.528300] asix 1-1.1:1.0 eth0: asix_rx_fixup() Bad Header Length 0x2881912, offset 4 [ 239.536413] asix 1-1.1:1.0 eth0: asix_rx_fixup() Bad Header Length 0x5638f7e2, offset 4 And network throughput ends up being pretty bursty and slow with a overall throughput of at best ~30kB/s. Looking through the commits since the v4.1 kernel where we didn't see this, I narrowed the regression down, and reverting the following two commits seems to avoid the problem: 6a570814cd430fa5ef4f278e8046dcf12ee63f13 asix: Continue processing URB if no RX netdev buffer 3f30b158eba5c604b6e0870027eef5d19fc9271d asix: On RX avoid creating bad Ethernet frames With these reverted, we don't see all the error messages, and we see better ~1.1MB/s throughput (I've got a mouse plugged in, so I think the usb host is only running at "full-speed" mode here). This worries me some, as the patches seem to describe trying to fix the issue they seem to cause, so I suspect a revert isn't the correct solution, but am not sure why we're having such trouble and the patch authors did not. I'd be happy to do further testing of patches if folks have any ideas. Originally Reported-by: Yongqin Liu thanks -john Hi John, Some ASIX chipsets span the Ethernet frame over consecutive URBs which requires successful transfer of 2 URBs. This means states of a previous URB influences the processing of the next URB including a dropped URB (causes a discontinuity in the data stream). In other words synchronisation of the in-band 32-bit header word needs to be tracked between URBs. Some ASIX chipsets allow the in-band 32-bit header word to be no longer fixed to the start of the URB buffer so it moves to any position within the URB buffer. I understand your point of suggesting it is a "regression" for your device but the driver was broken for DUB-E100 C1 (small black USB device). So you cannot revert the commits as this would break DUB-E100 C1 (small black USB device). 6a570814cd430fa5ef4f278e8046dcf12ee63f13 asix: Continue processing URB if no RX netdev buffer This commit is necessary because it avoids a crash when netdev buffer failed to be allocated for the 1st URB and the 2nd URB containing a spanned Ethernet frame is processed. The crash happens because the 2nd URB assumed that the netdev buffer had been allocated. 3f30b158eba5c604b6e0870027eef5d19fc9271d asix: On RX avoid creating bad Ethernet frames This commit is necessary to avoid sending bad Ethernet frames into the IP stack during loss of synchronisation and to dropping good Ethernet frames. This commit improves the synchronisation recovery mechanism of the in-band 32-bit header word. The ASIX USB to Ethernet devices these commits were tested on where DUB-E100 C1 (small black USB device). Embedded ARM based systems were used where memory resources can run out. It could be that for your USB to Ethernet device that the wrong configuration settings have been used. In other words the ASIX driver is flexible to support various variants of the ASIX chipsets. For example, does your device support Ethernet frames spanning multiple URBs (multiple USB transfers) ? So I doubt my commits are "broken" because we don't see your failures (not tested your device). It is more likely that your ASIX device needs to be properly identified and configured to be compatible with the ASIX driver. At least, I suggest that is the best place to start your investigation. Of course, your ASIX chipset might have a different behaviour for how the in-band 32-bit header word operates so perhaps special treatment is needed for your chipset ? Please send to the mailing list the output of lsusb for your device so that people can know the USB product ID and vendor ID for your device. This is allows people to assist with the investigation. Do you have any links to websites that sell your device ? Are you using UDP or TCP connections ? Regards, Dean -- Dean Jenkins Embedded Software Engineer Linux Transportation Solutions Mentor Embedded Software Division Mentor Graphics (UK) Ltd.
Re: [PATCH 09/11] bluetooth: hci_ldisc: fix deadlock condition
On 20/03/14 19:30, Felipe Balbi wrote:
LDISCs shouldn't call tty->ops->write() from within
->write_wakeup().
->write_wakeup() is called with port lock taken and
IRQs disabled, tty->ops->write() will try to acquire
the same port lock and we will deadlock.
I think the work queue should be placed into hci_uart_tty_wakeup() and
not hci_uart_tx_wakeup() as added by this patch.
The reason is that the kernel thread hci_uart_send_frame() calls
hci_uart_tx_wakeup() and this patch unnecessarily introduces a work
queue in the program flow of that kernel thread.
In other words, I think this patch has undesirable side-effects such as
adding latency and increased processor loading for hci_uart_send_frame().
Regards,
Dean
Reviewed-by: Peter Hurley
Reported-by: Huang Shijie
Signed-off-by: Felipe Balbi
---
drivers/bluetooth/hci_ldisc.c | 24 +++-
drivers/bluetooth/hci_uart.h | 1 +
2 files changed, 20 insertions(+), 5 deletions(-)
diff --git a/drivers/bluetooth/hci_ldisc.c b/drivers/bluetooth/hci_ldisc.c
index 6e06f6f..77af52f 100644
--- a/drivers/bluetooth/hci_ldisc.c
+++ b/drivers/bluetooth/hci_ldisc.c
@@ -118,10 +118,6 @@ static inline struct sk_buff *hci_uart_dequeue(struct
hci_uart *hu)
int hci_uart_tx_wakeup(struct hci_uart *hu)
{
- struct tty_struct *tty = hu->tty;
- struct hci_dev *hdev = hu->hdev;
- struct sk_buff *skb;
-
if (test_and_set_bit(HCI_UART_SENDING, &hu->tx_state)) {
set_bit(HCI_UART_TX_WAKEUP, &hu->tx_state);
return 0;
@@ -129,6 +125,22 @@ int hci_uart_tx_wakeup(struct hci_uart *hu)
BT_DBG("");
+ schedule_work(&hu->write_work);
+
+ return 0;
+}
+
+static void hci_uart_write_work(struct work_struct *work)
+{
+ struct hci_uart *hu = container_of(work, struct hci_uart, write_work);
+ struct tty_struct *tty = hu->tty;
+ struct hci_dev *hdev = hu->hdev;
+ struct sk_buff *skb;
+
+ /* REVISIT: should we cope with bad skbs or ->write() returning
+* and error value ?
+*/
+
restart:
clear_bit(HCI_UART_TX_WAKEUP, &hu->tx_state);
@@ -153,7 +165,6 @@ restart:
goto restart;
clear_bit(HCI_UART_SENDING, &hu->tx_state);
- return 0;
}
static void hci_uart_init_work(struct work_struct *work)
@@ -281,6 +292,7 @@ static int hci_uart_tty_open(struct tty_struct *tty)
tty->receive_room = 65536;
INIT_WORK(&hu->init_ready, hci_uart_init_work);
+ INIT_WORK(&hu->write_work, hci_uart_write_work);
spin_lock_init(&hu->rx_lock);
@@ -318,6 +330,8 @@ static void hci_uart_tty_close(struct tty_struct *tty)
if (hdev)
hci_uart_close(hdev);
+ cancel_work_sync(&hu->write_work);
+
if (test_and_clear_bit(HCI_UART_PROTO_SET, &hu->flags)) {
if (hdev) {
if (test_bit(HCI_UART_REGISTERED, &hu->flags))
diff --git a/drivers/bluetooth/hci_uart.h b/drivers/bluetooth/hci_uart.h
index fffa61f..12df101 100644
--- a/drivers/bluetooth/hci_uart.h
+++ b/drivers/bluetooth/hci_uart.h
@@ -68,6 +68,7 @@ struct hci_uart {
unsigned long hdev_flags;
struct work_struct init_ready;
+ struct work_struct write_work;
struct hci_uart_proto *proto;
void*priv;
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH 4/5] SLIP: Add error message for xleft non-zero
Add a printk to show when xleft is non-zero in sl_encaps. The idea is to see whether a previous SLIP frame failed to be fully transmitted. Signed-off-by: Dean Jenkins --- drivers/net/slip/slip.c | 4 1 file changed, 4 insertions(+) diff --git a/drivers/net/slip/slip.c b/drivers/net/slip/slip.c index f7303e0..e2eff84 100644 --- a/drivers/net/slip/slip.c +++ b/drivers/net/slip/slip.c @@ -395,6 +395,10 @@ static void sl_encaps(struct slip *sl, unsigned char *icp, int len) #endif count = slip_esc(p, sl->xbuff, len); + if (sl->xleft) + printk(KERN_ERR "%s: ERROR: xleft is non-zero %d\n", + __func__, sl->xleft); + /* ensure xleft set by the previous SLIP frame is zero for this frame * otherwise slip_write_wakeup() can cause a recursive loop. */ -- 1.8.1.5 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH 5/5] SLIP: Fix transmission segmentation mechanism
Binding SLIP with a PTY/TTY can cause truncated SLIP frames to be
transmitted when the first write to the TTY fails to consume all
the characters of the SLIP frame.
Asynchronous to the write function is a wakeup event from the TTY
that indicates the TTY can accept more data. The wakeup event
calls tty_wakeup() which calls slip_write_wakeup() when
TTY_DO_WRITE_WAKEUP is set.
To complete the transmission of a SLIP frame to the TTY,
slip_write_wakeup() must run at least once. Unfortunately, pty_write()
also calls tty_wakeup() and when TTY_DO_WRITE_WAKEUP is set,
slip_write_wakeup() is called. xleft is always zero and
causes slip_write_wakeup() to complete the transmission and
clears the TTY_DO_WRITE_WAKEUP flag. This can cause a truncated
SLIP frame because any remaining characters will not get sent.
The wakeup event is unable to process the remaining characters
because the TTY_DO_WRITE_WAKEUP flag has been cleared.
The code modification fixes the transmission segmentation
mechanism by preventing pty_write() from calling
slip_write_wakeup() by clearing TTY_DO_WRITE_WAKEUP before calling
pty_write(). After pty_write() returns, TTY_DO_WRITE_WAKEUP is set
to allow the TTY wakeup event to call slip_write_wakeup() to
attempt to complete the transmission of the SLIP frame.
This may not be foolproof because a timeout is needed to
break out of the cycle of transmission attempts. Note error
codes from the TTY layer will break out of the cycle of
transmission attempts.
Signed-off-by: Dean Jenkins
---
drivers/net/slip/slip.c | 29 -
1 file changed, 20 insertions(+), 9 deletions(-)
diff --git a/drivers/net/slip/slip.c b/drivers/net/slip/slip.c
index e2eff84..0ded23d 100644
--- a/drivers/net/slip/slip.c
+++ b/drivers/net/slip/slip.c
@@ -404,15 +404,13 @@ static void sl_encaps(struct slip *sl, unsigned char
*icp, int len)
*/
sl->xleft = 0;
- /* Order of next two lines is *very* important.
-* When we are sending a little amount of data,
-* the transfer may be completed inside the ops->write()
-* routine, because it's running with interrupts enabled.
-* In this case we *never* got WRITE_WAKEUP event,
-* if we did not request it before write operation.
-* 14 Oct 1994 Dmitry Gorodchanin.
+ /* ensure slip_write_wakeup() does not run due to write()
+* or write_wakeup event and this prevents slip_write_wakeup()
+* responding to an out of date xleft value.
*/
- set_bit(TTY_DO_WRITE_WAKEUP, &sl->tty->flags);
+ clear_bit(TTY_DO_WRITE_WAKEUP, &sl->tty->flags);
+
+ /* attempt to write the SLIP frame to the TTY buffer */
err = sl->tty->ops->write(sl->tty, sl->xbuff, count);
if (err < 0) {
@@ -432,6 +430,11 @@ static void sl_encaps(struct slip *sl, unsigned char *icp,
int len)
/* VSV */
clear_bit(SLF_OUTWAIT, &sl->flags); /* reset outfill flag */
#endif
+ /* xleft will be zero when all characters have been written.
+* if xleft is positive then additional writes are needed.
+* write_wakeup event is needed to complete the transmission.
+*/
+ set_bit(TTY_DO_WRITE_WAKEUP, &sl->tty->flags);
}
/*
@@ -447,15 +450,18 @@ static void slip_write_wakeup(struct tty_struct *tty)
if (!sl || sl->magic != SLIP_MAGIC || !netif_running(sl->dev))
return;
+ clear_bit(TTY_DO_WRITE_WAKEUP, &tty->flags);
+
if (sl->xleft <= 0) {
+ /* Whole SLIP frame has been written. */
/* Now serial buffer is almost free & we can start
* transmission of another packet */
sl->dev->stats.tx_packets++;
- clear_bit(TTY_DO_WRITE_WAKEUP, &tty->flags);
sl_unlock(sl);
return;
}
+ /* attempt to write the remaining SLIP frame characters */
err = tty->ops->write(tty, sl->xhead, sl->xleft);
if (err < 0) {
@@ -468,6 +474,11 @@ static void slip_write_wakeup(struct tty_struct *tty)
sl->xleft -= actual;
sl->xhead += actual;
+
+ /* allow the next tty wakeup event to attempt to complete
+* the transmission
+*/
+ set_bit(TTY_DO_WRITE_WAKEUP, &tty->flags);
}
static void sl_tx_timeout(struct net_device *dev)
--
1.8.1.5
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH 2/5] SLIP: Handle error codes from the TTY layer
It appears that SLIP does not handle error codes from the TTY layer.
This will result in a malfunction because the remaining length of
data will be corrupted by the negative error code values from the TTY
layer.
Therefore, add error code checks in sl_encaps() and sl_encaps_wakeup()
to prevent the corruption of the sent data length.
Note that SLIP is connectionless so on TTY error indicate that all data
was sent. It seems SLIP does not return error codes to the network
layer.
Signed-off-by: Dean Jenkins
---
drivers/net/slip/slip.c | 26 ++
1 file changed, 22 insertions(+), 4 deletions(-)
diff --git a/drivers/net/slip/slip.c b/drivers/net/slip/slip.c
index a34d6bf..bed819f 100644
--- a/drivers/net/slip/slip.c
+++ b/drivers/net/slip/slip.c
@@ -374,7 +374,7 @@ static void sl_bump(struct slip *sl)
static void sl_encaps(struct slip *sl, unsigned char *icp, int len)
{
unsigned char *p;
- int actual, count;
+ int actual, count, err;
if (len > sl->mtu) {/* Sigh, shouldn't occur BUT ... */
printk(KERN_WARNING "%s: truncating oversized transmit
packet!\n", sl->dev->name);
@@ -404,7 +404,16 @@ static void sl_encaps(struct slip *sl, unsigned char *icp,
int len)
* 14 Oct 1994 Dmitry Gorodchanin.
*/
set_bit(TTY_DO_WRITE_WAKEUP, &sl->tty->flags);
- actual = sl->tty->ops->write(sl->tty, sl->xbuff, count);
+ err = sl->tty->ops->write(sl->tty, sl->xbuff, count);
+
+ if (err < 0) {
+ /* error case, say all was sent as connectionless */
+ actual = count;
+ } else {
+ /* good case, err contains the number sent */
+ actual = err;
+ }
+
#ifdef SL_CHECK_TRANSMIT
sl->dev->trans_start = jiffies;
#endif
@@ -422,7 +431,7 @@ static void sl_encaps(struct slip *sl, unsigned char *icp,
int len)
*/
static void slip_write_wakeup(struct tty_struct *tty)
{
- int actual;
+ int actual, err;
struct slip *sl = tty->disc_data;
/* First make sure we're connected. */
@@ -438,7 +447,16 @@ static void slip_write_wakeup(struct tty_struct *tty)
return;
}
- actual = tty->ops->write(tty, sl->xhead, sl->xleft);
+ err = tty->ops->write(tty, sl->xhead, sl->xleft);
+
+ if (err < 0) {
+ /* error case, say all was sent as connectionless */
+ actual = sl->xleft;
+ } else {
+ /* good case, err contains the number sent */
+ actual = err;
+ }
+
sl->xleft -= actual;
sl->xhead += actual;
}
--
1.8.1.5
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH 3/5] SLIP: Prevent recursion stack overflow and scheduler crash
This is an issue when SLIP is bound to a PTY/TTY. If TTY_DO_WRITE_WAKEUP is set, pty_write() calls tty_wakeup() then slip_write_wakeup() can be called. slip_write_wakeup() can be called by pty_write(). This is a recursion loop. pty_write() is called in sl_encaps(). slip_write_wakeup() can be called by the TTY wakeup event. The pty_write() call in sl_encaps() will also call slip_write_wakeup() but xleft has not been updated and contains the value from the previous SLIP frame transmission. xleft is zero unless the previous SLIP frame failed to be fully transmitted in which case xleft has a positive value. A failed transmission causes the next SLIP frame pending transmission to cause a crash. In the failure case when xleft is positive in slip_write_wakeup(), recursion causes the stack to overflow and task structures located near the stack are corrupted by the stack overflow. The corrupted task structures crash the kernel's scheduler and the system crashes with exception handlers crashing and the emergency reboot fails. The recursion loop is: slip_write_wakeup()-->pty_write()-->tty_wakeup()-->slip_write_wakeup() etc. Therefore ensure xleft is zero before writing the SLIP frame to the PTY/TTY layers. This prevents the xleft value of the previous SLIP frame from interfering with the slip_write_wakeup() execution when SLIP is bound to a PTY/TTY. Note the transmission segmentation mechanism is broken and only a single call to the write() function pointer will take place per SLIP frame. This could cause missing or truncated SLIP frames to be transmitted when the write() function fails to write the complete frame. In other words the TTY wakeup event does nothing because the TTY_DO_WRITE_WAKEUP flag has been cleared. Signed-off-by: Dean Jenkins --- drivers/net/slip/slip.c | 5 + 1 file changed, 5 insertions(+) diff --git a/drivers/net/slip/slip.c b/drivers/net/slip/slip.c index bed819f..f7303e0 100644 --- a/drivers/net/slip/slip.c +++ b/drivers/net/slip/slip.c @@ -395,6 +395,11 @@ static void sl_encaps(struct slip *sl, unsigned char *icp, int len) #endif count = slip_esc(p, sl->xbuff, len); + /* ensure xleft set by the previous SLIP frame is zero for this frame +* otherwise slip_write_wakeup() can cause a recursive loop. +*/ + sl->xleft = 0; + /* Order of next two lines is *very* important. * When we are sending a little amount of data, * the transfer may be completed inside the ops->write() -- 1.8.1.5 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH 1/5] Bluetooth: Add RFCOMM TTY write return error codes
It appears that rfcomm_tty_write() does not check that the
passed in TTY device_data is not NULL and also does not check
that the RFCOMM DLC serial data link pointer is not NULL.
A kernel crash was observed whilst SLIP was bound to /dev/rfcomm0
but the /dev/rfcomm0 had subsequently disconnected. Unfortunately,
SLIP attempted to write to the now non-existant RFCOMM TTY device
which caused a NULL pointer dereference because the device_data
no longer existed.
Therefore, add NULL pointer checks for the dev and dlc pointers
and output kernel error debug to show that NULL had been detected.
Signed-off-by: Dean Jenkins
---
net/bluetooth/rfcomm/tty.c | 14 +-
1 file changed, 13 insertions(+), 1 deletion(-)
diff --git a/net/bluetooth/rfcomm/tty.c b/net/bluetooth/rfcomm/tty.c
index b6e44ad..56d28d1 100644
--- a/net/bluetooth/rfcomm/tty.c
+++ b/net/bluetooth/rfcomm/tty.c
@@ -761,12 +761,24 @@ static void rfcomm_tty_close(struct tty_struct *tty,
struct file *filp)
static int rfcomm_tty_write(struct tty_struct *tty, const unsigned char *buf,
int count)
{
struct rfcomm_dev *dev = (struct rfcomm_dev *) tty->driver_data;
- struct rfcomm_dlc *dlc = dev->dlc;
+ struct rfcomm_dlc *dlc;
struct sk_buff *skb;
int err = 0, sent = 0, size;
BT_DBG("tty %p count %d", tty, count);
+ if (!dev) {
+ BT_ERR("RFCOMM TTY device data structure does not exist");
+ return -ENODEV;
+ }
+
+ dlc = dev->dlc;
+
+ if (!dlc) {
+ BT_ERR("RFCOMM serial data link does not exist");
+ return -ENOLINK;
+ }
+
while (count) {
size = min_t(uint, count, dlc->mtu);
--
1.8.1.5
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH 0/5] SLIP SLIP-Improve robustness to crashing
Using SLIP bound to RFCOMM or PTY/TTY has identified some weaknesses to crashing under abnormal conditions. Here is a proposed patchset baselined and built on Linux 3.9. Note the patches have not been tested on x86 Linux 3.9. However similar patches have been used on ARM Linux 2.6.34 to avoid kernel crashes in a commercial project. I believe the same weaknesses still exist in Linux 3.9. If some or all of the patches look to be useful to the community then I may attempt to test on x86 but this is not straight forward for me. I welcome any feedback and whether the fixes are a suitable solution. Who is the maintainer of SLIP in the kernel ? The patchset consists of: 0001-Bluetooth-Add-RFCOMM-TTY-write-return-error-codes.patch 0002-SLIP-Handle-error-codes-from-the-TTY-layer.patch 0003-SLIP-Prevent-recursion-stack-overflow-and-scheduler-.patch 0004-SLIP-Add-error-message-for-xleft-non-zero.patch 0005-SLIP-Fix-transmission-segmentation-mechanism.patches Some background: 0001-Bluetooth-Add-RFCOMM-TTY-write-return-error-codes.patch This patch is a Bluetooth change to add some error return codes to RFCOMM to avoid NULL pointer dereference crashes. Note RFCOMM can already generate an error code that will cause SLIP to malfunction. 0002-SLIP-Handle-error-codes-from-the-TTY-layer.patches This patch allows SLIP to handle error codes from RFCOMM or other bound TTY layers. 0003-SLIP-Prevent-recursion-stack-overflow-and-scheduler-.patches This patch prevents SLIP from causing a recursive loop that overflows the stack and catastrophically crashes the kernel. The scenario is SLIP bound to PTY/TTY. The underlying trigger is a probably a failure to allocate a TTY buffer in tty_buffer_alloc() but this is unproven. The crash is sporadic in an ARM embedded environment where resources are limited. 0004-SLIP-Add-error-message-for-xleft-non-zero.patch This is an error message patch to identify when a SLIP frame has not been fully transmitted meaning the frame was truncated. 0005-SLIP-Fix-transmission-segmentation-mechanism.patches This patch allows multiple attempts to transmit segments of the SLIP frame. Currently only 1 attempt at writing the whole SLIP frame to PTY/TTY occurs. This could truncate transmitted SLIP frames. In addition the modification relies on the TTY write wake-up event to complete the transmission of the SLIP frame rather than the sl_encaps() call to pty_write(). Probably, pty_write() should not call tty_wakeup() but safer to modify SLIP rather than the PTY/TTY layer. Thanks, Dean Jenkins Mentor Graphics -- 1.8.1.5 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] kernel panic, pty.c: remove direct call to tty_wakup in pty_write
On 02/07/13 12:02, Andre Naujoks wrote: On 02.07.2013 11:39, schrieb Dean Jenkins: On 01/07/13 15:49, Andre Naujoks wrote: Hello. This patch removes the direct call to tty_wakeup in pty_write. I have not noticed any drawbacks with this but I am not familiar with the pty driver at all. I think what happens is a recursive loop, write_wakeup->write->write_wakeup ... Indeed there is a recursive loop that I have witnessed whilst using SLIP over USB HID. In the failure case for SLIP, xleft remains positive and recursion causes a stack overflow as xleft is not updated during the recursion: sl_encaps()-->pty_write()-->tty_wakeup()-->slip_write_wakeup()-->pty_write()-->tty_wakeup()-->slip_write_wakeup() etc. The funny thing about the SLIP driver is, that I could not reproduce this particular issue with it. The SLIP driver would just hang after a second of high load but never crash the kernel for me. It may be, because I used a network connection with socat, which is fast enough for the data to go through after a few recursions. In my case, failure occurred because the xleft variable remained positive at the end of the call to sl_encaps(). Normally, xleft is zero upon leaving sl_encaps() because only a single write per SLIP frame to PTY/TTY is necessary to write the whole frame to the TTY layer. In other words, xleft is NOT initialised to zero on the transmission of the next SLIP frame; xleft is normally zero because the previous SLIP was sent OK. Therefore, the kernel crashes on the transmission of the next SLIP frame after a failure to send a complete SLIP frame. You can trigger the recursion crash by hard-coding xleft to a positive value before exiting sl_encaps(). This simulates a failure to send the complete SLIP frame perhaps due a kmalloc failure in the TTY buffer allocation and therefore is a rare event. Note some TTY layers such as Bluetooth's RFCOMM TTY consumes all the SLIP frame characters in a single write from sl_encaps() or RFCOMM TTY returns an error code (not handled by SLIP so is another defect in SLIP). In other words, some TTY layers do not use the segmentation mechanism of multiple calls to pty_write() via the TTY wakeup mechanism. The underlying issue being pty_write() calling tty_wakeup() within the initial write thread. Note that the TTY wakeup event uses tty_wakeup() as a callback to request more data. The documentation for the tty interface forbids this direct call: (from Documentation/serial/tty.txt) write_wakeup() - May be called at any point between open and close. The TTY_DO_WRITE_WAKEUP flag indicates if a call is needed but always races versus calls. Thus the ldisc must be careful about setting order and to handle unexpected calls. Must not sleep. The driver is forbidden from calling this directly from the ->write call from the ldisc as the ldisc is permitted to call the driver write method from this function. In such a situation defer it. I also saw that documentation and the code seems to be breaking that description. It is possible to mitigate against pty-write-->tty_wakeup()-->slip_write_wakeup() by clearing and setting TTY_DO_WRITE_WAKEUP at the "correct" time in the layers bound to PTY/TTY. Indeed, I modified SLIP to do that and sent patches to the linux-netdev mailing list although I am not aware of any progress in having those patches accepted. See below, why I don't think, that that is a good idea. Perhaps this mitigation could be applied to your scenario ? Eventually, your patch could be used when all protocols have mitigration in place. The direct call caused a reproducable kernel panic (see bottom of this mail) for me with the following setup: - using can-utils from git://gitorious.org/linux-can/can-utils.git slcan_attach and cangen are used - create a network link between two serial CAN interfaces with: $ socat PTY,link=/tmp/slcan0,raw TCP4-LISTEN:5 & $ socat TCP4:localhost:5 PTY,link=/tmp/slcan1,raw & $ slcan_attach /tmp/slcan0 $ slcan_attach /tmp/slcan1 $ ip link set slcan0 up $ ip link set slcan1 up - produce a kernel panic by overloading the CAN interfaces: $ cangen slcan0 -g0 Please keep me in CC. I am not subscribed to the list. If I can provide any more information, I will be glad to do so. This is the patch. It applies to the current linux master branch: From 9f67139bebb938026406a66c1411e0b50628a238 Mon Sep 17 00:00:00 2001 From: Andre Naujoks Date: Mon, 1 Jul 2013 15:45:13 +0200 Subject: [PATCH 1/2] remove direct call to tty_wakeup in pty_write. Signed-off-by: Andre Naujoks --- drivers/tty/pty.c | 1 - 1 file changed, 1 deletion(-) diff --git a/drivers/tty/pty.c b/drivers/tty/pty.c index abfd990..5dcb782 100644 --- a/drivers/tty/pty.c +++ b/drivers/tty/pty.c @@ -127,7 +127,6 @@ static int pty_write(struct tty_struct *tty, const unsigned c
Re: [PATCH] kernel panic, pty.c: remove direct call to tty_wakup in pty_write
On 01/07/13 15:49, Andre Naujoks wrote:
Hello.
This patch removes the direct call to tty_wakeup in pty_write. I have
not noticed any drawbacks with this but I am not familiar with the pty
driver at all. I think what happens is a recursive loop,
write_wakeup->write->write_wakeup ...
Indeed there is a recursive loop that I have witnessed whilst using SLIP
over USB HID.
In the failure case for SLIP, xleft remains positive and recursion
causes a stack overflow as xleft is not updated during the recursion:
sl_encaps()-->pty_write()-->tty_wakeup()-->slip_write_wakeup()-->pty_write()-->tty_wakeup()-->slip_write_wakeup()
etc.
The underlying issue being pty_write() calling tty_wakeup() within the
initial write thread. Note that the TTY wakeup event uses tty_wakeup()
as a callback to request more data.
The documentation for the tty interface forbids this direct call:
(from Documentation/serial/tty.txt)
write_wakeup() - May be called at any point between open and close.
The TTY_DO_WRITE_WAKEUP flag indicates if a call
is needed but always races versus calls. Thus the
ldisc must be careful about setting order and to
handle unexpected calls. Must not sleep.
The driver is forbidden from calling this directly
from the ->write call from the ldisc as the ldisc
is permitted to call the driver write method from
this function. In such a situation defer it.
I also saw that documentation and the code seems to be breaking that
description.
It is possible to mitigate against
pty-write-->tty_wakeup()-->slip_write_wakeup() by clearing and setting
TTY_DO_WRITE_WAKEUP at the "correct" time in the layers bound to
PTY/TTY. Indeed, I modified SLIP to do that and sent patches to the
linux-netdev mailing list although I am not aware of any progress in
having those patches accepted.
Perhaps this mitigation could be applied to your scenario ?
Eventually, your patch could be used when all protocols have mitigration
in place.
The direct call caused a reproducable kernel panic (see bottom of this
mail) for me with the following setup:
- using can-utils from git://gitorious.org/linux-can/can-utils.git
slcan_attach and cangen are used
- create a network link between two serial CAN interfaces with:
$ socat PTY,link=/tmp/slcan0,raw TCP4-LISTEN:5 &
$ socat TCP4:localhost:5 PTY,link=/tmp/slcan1,raw &
$ slcan_attach /tmp/slcan0
$ slcan_attach /tmp/slcan1
$ ip link set slcan0 up
$ ip link set slcan1 up
- produce a kernel panic by overloading the CAN interfaces:
$ cangen slcan0 -g0
Please keep me in CC. I am not subscribed to the list.
If I can provide any more information, I will be glad to do so.
This is the patch. It applies to the current linux master branch:
From 9f67139bebb938026406a66c1411e0b50628a238 Mon Sep 17 00:00:00 2001
From: Andre Naujoks
Date: Mon, 1 Jul 2013 15:45:13 +0200
Subject: [PATCH 1/2] remove direct call to tty_wakeup in pty_write.
Signed-off-by: Andre Naujoks
---
drivers/tty/pty.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/drivers/tty/pty.c b/drivers/tty/pty.c
index abfd990..5dcb782 100644
--- a/drivers/tty/pty.c
+++ b/drivers/tty/pty.c
@@ -127,7 +127,6 @@ static int pty_write(struct tty_struct *tty, const
unsigned char *buf, int c)
/* And shovel */
if (c) {
tty_flip_buffer_push(to->port);
- tty_wakeup(tty);
}
}
return c;
I agree that this looks to be a simple remedy to the issue. However,
this code has existed in this state for many years so there is a risk
that some applications rely on this "feature" in order to work. For
example, SLIP uses this "feature" although I am not certain that the
existing SLIP code would break if your patch were applied. The TTY
wakeup event should take care of completing the transmission (in theory).
I think a lower risk approach would be to modify the upper layers in the
protocol write function to clear the TTY_DO_WRITE_WAKEUP flag before
pty_write is called and set TTY_DO_WRITE_WAKEUP afterwards to allow the
TTY wakeup event to complete the transmission. I did this in the SLIP
sl_encaps() and slip_write_wakeup() functions. Note that the SLIP
segmentation mechanism is also broken as it is possible to send
truncated SLIP frames upon a failure to send all characters of the
frame. This leads to the recursive loop on the transmission of next SLIP
frame because xleft remains positive (uninitialised) and this causes the
kernel to crash catastrophically. The scheduler crashes due to the stack
overflow overwriting the task data structures.
If anyone is interested, I could upload my SLIP patches to the
linux-kernel mailing list.
Regards,
Dean Jenkins
Mentor Graphics
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel"
Re: BUG: tty: memory corruption through tty_release/tty_ldisc_release
ocalhost kernel: RSP Jun 25 12:52:32 localhost kernel: ---[ end trace f219a9b765a4acc9 ]--- Jun 25 12:52:52 localhost vmnetBridge: Removing interface eth0 index:2 Jun 25 12:52:52 localhost vmnetBridge: RTM_NEWLINK: name:eth0 index:2 flags:0x00011043 Jun 25 12:52:52 localhost vmnetBridge: Adding interface eth0 index:2 Thanks, Regards, Dean Jenkins Mentor Graphics -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: SLIP: Is there a maintainer for drivers/net/slip/slip.c ?
Hi Peter, On 24/06/13 14:53, Peter Hurley wrote: On 06/24/2013 04:27 AM, Dean Jenkins wrote: Hi, Using the Linux v3.9 tag, I note that ./scripts/get_maintainer.pl -f drivers/net/slip/slip.c net...@vger.kernel.org (open list:NETWORKING DRIVERS) linux-kernel@vger.kernel.org (open list) There seems to be no maintainer for drivers/net/slip/slip.c, is that true ? I am asking because sl_encaps() and slip_write_wakeup() do not handle error codes from tty->ops->write() and a recursive stack overflow crash can occur if the tty->ops->write() fails to write all the characters. I have some patches to fix this but would like some feedback on an appropriate solution. SLIP changes would likely go through David Miller . TTY changes would go through Greg Kroah-Hartman I have already sent an E-mail to David Miller and the netdev mailing list but so far got not reply from David. So I guess I have sent my patches to the correct place ;) Thanks, Regards, Dean Jenkins Mentor Graphics -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
SLIP: Is there a maintainer for drivers/net/slip/slip.c ?
Hi, Using the Linux v3.9 tag, I note that ./scripts/get_maintainer.pl -f drivers/net/slip/slip.c net...@vger.kernel.org (open list:NETWORKING DRIVERS) linux-kernel@vger.kernel.org (open list) There seems to be no maintainer for drivers/net/slip/slip.c, is that true ? I am asking because sl_encaps() and slip_write_wakeup() do not handle error codes from tty->ops->write() and a recursive stack overflow crash can occur if the tty->ops->write() fails to write all the characters. I have some patches to fix this but would like some feedback on an appropriate solution. Thanks, Dean Jenkins Mentor Graphics -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: MMC/SDIO sub-system: block mode versus byte mode
Hi Pierre,
Thanks for information.
My card does support 256 byte transfers in byte mode but I was expecting
block mode to be used as block mode is supported by the card. Indeed,
sdio_io_rw_ext_helper() checks for support of block mode before using
byte mode. eg. block mode is preferred over byte mode in the design of
sdio_io_rw_ext_helper().
Yes, I do think single block transfers is broken :(
Both sdio_io_rw_ext_helper() and mmc_io_rw_extended() are broken for
single block transfers.
Will you be doing a fix or how does a fix get proposed ?
Thanks,
Regards,
Dean.
On Thu, 2007-11-22 at 12:42 +0100, Pierre Ossman wrote:
> On Thu, 22 Nov 2007 10:41:43 +
> Dean Jenkins <[EMAIL PROTECTED]> wrote:
>
> > Hi Pierre,
> >
> > I've been checking my SDIO CMD53 data transfers and I notice the
> > following:
> >
> > block size = 256 bytes for my SDIO card.
> >
> > transfers of 256 bytes uses byte mode ( I expected block mode )
> > transfers of 512 bytes uses block mode ( because blocks = 2 )
> >
> > From the SD spec a single block transfer seems to be valid.
> >
>
> Both are valid, which is the source of the problem. The API was designed
> under the assumption that hardware actually followed the register interface
> and didn't distinguish between byte and block transfers. Unfortunately, it
> seems to be quite common that they do. Your situation is a problematic corner
> case.
>
> >
> > If it is a bug then I suggest the sdio_io_rw_ext_helper() in sdio_io.c
> > be able to select byte mode by setting blocks to 0 and then blocks can
> > be set to 1 to select block mode for a single block.
> >
>
> I'm afraid I don't follow. Do you want to add another parameter? Even if you
> do, sdio_io_rw_ext_helper() is an internal API. Changing it won't make things
> better for the function drivers.
>
> >
> > Technically, there is a bug in sdio_io_rw_ext_helper() in sdio_io.c
> >
> > 211 while (remainder > func->cur_blksize) {
> >
> > should be, = missing
> >
> > 211 while (remainder >= func->cur_blksize) {
> >
>
> Unless some hardware is quirky in the opposite way and needs to be able to do
> byte transfer of 256 bytes. Proper cards won't care either way, but as you
> can see, we can't support both types of buggy cards.
>
> > however the resulting call to mmc_io_rw_extended() is the same as blocks
> > = 1 for both the "block mode" while loop (with the "fix") and the "byte
> > mode" while loop (without the "fix"). The "byte mode" while loop has a
> > hard coded value of 1 for the blocks parameter. So the bug is masked. As
> > I said above, I think the "byte mode" should use a value of 0 for the
> > blocks parameter.
> >
>
> Ah. mmc_io_rw_extended() is indeed broken in that it can't do single block
> requests. That should be fixed.
>
> However, it still won't affect what your function driver can do as
> mmc_io_rw_extended() is a bunch of layers down.
>
> I guess I'll have to consider adding a more low-level API. The big problem
> with such a call would be that it can fail requests because the host or card
> cannot satisfy them. So a function driver using such an API would have to use
> a much more defensive programming (which can incur a performance hit).
>
> Rgds
--
Dean Jenkins
Embedded Software Engineer
MontaVista Software (UK)
Professional Services Division
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
MMC/SDIO sub-system: block mode versus byte mode
Hi Pierre,
I've been checking my SDIO CMD53 data transfers and I notice the
following:
block size = 256 bytes for my SDIO card.
transfers of 256 bytes uses byte mode ( I expected block mode )
transfers of 512 bytes uses block mode ( because blocks = 2 )
>From the SD spec a single block transfer seems to be valid.
I notice the function mmc_io_rw_extended() in sdio_ops.c seems to select
byte mode when the transfer size is 512 or less when the blocks
parameter variable is 1. In fact blocks is never 0.
Therefore is this a feature or a bug that prevents 256 bytes being sent
in block mode when it is a single complete 1 block of data ?
If it is a bug then I suggest the sdio_io_rw_ext_helper() in sdio_io.c
be able to select byte mode by setting blocks to 0 and then blocks can
be set to 1 to select block mode for a single block.
Have I missed something ?
Technically, there is a bug in sdio_io_rw_ext_helper() in sdio_io.c
211 while (remainder > func->cur_blksize) {
should be, = missing
211 while (remainder >= func->cur_blksize) {
however the resulting call to mmc_io_rw_extended() is the same as blocks
= 1 for both the "block mode" while loop (with the "fix") and the "byte
mode" while loop (without the "fix"). The "byte mode" while loop has a
hard coded value of 1 for the blocks parameter. So the bug is masked. As
I said above, I think the "byte mode" should use a value of 0 for the
blocks parameter.
What do you think ?
Regards,
Dean.
--
Dean Jenkins
Embedded Software Engineer
MontaVista Software (UK)
Professional Services Division
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: MMC sub-system: SDIO block-mode with increment address issue
Hi Pierre, Thanks for explaining. I guess then I have a crappy SDIO card that needs the "voodoo" bit set for correct operation. There are other registers close to the START ADDRESS so it is a FIFO, but the Incrementing Address flag is needed to read from and write to the FIFO correctly. Regards, Dean. On Wed, 2007-11-21 at 14:08 +0100, Pierre Ossman wrote: > On Wed, 21 Nov 2007 11:57:01 + > Dean Jenkins <[EMAIL PROTECTED]> wrote: > > > Hi Pierre, > > > > I've looked at the SD Card Association's SDIO Part E1 V2.00 > > specification concerning the Incrementing Address OP Code field for > > CMD53. > > > > The specification indicates that the START ADDRESS is inserted into the > > Register Address register field. When the OP Code field has a value of 1 > > then the during the transfer the IO address is internally incremented > > for each byte transferred. This applies to a single CMD53 command. > > > > Looking at the implementation of sdio_io_rw_ext_helper() in sdio_io.c. > > This function can send multiple CMD53 commands. My concern is that with > > incrementing address mode selected the START ADDRESS is erroneously > > changed for subsequent CMD53 commands in the while loop. > > > > Since the caller is not supposed to care about the internal operation of > sdio_io_rw_ext_helper(), the address increase is a must to maintain a > consistent behaviour regardless of what transactions it decides to use. > > I.e. if I write 2048 bytes with start address 0x1000, I expect it to do a > single write to register 0x1000 through 0x17FF, not and unknown number of > writes to some unknown interval. > > Now what I suspect you're championing is to support some broken card that > treats the address increase bit in CMD53 as "Magic voodoo bit #5" instead of > the definition in the SDIO spec. I.e. the only address it cares about is the > start address and hence needs it to be the same for each transaction. This > kind of blatant disregard for the SDIO register design is of course not what > sdio_io_rw_ext_helper() was designed for, and probably never will be. > > > What I am trying to say is I don't believe the START ADDRESS should be > > changed by the core driver when incrementing address mode is used. I > > think incrementing address mode only applies internally to a single > > CMD53 command. The SDIO card must physically have a suitable register > > present at the START ADDRESS so changing this address to something > > dependent on the data size is going to fail I think. > > > > The SDIO card must physically have a suitable register present at the entire > relevant range, not just the start address. If it doesn't then it isn't > following the register interface design of SDIO (having the "increase > address" bit would just be silly if the arguments were arbitrary tokens and > not part of a consistent address space). > > > Do you have any evidence that any card drivers will use > > sdio_io_rw_ext_helper() in a manner that needs the START ADDRESS to be > > changed by the core driver ? > > > > There are no drivers using "increase address" yet. The ones so far have all > used a single byte FIFO port. > > Rgds -- Dean Jenkins Embedded Software Engineer MontaVista Software (UK) Professional Services Division - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: MMC sub-system: SDIO block-mode with increment address issue
Hi Pierre, I've looked at the SD Card Association's SDIO Part E1 V2.00 specification concerning the Incrementing Address OP Code field for CMD53. The specification indicates that the START ADDRESS is inserted into the Register Address register field. When the OP Code field has a value of 1 then the during the transfer the IO address is internally incremented for each byte transferred. This applies to a single CMD53 command. Looking at the implementation of sdio_io_rw_ext_helper() in sdio_io.c. This function can send multiple CMD53 commands. My concern is that with incrementing address mode selected the START ADDRESS is erroneously changed for subsequent CMD53 commands in the while loop. What I am trying to say is I don't believe the START ADDRESS should be changed by the core driver when incrementing address mode is used. I think incrementing address mode only applies internally to a single CMD53 command. The SDIO card must physically have a suitable register present at the START ADDRESS so changing this address to something dependent on the data size is going to fail I think. Do you have any evidence that any card drivers will use sdio_io_rw_ext_helper() in a manner that needs the START ADDRESS to be changed by the core driver ? Regards, Dean. On Tue, 2007-11-20 at 15:10 +0100, Pierre Ossman wrote: > On Tue, 20 Nov 2007 12:26:11 +0000 > Dean Jenkins <[EMAIL PROTECTED]> wrote: > > > Hi Pierre, > > > > IMHO the issue is there is no upper bound limit to the valid address > > range in sdio_io_rw_ext_helper() in sdio_io.c. > > > > I call sdio_memcpy_toio() as it enables the incrementing address flag in > > the CMD53 command but if I try passing too much data then the actual > > address of the subsequent CMD53 commands are erroneously incremented out > > of range. > > > > The difficulty is the SDIO card can transfer 8 blocks in a single CMD53 > > command using the incrementing address flag. However > > sdio_io_rw_ext_helper() will not prevent the attempt at sending 9 blocks > > transferred as 2 CMD53 commands of 8 blocks + 1 block and the last block > > goes to the wrong address. This causes a big system crash. I suspect the > > SDIO card internally resets and the MMC sub-system can't handle the > > error condition. > > I'm afraid I still can't see the problem. If you want to transfer 9 blocks, > then the method by which you do so shouldn't matter. So 9, or 8 + 1 should > give the same end result. > > > > > This means the card driver needs to know that it cannot use > > sdio_memcpy_toio() to send any size of data but must ensure it does not > > exceed 8 blocks before calling sdio_memcpy_toio(). IMHO this makes the > > card driver undesirably tightly coupled with the core driver. OK. I'll > > workaround it using multiple calls to sdio_memcpy_toio(). > > > > Well, the problem is that the abstraction used should work just fine > according to how the SDIO standard is defined. The problem seems to be that > some card vendors decided to go their own way and started treating the SDIO > interface as something other than a simple register interface. > > As long as that is the case, there will be a lot of pain supporting these > weird cards. We can only debate where to put that pain and what compromises > to make. > > > BTW. Is the API for the exported SDIO core functions documented > > anywhere ? > > Yes, as kerneldoc tags for the relevant functions. Have a look in > drivers/core/sdio_io.c if you don't want to build the full document. > > Rgds -- Dean Jenkins Embedded Software Engineer MontaVista Software (UK) Professional Services Division - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [Fwd: MMC sub-system: SDIO block-mode with increment address issue]
Hi Pierre,
My card driver needed to set the R/W E4MI bit in the Card Capability
register (0x08) in CCCR (function 0). Perhaps it is unnecessary ?
BTW. It is easy to for the card driver to access function 0 registers by
doing the following...
...
int old_num = func->num; /* note the current function number */
/* force access to function 0 area */
func->num = 0;
/* enable the e4mi in the card capability register */
x = sdio_readb(func, 0x08, &ret);
if ( ret == 0 ) {
x |= 0x20;
sdio_writeb(func, x, 0x08, &ret);
}
/* restore to original function number */
func->num = old_num;
...
Regards,
Dean.
On Tue, 2007-11-20 at 11:51 +0100, Pierre Ossman wrote:
> On Tue, 20 Nov 2007 10:28:53 +
> Dean Jenkins <[EMAIL PROTECTED]> wrote:
>
> > Hi Pierre,
> >
> > I've managed to find your E-mail address in the Linux kernel mailing
> > list. I hope it is OK to directly E-mail you ?
> >
>
> You should also find my address in the MAINTAINERS file, which is where you
> should look for contact info. But yes, you can contact me directly. I prefer
> that you also add a relevant mailing list though.
>
> > I work for MontaVista Software in the UK. I guess you know that
> > MontaVista are using the MMC/SDIO sub-system in their latest Mobilinux
> > 5.0 product ? It is using a 2.6.21 kernel.
> >
> > Yesterday, I sent an E-mail to you (attached) to the Linux kernel
> > mailing list. Do you have any comments to make ?
> >
>
> Didn't notice it. I'll have a look.
>
> > Do you have any bug reporting facility for the MMC/SDIO sub-system ? I
> > would like to report some SDIO API limitations for the SDIO card drivers
> > that I needed to workaround. For example, my SDIO driver needs to modify
> > the contents of the Card Capability register in function 0 to enable
> > block-mode support but the card driver has been restricted to only be
> > able to use function 1. There are other registers as well that the card
> > driver needs access to.
> >
>
> That is by design (as you probably can tell). Letting function drivers touch
> card global stuff under the feet of the SDIO core is a big layering violation
> and bound to screw things up eventually.
>
> Could you explain more in detail what it is you need to fiddle with in
> function 0 and why?
>
> Rgds
--
Dean Jenkins
Embedded Software Engineer
MontaVista Software (UK)
Professional Services Division
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: MMC sub-system: SDIO block-mode with increment address issue
Hi Pierre, IMHO the issue is there is no upper bound limit to the valid address range in sdio_io_rw_ext_helper() in sdio_io.c. I call sdio_memcpy_toio() as it enables the incrementing address flag in the CMD53 command but if I try passing too much data then the actual address of the subsequent CMD53 commands are erroneously incremented out of range. The difficulty is the SDIO card can transfer 8 blocks in a single CMD53 command using the incrementing address flag. However sdio_io_rw_ext_helper() will not prevent the attempt at sending 9 blocks transferred as 2 CMD53 commands of 8 blocks + 1 block and the last block goes to the wrong address. This causes a big system crash. I suspect the SDIO card internally resets and the MMC sub-system can't handle the error condition. This means the card driver needs to know that it cannot use sdio_memcpy_toio() to send any size of data but must ensure it does not exceed 8 blocks before calling sdio_memcpy_toio(). IMHO this makes the card driver undesirably tightly coupled with the core driver. OK. I'll workaround it using multiple calls to sdio_memcpy_toio(). BTW. Is the API for the exported SDIO core functions documented anywhere ? Thanks, Regards, Dean. On Tue, 2007-11-20 at 11:58 +0100, Pierre Ossman wrote: > On Mon, 19 Nov 2007 11:44:54 + > Dean Jenkins <[EMAIL PROTECTED]> wrote: > > > This E-mail is for the attention of Pierre Ossman (MMC sub-system > > maintainer) > > A cc helps if you want my attention. ;) > > > > > Hi Pierre, > > > > I've being trying to get SDIO block-mode with incrementing address to > > work on an OMAP2430 based reference board with an SDIO card. > > > > Looking at the latest code ( as of 19/11/2007 ) on the mmc-git tree (I'm > > not a git expert so I'm not sure how to reference the codebase). I have > > a comment to make concerning the following code snippet... > > git log or git show will give you your current top commit id. > > > > I think the lines > > > > 227 if (incr_addr) > > 228 addr += size; > > > > are incorrect and should be removed. I think the SDIO increment address > > parameter relates to the internal operation of the SDIO card and NOT to > > the external register address of the FIFO. In other words, I think with > > incrementing address enabled in block mode, the register address of the > > FIFO in the SDIO function register space will be erroneously changed on > > the next block write and will fail (it seems to fail on my card). It > > seems strange to change the register address ? > > > > I don't follow. The SDIO specification very clearly defines the behaviour of > incrementing address. The referenced code is very much needed to keep that > behaviour when we need to split up the transfer. > > I assume what you want is multiple transactions with incrementing address, > but each transaction restarting at the same base address. In that case you'll > have to call the sdio register functions multiple times. > > Rgds -- Dean Jenkins Embedded Software Engineer MontaVista Software (UK) Professional Services Division - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
MMC sub-system: SDIO block-mode with increment address issue
This E-mail is for the attention of Pierre Ossman (MMC sub-system maintainer) Hi Pierre, I've being trying to get SDIO block-mode with incrementing address to work on an OMAP2430 based reference board with an SDIO card. Looking at the latest code ( as of 19/11/2007 ) on the mmc-git tree (I'm not a git expert so I'm not sure how to reference the codebase). I have a comment to make concerning the following code snippet... In drivers/mmc/core/sdio_io.c Function: sdio_io_rw_ext_helper() 219 ret = mmc_io_rw_extended(func->card, write, 220 func->num, addr, incr_addr, buf, 221 blocks, func->cur_blksize); 222 if (ret) 223 return ret; 224 225 remainder -= size; 226 buf += size; 227 if (incr_addr) 228 addr += size; 229 } I think the lines 227 if (incr_addr) 228 addr += size; are incorrect and should be removed. I think the SDIO increment address parameter relates to the internal operation of the SDIO card and NOT to the external register address of the FIFO. In other words, I think with incrementing address enabled in block mode, the register address of the FIFO in the SDIO function register space will be erroneously changed on the next block write and will fail (it seems to fail on my card). It seems strange to change the register address ? This also relates to byte mode. What do you think ? Also, do you have a bug reporting procedure for mmc-git tree ? Regards, Dean. -- Dean Jenkins Embedded Software Engineer MontaVista Software (UK) Professional Services Division - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
