[PATCH][2.6.0-test7] digi_acceleport.c has bogus "address of" operator

[PATCH][2.6.0-test7] digi_acceleport.c has bogus "address of" operator

[Kurtis D. Rader]

http://bugme.osdl.org/show_bug.cgi?id=1365

The digi_acceleport.c USB serial driver has a bogus "address of" operator
that results in panics like this:

kernel BUG at include/asm/spinlock.h:120!
invalid operand: 0000 [#1]
Call Trace:
 [<f88e9000>] digi_wakeup_write_lock+0x0/0xaa [digi_acceleport]
 [<c0236842>] console_callback+0xa0/0xc2
 [<c0136ebc>] worker_thread+0x228/0x3ce
 [<f88e9000>] digi_wakeup_write_lock+0x0/0xaa [digi_acceleport]
 [<c011e8b6>] default_wake_function+0x0/0x2e
 [<c010993e>] ret_from_fork+0x6/0x14
 [<c011e8b6>] default_wake_function+0x0/0x2e
 [<c0136c94>] worker_thread+0x0/0x3ce
 [<c010740d>] kernel_thread_helper+0x5/0xc

The problem is that digi_wakeup_write_lock() takes a pointer to a struct
usb_serial_port. However, what gets passed is a pointer to a pointer to a
struct usb_serial_port.

=== diff -rup drivers/usb/serial/digi_acceleport.c.orig drivers/usb/serial/digi_acceleport.c
--- drivers/usb/serial/digi_acceleport.c.orig   2003-10-15 22:03:26.000000000 -0700
+++ drivers/usb/serial/digi_acceleport.c        2003-10-15 21:10:21.000000000 -0700
@@ -1728,8 +1728,7 @@ dbg( "digi_startup: TOP" );
                init_waitqueue_head( &priv->dp_flush_wait );
                priv->dp_in_close = 0;
                init_waitqueue_head( &priv->dp_close_wait );
-               INIT_WORK(&priv->dp_wakeup_work, (void *)digi_wakeup_write_lock,
-                               (void *)(&serial->port[i]));
+               INIT_WORK(&priv->dp_wakeup_work, digi_wakeup_write_lock, serial->port[i]);
 
                /* initialize write wait queue for this port */
                init_waitqueue_head( &serial->port[i]->write_wait );

-- 
Kurtis D. Rader, Systems Support Engineer    service: 800-IBM-SERV
IBM Integrated Technology Services           email: kdrader@us.ibm.com
15300 SW Koll Pkwy, MS RHE2-O2               DID: +1 503-578-3714
Beaverton, OR 97006-6063                     http://www.ibm.com

Re: [PATCH][2.6.0-test7] digi_acceleport.c has bogus "address of" operator

[Greg KH]

On Wed, Oct 15, 2003 at 10:48:21PM -0700, Kurtis D. Rader wrote:
> 
> === diff -rup drivers/usb/serial/digi_acceleport.c.orig drivers/usb/serial/digi_acceleport.c
> --- drivers/usb/serial/digi_acceleport.c.orig   2003-10-15 22:03:26.000000000 -0700
> +++ drivers/usb/serial/digi_acceleport.c        2003-10-15 21:10:21.000000000 -0700

This patch does not apply.  It looks like the tabs are converted to
spaces.  Can you send it to me again, so that I can apply it?

thanks,

greg k-h

Re: [PATCH][2.6.0-test7] digi_acceleport.c has bogus "address of" operator

[Andrew Morton]

Greg KH <greg@kroah.com> wrote:
>
> On Wed, Oct 15, 2003 at 10:48:21PM -0700, Kurtis D. Rader wrote:
> > 
> > === diff -rup drivers/usb/serial/digi_acceleport.c.orig drivers/usb/serial/digi_acceleport.c
> > --- drivers/usb/serial/digi_acceleport.c.orig   2003-10-15 22:03:26.000000000 -0700
> > +++ drivers/usb/serial/digi_acceleport.c        2003-10-15 21:10:21.000000000 -0700
> 
> This patch does not apply.  It looks like the tabs are converted to
> spaces.  Can you send it to me again, so that I can apply it?

It was applied.  But it added a compile-time warning, which this
fixes:


diff -puN drivers/usb/serial/digi_acceleport.c~digi_acceleport-warning-fix drivers/usb/serial/digi_acceleport.c
--- 25/drivers/usb/serial/digi_acceleport.c~digi_acceleport-warning-fix	2003-10-23 03:21:03.000000000 -0700
+++ 25-akpm/drivers/usb/serial/digi_acceleport.c	2003-10-23 03:21:40.000000000 -0700
@@ -444,7 +444,7 @@ struct digi_port {
 /* Local Function Declarations */
 
 static void digi_wakeup_write( struct usb_serial_port *port );
-static void digi_wakeup_write_lock( struct usb_serial_port *port );
+static void digi_wakeup_write_lock(void *);
 static int digi_write_oob_command( struct usb_serial_port *port,
 	unsigned char *buf, int count, int interruptible );
 static int digi_write_inb_command( struct usb_serial_port *port,
@@ -608,9 +608,9 @@ static inline long cond_wait_interruptib
 *  on writes.
 */
 
-static void digi_wakeup_write_lock( struct usb_serial_port *port )
+static void digi_wakeup_write_lock(void *arg)
 {
-
+	struct usb_serial_port *port = arg;
 	unsigned long flags;
 	struct digi_port *priv = usb_get_serial_port_data(port);
 

_

Re: [PATCH][2.6.0-test7] digi_acceleport.c has bogus "address of" operator

[Kurtis D. Rader]

On Thu, 2003-10-23 17:44:33, Andrew Morton wrote:
> Greg KH <greg@kroah.com> wrote:
> >
> > On Wed, Oct 15, 2003 at 10:48:21PM -0700, Kurtis D. Rader wrote:
> > > 
> > > === diff -rup drivers/usb/serial/digi_acceleport.c.orig drivers/usb/serial/digi_acceleport.c
> > > --- drivers/usb/serial/digi_acceleport.c.orig   2003-10-15 22:03:26.000000000 -0700
> > > +++ drivers/usb/serial/digi_acceleport.c        2003-10-15 21:10:21.000000000 -0700
> > 
> > This patch does not apply.  It looks like the tabs are converted to
> > spaces.  Can you send it to me again, so that I can apply it?
> 
> It was applied.  But it added a compile-time warning, which this
> fixes:

My apologies. Even though I knew, in the back of my mind, it was the
wrong thing to do I still "cut-and-pasted" the output of the diff :-(
I won't make that mistake again.

Also, note that even with the aforementioned change applied the driver
is still broken and results in oopses. It's now failing (see the oops
included below) a random interval after the device is opened. As soon as
I resolve my current customer crit-sit involving the O(1) scheduler I'll
get to the root cause of that problem, as well as any others that rear
their ugly heads, and submit a proper patch against the digi_acceleport
driver that makes it work in a v2.6 kernel.

The following trace is included for the benefit of the documented driver
maintainers in the hope they have an idea as to the proper fix. The
tainting is due to use of VMware, the use of which is unlikely to have
anything to do with the digi_accelport driver problems (probability =~ 0.0).

Unable to handle kernel NULL pointer dereference at virtual address 00000050
 printing eip:
f88ee3f0
*pde = 31ce8067
*pte = 00000000
Oops: 0000 [#1]
CPU:    0
EIP:    0060:[<f88ee3f0>]    Tainted: PF 
EFLAGS: 00010002
EIP is at digi_read_oob_callback+0x2a4/0x339 [digi_acceleport]
eax: 00000001   ebx: f544adf8   ecx: 00000000   edx: 00000000
esi: f5444df8   edi: 00000004   ebp: c047fe70   esp: c047fe38
ds: 007b   es: 007b   ss: 0068
Process swapper (pid: 0, threadinfo=c047e000 task=c03f4ea0)
Stack: 00000000 00000046 3ccba773 f2077dc4 f56c23c0 f621fbf8 c047e000 c047e000 
       00000040 00000008 f545821c f5447df8 f5458b68 f5441df8 c047fe98 f88edda1 
       f5458b68 f5f2f860 f7f0aef8 00000082 f5458b68 f5f2f864 f5458b68 c047ff8c 
Call Trace:
 [<f88edda1>] digi_read_bulk_callback+0xdd/0x158 [digi_acceleport]
 [<f8a86965>] usb_hcd_giveback_urb+0x27/0x3c [usbcore]
 [<f88cdf35>] dl_done_list+0x1cf/0x22e [ohci_hcd]
 [<f88cef4f>] ohci_irq+0x1d3/0x2da [ohci_hcd]
 [<f8a869af>] usb_hcd_irq+0x35/0x5c [usbcore]
 [<c010bd7d>] handle_IRQ_event+0x39/0x62
 [<c010c207>] do_IRQ+0xf7/0x23e
 [<c010717a>] default_idle+0x0/0x32
 [<c010a3d4>] common_interrupt+0x18/0x20
 [<c010717a>] default_idle+0x0/0x32
 [<c01071a7>] default_idle+0x2d/0x32
 [<c0107226>] cpu_idle+0x3a/0x44
 [<c0105000>] rest_init+0x0/0x94
 [<c0480870>] start_kernel+0x1a4/0x1e6
 [<c0480428>] unknown_bootoption+0x0/0xf6

-- 
Kurtis D. Rader, Level 3 Linux Support and Customer Service Tool Developer
Beaverton Service Center/Linux Change Team
T/L 775-3714, DID +1 503-578-3714