USB: serial: kfifo_len locking
Johan Hovold
I noticed that the locking that used to protect kfifo_len in
usb_serial_generic_chars_in_buffer was removed when the kifo api changed
to not use internal locking (c1e13f25674ed564948ecb7dfe5f83e578892896 --
kfifo: move out spinlock).
Was this intentional?
I found a related discussion here
http://lkml.org/lkml/2009/12/18/433
where you seem to say that no such locking is required as long as
kfifo_reset is never called (and that one could use kfifo_reset_out
instead)?
However, kfifo_reset was still being called when the locking was removed
and not until later was it changed to kfifo_reset_out
(119eecc831a42bd090543568932e440c6831f1bb -- Fix usb_serial_probe()
problem introduced by the recent kfifo changes).
Does this last change imply that no locking in
usb_serial_generic_chars_in_buffer is required? If this is the case,
perhaps such locking guidelines could be added to kfifo.h?
Thanks,
Johan
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majo...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Stefani Seibold
> Hi Stefani,
>
> I noticed that the locking that used to protect kfifo_len in
> usb_serial_generic_chars_in_buffer was removed when the kifo api changed
> to not use internal locking (c1e13f25674ed564948ecb7dfe5f83e578892896 --
> kfifo: move out spinlock).
>
> Was this intentional?
>
Yes, the locking is not necessary until only one reader and one writer
is using the fifo. If you don't trust this you can apply this patch:
--- 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);
Then everything kfifo_... access in the usb serial is handled with an
active spinlock.
> I found a related discussion here
>
> http://lkml.org/lkml/2009/12/18/433
>
> where you seem to say that no such locking is required as long as
> kfifo_reset is never called (and that one could use kfifo_reset_out
> instead)?
> However, kfifo_reset was still being called when the locking was removed
> and not until later was it changed to kfifo_reset_out
> (119eecc831a42bd090543568932e440c6831f1bb -- Fix usb_serial_probe()
> problem introduced by the recent kfifo changes).
>
In the reader part kfifo_reset_out() will make the reset safer, to
prevent side effects against kfifo_len()
> Does this last change imply that no locking in
> usb_serial_generic_chars_in_buffer is required?
Sorry, i don't understand the USB serial code, so i tried my best to
port it to the new API. The author must know if locking for the fifo
access is required.
> If this is the case,
> perhaps such locking guidelines could be added to kfifo.h?
>
The locking guidelines are still available in the function descriptions:
until only:
Note that with only one concurrent reader and one concurrent writer, you
don't need extra locking to use these functions.
Stefani
Pete Zaitcev
This actually was a side effect of the "byte lost on close" patch
that I submitted, it should be in Greg's tree. The relevant part goes
like this:
diff --git a/drivers/usb/serial/generic.c b/drivers/usb/serial/generic.c
index f1ea3a3..3372faa 100644
--- a/drivers/usb/serial/generic.c
+++ b/drivers/usb/serial/generic.c
@@ -386,12 +386,15 @@ int usb_serial_generic_chars_in_buffer(struct tty_struct *tty)
dbg("%s - port %d", __func__, port->number);
+ spin_lock_irqsave(&port->lock, flags);
if (serial->type->max_in_flight_urbs) {
- spin_lock_irqsave(&port->lock, flags);
chars = port->tx_bytes_flight;
- spin_unlock_irqrestore(&port->lock, flags);
- } else if (serial->num_bulk_out)
- chars = kfifo_len(&port->write_fifo);
+ } else if (serial->num_bulk_out) {
+ /* This overcounts badly, but is good enough for drain wait. */
+ chars = __kfifo_len(port->write_fifo);
+ chars += port->write_urb_busy * port->bulk_out_size;
+ }
+ spin_unlock_irqrestore(&port->lock, flags);
dbg("%s - returns %d", __func__, chars);
return chars;
Stefani Seibold
This is the same patch as my. But __kfifo_len is renamed into kfifo_len.
Who should submit this patch?
Stefani
Stefani Seibold
> Greg's tree:
>
> http://git.kernel.org/?p=linux/kernel/git/gregkh/patches.git;a=blob;f=usb/usb-serial-mct_usb232-add-drain-on-close.patch;h=c464f1a82e93df0dd41762a8cb33b0b22e90cdd7;hb=1ea72e7c40b239c6b6f88a4993196be66fc3d892
>
> 38 --- a/drivers/usb/serial/generic.c
> 39 +++ b/drivers/usb/serial/generic.c
> 40 @@ -369,8 +369,11 @@ int usb_serial_generic_write_room(struct
> 41 room = port->bulk_out_size *
> 42 (serial->type->max_in_flight_urbs -
> 43 port->urbs_in_flight);
> 44 - } else if (serial->num_bulk_out)
> 45 + } else if (serial->num_bulk_out) {
> 46 + /* This overcounts badly, but is good enough for drain wait. */
> 47 room = kfifo_avail(&port->write_fifo);
> 48 + room += port->write_urb_busy * port->bulk_out_size;
> 49 + }
> 50 spin_unlock_irqrestore(&port->lock, flags);
> 51
> 52 dbg("%s - returns %d", __func__, room);
>
>
> 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()
Stefani
Johan Hovold
> that I submitted, it should be in Greg's tree. The relevant part goes
> like this:
> diff --git a/drivers/usb/serial/generic.c b/drivers/usb/serial/generic.c
> index f1ea3a3..3372faa 100644
> --- a/drivers/usb/serial/generic.c
> +++ b/drivers/usb/serial/generic.c
> @@ -386,12 +386,15 @@ int usb_serial_generic_chars_in_buffer(struct tty_struct *tty)
>
> dbg("%s - port %d", __func__, port->number);
>
> + spin_lock_irqsave(&port->lock, flags);
> if (serial->type->max_in_flight_urbs) {
> - spin_lock_irqsave(&port->lock, flags);
> chars = port->tx_bytes_flight;
> - spin_unlock_irqrestore(&port->lock, flags);
> - } else if (serial->num_bulk_out)
> - chars = kfifo_len(&port->write_fifo);
> + } else if (serial->num_bulk_out) {
> + /* This overcounts badly, but is good enough for drain wait. */
> + chars = __kfifo_len(port->write_fifo);
> + chars += port->write_urb_busy * port->bulk_out_size;
> + }
> + spin_unlock_irqrestore(&port->lock, flags);
>
> dbg("%s - returns %d", __func__, chars);
> return chars;
That is indeed what you submitted on Dec 7, but this is what is in
Greg's tree:
38 --- a/drivers/usb/serial/generic.c
39 +++ b/drivers/usb/serial/generic.c
40 @@ -369,8 +369,11 @@ int usb_serial_generic_write_room(struct
41 room = port->bulk_out_size *
42 (serial->type->max_in_flight_urbs -
43 port->urbs_in_flight);
44 - } else if (serial->num_bulk_out)
45 + } else if (serial->num_bulk_out) {
46 + /* This overcounts badly, but is good enough for drain wait. */
47 room = kfifo_avail(&port->write_fifo);
48 + room += port->write_urb_busy * port->bulk_out_size;
49 + }
50 spin_unlock_irqrestore(&port->lock, flags);
51
52 dbg("%s - returns %d", __func__, room);
Which does not make any sense at all. Bad merge? What do you say Greg?
/Johan
Johan Hovold
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).
/Johan
Stefani Seibold
> > >
> > > http://git.kernel.org/?p=linux/kernel/git/gregkh/patches.git;a=blob;f=usb/usb-serial-mct_usb232-add-drain-on-close.patch;h=c464f1a82e93df0dd41762a8cb33b0b22e90cdd7;hb=1ea72e7c40b239c6b6f88a4993196be66fc3d892
> > >
> > > 38 --- a/drivers/usb/serial/generic.c
> > > 39 +++ b/drivers/usb/serial/generic.c
> > > 40 @@ -369,8 +369,11 @@ int usb_serial_generic_write_room(struct
> > > 41 room = port->bulk_out_size *
> > > 42 (serial->type->max_in_flight_urbs -
> > > 43 port->urbs_in_flight);
> > > 44 - } else if (serial->num_bulk_out)
> > > 45 + } else if (serial->num_bulk_out) {
> > > 46 + /* This overcounts badly, but is good enough for drain wait. */
> > > 47 room = kfifo_avail(&port->write_fifo);
> > > 48 + room += port->write_urb_busy * port->bulk_out_size;
> > > 49 + }
> > > 50 spin_unlock_irqrestore(&port->lock, flags);
> > > 51
> > > 52 dbg("%s - returns %d", __func__, room);
> > >
> > >
> > > 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:
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);
dbg("%s - returns %d", __func__, chars);
return chars;
@@ -507,7 +507,7 @@ void usb_serial_generic_write_bulk_callb
if (status) {
dbg("%s - nonzero multi-urb write bulk status "
"received: %d", __func__, status);
- kfifo_reset(port->write_fifo);
+ kfifo_reset_out(&port->write_fifo);
} else
usb_serial_generic_write_start(port);
}
diff -u -N -r -p old/drivers/usb/serial/usb-serial.c new/drivers/usb/serial/usb-serial.c
--- old/drivers/usb/serial/usb-serial.c 2009-12-23 08:54:23.204476351 +0100
+++ new/drivers/usb/serial/usb-serial.c 2009-12-23 09:06:39.664475312 +0100
@@ -595,8 +595,7 @@ static void port_release(struct device *
usb_free_urb(port->write_urb);
usb_free_urb(port->interrupt_in_urb);
usb_free_urb(port->interrupt_out_urb);
- if (!IS_ERR(port->write_fifo) && port->write_fifo)
- kfifo_free(port->write_fifo);
+ kfifo_free(&port->write_fifo);
kfree(port->bulk_in_buffer);
kfree(port->bulk_out_buffer);
kfree(port->interrupt_in_buffer);
@@ -939,7 +938,7 @@ int usb_serial_probe(struct usb_interfac
dev_err(&interface->dev, "No free urbs available\n");
goto probe_error;
}
- if (kfifo_alloc(port->write_fifo, PAGE_SIZE, GFP_KERNEL))
+ if (kfifo_alloc(&port->write_fifo, PAGE_SIZE, GFP_KERNEL))
goto probe_error;
buffer_size = le16_to_cpu(endpoint->wMaxPacketSize);
port->bulk_out_size = buffer_size;
diff -u -N -r -p old/include/linux/usb/serial.h new/include/linux/usb/serial.h
--- old/include/linux/usb/serial.h 2009-12-23 08:54:34.368476110 +0100
+++ new/include/linux/usb/serial.h 2009-12-23 09:06:32.870725683 +0100
@@ -16,6 +16,7 @@
#include <linux/kref.h>
#include <linux/mutex.h>
#include <linux/sysrq.h>
+#include <linux/kfifo.h>
#define SERIAL_TTY_MAJOR 188 /* Nice legal number now */
#define SERIAL_TTY_MINORS 254 /* loads of devices :) */
@@ -94,7 +95,7 @@ struct usb_serial_port {
unsigned char *bulk_out_buffer;
int bulk_out_size;
struct urb *write_urb;
- struct kfifo *write_fifo;
+ struct kfifo write_fifo;
int write_urb_busy;
__u8 bulk_out_endpointAddress;
As you can see i didn't changed noting about this drain thing.
Stefani
Johan Hovold
> > >
> > > 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:
Here's the change. The fifo used to be protected by a lock, but is no
longer.
Never say you did.
/Johan
Stefani Seibold
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
Johan Hovold
Yes, please do.
Stefani Seibold
the new kfifo API.
Please apply it to the 2.6.33-rc* tree.
Signed-off-by: Stefani Seibold <ste...@seibold.net>
---
generic.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
--- 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;
Stefani Seibold
cause a race in kfifo_out_locked.
Please apply it to the 2.6.33-rc* tree.
Signed-off-by: Stefani Seibold <ste...@seibold.net>
---
kfifo.h | 7 -------
1 file changed, 7 deletions(-)
--- linux-2.6.33-rc2.orig/include/linux/kfifo.h 2009-12-27
23:37:04.921185257 +0100
+++ kfifo.h 2010-01-05 14:32:31.414316321 +0100
@@ -228,13 +228,6 @@ static inline __must_check unsigned int
ret = kfifo_out(fifo, to, n);
- /*
- * optimization: if the FIFO is empty, set the indices to 0
- * so we don't wrap the next time
- */
- if (kfifo_is_empty(fifo))
- kfifo_reset(fifo);
-
spin_unlock_irqrestore(lock, flags);
return ret;
Greg KH
> This patch fix a possible race bug in drivers/usb/serial/generic with
> the new kfifo API.
>
> Please apply it to the 2.6.33-rc* tree.
>
> Signed-off-by: Stefani Seibold <ste...@seibold.net>
I will queue it up, thanks,
greg k-h
Pete Zaitcev
Stefani Seibold <ste...@seibold.net> wrote:
> > 39 +++ b/drivers/usb/serial/generic.c
> > 40 @@ -369,8 +369,11 @@ int usb_serial_generic_write_room(struct
> > 41 room = port->bulk_out_size *
> > 42 (serial->type->max_in_flight_urbs -
> > 43 port->urbs_in_flight);
> > 44 - } else if (serial->num_bulk_out)
> > 45 + } else if (serial->num_bulk_out) {
> > 46 + /* This overcounts badly, but is good enough for drain wait. */
> > 47 room = kfifo_avail(&port->write_fifo);
> > 48 + room += port->write_urb_busy * port->bulk_out_size;
> > 49 + }
> > 50 spin_unlock_irqrestore(&port->lock, flags);
> > 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()
Looks like a bad merge indeed. The fix that accounts for the write
urb has to apply to chars_in_buffer, so that we can wait until until
it goes to zero.
Please feel free to fix up the kfifo API, I'll pick up the pieces
regarding the lost bytes on close after you're done.
-- Pete
Andrew Morton
Stefani Seibold <ste...@seibold.net> wrote:
> This patch fix a wrong optimization in include/linux/kfifo.h which could
> cause a race in kfifo_out_locked.
>
> Please apply it to the 2.6.33-rc* tree.
>
> Signed-off-by: Stefani Seibold <ste...@seibold.net>
> ---
> kfifo.h | 7 -------
> 1 file changed, 7 deletions(-)
>
> --- linux-2.6.33-rc2.orig/include/linux/kfifo.h 2009-12-27
> 23:37:04.921185257 +0100
> +++ kfifo.h 2010-01-05 14:32:31.414316321 +0100
> @@ -228,13 +228,6 @@ static inline __must_check unsigned int
>
> ret = kfifo_out(fifo, to, n);
>
> - /*
> - * optimization: if the FIFO is empty, set the indices to 0
> - * so we don't wrap the next time
> - */
> - if (kfifo_is_empty(fifo))
> - kfifo_reset(fifo);
> -
> spin_unlock_irqrestore(lock, flags);
>
> return ret;
>
Thanks. Some nits:
- the patch is wordwrapped. I fixed that up.
- you removed the cc's of the particpants in the earlier dicsussion.
I restored them here.
- the changelog didn't describe the bug - what was the race??
- I added "Reported-by: Johan Hovold <jho...@gmail.com>" to the changelog
- the "[tip:urgent]" tag in the Subject: line is something which
Ingo's patch tools add to outgoing email notifications and wasn't
appropriate here. Plain old "[patch]" would be typical.