USB: serial: kfifo_len locking

USB: serial: kfifo_len locking

[Johan Hovold]

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?

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

Re: USB: serial: kfifo_len locking

[Stefani Seibold]

Am Montag, den 04.01.2010, 18:43 +0100 schrieb Johan Hovold:
> 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

Re: USB: serial: kfifo_len locking

[Pete Zaitcev]

On Mon, 04 Jan 2010 20:20:04 +0100
Stefani Seibold <stefani@seibold.net> wrote:
> Am Montag, den 04.01.2010, 18:43 +0100 schrieb 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?
> > 
> 
> 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.

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;

Re: USB: serial: kfifo_len locking

[Stefani Seibold]

Am Dienstag, den 05.01.2010, 00:43 -0700 schrieb Pete Zaitcev:
> On Mon, 04 Jan 2010 20:20:04 +0100
> Stefani Seibold <stefani@seibold.net> wrote:
> > Am Montag, den 04.01.2010, 18:43 +0100 schrieb 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?
> > > 
> > 
> > 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.
> 
> 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;

This is the same patch as my. But __kfifo_len is renamed into kfifo_len.
Who should submit this patch? 

Stefani

Re: USB: serial: kfifo_len locking

[Johan Hovold]

> 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;

That is indeed what you submitted on Dec 7, but this is what is in
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?

/Johan

Re: USB: serial: kfifo_len locking

[Stefani Seibold]

Am Dienstag, den 05.01.2010, 12:04 +0100 schrieb Johan Hovold:
> > 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;
> 
> That is indeed what you submitted on Dec 7, but this is what is in
> 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

Re: USB: serial: kfifo_len locking

[Johan Hovold]

On Tue, Jan 05, 2010 at 12:09:19PM +0100, Stefani Seibold wrote:
> Am Dienstag, den 05.01.2010, 12:04 +0100 schrieb Johan Hovold:
> > > 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;
> > 
> > That is indeed what you submitted on Dec 7, but this is what is in
> > 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()

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

Re: USB: serial: kfifo_len locking

[Stefani Seibold]

Am Dienstag, den 05.01.2010, 12:14 +0100 schrieb Johan Hovold:

> > > 
> > > 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

Re: USB: serial: kfifo_len locking

[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.

>  
>  	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.

Never say you did.

/Johan

Re: USB: serial: kfifo_len locking

[Stefani Seibold]

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

Re: USB: serial: kfifo_len locking

[Johan Hovold]

> > 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.

Yes, please do.

[tip:urgent] fix USB serial fix kfifo_len locking

[Stefani Seibold]

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 <stefani@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;

[tip:urgent] fix kfifo_out_locked race bug

[Stefani Seibold]

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 <stefani@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;

Re: [tip:urgent] fix USB serial fix kfifo_len locking

[Greg KH]

On Tue, Jan 05, 2010 at 02:30:31PM +0100, Stefani Seibold wrote:
> 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 <stefani@seibold.net>

I will queue it up, thanks,

greg k-h

Re: USB: serial: kfifo_len locking

[Pete Zaitcev]

On Tue, 05 Jan 2010 12:09:19 +0100
Stefani Seibold <stefani@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

Re: [tip:urgent] fix kfifo_out_locked race bug

[Andrew Morton]

On Tue, 05 Jan 2010 14:38:35 +0100
Stefani Seibold <stefani@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 <stefani@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 <jhovold@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.