Oops w/sysfs when closing a disconnected usb serial device

Oops w/sysfs when closing a disconnected usb serial device

[Mike Gorse]

With 2.6.0-test11, I get a panic if I disconnect a USB serial device with
a fd open on it and then close the fd.  When the device is disconnected,
usb_disconnect calls usb_disable_device, which calls device_del, which
calls kobject_del, which removes the device's sysfs directory.  If a user
space program has the tts device open, then kobject_cleanup and
destroy_serial do not get called until the device is closed, but by then
the kobject_del call to the interface has caused the tty device's sysfs
directory to be nuked from under it.  Eventually sysfs_remove_dir is
called and eventually calls simple_rmdir with a dentry with a NULL
d_inode, causing an oops.  I can make the Oops go away with the following
patch:

--- fs/sysfs/dir.c.orig	2003-11-30 18:59:34.395284712 -0500
+++ fs/sysfs/dir.c	2003-11-30 18:59:50.944768808 -0500
@@ -83,7 +83,7 @@
 	struct dentry * parent = dget(d->d_parent);
 	down(&parent->d_inode->i_sem);
 	d_delete(d);
-	simple_rmdir(parent->d_inode,d);
+	if (d->d_inode) simple_rmdir(parent->d_inode,d);
 
 	pr_debug(" o %s removing done (%d)\n",d->d_name.name,
 		 atomic_read(&d->d_count));

-- Michael Gorse / AIM:linvortex / http://mgorse.home.dhs.org --

Re: Oops w/sysfs when closing a disconnected usb serial device

[Maneesh Soni]

On Mon, Dec 01, 2003 at 12:21:14AM +0000, Mike Gorse wrote:
> With 2.6.0-test11, I get a panic if I disconnect a USB serial device with
> a fd open on it and then close the fd.  When the device is disconnected,
> usb_disconnect calls usb_disable_device, which calls device_del, which
> calls kobject_del, which removes the device's sysfs directory.  If a user
> space program has the tts device open, then kobject_cleanup and
> destroy_serial do not get called until the device is closed, but by then
> the kobject_del call to the interface has caused the tty device's sysfs
> directory to be nuked from under it.  Eventually sysfs_remove_dir is
> called and eventually calls simple_rmdir with a dentry with a NULL
> d_inode, causing an oops.  I can make the Oops go away with the following
> patch:
> 
> --- fs/sysfs/dir.c.orig	2003-11-30 18:59:34.395284712 -0500
> +++ fs/sysfs/dir.c	2003-11-30 18:59:50.944768808 -0500
> @@ -83,7 +83,7 @@
>  	struct dentry * parent = dget(d->d_parent);
>  	down(&parent->d_inode->i_sem);
>  	d_delete(d);
> -	simple_rmdir(parent->d_inode,d);
> +	if (d->d_inode) simple_rmdir(parent->d_inode,d);
>  
>  	pr_debug(" o %s removing done (%d)\n",d->d_name.name,
>  		 atomic_read(&d->d_count));
> 

Hi Mike,

IMO d->d_inode is not expected to be NULL at this point. The only
place it can become NULL is in d_delete(d) call, but as the dentry ref.
count will be atleast 2, even this will not make d_inode NULL and it should
only unhash the dentry. Probably it will become more clear if you post
the oops message.

Mean while, I think kobject_del should not remove corresponding sysfs directory
until all the other references to kobject has gone. There can be references
taken in sysfs_open_file() from user space. The following patch moves the  
sysfs_remove_dir() call, to kobject_cleanup() and I think it may solve your 
problem also. It will be nice if you can test it.

Pat, what do you think about the below patch?

Thanks
Maneesh


o kobject_del should not remove the corresponding sysfs directory untill all
  other references to kobject are gone for example references taken from 
  userspace programs doing sysfs_open_file(). Moving sysfs_remove_dir to 
  kobject_cleanup.


 lib/kobject.c |    2 +-
 1 files changed, 1 insertion(+), 1 deletion(-)

diff -puN lib/kobject.c~kobject_del-fix lib/kobject.c
--- linux-2.6.0-test11/lib/kobject.c~kobject_del-fix	2003-12-01 11:02:23.000000000 +0530
+++ linux-2.6.0-test11-maneesh/lib/kobject.c	2003-12-01 11:03:51.000000000 +0530
@@ -410,7 +410,6 @@ void kobject_del(struct kobject * kobj)
 	if (top_kobj->kset && top_kobj->kset->hotplug_ops)
 		kset_hotplug("remove", top_kobj->kset, kobj);
 
-	sysfs_remove_dir(kobj);
 	unlink(kobj);
 }
 
@@ -454,6 +453,7 @@ void kobject_cleanup(struct kobject * ko
 	struct kset * s = kobj->kset;
 
 	pr_debug("kobject %s: cleaning up\n",kobject_name(kobj));
+	sysfs_remove_dir(kobj);
 	if (kobj->k_name != kobj->name)
 		kfree(kobj->k_name);
 	kobj->k_name = NULL;

_

-- 
Maneesh Soni
Linux Technology Center, 
IBM Software Lab, Bangalore, India
email: maneesh@in.ibm.com
Phone: 91-80-5044999 Fax: 91-80-5268553
T/L : 9243696

Re: Oops w/sysfs when closing a disconnected usb serial device

[Mike Gorse]

Hi Maneesh,

On Mon, 1 Dec 2003, Maneesh Soni wrote:

> IMO d->d_inode is not expected to be NULL at this point. The only
> place it can become NULL is in d_delete(d) call, but as the dentry ref.
> count will be atleast 2, even this will not make d_inode NULL and it should
> only unhash the dentry. Probably it will become more clear if you post
> the oops message.
> 
It is trying to delete a directory which is gone already.  I'll post the 
oops below.

> Mean while, I think kobject_del should not remove corresponding sysfs directory
> until all the other references to kobject has gone. There can be references
> taken in sysfs_open_file() from user space. The following patch moves the  
> sysfs_remove_dir() call, to kobject_cleanup() and I think it may solve your 
> problem also. It will be nice if you can test it.
> 
I wish your patch solved things in itself, but, without the added sysfs 
check, I now get a new oops when disconnecting the device, even if no 
applications are using it.

-- original oops (new oops below this one)--
Unable to handle kernel NULL pointer dereference at virtual address 00000024
 printing eip:
c0175f95
*pde = 00000000
Oops: 0002 [#1]
CPU:    0
EIP:    0060:[<c0175f95>]    Not tainted
EFLAGS: 00010202
EIP is at simple_rmdir+0x35/0x50
eax: 00000000   ebx: cbcae620   ecx: cbcae628   edx: ffffffd9
esi: cbc865e0   edi: cfe56000   ebp: cbcae63c   esp: cfe57e30
ds: 007b   es: 007b   ss: 0068
Process gpsd (pid: 6012, threadinfo=cfe56000 task=d1766100)
Stack: cbcae620 cbc50320 cbcc6320 cbcae620 c018f1ec cbc865e0 cbcae620 cbcae580 
       cbcae620 c018f2dd cbcae620 cbcae580 cfe56000 d0fcaaa0 d0fcaecc d24b49a0 
       00000000 c01edfe3 d0fcaaa0 c03ac8a0 d0fcaaa0 d0fcaa78 c02323c0 d0fcaaa0 
Call Trace:
 [<c018f1ec>] remove_dir+0x4c/0x70
 [<c018f2dd>] sysfs_remove_dir+0xbd/0x130
 [<c01edfe3>] kobject_del+0x43/0x80
 [<c02323c0>] device_del+0x70/0xa0
 [<c0232403>] device_unregister+0x13/0x30
 [<d4977531>] destroy_serial+0x1a1/0x1e0 [usbserial]
 [<d4976c5e>] serial_set_termios+0xbe/0x110 [usbserial]
 [<c01ee125>] kobject_cleanup+0x85/0x90
 [<d49764b0>] serial_close+0x90/0xf0 [usbserial]
 [<c021e7b9>] release_dev+0x709/0x760
 [<c0223a95>] set_termios+0xd5/0x1a0
 [<c021ebda>] tty_release+0x2a/0x70
 [<c015714a>] __fput+0x10a/0x120
 [<c0155769>] filp_close+0x59/0x90
 [<c0155802>] sys_close+0x62/0xa0
 [<c010b4db>] syscall_call+0x7/0xb

Code: ff 48 24 89 5c 24 04 89 34 24 e8 9c ff ff ff ff 4e 24 31 d2 
 
--new oops--
hub 1-0:1.0: port 1, status 100, change 3, 12 Mb/s
usb 1-1: USB disconnect, address 2
usb 1-1: usb_disable_device nuking all URBs
usb 1-1: unregistering interface 1-1:1.0
drivers/usb/serial/usb-serial.c: usb_serial_disconnect
drivers/usb/serial/usb-serial.c: destroy_serial - 
drivers/usb/serial/usb-serial.c: serial_shutdown
drivers/usb/serial/ftdi_sio.c: ftdi_shutdown
drivers/usb/serial/usb-serial.c: return_serial
sysfs ttyUSB0: removing dir
 o dev (1): <7>removing<7> done
 o ttyUSB0 removing done (1)
FTDI 8U232AM Compatible ttyUSB0: FTDI 8U232AM Compatible converter now disconnected from ttyUSB0
 o power removing done (1)
sysfs ttyUSB0: removing dir
 o power (1): <7>removing<7> done
 o detach_state (1): <7>removing<7> done
 o ttyUSB0 removing done (1)
drivers/usb/serial/usb-serial.c: port_release - ttyUSB0
usb 1-1: hcd_unlink_urb d3d33f60 fail -22
usbserial 1-1:1.0: device disconnected
 o power removing done (1)
drivers/usb/core/usb.c: usb_hotplug
usb 1-1: unregistering device
 o power removing done (1)
drivers/usb/core/usb.c: usb_hotplug
sysfs 1-1: removing dir
 o 1-1:1.0 (10): <7>removing<7> done
 o product (1): <7>removing<7> done
 o manufacturer (1): <7>removing<7> done
 o speed (1): <7>removing<7> done
 o bNumConfigurations (1): <7>removing<7> done
 o bDeviceProtocol (1): <7>removing<7> done
 o bDeviceSubClass (1): <7>removing<7> done
 o bDeviceClass (1): <7>removing<7> done
 o bcdDevice (1): <7>removing<7> done
 o idProduct (1): <7>removing<7> done
 o idVendor (1): <7>removing<7> done
 o bMaxPower (1): <7>removing<7> done
 o bmAttributes (1): <7>removing<7> done
 o bConfigurationValue (1): <7>removing<7> done
 o bNumInterfaces (1): <7>removing<7> done
 o power (1): <7>removing<7> done
 o detach_state (1): <7>removing<7> done
 o 1-1 removing done (2)
sysfs 1-1:1.0: removing dir
 o iInterface (1): <7>removing<7> done
 o bInterfaceProtocol (1): <7>removing<7> done
 o bInterfaceSubClass (1): <7>removing<7> done
 o bInterfaceClass (1): <7>removing<7> done
 o bNumEndpoints (1): <7>removing<7> done
 o bAlternateSetting (1): <7>removing<7> done
 o bInterfaceNumber (1): <7>removing<7> done
 o power (1): <7>removing<7> done
 o detach_state (1): <7>removing<7> done
Unable to handle kernel NULL pointer dereference at virtual address 00000024
 printing eip:
c0174b03
*pde = 00000000
Oops: 0002 [#1]
CPU:    0
EIP:    0060:[<c0174b03>]    Not tainted
EFLAGS: 00010202
EIP is at simple_rmdir+0x33/0x50
eax: 00000000   ebx: d3d0d5a0   ecx: 00000001   edx: ffffffd9
esi: d3d49780   edi: d3df0000   ebp: d3df1e68   esp: d3df1e58
ds: 007b   es: 007b   ss: 0068
Process khubd (pid: 5, threadinfo=d3df0000 task=c133a040)
Stack: d3d0d5a0 d3d0aa00 d3d4b6c0 d3d0d5a0 d3df1e84 c018d69c d3d49780 d3d0d5a0 
       d3d0d500 d3d0d500 d3d0d5a0 d3df1eac c018d7e4 d3d0d5a0 d3d0d500 00000001 
       d3d0d5bc d3df0000 d3d60d1c c03c5b30 c03c5b60 d3df1ec4 c01ea9cd d3d60d1c 
Call Trace:
 [<c018d69c>] remove_dir+0x4c/0x90
 [<c018d7e4>] sysfs_remove_dir+0xf4/0x170
 [<c01ea9cd>] kobject_cleanup+0x2d/0x80
 [<c0280983>] usb_destroy_configuration+0xc3/0x110
 [<c0278c02>] usb_release_dev+0x32/0x60
 [<c022e771>] device_release+0x21/0x80
 [<c01eaa1c>] kobject_cleanup+0x7c/0x80
 [<c027be3f>] hub_port_connect_change+0x38f/0x3a0
 [<c027c28f>] hub_events+0x43f/0x4d0
 [<c027c355>] hub_thread+0x35/0x110
 [<c011d940>] default_wake_function+0x0/0x20
 [<c027c320>] hub_thread+0x0/0x110
 [<c01092d9>] kernel_thread_helper+0x5/0xc

Code: ff 48 24 89 5c 24 04 89 34 24 e8 9e ff ff ff ff 4e 24 31 d2 

Re: Oops w/sysfs when closing a disconnected usb serial device

[Maneesh Soni]

Hi Mike,

I think now I understand the problem better. It looks to me a case of
parent kobject going away before child. 

As we have 

> sysfs 1-1: removing dir
>  o 1-1:1.0 (10): <7>removing<7> done

and then
> sysfs 1-1:1.0: removing dir

Its good that you enabled debug printks. We can see error 

> usb 1-1: hcd_unlink_urb d3d33f60 fail -22

I think Greg or Pat can help you better in this case. I have no idea if 
this error is creating this problem.

Regards,
Maneesh

On Mon, Dec 01, 2003 at 06:55:38PM -0500, Mike Gorse wrote:
> Hi Maneesh,
> 
> On Mon, 1 Dec 2003, Maneesh Soni wrote:
> 
> > IMO d->d_inode is not expected to be NULL at this point. The only
> > place it can become NULL is in d_delete(d) call, but as the dentry ref.
> > count will be atleast 2, even this will not make d_inode NULL and it should
> > only unhash the dentry. Probably it will become more clear if you post
> > the oops message.
> > 
> It is trying to delete a directory which is gone already.  I'll post the 
> oops below.
> 
> > Mean while, I think kobject_del should not remove corresponding sysfs directory
> > until all the other references to kobject has gone. There can be references
> > taken in sysfs_open_file() from user space. The following patch moves the  
> > sysfs_remove_dir() call, to kobject_cleanup() and I think it may solve your 
> > problem also. It will be nice if you can test it.
> > 
> I wish your patch solved things in itself, but, without the added sysfs 
> check, I now get a new oops when disconnecting the device, even if no 
> applications are using it.
> 
> -- original oops (new oops below this one)--
> Unable to handle kernel NULL pointer dereference at virtual address 00000024
>  printing eip:
> c0175f95
> *pde = 00000000
> Oops: 0002 [#1]
> CPU:    0
> EIP:    0060:[<c0175f95>]    Not tainted
> EFLAGS: 00010202
> EIP is at simple_rmdir+0x35/0x50
> eax: 00000000   ebx: cbcae620   ecx: cbcae628   edx: ffffffd9
> esi: cbc865e0   edi: cfe56000   ebp: cbcae63c   esp: cfe57e30
> ds: 007b   es: 007b   ss: 0068
> Process gpsd (pid: 6012, threadinfo=cfe56000 task=d1766100)
> Stack: cbcae620 cbc50320 cbcc6320 cbcae620 c018f1ec cbc865e0 cbcae620 cbcae580 
>        cbcae620 c018f2dd cbcae620 cbcae580 cfe56000 d0fcaaa0 d0fcaecc d24b49a0 
>        00000000 c01edfe3 d0fcaaa0 c03ac8a0 d0fcaaa0 d0fcaa78 c02323c0 d0fcaaa0 
> Call Trace:
>  [<c018f1ec>] remove_dir+0x4c/0x70
>  [<c018f2dd>] sysfs_remove_dir+0xbd/0x130
>  [<c01edfe3>] kobject_del+0x43/0x80
>  [<c02323c0>] device_del+0x70/0xa0
>  [<c0232403>] device_unregister+0x13/0x30
>  [<d4977531>] destroy_serial+0x1a1/0x1e0 [usbserial]
>  [<d4976c5e>] serial_set_termios+0xbe/0x110 [usbserial]
>  [<c01ee125>] kobject_cleanup+0x85/0x90
>  [<d49764b0>] serial_close+0x90/0xf0 [usbserial]
>  [<c021e7b9>] release_dev+0x709/0x760
>  [<c0223a95>] set_termios+0xd5/0x1a0
>  [<c021ebda>] tty_release+0x2a/0x70
>  [<c015714a>] __fput+0x10a/0x120
>  [<c0155769>] filp_close+0x59/0x90
>  [<c0155802>] sys_close+0x62/0xa0
>  [<c010b4db>] syscall_call+0x7/0xb
> 
> Code: ff 48 24 89 5c 24 04 89 34 24 e8 9c ff ff ff ff 4e 24 31 d2 
>  
> --new oops--
> hub 1-0:1.0: port 1, status 100, change 3, 12 Mb/s
> usb 1-1: USB disconnect, address 2
> usb 1-1: usb_disable_device nuking all URBs
> usb 1-1: unregistering interface 1-1:1.0
> drivers/usb/serial/usb-serial.c: usb_serial_disconnect
> drivers/usb/serial/usb-serial.c: destroy_serial - 
> drivers/usb/serial/usb-serial.c: serial_shutdown
> drivers/usb/serial/ftdi_sio.c: ftdi_shutdown
> drivers/usb/serial/usb-serial.c: return_serial
> sysfs ttyUSB0: removing dir
>  o dev (1): <7>removing<7> done
>  o ttyUSB0 removing done (1)
> FTDI 8U232AM Compatible ttyUSB0: FTDI 8U232AM Compatible converter now disconnected from ttyUSB0
>  o power removing done (1)
> sysfs ttyUSB0: removing dir
>  o power (1): <7>removing<7> done
>  o detach_state (1): <7>removing<7> done
>  o ttyUSB0 removing done (1)
> drivers/usb/serial/usb-serial.c: port_release - ttyUSB0
> usb 1-1: hcd_unlink_urb d3d33f60 fail -22
> usbserial 1-1:1.0: device disconnected
>  o power removing done (1)
> drivers/usb/core/usb.c: usb_hotplug
> usb 1-1: unregistering device
>  o power removing done (1)
> drivers/usb/core/usb.c: usb_hotplug
> sysfs 1-1: removing dir
>  o 1-1:1.0 (10): <7>removing<7> done
>  o product (1): <7>removing<7> done
>  o manufacturer (1): <7>removing<7> done
>  o speed (1): <7>removing<7> done
>  o bNumConfigurations (1): <7>removing<7> done
>  o bDeviceProtocol (1): <7>removing<7> done
>  o bDeviceSubClass (1): <7>removing<7> done
>  o bDeviceClass (1): <7>removing<7> done
>  o bcdDevice (1): <7>removing<7> done
>  o idProduct (1): <7>removing<7> done
>  o idVendor (1): <7>removing<7> done
>  o bMaxPower (1): <7>removing<7> done
>  o bmAttributes (1): <7>removing<7> done
>  o bConfigurationValue (1): <7>removing<7> done
>  o bNumInterfaces (1): <7>removing<7> done
>  o power (1): <7>removing<7> done
>  o detach_state (1): <7>removing<7> done
>  o 1-1 removing done (2)
> sysfs 1-1:1.0: removing dir
>  o iInterface (1): <7>removing<7> done
>  o bInterfaceProtocol (1): <7>removing<7> done
>  o bInterfaceSubClass (1): <7>removing<7> done
>  o bInterfaceClass (1): <7>removing<7> done
>  o bNumEndpoints (1): <7>removing<7> done
>  o bAlternateSetting (1): <7>removing<7> done
>  o bInterfaceNumber (1): <7>removing<7> done
>  o power (1): <7>removing<7> done
>  o detach_state (1): <7>removing<7> done
> Unable to handle kernel NULL pointer dereference at virtual address 00000024
>  printing eip:
> c0174b03
> *pde = 00000000
> Oops: 0002 [#1]
> CPU:    0
> EIP:    0060:[<c0174b03>]    Not tainted
> EFLAGS: 00010202
> EIP is at simple_rmdir+0x33/0x50
> eax: 00000000   ebx: d3d0d5a0   ecx: 00000001   edx: ffffffd9
> esi: d3d49780   edi: d3df0000   ebp: d3df1e68   esp: d3df1e58
> ds: 007b   es: 007b   ss: 0068
> Process khubd (pid: 5, threadinfo=d3df0000 task=c133a040)
> Stack: d3d0d5a0 d3d0aa00 d3d4b6c0 d3d0d5a0 d3df1e84 c018d69c d3d49780 d3d0d5a0 
>        d3d0d500 d3d0d500 d3d0d5a0 d3df1eac c018d7e4 d3d0d5a0 d3d0d500 00000001 
>        d3d0d5bc d3df0000 d3d60d1c c03c5b30 c03c5b60 d3df1ec4 c01ea9cd d3d60d1c 
> Call Trace:
>  [<c018d69c>] remove_dir+0x4c/0x90
>  [<c018d7e4>] sysfs_remove_dir+0xf4/0x170
>  [<c01ea9cd>] kobject_cleanup+0x2d/0x80
>  [<c0280983>] usb_destroy_configuration+0xc3/0x110
>  [<c0278c02>] usb_release_dev+0x32/0x60
>  [<c022e771>] device_release+0x21/0x80
>  [<c01eaa1c>] kobject_cleanup+0x7c/0x80
>  [<c027be3f>] hub_port_connect_change+0x38f/0x3a0
>  [<c027c28f>] hub_events+0x43f/0x4d0
>  [<c027c355>] hub_thread+0x35/0x110
>  [<c011d940>] default_wake_function+0x0/0x20
>  [<c027c320>] hub_thread+0x0/0x110
>  [<c01092d9>] kernel_thread_helper+0x5/0xc
> 
> Code: ff 48 24 89 5c 24 04 89 34 24 e8 9e ff ff ff ff 4e 24 31 d2 

-- 
Maneesh Soni
Linux Technology Center, 
IBM Software Lab, Bangalore, India
email: maneesh@in.ibm.com
Phone: 91-80-5044999 Fax: 91-80-5268553
T/L : 9243696

Re: Oops w/sysfs when closing a disconnected usb serial device

[Greg KH]

On Tue, Dec 02, 2003 at 12:41:13PM +0530, Maneesh Soni wrote:
> Hi Mike,
> 
> I think now I understand the problem better. It looks to me a case of
> parent kobject going away before child. 

Yes, but I don't see how that is happening in the usb code :(

> As we have 
> 
> > sysfs 1-1: removing dir
> >  o 1-1:1.0 (10): <7>removing<7> done
> 
> and then
> > sysfs 1-1:1.0: removing dir
> 
> Its good that you enabled debug printks. We can see error 
> 
> > usb 1-1: hcd_unlink_urb d3d33f60 fail -22

This isn't an error, it can be ignored.  It just means that the driver
told the usb core to unlink a urb that was already unlinked.  Very
common thing.

> I think Greg or Pat can help you better in this case. I have no idea if 
> this error is creating this problem.

It's not this error, but something else.  The usb-serial code hasn't
changed in a while.  Can you try figuring out what kernel version this
bug shows up in?

thanks,

greg k-h

Re: Oops w/sysfs when closing a disconnected usb serial device

[Greg KH]

On Mon, Dec 01, 2003 at 03:08:04PM +0530, Maneesh Soni wrote:
> On Mon, Dec 01, 2003 at 12:21:14AM +0000, Mike Gorse wrote:
> > With 2.6.0-test11, I get a panic if I disconnect a USB serial device with
> > a fd open on it and then close the fd.  When the device is disconnected,
> > usb_disconnect calls usb_disable_device, which calls device_del, which
> > calls kobject_del, which removes the device's sysfs directory.  If a user
> > space program has the tts device open, then kobject_cleanup and
> > destroy_serial do not get called until the device is closed, but by then
> > the kobject_del call to the interface has caused the tty device's sysfs
> > directory to be nuked from under it.  Eventually sysfs_remove_dir is
> > called and eventually calls simple_rmdir with a dentry with a NULL
> > d_inode, causing an oops.  I can make the Oops go away with the following
> > patch:
> > 
> > --- fs/sysfs/dir.c.orig	2003-11-30 18:59:34.395284712 -0500
> > +++ fs/sysfs/dir.c	2003-11-30 18:59:50.944768808 -0500
> > @@ -83,7 +83,7 @@
> >  	struct dentry * parent = dget(d->d_parent);
> >  	down(&parent->d_inode->i_sem);
> >  	d_delete(d);
> > -	simple_rmdir(parent->d_inode,d);
> > +	if (d->d_inode) simple_rmdir(parent->d_inode,d);
> >  
> >  	pr_debug(" o %s removing done (%d)\n",d->d_name.name,
> >  		 atomic_read(&d->d_count));
> > 
> 
> Hi Mike,
> 
> IMO d->d_inode is not expected to be NULL at this point. The only
> place it can become NULL is in d_delete(d) call, but as the dentry ref.
> count will be atleast 2, even this will not make d_inode NULL and it should
> only unhash the dentry. Probably it will become more clear if you post
> the oops message.
> 
> Mean while, I think kobject_del should not remove corresponding sysfs directory
> until all the other references to kobject has gone. There can be references
> taken in sysfs_open_file() from user space. The following patch moves the  
> sysfs_remove_dir() call, to kobject_cleanup() and I think it may solve your 
> problem also. It will be nice if you can test it.
> 
> Pat, what do you think about the below patch?

I agree with this patch.  It fixes the usbserial oops for me in my
testing.  

But wait, no, this patch is not good...

I think Mike's patch is correct.  Here's the problem (tree simplified
for this example to make it readable):
	- insert usb device, this creates a sysfs directory something
	  like:
		- pci.../usb1/1.0
	- the usbserial driver binds to this device and creates a
	  ttyUSB0 directory:
	  	- pci.../usb1/1.0/ttyUSB0
	- a user opens the ttyUSB0 device node (in /dev) which
	  increments the reference count of the kobject that controls
	  the ttyUSB0 directory in sysfs.
	- the usb device is removed from the system.  In doing this,
	  the driver core, and then the kobject code has to delete the
	  tree 1.0 directory.  
	  
Now at this point, Maneesh, your patch would prevent this directory from
being removed, until after the ttyUSB0 directory is removed.  That's all
well and good, but what happens if we have another USB device plugged
into the same place before this happens.  Then the USB code would try to
create the 1.0 directory over again (the directory names are built off
of the USB topology.  but even if they were built off of something
unique, we would still have a mess...)  If that happens, the creation of
the directory would fail, which is not acceptable.

But Mike's patch allows the whole tree from 1.0 on down to be removed,
and then later, when the ttyUSB0 node is really closed, the directory
will not tried to be removed.  Memory is still cleaned up properly, and
the kobjects are properly reference counted.

I know in the past I've argued against this kind of patch (sorry scsi
people), but now in thinking about it a bunch (and sitting though a
zillion oops messages trying to figure out what's wrong here) I think
this is the correct fix.

Any other opinions?

thanks,

greg k-h

Re: Oops w/sysfs when closing a disconnected usb serial device

[Greg KH]

On Mon, Dec 01, 2003 at 06:55:38PM -0500, Mike Gorse wrote:
> Hi Maneesh,
> 
> On Mon, 1 Dec 2003, Maneesh Soni wrote:
> 
> > IMO d->d_inode is not expected to be NULL at this point. The only
> > place it can become NULL is in d_delete(d) call, but as the dentry ref.
> > count will be atleast 2, even this will not make d_inode NULL and it should
> > only unhash the dentry. Probably it will become more clear if you post
> > the oops message.
> > 
> It is trying to delete a directory which is gone already.  I'll post the 
> oops below.
> 
> > Mean while, I think kobject_del should not remove corresponding sysfs directory
> > until all the other references to kobject has gone. There can be references
> > taken in sysfs_open_file() from user space. The following patch moves the  
> > sysfs_remove_dir() call, to kobject_cleanup() and I think it may solve your 
> > problem also. It will be nice if you can test it.
> > 
> I wish your patch solved things in itself, but, without the added sysfs 
> check, I now get a new oops when disconnecting the device, even if no 
> applications are using it.

Try the patch below.  With your sysfs patch I don't get any oopses
anymore.  I still need to beat on this patch a lot more before I think
it solves all of the current issues.  Can you let me know if it works
for you or not?

thanks,

greg k-h

diff -Nru a/drivers/usb/serial/usb-serial.c b/drivers/usb/serial/usb-serial.c
--- a/drivers/usb/serial/usb-serial.c	Fri Dec  5 17:37:16 2003
+++ b/drivers/usb/serial/usb-serial.c	Fri Dec  5 17:37:16 2003
@@ -493,12 +493,15 @@
 	return retval;
 }
 
-static void __serial_close(struct usb_serial_port *port, struct file *filp)
+static void serial_close(struct tty_struct *tty, struct file * filp)
 {
-	if (!port->open_count) {
-		dbg ("%s - port not opened", __FUNCTION__);
+	struct usb_serial_port *port = (struct usb_serial_port *) tty->driver_data;
+	struct usb_serial *serial = get_usb_serial (port, __FUNCTION__);
+
+	if (!serial)
 		return;
-	}
+
+	dbg("%s - port %d", __FUNCTION__, port->number);
 
 	--port->open_count;
 	if (port->open_count <= 0) {
@@ -506,30 +509,18 @@
 		 * port is being closed by the last owner */
 		port->serial->type->close(port, filp);
 		port->open_count = 0;
+
+		if (port->tty) {
+			if (port->tty->driver_data)
+				port->tty->driver_data = NULL;
+			port->tty = NULL;
+		}
 	}
 
 	module_put(port->serial->type->owner);
 	kobject_put(&port->serial->kobj);
 }
 
-static void serial_close(struct tty_struct *tty, struct file * filp)
-{
-	struct usb_serial_port *port = (struct usb_serial_port *) tty->driver_data;
-	struct usb_serial *serial = get_usb_serial (port, __FUNCTION__);
-
-	if (!serial)
-		return;
-
-	dbg("%s - port %d", __FUNCTION__, port->number);
-
-	/* if disconnect beat us to the punch here, there's nothing to do */
-	if (tty && tty->driver_data) {
-		__serial_close(port, filp);
-		tty->driver_data = NULL;
-	}
-	port->tty = NULL;
-}
-
 static int serial_write (struct tty_struct * tty, int from_user, const unsigned char *buf, int count)
 {
 	struct usb_serial_port *port = (struct usb_serial_port *) tty->driver_data;
@@ -848,19 +839,6 @@
 	dbg ("%s - %s", __FUNCTION__, kobj->name);
 
 	serial = to_usb_serial(kobj);
-
-	/* fail all future close/read/write/ioctl/etc calls */
-	for (i = 0; i < serial->num_ports; ++i) {
-		port = serial->port[i];
-		if (port->tty != NULL) {
-			port->tty->driver_data = NULL;
-			while (port->open_count > 0) {
-				__serial_close(port, NULL);
-			}
-			port->tty = NULL;
-		}
-	}
-
 	serial_shutdown (serial);
 
 	/* return the minor range that this device had */
@@ -1242,7 +1220,7 @@
 	/* register all of the individual ports with the driver core */
 	for (i = 0; i < num_ports; ++i) {
 		port = serial->port[i];
-		port->dev.parent = &serial->dev->dev;
+		port->dev.parent = &interface->dev;
 		port->dev.driver = NULL;
 		port->dev.bus = &usb_serial_bus_type;
 		port->dev.release = &port_release;

Re: Oops w/sysfs when closing a disconnected usb serial device

[Mike Gorse]

On Fri, 5 Dec 2003, Greg KH wrote:

> Try the patch below.  With your sysfs patch I don't get any oopses
> anymore.  I still need to beat on this patch a lot more before I think
> it solves all of the current issues.  Can you let me know if it works
> for you or not?
> 
I tried it briefly, and it made no difference with the issue I am having.  
When disconnecting the gps receiver with a fd still open on it, it still 
gives an oops on closing it without the sysfs patch.  The sysfs patch 
eliminates the oops, but I wasn't getting an oops without your patch if I 
include the sysfs patch.  Anyway, I have over 200k of debugging output, so 
I'll send it to you off list.

Thanks,
-Mike G

Re: Oops w/sysfs when closing a disconnected usb serial device

[Greg KH]

On Fri, Dec 05, 2003 at 09:16:12PM -0500, Mike Gorse wrote:
> On Fri, 5 Dec 2003, Greg KH wrote:
> 
> > Try the patch below.  With your sysfs patch I don't get any oopses
> > anymore.  I still need to beat on this patch a lot more before I think
> > it solves all of the current issues.  Can you let me know if it works
> > for you or not?
> > 
> I tried it briefly, and it made no difference with the issue I am having.  
> When disconnecting the gps receiver with a fd still open on it, it still 
> gives an oops on closing it without the sysfs patch.  The sysfs patch 
> eliminates the oops, but I wasn't getting an oops without your patch if I 
> include the sysfs patch.  Anyway, I have over 200k of debugging output, so 
> I'll send it to you off list.

Hm, wait.  I mean use your sysfs patch, _and_ my patch.  Previously you
had said your sysfs patch had only moved the oops elsewhere.  I found
that true myself, hence my patch.  With both patches, things work for
me, how about you?

Oh, about all of that debugging output, just enable debugging in the
usbserial core, not the usb serial driver.  That makes it much more
managable :)

thanks,

greg k-h

Re: Oops w/sysfs when closing a disconnected usb serial device

[Mike Gorse]

On Sat, 6 Dec 2003, Greg KH wrote:

> Hm, wait.  I mean use your sysfs patch, _and_ my patch.  Previously you
> had said your sysfs patch had only moved the oops elsewhere.  I found
> that true myself, hence my patch.  With both patches, things work for
> me, how about you?
> 
The sysfs patch has always eliminated my oops; applying the kobject patch 
that Maneesh created without the sysfs patch moved the oops elsewhere.

-Mike G-

Re: Oops w/sysfs when closing a disconnected usb serial device

[Maneesh Soni]

On Fri, Dec 05, 2003 at 04:56:44PM -0800, Greg KH wrote:
[..]
> 
> I agree with this patch.  It fixes the usbserial oops for me in my
> testing.  
> 
> But wait, no, this patch is not good...
> 
> I think Mike's patch is correct.  Here's the problem (tree simplified
> for this example to make it readable):
> 	- insert usb device, this creates a sysfs directory something
> 	  like:
> 		- pci.../usb1/1.0
> 	- the usbserial driver binds to this device and creates a
> 	  ttyUSB0 directory:
> 	  	- pci.../usb1/1.0/ttyUSB0
> 	- a user opens the ttyUSB0 device node (in /dev) which
> 	  increments the reference count of the kobject that controls
> 	  the ttyUSB0 directory in sysfs.
> 	- the usb device is removed from the system.  In doing this,
> 	  the driver core, and then the kobject code has to delete the
> 	  tree 1.0 directory.  
> 	  
> Now at this point, Maneesh, your patch would prevent this directory from
> being removed, until after the ttyUSB0 directory is removed.  That's all
> well and good, but what happens if we have another USB device plugged
> into the same place before this happens.  Then the USB code would try to
> create the 1.0 directory over again (the directory names are built off
> of the USB topology.  but even if they were built off of something
> unique, we would still have a mess...)  If that happens, the creation of
> the directory would fail, which is not acceptable.

I see and agree, in this case my patch will create more problems.. Actually the 
problem here is _not_ that the directory is removed even though it is in use. 
The inherent dentry ref. counting should take care for that and 
corresponding dentry will be around as long as there are existing users. The 
problem here is parent going away before child and we are trying to remove the 
child directory more than once.

> But Mike's patch allows the whole tree from 1.0 on down to be removed,
> and then later, when the ttyUSB0 node is really closed, the directory
> will not tried to be removed.  Memory is still cleaned up properly, and
> the kobjects are properly reference counted.
> 
> I know in the past I've argued against this kind of patch (sorry scsi
> people), but now in thinking about it a bunch (and sitting though a
> zillion oops messages trying to figure out what's wrong here) I think
> this is the correct fix.
> 
> Any other opinions?
> 

The only problem with Mike's fix is that it fixes the symptom and does
not fix the real cause. I don't see in which case we can have a NULL
d_inode at that point, checking for that means hiding the real problem.

Probably dump_stack() in sysfs_remove_dir() could bring more light in this 
problem. 

Maneesh

-- 
Maneesh Soni
Linux Technology Center, 
IBM Software Lab, Bangalore, India
email: maneesh@in.ibm.com
Phone: 91-80-5044999 Fax: 91-80-5268553
T/L : 9243696