[PATCH v2] usb: typec: fusb302: Fix debugfs mutex initialisation

[PATCH v2] usb: typec: fusb302: Fix debugfs mutex initialisation

[Marc Zyngier]

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(-)

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

Re: [PATCH v2] usb: typec: fusb302: Fix debugfs mutex initialisation

[Guenter Roeck]

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
> 

Re: [PATCH v2] usb: typec: fusb302: Fix debugfs mutex initialisation

[Heikki Krogerus]

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,

-- 
heikki

Re: [PATCH v2] usb: typec: fusb302: Fix debugfs mutex initialisation

[Marc Zyngier]

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.
-- 
Without deviation from the norm, progress is not possible.

Re: [PATCH v2] usb: typec: fusb302: Fix debugfs mutex initialisation

[Marc Zyngier]

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.
-- 
Without deviation from the norm, progress is not possible.

Re: [PATCH v2] usb: typec: fusb302: Fix debugfs mutex initialisation

[Heikki Krogerus]

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,

-- 
heikki

Re: [PATCH v2] usb: typec: fusb302: Fix debugfs mutex initialisation

[Heikki Krogerus]

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,

-- 
heikki

Re: [PATCH v2] usb: typec: fusb302: Fix debugfs mutex initialisation

[Marc Zyngier]

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.
-- 
Without deviation from the norm, progress is not possible.

Re: [PATCH v2] usb: typec: fusb302: Fix debugfs mutex initialisation

[Heikki Krogerus]

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,

-- 
heikki