lkml.org 
[lkml]   [2010]   [Jan]   [5]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    SubjectRe: USB: serial: kfifo_len locking
    From
    Date
    Am Dienstag, den 05.01.2010, 12:35 +0100 schrieb Johan Hovold:
    > > > > > Which does not make any sense at all. Bad merge? What do you say Greg?
    > > > >
    > > > > I don't know where is your problem? This are two different functions
    > > > > usb_serial_generic_write_room() and usb_serial_generic_chars_in_buffer()
    > > >
    > > > Exactly my point.
    > > >
    > > > The drain patch needed to modify chars_in_buffer, but the patch in Greg's
    > > > tree modifies write_room instead (which does not make sense and was
    > > > neither part of the submitted patch).
    > > >
    > > Sorry, but i am not sure if i the right address about your complains.
    > > The only thing i have done in the usb serial driver is the port to the
    > > new kfifo API. This is the original patch i had posted:
    >
    > The drain patch merge was a side-track and you were CC:d as you
    > were part of the original thread.
    >
    > You did however remove the locking on kfifo_len that the original author
    > had put there with the exact patch you're quoting:
    >
    > > diff -u -N -r -p old/drivers/usb/serial/generic.c new/drivers/usb/serial/generic.c
    > > --- old/drivers/usb/serial/generic.c 2009-12-23 08:54:06.966476248 +0100
    > > +++ new/drivers/usb/serial/generic.c 2009-12-23 09:06:25.778474708 +0100
    > > @@ -276,7 +276,7 @@ static int usb_serial_generic_write_star
    > > if (port->write_urb_busy)
    > > start_io = false;
    > > else {
    > > - start_io = (kfifo_len(port->write_fifo) != 0);
    > > + start_io = (kfifo_len(&port->write_fifo) != 0);
    > > port->write_urb_busy = start_io;
    > > }
    > > spin_unlock_irqrestore(&port->lock, flags);
    > > @@ -285,7 +285,7 @@ static int usb_serial_generic_write_star
    > > return 0;
    > >
    > > data = port->write_urb->transfer_buffer;
    > > - count = kfifo_out_locked(port->write_fifo, data, port->bulk_out_size, &port->lock);
    > > + count = kfifo_out_locked(&port->write_fifo, data, port->bulk_out_size, &port->lock);
    > > usb_serial_debug_data(debug, &port->dev, __func__, count, data);
    > >
    > > /* set up our urb */
    > > @@ -345,7 +345,7 @@ int usb_serial_generic_write(struct tty_
    > > return usb_serial_multi_urb_write(tty, port,
    > > buf, count);
    > >
    > > - count = kfifo_in_locked(port->write_fifo, buf, count, &port->lock);
    > > + count = kfifo_in_locked(&port->write_fifo, buf, count, &port->lock);
    > > result = usb_serial_generic_write_start(port);
    > >
    > > if (result >= 0)
    > > @@ -370,7 +370,7 @@ int usb_serial_generic_write_room(struct
    > > (serial->type->max_in_flight_urbs -
    > > port->urbs_in_flight);
    > > } else if (serial->num_bulk_out)
    > > - room = port->write_fifo->size - kfifo_len(port->write_fifo);
    > > + room = kfifo_avail(&port->write_fifo);
    > > spin_unlock_irqrestore(&port->lock, flags);
    > >
    > > dbg("%s - returns %d", __func__, room);
    > > @@ -391,7 +391,7 @@ int usb_serial_generic_chars_in_buffer(s
    > > chars = port->tx_bytes_flight;
    > > spin_unlock_irqrestore(&port->lock, flags);
    > > } else if (serial->num_bulk_out)
    > > - chars = kfifo_len(port->write_fifo);
    > > + chars = kfifo_len(&port->write_fifo);
    >
    > Here's the change. The fifo used to be protected by a lock, but is no
    > longer.
    >

    I posted yesterday a patch to this thread. It would be great if you read
    and check this patch before complaining again!!!!

    > Never say you did.
    >

    Sorry, i had no real idea what is your problem, if this is not what you
    want. As i mentioned i posted to you yesterday a fix for the possible
    kfifo_len() bug, but i didn't get a response if this is fixing your
    problem. Again the patch:

    diff -u -N -r -p linux-2.6.33-rc2.orig/drivers/usb/serial/generic.c linux-2.6.33-rc2.new/drivers/usb/serial/generic.c
    --- linux-2.6.33-rc2.orig/drivers/usb/serial/generic.c 2009-12-27 23:37:03.566060210 +0100
    +++ linux-2.6.33-rc2.new/drivers/usb/serial/generic.c 2010-01-04 20:15:38.023351711 +0100
    @@ -386,12 +386,12 @@ int usb_serial_generic_chars_in_buffer(s

    dbg("%s - port %d", __func__, port->number);

    - if (serial->type->max_in_flight_urbs) {
    - spin_lock_irqsave(&port->lock, flags);
    + spin_lock_irqsave(&port->lock, flags);
    + if (serial->type->max_in_flight_urbs)
    chars = port->tx_bytes_flight;
    - spin_unlock_irqrestore(&port->lock, flags);
    - } else if (serial->num_bulk_out)
    + else if (serial->num_bulk_out)
    chars = kfifo_len(&port->write_fifo);
    + spin_unlock_irqrestore(&port->lock, flags);

    dbg("%s - returns %d", __func__, chars);
    return chars;
    This patch should solve the possible race (if there is one). With this
    patch all kfifo_... access are locked by the port->lock spinlock. If
    this is what you want i will posted it as a bug fix to andrew.

    Stefani




    \
     
     \ /
      Last update: 2010-01-05 13:03    [W:0.026 / U:13.320 seconds]
    ©2003-2017 Jasper Spaans. hosted at Digital OceanAdvertise on this site