visor: Always do generic_startup

PATCH Applied Roger Luethi 4 participants 15 messages Nov 16, 2004 – Nov 30, 2004

Subsystems (2)

USB USB serial

lore

Share & tools

lore

Roger Luethi

author

2004-11-16 15:50 UTC #0 lore raw

PATCH 1/1 visor: Always do generic_startup

generic_startup in visor.c was not called for some hardware, resulting in attempts to access memory that had never been allocated, which in turn caused the problem several people reported with recent (2.6.10ish) kernels.

Signed-off-by Roger Luethi <[email protected]>

drivers/usb/serial/visor.c renamed from drivers/usb/serial/visor.c.orig +2 -1

@@ -930,7 +930,7 @@ static int treo_attach (struct usb_seria
 	if (!((serial->dev->descriptor.idVendor == HANDSPRING_VENDOR_ID) ||
 	      (serial->dev->descriptor.idVendor == KYOCERA_VENDOR_ID)) ||
 	    (serial->num_interrupt_in == 0))
-		return 0;
+		goto generic_startup;
 
 	dbg("%s", __FUNCTION__);

@@ -957,6 +957,7 @@ static int treo_attach (struct usb_seria
 	COPY_PORT(serial->port[1], swap_port);
 	kfree(swap_port);
 
+generic_startup:
 	return generic_startup(serial);
 }

Greg KH

Applied

2004-11-19 17:49 UTC #1 lore raw

Quoted 6 lines · Roger Luethi

generic_startup in visor.c was not called for some hardware, resulting in attempts to access memory that had never been allocated, which in turn caused the problem several people reported with recent (2.6.10ish) kernels.
Signed-off-by Roger Luethi <[email protected]>

Thanks for finding this.

Applied.

greg k-h

Simon Fowler

2004-11-21 01:24 UTC #2 lore raw

Quoted 6 lines · Greg KH

Quoted 5 lines · Roger Luethi
generic_startup in visor.c was not called for some hardware, resulting in attempts to access memory that had never been allocated, which in turn caused the problem several people reported with recent (2.6.10ish) kernels.

Quoted 6 lines

Quoted 1 line
Signed-off-by Roger Luethi <[email protected]>
Thanks for finding this.
Applied.

This patch fixes the oops, but after applying it I can no longer sync my palm 5 - it starts, but part way through the connection is lost.

I can sync perfectly with 2.9.10-rc1.

Simon

Signature · 4 lines

PGP public key Id 0x144A991C, or http://himi.org/stuff/himi.asc
(crappy) Homepage: http://himi.org
doe #237 (see http://www.lemuria.org/DeCSS) 
My DeCSS mirror: ftp://himi.org/pub/mirrors/css/

Greg KH

2004-11-21 04:09 UTC #3 lore raw

Quoted 9 lines · Simon Fowler

Quoted 8 lines · Greg KH
Quoted 6 lines · Roger Luethi
generic_startup in visor.c was not called for some hardware, resulting in attempts to access memory that had never been allocated, which in turn caused the problem several people reported with recent (2.6.10ish) kernels.
Signed-off-by Roger Luethi <[email protected]>

Quoted 7 lines

Quoted 4 lines
Thanks for finding this.
Applied.
This patch fixes the oops, but after applying it I can no longer sync my palm 5 - it starts, but part way through the connection is lost.

Can you enable debugging in the visor driver (either through the modprobe paramater, or through the /sys/module/paramater/debug file, and send it to us?

thanks,

greg k-h

Simon Fowler

2004-11-21 07:15 UTC #4 lore raw

Quoted 14 lines · Greg KH

Quoted 13 lines · Simon Fowler
Quoted 12 lines · Greg KH
Quoted 6 lines · Roger Luethi
generic_startup in visor.c was not called for some hardware, resulting in attempts to access memory that had never been allocated, which in turn caused the problem several people reported with recent (2.6.10ish) kernels.
Signed-off-by Roger Luethi <[email protected]>
Thanks for finding this.
Applied.

Quoted 8 lines

Quoted 3 lines
This patch fixes the oops, but after applying it I can no longer sync my palm 5 - it starts, but part way through the connection is lost.
Can you enable debugging in the visor driver (either through the modprobe paramater, or through the /sys/module/paramater/debug file, and send it to us?

I've attached the gzipped log file, from the point it the new USB device is registered to the point the device nodes are deleted by udev.

Simon

Signature · 4 lines

PGP public key Id 0x144A991C, or http://himi.org/stuff/himi.asc
(crappy) Homepage: http://himi.org
doe #237 (see http://www.lemuria.org/DeCSS) 
My DeCSS mirror: ftp://himi.org/pub/mirrors/css/

Greg KH

2004-11-22 21:30 UTC #5 lore raw

Quoted 3 lines · Simon Fowler

I've attached the gzipped log file, from the point it the new USB device is registered to the point the device nodes are deleted by udev.

I don't see anything unusual in the log, sorry.

If you replace the version in your kernel with the visor.c version in 2.6.9, does it work properly for you?

thanks,

greg k-h

Roger Luethi

author

2004-11-23 19:40 UTC #6 lore raw

PATCH 1/1 visor: Don't count outstanding URBs twice

In reply to Greg KH

Guys, can you please CC me when discussing patches of mine? I don't read LKML religiously, and my procmail filters are pretty dumb. Thanks. So my previous patch fixed the oops, but the driver's still borked.

Incrementing the outstanding_urbs counter twice for the same URB can't be good. No wonder Simon didn't get far syncing his Palm.

Signed-off-by Roger Luethi <[email protected]>

drivers/usb/serial/visor.c renamed from drivers/usb/serial/visor.c.orig +0 -1

@@ -497,7 +497,6 @@ static int visor_write (struct usb_seria
 		dev_dbg(&port->dev, "write limit hit\n");
 		return 0;
 	}
-	++priv->outstanding_urbs;
 	spin_unlock_irqrestore(&priv->lock, flags);
 
 	buffer = kmalloc (count, GFP_ATOMIC);

Greg KH

2004-11-23 19:52 UTC #7 lore raw

Re: [2.6 PATCH] visor: Don't count outstanding URBs twice

Quoted 1 line · Roger Luethi

Guys, can you please CC me when discussing patches of mine?

Your email client is putting headers in the messages that say not to do this. Please fix your client :)

Quoted 12 lines

I don't read
LKML religiously, and my procmail filters are pretty dumb. Thanks. So my previous patch fixed the oops, but the driver's still borked.
Incrementing the outstanding_urbs counter twice for the same URB can't be good. No wonder Simon didn't get far syncing his Palm.
Signed-off-by Roger Luethi <[email protected]>

drivers/usb/serial/visor.c static int visor_write (struct usb_seria · line 502

Quoted 7 lines

drivers/usb/serial/visor.c renamed from drivers/usb/serial/visor.c.orig +0 -1

@@ -497,7 +497,6 @@ static int visor_write (struct usb_seria
 		dev_dbg(&port->dev, "write limit hit\n");
 		return 0;
 	}
-	++priv->outstanding_urbs;
 	spin_unlock_irqrestore(&priv->lock, flags);
 
 	buffer = kmalloc (count, GFP_ATOMIC);

Good catch.

But I'm not seeing people actually hit the write limit, according to the logs that people are posting.

Can anyone test this patch to see if it fixes their issues?

thanks,

greg k-h

Roger Luethi

author

2004-11-23 20:34 UTC #8 lore raw

Re: [2.6 PATCH] visor: Don't count outstanding URBs twice

Quoted 2 lines · Greg KH

Your email client is putting headers in the messages that say not to do this. Please fix your client :)

D'oh! Fixed (I think).

Quoted 2 lines

But I'm not seeing people actually hit the write limit, according to the logs that people are posting.

What I found is bound to cause exactly the kind of problem Simon described, so I didn't check any further. _But_ comparing his log and the code, I can't help but notice that the missing "write limit hit" is the only instance of a dev_dbg in this driver. Coincidence?

Roger

Simon Fowler

2004-11-24 00:46 UTC #9 lore raw

Re: [2.6 PATCH] visor: Don't count outstanding URBs twice

Quoted 9 lines · Roger Luethi

Quoted 2 lines · Greg KH
Your email client is putting headers in the messages that say not to do this. Please fix your client :)
D'oh! Fixed (I think).
Quoted 2 lines
But I'm not seeing people actually hit the write limit, according to the logs that people are posting.

Quoted 7 lines

What I found is bound to cause exactly the kind of problem Simon described, so I didn't check any further. _But_ comparing his log and the code, I can't help but notice that the missing "write limit hit" is the only instance of a dev_dbg in this driver. Coincidence?
Roger

Your extra patch has fixed the problem - I was able to do a full sync with it.

Greg: your suggestion of backing visor.c out to an earlier working version made all sorts of badness happen - I can get you the logs if you want, but I assume since Roger's patches fix the problem you don't care about it any more . . .

Thanks for the prompt fix!

Simon

Signature · 4 lines

PGP public key Id 0x144A991C, or http://himi.org/stuff/himi.asc
(crappy) Homepage: http://himi.org
doe #237 (see http://www.lemuria.org/DeCSS) 
My DeCSS mirror: ftp://himi.org/pub/mirrors/css/

Greg KH

Applied

2004-11-24 23:52 UTC #10 lore raw

Re: [2.6 PATCH] visor: Don't count outstanding URBs twice

In reply to Roger Luethi

Quoted 8 lines · Roger Luethi

Guys, can you please CC me when discussing patches of mine? I don't read LKML religiously, and my procmail filters are pretty dumb. Thanks. So my previous patch fixed the oops, but the driver's still borked.
Incrementing the outstanding_urbs counter twice for the same URB can't be good. No wonder Simon didn't get far syncing his Palm.
Signed-off-by Roger Luethi <[email protected]>

Applied, thanks.

greg k-h

Greg KH

2004-11-27 02:11 UTC #12 lore raw

Re: [2.6 PATCH] visor: Make URB limit error more visible

In reply to Roger Luethi

Quoted 7 lines · Roger Luethi

There is only one call to dev_dbg in all of visor.c, the rest is dbg or dev_err. It already bit us once when warnings didn't turn up in a debug log. I would argue that a flood of those warnings will warrant report and inspection anyway (broken app, broken driver, or lame DoS attempt), so I replaced the dev_dbg with dev_err.
Signed-off-by Roger Luethi <[email protected]>

Thanks, but I already fixed this up in my trees yesterday.

greg k-h

Roger Luethi

author

2004-11-27 06:26 UTC #11 lore raw

PATCH 1/1 visor: Make URB limit error more visible

In reply to Greg KH

There is only one call to dev_dbg in all of visor.c, the rest is dbg or dev_err. It already bit us once when warnings didn't turn up in a debug log. I would argue that a flood of those warnings will warrant report and inspection anyway (broken app, broken driver, or lame DoS attempt), so I replaced the dev_dbg with dev_err.

Signed-off-by Roger Luethi <[email protected]>

drivers/usb/serial/visor.c renamed from drivers/usb/serial/visor.c.orig +1 -1

@@ -494,7 +494,7 @@ static int visor_write (struct usb_seria
 	spin_lock_irqsave(&priv->lock, flags);
 	if (priv->outstanding_urbs > URB_UPPER_LIMIT) {
 		spin_unlock_irqrestore(&priv->lock, flags);
-		dev_dbg(&port->dev, "write limit hit\n");
+		dev_err(&port->dev, "write limit hit\n");
 		return 0;
 	}
 	spin_unlock_irqrestore(&priv->lock, flags);

Alan Cox

2004-11-28 18:14 UTC #13 lore raw

Re: [2.6 PATCH] visor: Make URB limit error more visible

Quoted 7 lines · Roger Luethi

There is only one call to dev_dbg in all of visor.c, the rest is dbg or dev_err. It already bit us once when warnings didn't turn up in a debug log. I would argue that a flood of those warnings will warrant report and inspection anyway (broken app, broken driver, or lame DoS attempt), so I replaced the dev_dbg with dev_err.
Signed-off-by Roger Luethi <[email protected]>

Since it is trivially user caused should it not be rate limited or it becomes a DoS of its own to the syslog

Greg KH

2004-11-30 00:39 UTC #14 lore raw

Re: [2.6 PATCH] visor: Make URB limit error more visible

Quoted 11 lines · Alan Cox

Quoted 7 lines · Roger Luethi
There is only one call to dev_dbg in all of visor.c, the rest is dbg or dev_err. It already bit us once when warnings didn't turn up in a debug log. I would argue that a flood of those warnings will warrant report and inspection anyway (broken app, broken driver, or lame DoS attempt), so I replaced the dev_dbg with dev_err.
Signed-off-by Roger Luethi <[email protected]>
Since it is trivially user caused should it not be rate limited or it becomes a DoS of its own to the syslog

Agreed, that's why the change I commited doesn't do it this way :)

thanks,

greg k-h