linux-next: manual merge of the ttydev tree with the usb.current tree

linux-next: manual merge of the ttydev tree with the usb.current tree

[Stephen Rothwell]

Hi Alan,

Today's linux-next merge of the ttydev tree got conflicts in
drivers/usb/serial/generic.c and include/linux/usb/serial.h between commit
d8298d029cdd5d944c3d8369d2862de94d12ae4b ("USB: serial: regression fix to
move sysrq from hot path") from the usb.current tree and commit
cf8456fe1aaed1776bf4c26c18adef76e472b9fa ("tty-usb-fix-perf-regression")
from the ttydev tree.  Also between commit
a2170a444ef9a89a7ec9acc72d79e5db433effc4 ("USB: serial: optimize sysrq
function calls") from the usb.current tree and commit
bfce6b042a47c9cdd55c7cd231e58710ec1fa4e1 ("tty-usb-fix-kref-leak") from
the ttydev tree.

The first two commits above seem to be trying to do the same thing (or
similar) so I have used the ttydev version for now.

I fixed them up (see below) and can carry the fixes as necessary.

-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au

diff --cc drivers/usb/serial/generic.c
index 48f6743,21dd6e7..0000000
--- a/drivers/usb/serial/generic.c
+++ b/drivers/usb/serial/generic.c
diff --cc include/linux/usb/serial.h
index 4601e0d,0ec50ba..0000000
--- a/include/linux/usb/serial.h
+++ b/include/linux/usb/serial.h
@@@ -325,34 -330,6 +325,35 @@@ extern struct usb_serial_driver usb_ser
  extern struct bus_type usb_serial_bus_type;
  extern struct tty_driver *usb_serial_tty_driver;
  
- static inline int usb_serial_handle_sysrq_char(struct usb_serial_port *port,
++static inline int usb_serial_handle_sysrq_char(struct tty_struct *tty,
++					       struct usb_serial_port *port,
 +					       unsigned int ch)
 +{
 +#ifdef CONFIG_MAGIC_SYSRQ
 +	if (port->sysrq && port->console) {
 +		if (ch && time_before(jiffies, port->sysrq)) {
- 			handle_sysrq(ch, tty_port_tty_get(&port->port));
++			handle_sysrq(ch, tty);
 +			port->sysrq = 0;
 +			return 1;
 +		}
 +		port->sysrq = 0;
 +	}
 +#endif /* CONFIG_MAGIC_SYSRQ */
 +	return 0;
 +}
 +
 +static inline int usb_serial_handle_break(struct usb_serial_port *port)
 +{
 +#ifdef CONFIG_MAGIC_SYSRQ
 +	if (!port->sysrq) {
 +		port->sysrq = jiffies + HZ*5;
 +		return 1;
 +	}
 +	port->sysrq = 0;
 +#endif /* CONFIG_MAGIC_SYSRQ */
 +	return 0;
 +}
 +
  static inline void usb_serial_debug_data(int debug,
  					 struct device *dev,
  					 const char *function, int size,

Re: linux-next: manual merge of the ttydev tree with the usb.current tree

[Greg KH]

On Thu, Jul 09, 2009 at 01:04:30PM +1000, Stephen Rothwell wrote:
> Hi Alan,
> 
> Today's linux-next merge of the ttydev tree got conflicts in
> drivers/usb/serial/generic.c and include/linux/usb/serial.h between commit
> d8298d029cdd5d944c3d8369d2862de94d12ae4b ("USB: serial: regression fix to
> move sysrq from hot path") from the usb.current tree and commit
> cf8456fe1aaed1776bf4c26c18adef76e472b9fa ("tty-usb-fix-perf-regression")
> from the ttydev tree.  Also between commit
> a2170a444ef9a89a7ec9acc72d79e5db433effc4 ("USB: serial: optimize sysrq
> function calls") from the usb.current tree and commit
> bfce6b042a47c9cdd55c7cd231e58710ec1fa4e1 ("tty-usb-fix-kref-leak") from
> the ttydev tree.
> 
> The first two commits above seem to be trying to do the same thing (or
> similar) so I have used the ttydev version for now.
> 
> I fixed them up (see below) and can carry the fixes as necessary.

Thanks for the fix, I don't know what happened in the tty tree, I've not
touched the USB tree for a week or so now...

thanks,

greg k-h

Re: linux-next: manual merge of the ttydev tree with the usb.current tree

[Stephen Rothwell]

[-- Attachment #1: Type: text/plain, Size: 1349 bytes --]

Hi Greg,

On Thu, 9 Jul 2009 16:19:56 -0700 Greg KH <greg@kroah.com> wrote:
>
> On Thu, Jul 09, 2009 at 01:04:30PM +1000, Stephen Rothwell wrote:
> > 
> > Today's linux-next merge of the ttydev tree got conflicts in
> > drivers/usb/serial/generic.c and include/linux/usb/serial.h between commit
> > d8298d029cdd5d944c3d8369d2862de94d12ae4b ("USB: serial: regression fix to
> > move sysrq from hot path") from the usb.current tree and commit
> > cf8456fe1aaed1776bf4c26c18adef76e472b9fa ("tty-usb-fix-perf-regression")
> > from the ttydev tree.  Also between commit
> > a2170a444ef9a89a7ec9acc72d79e5db433effc4 ("USB: serial: optimize sysrq
> > function calls") from the usb.current tree and commit
> > bfce6b042a47c9cdd55c7cd231e58710ec1fa4e1 ("tty-usb-fix-kref-leak") from
> > the ttydev tree.
> > 
> > The first two commits above seem to be trying to do the same thing (or
> > similar) so I have used the ttydev version for now.
> > 
> > I fixed them up (see below) and can carry the fixes as necessary.
> 
> Thanks for the fix, I don't know what happened in the tty tree, I've not
> touched the USB tree for a week or so now...

In case you missed it, Alan has submitted the above two ttydev patches to
Linus.

-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au
http://www.canb.auug.org.au/~sfr/

[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]

Re: linux-next: manual merge of the ttydev tree with the usb.current tree

[Greg KH]

On Fri, Jul 10, 2009 at 10:06:25AM +1000, Stephen Rothwell wrote:
> Hi Greg,
> 
> On Thu, 9 Jul 2009 16:19:56 -0700 Greg KH <greg@kroah.com> wrote:
> >
> > On Thu, Jul 09, 2009 at 01:04:30PM +1000, Stephen Rothwell wrote:
> > > 
> > > Today's linux-next merge of the ttydev tree got conflicts in
> > > drivers/usb/serial/generic.c and include/linux/usb/serial.h between commit
> > > d8298d029cdd5d944c3d8369d2862de94d12ae4b ("USB: serial: regression fix to
> > > move sysrq from hot path") from the usb.current tree and commit
> > > cf8456fe1aaed1776bf4c26c18adef76e472b9fa ("tty-usb-fix-perf-regression")
> > > from the ttydev tree.  Also between commit
> > > a2170a444ef9a89a7ec9acc72d79e5db433effc4 ("USB: serial: optimize sysrq
> > > function calls") from the usb.current tree and commit
> > > bfce6b042a47c9cdd55c7cd231e58710ec1fa4e1 ("tty-usb-fix-kref-leak") from
> > > the ttydev tree.
> > > 
> > > The first two commits above seem to be trying to do the same thing (or
> > > similar) so I have used the ttydev version for now.
> > > 
> > > I fixed them up (see below) and can carry the fixes as necessary.
> > 
> > Thanks for the fix, I don't know what happened in the tty tree, I've not
> > touched the USB tree for a week or so now...
> 
> In case you missed it, Alan has submitted the above two ttydev patches to
> Linus.

Ah, thanks, I'll handle the merge when they hit Linus's tree.

greg k-h

linux-next: manual merge of the ttydev tree with the usb.current tree

[Stephen Rothwell]

Hi Alan,

Today's linux-next merge of the ttydev tree got a conflict in
drivers/usb/serial/usb-serial.c between commit
a00b8d98aca97bfb6fa983f41dae25b424058592 ("USB: fix usage count in usb
serial generic open regarding autoresume") from the usb.current tree and
commit 73fed8403623653560cc2f35c9b62891adaf6b4b
("tty-usb-error-unlock-fix") from the ttydev tree.

Just context changes.  I have fixed it up (see below) and can carry the
fix as necessary.
-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au

diff --cc drivers/usb/serial/usb-serial.c
index 06579a8,99188c9..0000000
--- a/drivers/usb/serial/usb-serial.c
+++ b/drivers/usb/serial/usb-serial.c
@@@ -220,8 -224,8 +222,8 @@@ static int serial_open (struct tty_stru
  	tty_port_tty_set(&port->port, tty);
  
  	/* If the console is attached, the device is already open */
 -	if (port->port.count == 1 && !port->console) {
 +	if (!port->port.count && !port->console) {
- 
+ 		first = 1;
  		/* lock this module before we call it
  		 * this may fail, which means we must bail out,
  		 * safe because we are called with BKL held */
@@@ -246,8 -248,7 +248,9 @@@
  		if (retval)
  			goto bailout_interface_put;
  		mutex_unlock(&serial->disc_mutex);
+ 		set_bit(ASYNCB_INITIALIZED, &port->port.flags);
 +	} else {
 +		++port->port.count;
  	}
  	mutex_unlock(&port->mutex);
  	/* Now do the correct tty layer semantics */

Re: linux-next: manual merge of the ttydev tree with the usb.current tree

[Alan Cox]

On Tue, 28 Jul 2009 14:01:10 +1000
Stephen Rothwell <sfr@canb.auug.org.au> wrote:

> Hi Alan,
> 
> Today's linux-next merge of the ttydev tree got a conflict in
> drivers/usb/serial/usb-serial.c between commit
> a00b8d98aca97bfb6fa983f41dae25b424058592 ("USB: fix usage count in usb
> serial generic open regarding autoresume") from the usb.current tree

That looks wrong. The extra fiddling with port.count looks both unsafe
and incorrect. I've never seen the "autoresume" patch referred to so I've
no idea what it is trying to do

Greg ?

Re: linux-next: manual merge of the ttydev tree with the usb.current tree

[Oliver Neukum]

Am Dienstag, 28. Juli 2009 12:26:08 schrieb Alan Cox:
> On Tue, 28 Jul 2009 14:01:10 +1000
>
> Stephen Rothwell <sfr@canb.auug.org.au> wrote:
> > Hi Alan,
> >
> > Today's linux-next merge of the ttydev tree got a conflict in
> > drivers/usb/serial/usb-serial.c between commit
> > a00b8d98aca97bfb6fa983f41dae25b424058592 ("USB: fix usage count in usb
> > serial generic open regarding autoresume") from the usb.current tree	
>
> That looks wrong. The extra fiddling with port.count looks both unsafe
> and incorrect. I've never seen the "autoresume" patch referred to so I've
> no idea what it is trying to do

The problem is subdrivers looking at port->count to determine
whether URBs have to be resubmitted. That must happen once.
Therefore if serial_open() needs to do it itself it must make sure
resume() runs without port->count already incremented.

As serial_open() takes the lock, resume() must not do so.
If however, remote wakeup is used a lock must be taken
to look at port->count reliably. As this is impossible, the patch
implementing use of remote wakeup for the option driver
implements a private flag for opened ports.

This code from option_resume():

for (i = 0; i < serial->num_ports; i++) {
		/* walk all ports */
		port = serial->port[i];
		portdata = usb_get_serial_port_data(port);
		mutex_lock(&port->mutex);

		/* skip closed ports */
		if (!port->port.count) {
			mutex_unlock(&port->mutex);
			continue;
		}

		for (j = 0; j < N_IN_URB; j++) {
			urb = portdata->in_urbs[j];
			err = usb_submit_urb(urb, GFP_NOIO);
			if (err < 0) {
				mutex_unlock(&port->mutex);
				err("%s: Error %d for bulk URB %d",
					 __func__, err, i);
				return err;
			}
		}
		mutex_unlock(&port->mutex);

must not take the port->mutex.

Unfortunately I have no access to my trees today and yesterday.

	Regards
		Oliver

Re: linux-next: manual merge of the ttydev tree with the usb.current tree

[Alan Cox]

> As serial_open() takes the lock, resume() must not do so.
> If however, remote wakeup is used a lock must be taken
> to look at port->count reliably. As this is impossible, the patch
> implementing use of remote wakeup for the option driver
> implements a private flag for opened ports.

I need to look at the actual diff, but the combination looks completely
bogus unless I'm misreading the fixup which is possible.

The way the core serial code and most other drivers do this is to keep
track of the port ASYNC_INITIALIZED flag, which indicates (in the non USB
cases but trivially fixable for the USB cases) that the port hardware is
initialized. We now set it on an open but we don't clear it on
serial_do_down() finishing.

Re: linux-next: manual merge of the ttydev tree with the usb.current tree

[Stephen Rothwell]

Hi Alan,

On Tue, 28 Jul 2009 13:29:11 +0100 Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:
>
> I need to look at the actual diff, but the combination looks completely
> bogus unless I'm misreading the fixup which is possible.

Below is the actual patch from the usb.current tree.
-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au

>From oliver@neukum.org  Sat Jul 18 15:45:44 2009
From: Oliver Neukum <oliver@neukum.org>
Date: Sat, 18 Jul 2009 15:47:17 +0200
Subject: USB: fix usage count in usb serial generic open regarding autoresume
To: Matthew Garrett <mjg59@srcf.ucam.org>, Greg KH <greg@kroah.com>, Alan Stern <stern@rowland.harvard.edu>, linux-usb@vger.kernel.org
Message-ID: <200907181547.17813.oliver@neukum.org>
Content-Disposition: inline


The resume handler has to resubmit the reading URBs if the device is
resumed while opened. But the open() method bumps the counter before
it resumes a device. The fix is to increment the counter only after
the resumption or if no resumption is attempted.

Signed-off-by: Oliver Neukum <oliver@neukum.org>
Tested-by: Matthew Garrett <mjg@redhat.com>
Cc: stable <stable@kernel.org>
Signed-off-by: Greg Kroah-Hartman <gregkh@suse.de>


---
 drivers/usb/serial/usb-serial.c |    8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

--- a/drivers/usb/serial/usb-serial.c
+++ b/drivers/usb/serial/usb-serial.c
@@ -214,15 +214,13 @@ static int serial_open (struct tty_struc
 		goto bailout_port_put;
 	}
 
-	++port->port.count;
-
 	/* set up our port structure making the tty driver
 	 * remember our port object, and us it */
 	tty->driver_data = port;
 	tty_port_tty_set(&port->port, tty);
 
 	/* If the console is attached, the device is already open */
-	if (port->port.count == 1 && !port->console) {
+	if (!port->port.count && !port->console) {
 
 		/* lock this module before we call it
 		 * this may fail, which means we must bail out,
@@ -240,12 +238,16 @@ static int serial_open (struct tty_struc
 		if (retval)
 			goto bailout_module_put;
 
+		++port->port.count;
+
 		/* only call the device specific open if this
 		 * is the first time the port is opened */
 		retval = serial->type->open(tty, port, filp);
 		if (retval)
 			goto bailout_interface_put;
 		mutex_unlock(&serial->disc_mutex);
+	} else {
+		++port->port.count;
 	}
 	mutex_unlock(&port->mutex);
 	/* Now do the correct tty layer semantics */

Re: linux-next: manual merge of the ttydev tree with the usb.current tree

[Alan Cox]

On Tue, 28 Jul 2009 22:35:04 +1000
Stephen Rothwell <sfr@canb.auug.org.au> wrote:

> Hi Alan,
> 
> On Tue, 28 Jul 2009 13:29:11 +0100 Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:
> >
> > I need to look at the actual diff, but the combination looks completely
> > bogus unless I'm misreading the fixup which is possible.
> 
> Below is the actual patch from the usb.current tree.

Thanks - ok that is probably safe. The change I was worried about (the
error paths not adjusting port->count are ok as it gets zeroed within the
mutex)

Not sure its safe versus hangup but neither was the old code 8)

Oliver: I'll send you an alternative patch later today/tomorrow that uses
the ASYNC flags.

Alan

Re: linux-next: manual merge of the ttydev tree with the usb.current tree

[Greg KH]

On Tue, Jul 28, 2009 at 02:17:40PM +0100, Alan Cox wrote:
> On Tue, 28 Jul 2009 22:35:04 +1000
> Stephen Rothwell <sfr@canb.auug.org.au> wrote:
> 
> > Hi Alan,
> > 
> > On Tue, 28 Jul 2009 13:29:11 +0100 Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:
> > >
> > > I need to look at the actual diff, but the combination looks completely
> > > bogus unless I'm misreading the fixup which is possible.
> > 
> > Below is the actual patch from the usb.current tree.
> 
> Thanks - ok that is probably safe. The change I was worried about (the
> error paths not adjusting port->count are ok as it gets zeroed within the
> mutex)
> 
> Not sure its safe versus hangup but neither was the old code 8)
> 
> Oliver: I'll send you an alternative patch later today/tomorrow that uses
> the ASYNC flags.

Should I just drop Oliver's existing patch from my tree for now then?

thanks,

greg k-h

Re: linux-next: manual merge of the ttydev tree with the usb.current tree

[Greg KH]

On Tue, Jul 28, 2009 at 09:32:22AM -0700, Greg KH wrote:
> On Tue, Jul 28, 2009 at 02:17:40PM +0100, Alan Cox wrote:
> > On Tue, 28 Jul 2009 22:35:04 +1000
> > Stephen Rothwell <sfr@canb.auug.org.au> wrote:
> > 
> > > Hi Alan,
> > > 
> > > On Tue, 28 Jul 2009 13:29:11 +0100 Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:
> > > >
> > > > I need to look at the actual diff, but the combination looks completely
> > > > bogus unless I'm misreading the fixup which is possible.
> > > 
> > > Below is the actual patch from the usb.current tree.
> > 
> > Thanks - ok that is probably safe. The change I was worried about (the
> > error paths not adjusting port->count are ok as it gets zeroed within the
> > mutex)
> > 
> > Not sure its safe versus hangup but neither was the old code 8)
> > 
> > Oliver: I'll send you an alternative patch later today/tomorrow that uses
> > the ASYNC flags.
> 
> Should I just drop Oliver's existing patch from my tree for now then?

Ok, in order to get a good merge for today, I've dropped it now.

Oliver, care to work with Alan to get something that is agreeable to
everyone?

thanks,

greg k-h

Re: linux-next: manual merge of the ttydev tree with the usb.current tree

[Oliver Neukum]

Am Dienstag, 28. Juli 2009 18:34:20 schrieb Greg KH:
> > > Oliver: I'll send you an alternative patch later today/tomorrow that
> > > uses the ASYNC flags.
> >
> > Should I just drop Oliver's existing patch from my tree for now then?
>
> Ok, in order to get a good merge for today, I've dropped it now.
>
> Oliver, care to work with Alan to get something that is agreeable to
> everyone?

Sure. Alan, is there a driver you can recommend for looking at to learn how
to correctly use the tty layer?

	Regards
		Oliver

Re: linux-next: manual merge of the ttydev tree with the usb.current tree

[Alan Cox]

On Tue, 28 Jul 2009 21:21:32 +0200
Oliver Neukum <oliver@neukum.org> wrote:

> Am Dienstag, 28. Juli 2009 18:34:20 schrieb Greg KH:
> > > > Oliver: I'll send you an alternative patch later today/tomorrow that
> > > > uses the ASYNC flags.
> > >
> > > Should I just drop Oliver's existing patch from my tree for now then?
> >
> > Ok, in order to get a good merge for today, I've dropped it now.
> >
> > Oliver, care to work with Alan to get something that is agreeable to
> > everyone?
> 
> Sure. Alan, is there a driver you can recommend for looking at to learn how
> to correctly use the tty layer?

I'm no longer tty maintainer, please take it up with Linus.

Alan