[2.6 PATCH] visor: Always do generic_startup

[2.6 PATCH] visor: Always do generic_startup

[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 <rl@hellgate.ch>

--- linux-2.6.10-rc2/drivers/usb/serial/visor.c.orig	2004-11-16 16:03:05.000000000 +0100
+++ linux-2.6.10-rc2/drivers/usb/serial/visor.c	2004-11-16 16:31:24.235249944 +0100
@@ -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);
 }
 

Re: [2.6 PATCH] visor: Always do generic_startup

[Greg KH]

On Tue, Nov 16, 2004 at 04:49:43PM +0100, Roger Luethi wrote:
> 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 <rl@hellgate.ch>

Thanks for finding this.

Applied.

greg k-h

Re: [2.6 PATCH] visor: Always do generic_startup

[Simon Fowler]

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

On Fri, Nov 19, 2004 at 09:44:05AM -0800, Greg KH wrote:
> On Tue, Nov 16, 2004 at 04:49:43PM +0100, Roger Luethi wrote:
> > 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 <rl@hellgate.ch>
> 
> 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

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

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

Re: [2.6 PATCH] visor: Always do generic_startup

[Greg KH]

On Sun, Nov 21, 2004 at 12:23:53PM +1100, Simon Fowler wrote:
> On Fri, Nov 19, 2004 at 09:44:05AM -0800, Greg KH wrote:
> > On Tue, Nov 16, 2004 at 04:49:43PM +0100, Roger Luethi wrote:
> > > 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 <rl@hellgate.ch>
> > 
> > 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

Re: [2.6 PATCH] visor: Always do generic_startup

[Simon Fowler]

[-- Attachment #1.1: Type: text/plain, Size: 1325 bytes --]

On Sat, Nov 20, 2004 at 08:08:56PM -0800, Greg KH wrote:
> On Sun, Nov 21, 2004 at 12:23:53PM +1100, Simon Fowler wrote:
> > On Fri, Nov 19, 2004 at 09:44:05AM -0800, Greg KH wrote:
> > > On Tue, Nov 16, 2004 at 04:49:43PM +0100, Roger Luethi wrote:
> > > > 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 <rl@hellgate.ch>
> > > 
> > > 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?
> 
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

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

[-- Attachment #1.2: visor-debug.gz --]
[-- Type: application/octet-stream, Size: 2425 bytes --]

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

Re: [2.6 PATCH] visor: Always do generic_startup

[Greg KH]

On Sun, Nov 21, 2004 at 06:15:30PM +1100, Simon Fowler wrote:
> 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

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

[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 <rl@hellgate.ch>

--- linux-2.6.10-rc2-bk8/drivers/usb/serial/visor.c.orig	2004-11-23 20:23:27.592097112 +0100
+++ linux-2.6.10-rc2-bk8/drivers/usb/serial/visor.c	2004-11-23 20:24:53.496037728 +0100
@@ -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);

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

[Greg KH]

On Tue, Nov 23, 2004 at 08:36:04PM +0100, Roger Luethi wrote:
> 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 :)

> 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 <rl@hellgate.ch>
> 
> --- linux-2.6.10-rc2-bk8/drivers/usb/serial/visor.c.orig	2004-11-23 20:23:27.592097112 +0100
> +++ linux-2.6.10-rc2-bk8/drivers/usb/serial/visor.c	2004-11-23 20:24:53.496037728 +0100
> @@ -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

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

[Roger Luethi]

On Tue, 23 Nov 2004 11:45:58 -0800, Greg KH wrote:
> Your email client is putting headers in the messages that say not to do
> this.  Please fix your client :)

D'oh! Fixed (I think).

> 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

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

[Simon Fowler]

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

On Tue, Nov 23, 2004 at 09:30:23PM +0100, Roger Luethi wrote:
> On Tue, 23 Nov 2004 11:45:58 -0800, Greg KH wrote:
> > Your email client is putting headers in the messages that say not to do
> > this.  Please fix your client :)
> 
> D'oh! Fixed (I think).
> 
> > 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
> 
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

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

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

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

[Greg KH]

On Tue, Nov 23, 2004 at 08:36:04PM +0100, Roger Luethi wrote:
> 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 <rl@hellgate.ch>

Applied, thanks.

greg k-h

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

[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 <rl@hellgate.ch>

--- linux-2.6.10-rc2-bk8/drivers/usb/serial/visor.c.orig	2004-11-25 17:03:18.056410624 +0100
+++ linux-2.6.10-rc2-bk8/drivers/usb/serial/visor.c	2004-11-25 17:05:04.113287528 +0100
@@ -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);

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

[Greg KH]

On Thu, Nov 25, 2004 at 05:16:19PM +0100, Roger Luethi wrote:
> 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 <rl@hellgate.ch>

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

greg k-h

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

[Alan Cox]

On Iau, 2004-11-25 at 16:16, Roger Luethi wrote:
> 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 <rl@hellgate.ch>

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

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

[Greg KH]

On Sun, Nov 28, 2004 at 05:11:25PM +0000, Alan Cox wrote:
> On Iau, 2004-11-25 at 16:16, Roger Luethi wrote:
> > 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 <rl@hellgate.ch>
> 
> 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