* Oops w/sysfs when closing a disconnected usb serial device @ 2003-12-01 0:18 Mike Gorse 2003-12-01 9:38 ` Maneesh Soni 0 siblings, 1 reply; 11+ messages in thread From: Mike Gorse @ 2003-12-01 0:18 UTC (permalink / raw) To: linux-kernel 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 -- ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: Oops w/sysfs when closing a disconnected usb serial device 2003-12-01 0:18 Oops w/sysfs when closing a disconnected usb serial device Mike Gorse @ 2003-12-01 9:38 ` Maneesh Soni 2003-12-01 23:55 ` Mike Gorse 2003-12-06 0:56 ` Greg KH 0 siblings, 2 replies; 11+ messages in thread From: Maneesh Soni @ 2003-12-01 9:38 UTC (permalink / raw) To: Mike Gorse; +Cc: linux-kernel, Patrick Mochel, Greg KH 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: Oops w/sysfs when closing a disconnected usb serial device 2003-12-01 9:38 ` Maneesh Soni @ 2003-12-01 23:55 ` Mike Gorse 2003-12-02 7:11 ` Maneesh Soni 2003-12-06 1:38 ` Greg KH 2003-12-06 0:56 ` Greg KH 1 sibling, 2 replies; 11+ messages in thread From: Mike Gorse @ 2003-12-01 23:55 UTC (permalink / raw) To: Maneesh Soni; +Cc: linux-kernel, Patrick Mochel, Greg KH 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: Oops w/sysfs when closing a disconnected usb serial device 2003-12-01 23:55 ` Mike Gorse @ 2003-12-02 7:11 ` Maneesh Soni 2003-12-05 23:36 ` Greg KH 2003-12-06 1:38 ` Greg KH 1 sibling, 1 reply; 11+ messages in thread From: Maneesh Soni @ 2003-12-02 7:11 UTC (permalink / raw) To: Mike Gorse; +Cc: linux-kernel, Patrick Mochel, Greg KH 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: Oops w/sysfs when closing a disconnected usb serial device 2003-12-02 7:11 ` Maneesh Soni @ 2003-12-05 23:36 ` Greg KH 0 siblings, 0 replies; 11+ messages in thread From: Greg KH @ 2003-12-05 23:36 UTC (permalink / raw) To: Maneesh Soni; +Cc: Mike Gorse, linux-kernel, Patrick Mochel 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: Oops w/sysfs when closing a disconnected usb serial device 2003-12-01 23:55 ` Mike Gorse 2003-12-02 7:11 ` Maneesh Soni @ 2003-12-06 1:38 ` Greg KH 2003-12-06 2:16 ` Mike Gorse 1 sibling, 1 reply; 11+ messages in thread From: Greg KH @ 2003-12-06 1:38 UTC (permalink / raw) To: Mike Gorse; +Cc: Maneesh Soni, linux-kernel, mochel 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; ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: Oops w/sysfs when closing a disconnected usb serial device 2003-12-06 1:38 ` Greg KH @ 2003-12-06 2:16 ` Mike Gorse 2003-12-07 2:58 ` Greg KH 0 siblings, 1 reply; 11+ messages in thread From: Mike Gorse @ 2003-12-06 2:16 UTC (permalink / raw) To: Greg KH; +Cc: Maneesh Soni, linux-kernel, mochel 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: Oops w/sysfs when closing a disconnected usb serial device 2003-12-06 2:16 ` Mike Gorse @ 2003-12-07 2:58 ` Greg KH 2003-12-07 3:09 ` Mike Gorse 0 siblings, 1 reply; 11+ messages in thread From: Greg KH @ 2003-12-07 2:58 UTC (permalink / raw) To: Mike Gorse; +Cc: Maneesh Soni, linux-kernel, mochel 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: Oops w/sysfs when closing a disconnected usb serial device 2003-12-07 2:58 ` Greg KH @ 2003-12-07 3:09 ` Mike Gorse 0 siblings, 0 replies; 11+ messages in thread From: Mike Gorse @ 2003-12-07 3:09 UTC (permalink / raw) To: Greg KH; +Cc: Maneesh Soni, linux-kernel, mochel 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- ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: Oops w/sysfs when closing a disconnected usb serial device 2003-12-01 9:38 ` Maneesh Soni 2003-12-01 23:55 ` Mike Gorse @ 2003-12-06 0:56 ` Greg KH 2003-12-08 4:56 ` Maneesh Soni 1 sibling, 1 reply; 11+ messages in thread From: Greg KH @ 2003-12-06 0:56 UTC (permalink / raw) To: Maneesh Soni; +Cc: Mike Gorse, linux-kernel, Patrick Mochel 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: Oops w/sysfs when closing a disconnected usb serial device 2003-12-06 0:56 ` Greg KH @ 2003-12-08 4:56 ` Maneesh Soni 0 siblings, 0 replies; 11+ messages in thread From: Maneesh Soni @ 2003-12-08 4:56 UTC (permalink / raw) To: Greg KH; +Cc: Mike Gorse, linux-kernel, Patrick Mochel 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2003-12-08 4:57 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2003-12-01 0:18 Oops w/sysfs when closing a disconnected usb serial device Mike Gorse 2003-12-01 9:38 ` Maneesh Soni 2003-12-01 23:55 ` Mike Gorse 2003-12-02 7:11 ` Maneesh Soni 2003-12-05 23:36 ` Greg KH 2003-12-06 1:38 ` Greg KH 2003-12-06 2:16 ` Mike Gorse 2003-12-07 2:58 ` Greg KH 2003-12-07 3:09 ` Mike Gorse 2003-12-06 0:56 ` Greg KH 2003-12-08 4:56 ` Maneesh Soni
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).