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]>
--- 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);
}
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 <[email protected]>
Thanks for finding this.
Applied.
greg k-h
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 <[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
--
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/
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 <[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.
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
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 <[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.
>
> 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/
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
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]>
--- 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);
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 <[email protected]>
>
> --- 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
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
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/
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 <[email protected]>
Applied, thanks.
greg k-h
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 <[email protected]>
Thanks, but I already fixed this up in my trees yesterday.
greg k-h
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]>
--- 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);
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 <[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
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 <[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