| Message ID | 20190318174906.10429-1-marc.zyngier@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] usb: typec: fusb302: Fix debugfs mutex initialisation | expand |
On Mon, Mar 18, 2019 at 05:49:06PM +0000, Marc Zyngier wrote: > Running a kernel with the fusb302 driver and lockdep enabled > leads to an unpleasant warning: > > [ 4.617477] INFO: trying to register non-static key. > [ 4.617930] the code is fine but needs lockdep annotation. > [ 4.618418] turning off the locking correctness validator. > [ 4.618913] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 5.1.0-rc1-00007-g3542533f3fc9 #13 > [ 4.619620] Hardware name: rockchip evb_rk3399/evb_rk3399, BIOS 2019.04-rc3-00124-g2feec69fb1 03/15/2019 > [ 4.620454] Call trace: > [ 4.620693] dump_backtrace+0x0/0x138 > [ 4.621028] show_stack+0x24/0x30 > [ 4.621336] dump_stack+0xbc/0x104 > [ 4.621649] register_lock_class+0x594/0x598 > [ 4.622036] __lock_acquire+0x80/0x11b8 > [ 4.622384] lock_acquire+0xdc/0x260 > [ 4.622711] __mutex_lock+0x90/0x8a0 > [ 4.623037] mutex_lock_nested+0x3c/0x50 > [ 4.623394] _fusb302_log+0x88/0x1f0 > [ 4.623721] fusb302_log+0x7c/0xa0 > [ 4.624033] tcpm_init+0x5c/0x190 > [ 4.624336] tcpm_init+0x3c/0x130 > [ 4.624640] tcpm_register_port+0x574/0x878 > [ 4.625019] fusb302_probe+0x2c8/0x590 > > Despite what the message says, the code isn't fine, as it tries to > make use of the fusb302_log facility pretty early. This requires the > logbuffer_lock mutex to be initialised, but that only happens much > later. Boo. > > Hoist the fusb302_debugfs_init call before tcpm_register_port so that > we can enjoy a working mutex. At Guenter's request, also add teardown > of the debugfs facility on the error path. > > Signed-off-by: Marc Zyngier <marc.zyngier@arm.com> Reviewed-by: Guenter Roeck <linux@roeck-us.net> > --- > drivers/usb/typec/tcpm/fusb302.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/drivers/usb/typec/tcpm/fusb302.c b/drivers/usb/typec/tcpm/fusb302.c > index e9344997329c..76cb8be6f3eb 100644 > --- a/drivers/usb/typec/tcpm/fusb302.c > +++ b/drivers/usb/typec/tcpm/fusb302.c > @@ -1790,12 +1790,13 @@ static int fusb302_probe(struct i2c_client *client, > goto destroy_workqueue; > } > > + fusb302_debugfs_init(chip); > chip->tcpm_port = tcpm_register_port(&client->dev, &chip->tcpc_dev); > if (IS_ERR(chip->tcpm_port)) { > ret = PTR_ERR(chip->tcpm_port); > if (ret != -EPROBE_DEFER) > dev_err(dev, "cannot register tcpm port, ret=%d", ret); > - goto destroy_workqueue; > + goto unregister_debugfs; > } > > ret = devm_request_threaded_irq(chip->dev, chip->gpio_int_n_irq, > @@ -1807,13 +1808,14 @@ static int fusb302_probe(struct i2c_client *client, > goto tcpm_unregister_port; > } > enable_irq_wake(chip->gpio_int_n_irq); > - fusb302_debugfs_init(chip); > i2c_set_clientdata(client, chip); > > return ret; > > tcpm_unregister_port: > tcpm_unregister_port(chip->tcpm_port); > +unregister_debugfs: > + fusb302_debugfs_exit(chip); > destroy_workqueue: > destroy_workqueue(chip->wq); > > -- > 2.20.1 >
On Mon, Mar 18, 2019 at 05:49:06PM +0000, Marc Zyngier wrote: > Running a kernel with the fusb302 driver and lockdep enabled > leads to an unpleasant warning: > > [ 4.617477] INFO: trying to register non-static key. > [ 4.617930] the code is fine but needs lockdep annotation. > [ 4.618418] turning off the locking correctness validator. > [ 4.618913] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 5.1.0-rc1-00007-g3542533f3fc9 #13 > [ 4.619620] Hardware name: rockchip evb_rk3399/evb_rk3399, BIOS 2019.04-rc3-00124-g2feec69fb1 03/15/2019 > [ 4.620454] Call trace: > [ 4.620693] dump_backtrace+0x0/0x138 > [ 4.621028] show_stack+0x24/0x30 > [ 4.621336] dump_stack+0xbc/0x104 > [ 4.621649] register_lock_class+0x594/0x598 > [ 4.622036] __lock_acquire+0x80/0x11b8 > [ 4.622384] lock_acquire+0xdc/0x260 > [ 4.622711] __mutex_lock+0x90/0x8a0 > [ 4.623037] mutex_lock_nested+0x3c/0x50 > [ 4.623394] _fusb302_log+0x88/0x1f0 > [ 4.623721] fusb302_log+0x7c/0xa0 > [ 4.624033] tcpm_init+0x5c/0x190 > [ 4.624336] tcpm_init+0x3c/0x130 > [ 4.624640] tcpm_register_port+0x574/0x878 > [ 4.625019] fusb302_probe+0x2c8/0x590 > > Despite what the message says, the code isn't fine, as it tries to > make use of the fusb302_log facility pretty early. This requires the > logbuffer_lock mutex to be initialised, but that only happens much > later. Boo. > > Hoist the fusb302_debugfs_init call before tcpm_register_port so that > we can enjoy a working mutex. At Guenter's request, also add teardown > of the debugfs facility on the error path. > > Signed-off-by: Marc Zyngier <marc.zyngier@arm.com> After applying this there was no more "fusb302" debugfs directory, and attempt to unload the fusb302 module dead locked. Also, attempt to reboot caused this to happen on my GDPWin board after applying the patch: BUG: Dentry 0000000012f2a05d{i=149,n=i2c-fusb302} still in use (1) [unmount of sysfs sysfs] WARNING: CPU: 3 PID: 1639 at fs/dcache.c:1529 umount_check.cold.55+0x2e/0x3a Modules linked in: intel_xhci_usb_role_switch roles pi3usb30532 typec i915 intel_gtt intel_cht_int33fe [last unloaded: tcpm] CPU: 3 PID: 1639 Comm: umount Not tainted 5.1.0-rc1-heikki+ #916 Hardware name: Default string Default string/Default string, BIOS 5.11 05/25/2017 RIP: 0010:umount_check.cold.55+0x2e/0x3a ... Note. Your patch has also a conflict with patches from Hans, I think with this one: https://patchwork.kernel.org/patch/10847275/ I can take care of that, but you can also rebase the next version on top of my typec-next branch to solve that problem: https://github.com/krohei/linux/commits/typec-next I think I need to add that to the MAINTAINERS file. thanks,
On Tue, 19 Mar 2019 13:45:11 +0200 Heikki Krogerus <heikki.krogerus@linux.intel.com> wrote: > On Mon, Mar 18, 2019 at 05:49:06PM +0000, Marc Zyngier wrote: > > Running a kernel with the fusb302 driver and lockdep enabled > > leads to an unpleasant warning: > > > > [ 4.617477] INFO: trying to register non-static key. > > [ 4.617930] the code is fine but needs lockdep annotation. > > [ 4.618418] turning off the locking correctness validator. > > [ 4.618913] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 5.1.0-rc1-00007-g3542533f3fc9 #13 > > [ 4.619620] Hardware name: rockchip evb_rk3399/evb_rk3399, BIOS 2019.04-rc3-00124-g2feec69fb1 03/15/2019 > > [ 4.620454] Call trace: > > [ 4.620693] dump_backtrace+0x0/0x138 > > [ 4.621028] show_stack+0x24/0x30 > > [ 4.621336] dump_stack+0xbc/0x104 > > [ 4.621649] register_lock_class+0x594/0x598 > > [ 4.622036] __lock_acquire+0x80/0x11b8 > > [ 4.622384] lock_acquire+0xdc/0x260 > > [ 4.622711] __mutex_lock+0x90/0x8a0 > > [ 4.623037] mutex_lock_nested+0x3c/0x50 > > [ 4.623394] _fusb302_log+0x88/0x1f0 > > [ 4.623721] fusb302_log+0x7c/0xa0 > > [ 4.624033] tcpm_init+0x5c/0x190 > > [ 4.624336] tcpm_init+0x3c/0x130 > > [ 4.624640] tcpm_register_port+0x574/0x878 > > [ 4.625019] fusb302_probe+0x2c8/0x590 > > > > Despite what the message says, the code isn't fine, as it tries to > > make use of the fusb302_log facility pretty early. This requires the > > logbuffer_lock mutex to be initialised, but that only happens much > > later. Boo. > > > > Hoist the fusb302_debugfs_init call before tcpm_register_port so that > > we can enjoy a working mutex. At Guenter's request, also add teardown > > of the debugfs facility on the error path. > > > > Signed-off-by: Marc Zyngier <marc.zyngier@arm.com> > > After applying this there was no more "fusb302" debugfs directory, and > attempt to unload the fusb302 module dead locked. Also, attempt to > reboot caused this to happen on my GDPWin board after applying the > patch: > > BUG: Dentry 0000000012f2a05d{i=149,n=i2c-fusb302} still in use (1) [unmount of sysfs sysfs] > WARNING: CPU: 3 PID: 1639 at fs/dcache.c:1529 umount_check.cold.55+0x2e/0x3a > Modules linked in: intel_xhci_usb_role_switch roles pi3usb30532 typec i915 intel_gtt intel_cht_int33fe [last unloaded: tcpm] > CPU: 3 PID: 1639 Comm: umount Not tainted 5.1.0-rc1-heikki+ #916 > Hardware name: Default string Default string/Default string, BIOS 5.11 05/25/2017 > RIP: 0010:umount_check.cold.55+0x2e/0x3a > ... OK, that's interesting. On my system, I do have the debugfs file: root@city-of-tiny-lites:~# cat /sys/kernel/debug/fusb302/4-0022 [ 4.517682] sw reset [ 4.520379] fusb302 device ID: 0x81 [ 4.523544] pd := off [ 4.523593] vbus is already Off [ 4.523603] charge is already Off [ 4.523615] vconn is already Off [ 4.524274] pd header := Sink, Device [ 4.525948] cc1=Open, cc2=Open [ 4.530345] pd := off [ 4.530378] vbus is already Off [ 4.530388] charge is already Off [ 4.530405] vconn is already Off [ 4.530931] pd header := Sink, Device [ 4.533773] cc := Open [ 4.642347] start drp toggling I don't use any module though, so maybe there is something there. I'll investigate. > Note. Your patch has also a conflict with patches from Hans, I > think with this one: https://patchwork.kernel.org/patch/10847275/ > I can take care of that, but you can also rebase the next version on > top of my typec-next branch to solve that problem: > https://github.com/krohei/linux/commits/typec-next > > I think I need to add that to the MAINTAINERS file. Fair enough. I'll come back once I know what happens here... Thanks, M.
On Tue, 19 Mar 2019 13:45:11 +0200 Heikki Krogerus <heikki.krogerus@linux.intel.com> wrote: Heikki, > On Mon, Mar 18, 2019 at 05:49:06PM +0000, Marc Zyngier wrote: > > Running a kernel with the fusb302 driver and lockdep enabled > > leads to an unpleasant warning: > > > > [ 4.617477] INFO: trying to register non-static key. > > [ 4.617930] the code is fine but needs lockdep annotation. > > [ 4.618418] turning off the locking correctness validator. > > [ 4.618913] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 5.1.0-rc1-00007-g3542533f3fc9 #13 > > [ 4.619620] Hardware name: rockchip evb_rk3399/evb_rk3399, BIOS 2019.04-rc3-00124-g2feec69fb1 03/15/2019 > > [ 4.620454] Call trace: > > [ 4.620693] dump_backtrace+0x0/0x138 > > [ 4.621028] show_stack+0x24/0x30 > > [ 4.621336] dump_stack+0xbc/0x104 > > [ 4.621649] register_lock_class+0x594/0x598 > > [ 4.622036] __lock_acquire+0x80/0x11b8 > > [ 4.622384] lock_acquire+0xdc/0x260 > > [ 4.622711] __mutex_lock+0x90/0x8a0 > > [ 4.623037] mutex_lock_nested+0x3c/0x50 > > [ 4.623394] _fusb302_log+0x88/0x1f0 > > [ 4.623721] fusb302_log+0x7c/0xa0 > > [ 4.624033] tcpm_init+0x5c/0x190 > > [ 4.624336] tcpm_init+0x3c/0x130 > > [ 4.624640] tcpm_register_port+0x574/0x878 > > [ 4.625019] fusb302_probe+0x2c8/0x590 > > > > Despite what the message says, the code isn't fine, as it tries to > > make use of the fusb302_log facility pretty early. This requires the > > logbuffer_lock mutex to be initialised, but that only happens much > > later. Boo. > > > > Hoist the fusb302_debugfs_init call before tcpm_register_port so that > > we can enjoy a working mutex. At Guenter's request, also add teardown > > of the debugfs facility on the error path. > > > > Signed-off-by: Marc Zyngier <marc.zyngier@arm.com> > > After applying this there was no more "fusb302" debugfs directory, and > attempt to unload the fusb302 module dead locked. Also, attempt to > reboot caused this to happen on my GDPWin board after applying the > patch: > > BUG: Dentry 0000000012f2a05d{i=149,n=i2c-fusb302} still in use (1) [unmount of sysfs sysfs] > WARNING: CPU: 3 PID: 1639 at fs/dcache.c:1529 umount_check.cold.55+0x2e/0x3a > Modules linked in: intel_xhci_usb_role_switch roles pi3usb30532 typec i915 intel_gtt intel_cht_int33fe [last unloaded: tcpm] > CPU: 3 PID: 1639 Comm: umount Not tainted 5.1.0-rc1-heikki+ #916 > Hardware name: Default string Default string/Default string, BIOS 5.11 05/25/2017 > RIP: 0010:umount_check.cold.55+0x2e/0x3a > ... > > Note. Your patch has also a conflict with patches from Hans, I > think with this one: https://patchwork.kernel.org/patch/10847275/ > I can take care of that, but you can also rebase the next version on > top of my typec-next branch to solve that problem: > https://github.com/krohei/linux/commits/typec-next OK, this is very weird. I can't reproduce any of the issues you're reporting: - the patch applies cleanly on top of typec-next - removing the fusb302 module works - I see the debugfs file whenever fsusb302 is inserted Maybe you were trying this on another branch? Thanks, M.
Hi Marc, On Wed, Mar 20, 2019 at 04:07:08PM +0000, Marc Zyngier wrote: > On Tue, 19 Mar 2019 13:45:11 +0200 > Heikki Krogerus <heikki.krogerus@linux.intel.com> wrote: > > Heikki, > > > On Mon, Mar 18, 2019 at 05:49:06PM +0000, Marc Zyngier wrote: > > > Running a kernel with the fusb302 driver and lockdep enabled > > > leads to an unpleasant warning: > > > > > > [ 4.617477] INFO: trying to register non-static key. > > > [ 4.617930] the code is fine but needs lockdep annotation. > > > [ 4.618418] turning off the locking correctness validator. > > > [ 4.618913] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 5.1.0-rc1-00007-g3542533f3fc9 #13 > > > [ 4.619620] Hardware name: rockchip evb_rk3399/evb_rk3399, BIOS 2019.04-rc3-00124-g2feec69fb1 03/15/2019 > > > [ 4.620454] Call trace: > > > [ 4.620693] dump_backtrace+0x0/0x138 > > > [ 4.621028] show_stack+0x24/0x30 > > > [ 4.621336] dump_stack+0xbc/0x104 > > > [ 4.621649] register_lock_class+0x594/0x598 > > > [ 4.622036] __lock_acquire+0x80/0x11b8 > > > [ 4.622384] lock_acquire+0xdc/0x260 > > > [ 4.622711] __mutex_lock+0x90/0x8a0 > > > [ 4.623037] mutex_lock_nested+0x3c/0x50 > > > [ 4.623394] _fusb302_log+0x88/0x1f0 > > > [ 4.623721] fusb302_log+0x7c/0xa0 > > > [ 4.624033] tcpm_init+0x5c/0x190 > > > [ 4.624336] tcpm_init+0x3c/0x130 > > > [ 4.624640] tcpm_register_port+0x574/0x878 > > > [ 4.625019] fusb302_probe+0x2c8/0x590 > > > > > > Despite what the message says, the code isn't fine, as it tries to > > > make use of the fusb302_log facility pretty early. This requires the > > > logbuffer_lock mutex to be initialised, but that only happens much > > > later. Boo. > > > > > > Hoist the fusb302_debugfs_init call before tcpm_register_port so that > > > we can enjoy a working mutex. At Guenter's request, also add teardown > > > of the debugfs facility on the error path. > > > > > > Signed-off-by: Marc Zyngier <marc.zyngier@arm.com> > > > > After applying this there was no more "fusb302" debugfs directory, and > > attempt to unload the fusb302 module dead locked. Also, attempt to > > reboot caused this to happen on my GDPWin board after applying the > > patch: > > > > BUG: Dentry 0000000012f2a05d{i=149,n=i2c-fusb302} still in use (1) [unmount of sysfs sysfs] > > WARNING: CPU: 3 PID: 1639 at fs/dcache.c:1529 umount_check.cold.55+0x2e/0x3a > > Modules linked in: intel_xhci_usb_role_switch roles pi3usb30532 typec i915 intel_gtt intel_cht_int33fe [last unloaded: tcpm] > > CPU: 3 PID: 1639 Comm: umount Not tainted 5.1.0-rc1-heikki+ #916 > > Hardware name: Default string Default string/Default string, BIOS 5.11 05/25/2017 > > RIP: 0010:umount_check.cold.55+0x2e/0x3a > > ... > > > > Note. Your patch has also a conflict with patches from Hans, I > > think with this one: https://patchwork.kernel.org/patch/10847275/ > > I can take care of that, but you can also rebase the next version on > > top of my typec-next branch to solve that problem: > > https://github.com/krohei/linux/commits/typec-next > > OK, this is very weird. I can't reproduce any of the issues you're > reporting: > > - the patch applies cleanly on top of typec-next > - removing the fusb302 module works > - I see the debugfs file whenever fsusb302 is inserted > > Maybe you were trying this on another branch? No, the branch is correct. Actually, I tested this on top of mainline and linux-next. I saw that happen on both. On these Intel Cherrytrail based boards like my GDBWin, fusb302 is one of the functions of a weir MFD device (the driver for that device is drivers/platform/x86/intel_cht_int33fe.c). It's entirely possible that we are doing something wrong in that driver, and your patch just makes the problem visible. I'll continue debugging. thanks,
Hi, On Wed, Mar 20, 2019 at 06:34:33PM +0200, Heikki Krogerus wrote: > > > After applying this there was no more "fusb302" debugfs directory, and > > > attempt to unload the fusb302 module dead locked. Also, attempt to > > > reboot caused this to happen on my GDPWin board after applying the > > > patch: > > > > > > BUG: Dentry 0000000012f2a05d{i=149,n=i2c-fusb302} still in use (1) [unmount of sysfs sysfs] > > > WARNING: CPU: 3 PID: 1639 at fs/dcache.c:1529 umount_check.cold.55+0x2e/0x3a > > > Modules linked in: intel_xhci_usb_role_switch roles pi3usb30532 typec i915 intel_gtt intel_cht_int33fe [last unloaded: tcpm] > > > CPU: 3 PID: 1639 Comm: umount Not tainted 5.1.0-rc1-heikki+ #916 > > > Hardware name: Default string Default string/Default string, BIOS 5.11 05/25/2017 > > > RIP: 0010:umount_check.cold.55+0x2e/0x3a > > > ... > > > > > > Note. Your patch has also a conflict with patches from Hans, I > > > think with this one: https://patchwork.kernel.org/patch/10847275/ > > > I can take care of that, but you can also rebase the next version on > > > top of my typec-next branch to solve that problem: > > > https://github.com/krohei/linux/commits/typec-next > > > > OK, this is very weird. I can't reproduce any of the issues you're > > reporting: > > > > - the patch applies cleanly on top of typec-next > > - removing the fusb302 module works > > - I see the debugfs file whenever fsusb302 is inserted > > > > Maybe you were trying this on another branch? > > No, the branch is correct. Actually, I tested this on top of mainline > and linux-next. I saw that happen on both. > > On these Intel Cherrytrail based boards like my GDBWin, fusb302 is one > of the functions of a weir MFD device (the driver for that device is > drivers/platform/x86/intel_cht_int33fe.c). It's entirely possible that > we are doing something wrong in that driver, and your patch just makes > the problem visible. > > I'll continue debugging. I figured out what's the problem. It seems that the driver does not probe successfully, which is why I don't see that "fusb302" debugfs directory. The reason is that if tcpm_register_port() returns with -EPROBE_DEFER, we end up with that rootdir already pointing to something, even though the entry is destroyed in that case. So next time the driver is probed, that "fusb302" directory does get created as rootdir has a value, and debugfs_create_file() fails. I think the correct fix is to just initialize the mutex earlier. Something like this should work: diff --git a/drivers/usb/typec/tcpm/fusb302.c b/drivers/usb/typec/tcpm/fusb302.c index 261b82900fec..8e43ea27f26d 100644 --- a/drivers/usb/typec/tcpm/fusb302.c +++ b/drivers/usb/typec/tcpm/fusb302.c @@ -211,7 +211,6 @@ static struct dentry *rootdir; static void fusb302_debugfs_init(struct fusb302_chip *chip) { - mutex_init(&chip->logbuffer_lock); if (!rootdir) rootdir = debugfs_create_dir("fusb302", NULL); @@ -1667,6 +1666,7 @@ static int fusb302_probe(struct i2c_client *client, chip->tcpc_config = fusb302_tcpc_config; chip->tcpc_dev.config = &chip->tcpc_config; mutex_init(&chip->lock); + mutex_init(&chip->logbuffer_lock); chip->tcpc_dev.fwnode = device_get_named_child_node(dev, "connector"); thanks,
On Thu, 21 Mar 2019 15:24:18 +0200 Heikki Krogerus <heikki.krogerus@linux.intel.com> wrote: > Hi, > > On Wed, Mar 20, 2019 at 06:34:33PM +0200, Heikki Krogerus wrote: > > > > After applying this there was no more "fusb302" debugfs directory, and > > > > attempt to unload the fusb302 module dead locked. Also, attempt to > > > > reboot caused this to happen on my GDPWin board after applying the > > > > patch: > > > > > > > > BUG: Dentry 0000000012f2a05d{i=149,n=i2c-fusb302} still in use (1) [unmount of sysfs sysfs] > > > > WARNING: CPU: 3 PID: 1639 at fs/dcache.c:1529 umount_check.cold.55+0x2e/0x3a > > > > Modules linked in: intel_xhci_usb_role_switch roles pi3usb30532 typec i915 intel_gtt intel_cht_int33fe [last unloaded: tcpm] > > > > CPU: 3 PID: 1639 Comm: umount Not tainted 5.1.0-rc1-heikki+ #916 > > > > Hardware name: Default string Default string/Default string, BIOS 5.11 05/25/2017 > > > > RIP: 0010:umount_check.cold.55+0x2e/0x3a > > > > ... > > > > > > > > Note. Your patch has also a conflict with patches from Hans, I > > > > think with this one: https://patchwork.kernel.org/patch/10847275/ > > > > I can take care of that, but you can also rebase the next version on > > > > top of my typec-next branch to solve that problem: > > > > https://github.com/krohei/linux/commits/typec-next > > > > > > OK, this is very weird. I can't reproduce any of the issues you're > > > reporting: > > > > > > - the patch applies cleanly on top of typec-next > > > - removing the fusb302 module works > > > - I see the debugfs file whenever fsusb302 is inserted > > > > > > Maybe you were trying this on another branch? > > > > No, the branch is correct. Actually, I tested this on top of mainline > > and linux-next. I saw that happen on both. > > > > On these Intel Cherrytrail based boards like my GDBWin, fusb302 is one > > of the functions of a weir MFD device (the driver for that device is > > drivers/platform/x86/intel_cht_int33fe.c). It's entirely possible that > > we are doing something wrong in that driver, and your patch just makes > > the problem visible. > > > > I'll continue debugging. > > I figured out what's the problem. It seems that the driver does not > probe successfully, which is why I don't see that "fusb302" debugfs > directory. > > The reason is that if tcpm_register_port() returns with -EPROBE_DEFER, > we end up with that rootdir already pointing to something, even though > the entry is destroyed in that case. So next time the driver is > probed, that "fusb302" directory does get created as rootdir has a > value, and debugfs_create_file() fails. > > I think the correct fix is to just initialize the mutex earlier. > Something like this should work: > > diff --git a/drivers/usb/typec/tcpm/fusb302.c b/drivers/usb/typec/tcpm/fusb302.c > index 261b82900fec..8e43ea27f26d 100644 > --- a/drivers/usb/typec/tcpm/fusb302.c > +++ b/drivers/usb/typec/tcpm/fusb302.c > @@ -211,7 +211,6 @@ static struct dentry *rootdir; > > static void fusb302_debugfs_init(struct fusb302_chip *chip) > { > - mutex_init(&chip->logbuffer_lock); > if (!rootdir) > rootdir = debugfs_create_dir("fusb302", NULL); > > @@ -1667,6 +1666,7 @@ static int fusb302_probe(struct i2c_client *client, > chip->tcpc_config = fusb302_tcpc_config; > chip->tcpc_dev.config = &chip->tcpc_config; > mutex_init(&chip->lock); > + mutex_init(&chip->logbuffer_lock); > > chip->tcpc_dev.fwnode = > device_get_named_child_node(dev, "connector"); Looks good to me, although you probably want to make that conditional on CONFIG_DEBUG_FS being set. Thanks, M.
On Thu, Mar 21, 2019 at 04:02:27PM +0000, Marc Zyngier wrote: > On Thu, 21 Mar 2019 15:24:18 +0200 > Heikki Krogerus <heikki.krogerus@linux.intel.com> wrote: > > > Hi, > > > > On Wed, Mar 20, 2019 at 06:34:33PM +0200, Heikki Krogerus wrote: > > > > > After applying this there was no more "fusb302" debugfs directory, and > > > > > attempt to unload the fusb302 module dead locked. Also, attempt to > > > > > reboot caused this to happen on my GDPWin board after applying the > > > > > patch: > > > > > > > > > > BUG: Dentry 0000000012f2a05d{i=149,n=i2c-fusb302} still in use (1) [unmount of sysfs sysfs] > > > > > WARNING: CPU: 3 PID: 1639 at fs/dcache.c:1529 umount_check.cold.55+0x2e/0x3a > > > > > Modules linked in: intel_xhci_usb_role_switch roles pi3usb30532 typec i915 intel_gtt intel_cht_int33fe [last unloaded: tcpm] > > > > > CPU: 3 PID: 1639 Comm: umount Not tainted 5.1.0-rc1-heikki+ #916 > > > > > Hardware name: Default string Default string/Default string, BIOS 5.11 05/25/2017 > > > > > RIP: 0010:umount_check.cold.55+0x2e/0x3a > > > > > ... > > > > > > > > > > Note. Your patch has also a conflict with patches from Hans, I > > > > > think with this one: https://patchwork.kernel.org/patch/10847275/ > > > > > I can take care of that, but you can also rebase the next version on > > > > > top of my typec-next branch to solve that problem: > > > > > https://github.com/krohei/linux/commits/typec-next > > > > > > > > OK, this is very weird. I can't reproduce any of the issues you're > > > > reporting: > > > > > > > > - the patch applies cleanly on top of typec-next > > > > - removing the fusb302 module works > > > > - I see the debugfs file whenever fsusb302 is inserted > > > > > > > > Maybe you were trying this on another branch? > > > > > > No, the branch is correct. Actually, I tested this on top of mainline > > > and linux-next. I saw that happen on both. > > > > > > On these Intel Cherrytrail based boards like my GDBWin, fusb302 is one > > > of the functions of a weir MFD device (the driver for that device is > > > drivers/platform/x86/intel_cht_int33fe.c). It's entirely possible that > > > we are doing something wrong in that driver, and your patch just makes > > > the problem visible. > > > > > > I'll continue debugging. > > > > I figured out what's the problem. It seems that the driver does not > > probe successfully, which is why I don't see that "fusb302" debugfs > > directory. > > > > The reason is that if tcpm_register_port() returns with -EPROBE_DEFER, > > we end up with that rootdir already pointing to something, even though > > the entry is destroyed in that case. So next time the driver is > > probed, that "fusb302" directory does get created as rootdir has a > > value, and debugfs_create_file() fails. > > > > I think the correct fix is to just initialize the mutex earlier. > > Something like this should work: > > > > diff --git a/drivers/usb/typec/tcpm/fusb302.c b/drivers/usb/typec/tcpm/fusb302.c > > index 261b82900fec..8e43ea27f26d 100644 > > --- a/drivers/usb/typec/tcpm/fusb302.c > > +++ b/drivers/usb/typec/tcpm/fusb302.c > > @@ -211,7 +211,6 @@ static struct dentry *rootdir; > > > > static void fusb302_debugfs_init(struct fusb302_chip *chip) > > { > > - mutex_init(&chip->logbuffer_lock); > > if (!rootdir) > > rootdir = debugfs_create_dir("fusb302", NULL); > > > > @@ -1667,6 +1666,7 @@ static int fusb302_probe(struct i2c_client *client, > > chip->tcpc_config = fusb302_tcpc_config; > > chip->tcpc_dev.config = &chip->tcpc_config; > > mutex_init(&chip->lock); > > + mutex_init(&chip->logbuffer_lock); > > > > chip->tcpc_dev.fwnode = > > device_get_named_child_node(dev, "connector"); > > Looks good to me, although you probably want to make that conditional > on CONFIG_DEBUG_FS being set. Just move that logbuffer_lock member outside of the ifdef CONFIG_DEBUG_FS condition. For the record, I don't see any use for those ifdef checks. Those logbuffer members in struct fusb302_chip could be kept in their own structure, for example struct fusb302_log, that we allocate separately and only if debugfs_initialized() returns true. thanks,
diff --git a/drivers/usb/typec/tcpm/fusb302.c b/drivers/usb/typec/tcpm/fusb302.c index e9344997329c..76cb8be6f3eb 100644 --- a/drivers/usb/typec/tcpm/fusb302.c +++ b/drivers/usb/typec/tcpm/fusb302.c @@ -1790,12 +1790,13 @@ static int fusb302_probe(struct i2c_client *client, goto destroy_workqueue; } + fusb302_debugfs_init(chip); chip->tcpm_port = tcpm_register_port(&client->dev, &chip->tcpc_dev); if (IS_ERR(chip->tcpm_port)) { ret = PTR_ERR(chip->tcpm_port); if (ret != -EPROBE_DEFER) dev_err(dev, "cannot register tcpm port, ret=%d", ret); - goto destroy_workqueue; + goto unregister_debugfs; } ret = devm_request_threaded_irq(chip->dev, chip->gpio_int_n_irq, @@ -1807,13 +1808,14 @@ static int fusb302_probe(struct i2c_client *client, goto tcpm_unregister_port; } enable_irq_wake(chip->gpio_int_n_irq); - fusb302_debugfs_init(chip); i2c_set_clientdata(client, chip); return ret; tcpm_unregister_port: tcpm_unregister_port(chip->tcpm_port); +unregister_debugfs: + fusb302_debugfs_exit(chip); destroy_workqueue: destroy_workqueue(chip->wq);
Running a kernel with the fusb302 driver and lockdep enabled leads to an unpleasant warning: [ 4.617477] INFO: trying to register non-static key. [ 4.617930] the code is fine but needs lockdep annotation. [ 4.618418] turning off the locking correctness validator. [ 4.618913] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 5.1.0-rc1-00007-g3542533f3fc9 #13 [ 4.619620] Hardware name: rockchip evb_rk3399/evb_rk3399, BIOS 2019.04-rc3-00124-g2feec69fb1 03/15/2019 [ 4.620454] Call trace: [ 4.620693] dump_backtrace+0x0/0x138 [ 4.621028] show_stack+0x24/0x30 [ 4.621336] dump_stack+0xbc/0x104 [ 4.621649] register_lock_class+0x594/0x598 [ 4.622036] __lock_acquire+0x80/0x11b8 [ 4.622384] lock_acquire+0xdc/0x260 [ 4.622711] __mutex_lock+0x90/0x8a0 [ 4.623037] mutex_lock_nested+0x3c/0x50 [ 4.623394] _fusb302_log+0x88/0x1f0 [ 4.623721] fusb302_log+0x7c/0xa0 [ 4.624033] tcpm_init+0x5c/0x190 [ 4.624336] tcpm_init+0x3c/0x130 [ 4.624640] tcpm_register_port+0x574/0x878 [ 4.625019] fusb302_probe+0x2c8/0x590 Despite what the message says, the code isn't fine, as it tries to make use of the fusb302_log facility pretty early. This requires the logbuffer_lock mutex to be initialised, but that only happens much later. Boo. Hoist the fusb302_debugfs_init call before tcpm_register_port so that we can enjoy a working mutex. At Guenter's request, also add teardown of the debugfs facility on the error path. Signed-off-by: Marc Zyngier <marc.zyngier@arm.com> --- drivers/usb/typec/tcpm/fusb302.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)