Hi,
On Tue, Jan 21, 2014 at 10:35:54AM +0100, Philip Philippe wrote:
> Hi,
>
> When using the USB serial gadget it regularly occurs that USB packets
> arrive at the host in the wrong order. I am using kernel version 3.4
> with the corresponding RT patch on an ARM Cortex-A8 platform.
>
> The problem appears to be in file 'drivers/usb/gadget/u_serial.c'.
>
> It seems that kernel thread "irq/92-musb-hdr" and a userspace thread
> have a race when sending data, in function 'gs_start_tx'. The kernel
> thread calls 'gs_start_tx' from the callback function
> 'gs_write_complete', a userspace thread through 'gs_write' and
> 'gs_flush_chars'. The problem is triggered by the fact that the
> spinlock 'port_lock' is dropped temporarily in 'gs_start_tx' during
> the time 'usb_ep_queue' is executed. There might be good reasons,
> which I am unaware off, to do that. So, suppose that the userspace
> thread has just released the spinlock and the scheduler selects at
> that moment the (normally) higher prioritized kernel thread, which was
> waiting at the 'port_lock' in 'gs_write_complete' to run. This will
> allow the kernel thread to overtake the sleeping userspace thread and
> send out the "next" USB packet first!
>
> I can't reproduce the problem if I disable the RT patch, but I think
> that the problem might as well appear, if for instance an interrupt
> just happens during the time the lock is dropped in 'gs_start_rx'. But
> if you prove me wrong, I'm happy to post it in the RT mailing list
> instead.
>
> The provided patch is a diff to the mainline. I've introduced a flag
> to mark when there is already a thread busy in the loop emptying the
> pool in 'gs_start_tx'. In that case another thread would just skip the
> loop. This seems to work for me but it might not be foolproof and not
> elegant enough.
>
> By checking the code of the mainline I didn't find any relevant change
> that would indicate that the issue has been already solved in the
> meantime. For the moment it would be too much work for me to make the
> mainline runnable on my platform.
>
> Please CC me and arnout@xxxxxxx as we are not subscribed.
cool, please have a look at Documentation/SubmittingPatches in order to
send it in the proper format. You have information in your commit log
which shouldn't be in the commit log.
Also, let us know how you tested this and, if you have a simple test
application, it would be nice to share it.
cheers
> diff --git a/drivers/usb/gadget/u_serial.c b/drivers/usb/gadget/u_serial.c
> index b369292..d2682f5 100644
> --- a/drivers/usb/gadget/u_serial.c
> +++ b/drivers/usb/gadget/u_serial.c
> @@ -116,6 +116,7 @@ struct gs_port {
> int write_allocated;
> struct gs_buf port_write_buf;
> wait_queue_head_t drain_wait; /* wait while writes drain */
> + bool write_busy;
>
> /* REVISIT this state ... */
> struct usb_cdc_line_coding port_line_coding; /* 8-N-1 etc */
> @@ -366,7 +367,7 @@ __acquires(&port->port_lock)
> int status = 0;
> bool do_tty_wake = false;
>
> - while (!list_empty(pool)) {
> + while (!port->write_busy && !list_empty(pool)) {
> struct usb_request *req;
> int len;
>
> @@ -396,9 +397,11 @@ __acquires(&port->port_lock)
> * NOTE that we may keep sending data for a while after
> * the TTY closed (dev->ioport->port_tty is NULL).
> */
> + port->write_busy = true;
> spin_unlock(&port->port_lock);
> status = usb_ep_queue(in, req, GFP_ATOMIC);
> spin_lock(&port->port_lock);
> + port->write_busy = false;
>
> if (status) {
> pr_debug("%s: %s %s err %d\n",
> @@ -770,6 +773,7 @@ static int gs_open(struct tty_struct *tty, struct file *file)
> /* Do the "real open" */
> spin_lock_irq(&port->port_lock);
>
> + port->write_busy = false;
seems like you should make sure to clear this during close too.
--
balbi
Attachment:
signature.asc
Description: Digital signature
