| Message ID | 20190813101524.80673-1-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | usb: typec: fusb302: Call fusb302_debugfs_init earlier | expand |
Hi Hans, On Tue, Aug 13, 2019 at 12:15:24PM +0200, Hans de Goede wrote: > tcpm_register_port() will call some of the fusb302 code's callbacks > wich in turn will call fusb302_log(). So we need to call > fusb302_debugfs_init() before we call tcpm_register_port(). > > This fixes the following warning, which was caused by the logbuffer_lock > not yet being initialized (which is done by fusb302_debugfs_init): > > DEBUG_LOCKS_WARN_ON(lock->magic != lock) > WARNING: CPU: 0 PID: 1306 at kernel/locking/mutex.c:912 __mutex_lock+0x978/0x9a0 > Modules linked in: fusb302(+) tcpm pi3usb30532 typec bq24190_charger snd_soc_sst_cht_bsw_rt5645 mei_hdcp dwc3 intel_rapl_msr udc_core ulpi gpio_keys intel_powerclamp coretemp kvm_intel brcmfmac kvm brcmutil joydev cfg80211 wdat_wdt irqbypass pcspkr intel_cstate extcon_intel_cht_wc i2c_cht_wc(E) snd_intel_sst_acpi snd_intel_sst_core snd_soc_rt5645 snd_soc_sst_atom_hifi2_platform snd_soc_acpi_intel_match snd_soc_rl6231 snd_soc_acpi intel_xhci_usb_role_switch roles hci_uart snd_soc_core btqca mei_txe btrtl processor_thermal_device mei snd_hdmi_lpe_audio lpc_ich snd_compress btbcm intel_rapl_common ac97_bus dwc3_pci snd_pcm_dmaengine intel_soc_dts_iosf btintel snd_seq bluetooth snd_seq_device snd_pcm intel_cht_int33fe_musb snd_timer intel_cht_int33fe_typec intel_hid intel_cht_int33fe_common sparse_keymap snd ecdh_generic goodix rfkill soundcore ecc spi_pxa2xx_platform max17042_battery dw_dmac int3406_thermal dptf_power acpi_pad soc_button_array int3400_thermal int3403_thermal > gpd_pocket_fan intel_int0002_vgpio int340x_thermal_zone acpi_thermal_rel dm_crypt mmc_block i915 crct10dif_pclmul crc32_pclmul crc32c_intel ghash_clmulni_intel i2c_algo_bit drm_kms_helper drm video sdhci_acpi sdhci mmc_core pwm_lpss_platform pwm_lpss i2c_dev > CPU: 0 PID: 1306 Comm: systemd-udevd Tainted: G E 5.3.0-rc4+ #83 > Hardware name: Default string Default string/Default string, BIOS 5.11 06/28/2017 > RIP: 0010:__mutex_lock+0x978/0x9a0 > Code: c0 0f 84 26 f7 ff ff 44 8b 05 24 25 c8 00 45 85 c0 0f 85 16 f7 ff ff 48 c7 c6 da 55 2f ae 48 c7 c7 98 8c 2d ae e8 a0 f9 5c ff <0f> 0b e9 fc f6 ff ff 4c 89 f0 4d 89 fe 49 89 c7 e9 cf fa ff ff e8 > RSP: 0018:ffffb7a8c0523800 EFLAGS: 00010286 > RAX: 0000000000000000 RBX: 0000000000000000 RCX: 0000000000000000 > RDX: 0000000000000002 RSI: 0000000000000001 RDI: 0000000000000246 > RBP: ffffb7a8c05238c0 R08: 0000000000000000 R09: 0000000000000000 > R10: ffffb7a8c0523648 R11: 0000000000000030 R12: 0000000000000000 > R13: ffffb7a8c0523990 R14: ffff9bf22f70c028 R15: ffff9bf22f70c360 > FS: 00007f39ca234940(0000) GS:ffff9bf237400000(0000) knlGS:0000000000000000 > CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 > CR2: 00007f1f108481a0 CR3: 0000000271f28000 CR4: 00000000001006f0 > Call Trace: > ? find_held_lock+0x39/0x90 > ? _fusb302_log+0x81/0x1d0 [fusb302] > ? vsnprintf+0x3aa/0x4f0 > ? _fusb302_log+0x81/0x1d0 [fusb302] > _fusb302_log+0x81/0x1d0 [fusb302] > ... > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > --- > drivers/usb/typec/tcpm/fusb302.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/usb/typec/tcpm/fusb302.c b/drivers/usb/typec/tcpm/fusb302.c > index ccfc7e91e7a3..04c76b9d0065 100644 > --- a/drivers/usb/typec/tcpm/fusb302.c > +++ b/drivers/usb/typec/tcpm/fusb302.c > @@ -1759,6 +1759,7 @@ static int fusb302_probe(struct i2c_client *client, > INIT_WORK(&chip->irq_work, fusb302_irq_work); > INIT_DELAYED_WORK(&chip->bc_lvl_handler, fusb302_bc_lvl_handler_work); > init_tcpc_dev(&chip->tcpc_dev); > + fusb302_debugfs_init(chip); > > if (client->irq) { > chip->gpio_int_n_irq = client->irq; > @@ -1784,7 +1785,6 @@ 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); That leaves the rootdir variable pointing to something again for example if a failure happens (like -EPROBE_AGAIN) during probe (the "fusb302" directory is removed, but the rootdir static variable still points to something). Let's just create that rootdir directory during driver init. I don't really understand why should we only create it when/if the first instance of fusb302 is registered. I think something like this would work: diff --git a/drivers/usb/typec/tcpm/fusb302.c b/drivers/usb/typec/tcpm/fusb302.c index c524088246ee..7a950a6e5f0d 100644 --- a/drivers/usb/typec/tcpm/fusb302.c +++ b/drivers/usb/typec/tcpm/fusb302.c @@ -212,9 +212,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); - chip->dentry = debugfs_create_file(dev_name(chip->dev), S_IFREG | 0444, rootdir, chip, &fusb302_debug_fops); @@ -223,7 +220,6 @@ static void fusb302_debugfs_init(struct fusb302_chip *chip) static void fusb302_debugfs_exit(struct fusb302_chip *chip) { debugfs_remove(chip->dentry); - debugfs_remove(rootdir); } #else @@ -1863,7 +1859,24 @@ static struct i2c_driver fusb302_driver = { .remove = fusb302_remove, .id_table = fusb302_i2c_device_id, }; -module_i2c_driver(fusb302_driver); + +static int __init fusb302_init(void) +{ + rootdir = debugfs_create_dir("fusb302", NULL); + if (IS_ERR(rootdir)) + return PTR_ERR(rootdir); + + return i2c_add_driver(&fusb302_driver); +} + +static void __exit fusb302_exit(void) +{ + i2c_del_driver(&fusb302_driver); + debugfs_remove(rootdir); +} + +module_init(fusb302_init); +module_exit(fusb302_exit); MODULE_AUTHOR("Yueyao Zhu <yueyao.zhu@gmail.com>"); MODULE_DESCRIPTION("Fairchild FUSB302 Type-C Chip Driver"); If it's OK, could you include that into this patch? thanks,
Hi Heikki, On 13-08-19 12:52, Heikki Krogerus wrote: > Hi Hans, > > On Tue, Aug 13, 2019 at 12:15:24PM +0200, Hans de Goede wrote: >> tcpm_register_port() will call some of the fusb302 code's callbacks >> wich in turn will call fusb302_log(). So we need to call >> fusb302_debugfs_init() before we call tcpm_register_port(). >> >> This fixes the following warning, which was caused by the logbuffer_lock >> not yet being initialized (which is done by fusb302_debugfs_init): >> >> DEBUG_LOCKS_WARN_ON(lock->magic != lock) >> WARNING: CPU: 0 PID: 1306 at kernel/locking/mutex.c:912 __mutex_lock+0x978/0x9a0 >> Modules linked in: fusb302(+) tcpm pi3usb30532 typec bq24190_charger snd_soc_sst_cht_bsw_rt5645 mei_hdcp dwc3 intel_rapl_msr udc_core ulpi gpio_keys intel_powerclamp coretemp kvm_intel brcmfmac kvm brcmutil joydev cfg80211 wdat_wdt irqbypass pcspkr intel_cstate extcon_intel_cht_wc i2c_cht_wc(E) snd_intel_sst_acpi snd_intel_sst_core snd_soc_rt5645 snd_soc_sst_atom_hifi2_platform snd_soc_acpi_intel_match snd_soc_rl6231 snd_soc_acpi intel_xhci_usb_role_switch roles hci_uart snd_soc_core btqca mei_txe btrtl processor_thermal_device mei snd_hdmi_lpe_audio lpc_ich snd_compress btbcm intel_rapl_common ac97_bus dwc3_pci snd_pcm_dmaengine intel_soc_dts_iosf btintel snd_seq bluetooth snd_seq_device snd_pcm intel_cht_int33fe_musb snd_timer intel_cht_int33fe_typec intel_hid intel_cht_int33fe_common sparse_keymap snd ecdh_generic goodix rfkill soundcore ecc spi_pxa2xx_platform max17042_battery dw_dmac int3406_thermal dptf_power acpi_pad soc_button_array int3400_thermal int3403_thermal >> gpd_pocket_fan intel_int0002_vgpio int340x_thermal_zone acpi_thermal_rel dm_crypt mmc_block i915 crct10dif_pclmul crc32_pclmul crc32c_intel ghash_clmulni_intel i2c_algo_bit drm_kms_helper drm video sdhci_acpi sdhci mmc_core pwm_lpss_platform pwm_lpss i2c_dev >> CPU: 0 PID: 1306 Comm: systemd-udevd Tainted: G E 5.3.0-rc4+ #83 >> Hardware name: Default string Default string/Default string, BIOS 5.11 06/28/2017 >> RIP: 0010:__mutex_lock+0x978/0x9a0 >> Code: c0 0f 84 26 f7 ff ff 44 8b 05 24 25 c8 00 45 85 c0 0f 85 16 f7 ff ff 48 c7 c6 da 55 2f ae 48 c7 c7 98 8c 2d ae e8 a0 f9 5c ff <0f> 0b e9 fc f6 ff ff 4c 89 f0 4d 89 fe 49 89 c7 e9 cf fa ff ff e8 >> RSP: 0018:ffffb7a8c0523800 EFLAGS: 00010286 >> RAX: 0000000000000000 RBX: 0000000000000000 RCX: 0000000000000000 >> RDX: 0000000000000002 RSI: 0000000000000001 RDI: 0000000000000246 >> RBP: ffffb7a8c05238c0 R08: 0000000000000000 R09: 0000000000000000 >> R10: ffffb7a8c0523648 R11: 0000000000000030 R12: 0000000000000000 >> R13: ffffb7a8c0523990 R14: ffff9bf22f70c028 R15: ffff9bf22f70c360 >> FS: 00007f39ca234940(0000) GS:ffff9bf237400000(0000) knlGS:0000000000000000 >> CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 >> CR2: 00007f1f108481a0 CR3: 0000000271f28000 CR4: 00000000001006f0 >> Call Trace: >> ? find_held_lock+0x39/0x90 >> ? _fusb302_log+0x81/0x1d0 [fusb302] >> ? vsnprintf+0x3aa/0x4f0 >> ? _fusb302_log+0x81/0x1d0 [fusb302] >> _fusb302_log+0x81/0x1d0 [fusb302] >> ... >> >> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >> --- >> drivers/usb/typec/tcpm/fusb302.c | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/usb/typec/tcpm/fusb302.c b/drivers/usb/typec/tcpm/fusb302.c >> index ccfc7e91e7a3..04c76b9d0065 100644 >> --- a/drivers/usb/typec/tcpm/fusb302.c >> +++ b/drivers/usb/typec/tcpm/fusb302.c >> @@ -1759,6 +1759,7 @@ static int fusb302_probe(struct i2c_client *client, >> INIT_WORK(&chip->irq_work, fusb302_irq_work); >> INIT_DELAYED_WORK(&chip->bc_lvl_handler, fusb302_bc_lvl_handler_work); >> init_tcpc_dev(&chip->tcpc_dev); >> + fusb302_debugfs_init(chip); >> >> if (client->irq) { >> chip->gpio_int_n_irq = client->irq; >> @@ -1784,7 +1785,6 @@ 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); > > That leaves the rootdir variable pointing to something again for > example if a failure happens (like -EPROBE_AGAIN) during probe (the > "fusb302" directory is removed, but the rootdir static variable still > points to something). > > Let's just create that rootdir directory during driver init. I don't > really understand why should we only create it when/if the first > instance of fusb302 is registered. I think something like this would > work: > > diff --git a/drivers/usb/typec/tcpm/fusb302.c b/drivers/usb/typec/tcpm/fusb302.c > index c524088246ee..7a950a6e5f0d 100644 > --- a/drivers/usb/typec/tcpm/fusb302.c > +++ b/drivers/usb/typec/tcpm/fusb302.c > @@ -212,9 +212,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); > - > chip->dentry = debugfs_create_file(dev_name(chip->dev), > S_IFREG | 0444, rootdir, > chip, &fusb302_debug_fops); > @@ -223,7 +220,6 @@ static void fusb302_debugfs_init(struct fusb302_chip *chip) > static void fusb302_debugfs_exit(struct fusb302_chip *chip) > { > debugfs_remove(chip->dentry); > - debugfs_remove(rootdir); > } > > #else > @@ -1863,7 +1859,24 @@ static struct i2c_driver fusb302_driver = { > .remove = fusb302_remove, > .id_table = fusb302_i2c_device_id, > }; > -module_i2c_driver(fusb302_driver); > + > +static int __init fusb302_init(void) > +{ > + rootdir = debugfs_create_dir("fusb302", NULL); > + if (IS_ERR(rootdir)) > + return PTR_ERR(rootdir); > + > + return i2c_add_driver(&fusb302_driver); > +} > + > +static void __exit fusb302_exit(void) > +{ > + i2c_del_driver(&fusb302_driver); > + debugfs_remove(rootdir); > +} > + > +module_init(fusb302_init); > +module_exit(fusb302_exit); > > MODULE_AUTHOR("Yueyao Zhu <yueyao.zhu@gmail.com>"); > MODULE_DESCRIPTION("Fairchild FUSB302 Type-C Chip Driver"); > > > If it's OK, could you include that into this patch? I agree that we should do something about the rootdir leakage on rmmod; and your patch seems like a good solution; and I can test your patch if you want. But the rootdir leakage is a pre-existing problem, even without my patch to call fusb302_debugfs_init earlier, we would still leak it after a rmmod, assuming probe() has succeeded at least once. As for -EPROBE_DEFER, then eventually probe should succeed and the "if (!rootdir)" in: if (!rootdir) rootdir = debugfs_create_dir("fusb302", NULL); Ensures that we only do it once, so the only change to the rootdir leak my patch makes is that it gets leaked on rmmod even if probe() never succeeded. As said I agree that we should do something about it; and your suggestion seems like a good solution, but it seems like an almost orthogonal problem, so IMHO this should be fixed in a separate patch, not squashed into the "Call fusb302_debugfs_init earlier" patch. Do you want me to turn your POC code into a proper patch with you as author and your S-o-b added (with your permission) and that I then test it and add my Tested-by? Regards, Hans
On Tue, Aug 13, 2019 at 02:13:45PM +0200, Hans de Goede wrote: > Hi Heikki, > > On 13-08-19 12:52, Heikki Krogerus wrote: > > Hi Hans, > > > > On Tue, Aug 13, 2019 at 12:15:24PM +0200, Hans de Goede wrote: > > > tcpm_register_port() will call some of the fusb302 code's callbacks > > > wich in turn will call fusb302_log(). So we need to call > > > fusb302_debugfs_init() before we call tcpm_register_port(). > > > > > > This fixes the following warning, which was caused by the logbuffer_lock > > > not yet being initialized (which is done by fusb302_debugfs_init): > > > > > > DEBUG_LOCKS_WARN_ON(lock->magic != lock) > > > WARNING: CPU: 0 PID: 1306 at kernel/locking/mutex.c:912 __mutex_lock+0x978/0x9a0 > > > Modules linked in: fusb302(+) tcpm pi3usb30532 typec bq24190_charger snd_soc_sst_cht_bsw_rt5645 mei_hdcp dwc3 intel_rapl_msr udc_core ulpi gpio_keys intel_powerclamp coretemp kvm_intel brcmfmac kvm brcmutil joydev cfg80211 wdat_wdt irqbypass pcspkr intel_cstate extcon_intel_cht_wc i2c_cht_wc(E) snd_intel_sst_acpi snd_intel_sst_core snd_soc_rt5645 snd_soc_sst_atom_hifi2_platform snd_soc_acpi_intel_match snd_soc_rl6231 snd_soc_acpi intel_xhci_usb_role_switch roles hci_uart snd_soc_core btqca mei_txe btrtl processor_thermal_device mei snd_hdmi_lpe_audio lpc_ich snd_compress btbcm intel_rapl_common ac97_bus dwc3_pci snd_pcm_dmaengine intel_soc_dts_iosf btintel snd_seq bluetooth snd_seq_device snd_pcm intel_cht_int33fe_musb snd_timer intel_cht_int33fe_typec intel_hid intel_cht_int33fe_common sparse_keymap snd ecdh_generic goodix rfkill soundcore ecc spi_pxa2xx_platform max17042_battery dw_dmac int3406_thermal dptf_power acpi_pad soc_button_array int3400_thermal int3403_the rmal > > > gpd_pocket_fan intel_int0002_vgpio int340x_thermal_zone acpi_thermal_rel dm_crypt mmc_block i915 crct10dif_pclmul crc32_pclmul crc32c_intel ghash_clmulni_intel i2c_algo_bit drm_kms_helper drm video sdhci_acpi sdhci mmc_core pwm_lpss_platform pwm_lpss i2c_dev > > > CPU: 0 PID: 1306 Comm: systemd-udevd Tainted: G E 5.3.0-rc4+ #83 > > > Hardware name: Default string Default string/Default string, BIOS 5.11 06/28/2017 > > > RIP: 0010:__mutex_lock+0x978/0x9a0 > > > Code: c0 0f 84 26 f7 ff ff 44 8b 05 24 25 c8 00 45 85 c0 0f 85 16 f7 ff ff 48 c7 c6 da 55 2f ae 48 c7 c7 98 8c 2d ae e8 a0 f9 5c ff <0f> 0b e9 fc f6 ff ff 4c 89 f0 4d 89 fe 49 89 c7 e9 cf fa ff ff e8 > > > RSP: 0018:ffffb7a8c0523800 EFLAGS: 00010286 > > > RAX: 0000000000000000 RBX: 0000000000000000 RCX: 0000000000000000 > > > RDX: 0000000000000002 RSI: 0000000000000001 RDI: 0000000000000246 > > > RBP: ffffb7a8c05238c0 R08: 0000000000000000 R09: 0000000000000000 > > > R10: ffffb7a8c0523648 R11: 0000000000000030 R12: 0000000000000000 > > > R13: ffffb7a8c0523990 R14: ffff9bf22f70c028 R15: ffff9bf22f70c360 > > > FS: 00007f39ca234940(0000) GS:ffff9bf237400000(0000) knlGS:0000000000000000 > > > CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 > > > CR2: 00007f1f108481a0 CR3: 0000000271f28000 CR4: 00000000001006f0 > > > Call Trace: > > > ? find_held_lock+0x39/0x90 > > > ? _fusb302_log+0x81/0x1d0 [fusb302] > > > ? vsnprintf+0x3aa/0x4f0 > > > ? _fusb302_log+0x81/0x1d0 [fusb302] > > > _fusb302_log+0x81/0x1d0 [fusb302] > > > ... > > > > > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > > > --- > > > drivers/usb/typec/tcpm/fusb302.c | 3 ++- > > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/usb/typec/tcpm/fusb302.c b/drivers/usb/typec/tcpm/fusb302.c > > > index ccfc7e91e7a3..04c76b9d0065 100644 > > > --- a/drivers/usb/typec/tcpm/fusb302.c > > > +++ b/drivers/usb/typec/tcpm/fusb302.c > > > @@ -1759,6 +1759,7 @@ static int fusb302_probe(struct i2c_client *client, > > > INIT_WORK(&chip->irq_work, fusb302_irq_work); > > > INIT_DELAYED_WORK(&chip->bc_lvl_handler, fusb302_bc_lvl_handler_work); > > > init_tcpc_dev(&chip->tcpc_dev); > > > + fusb302_debugfs_init(chip); > > > if (client->irq) { > > > chip->gpio_int_n_irq = client->irq; > > > @@ -1784,7 +1785,6 @@ 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); > > > > That leaves the rootdir variable pointing to something again for > > example if a failure happens (like -EPROBE_AGAIN) during probe (the > > "fusb302" directory is removed, but the rootdir static variable still > > points to something). > > > > Let's just create that rootdir directory during driver init. I don't > > really understand why should we only create it when/if the first > > instance of fusb302 is registered. I think something like this would > > work: > > > > diff --git a/drivers/usb/typec/tcpm/fusb302.c b/drivers/usb/typec/tcpm/fusb302.c > > index c524088246ee..7a950a6e5f0d 100644 > > --- a/drivers/usb/typec/tcpm/fusb302.c > > +++ b/drivers/usb/typec/tcpm/fusb302.c > > @@ -212,9 +212,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); > > - > > chip->dentry = debugfs_create_file(dev_name(chip->dev), > > S_IFREG | 0444, rootdir, > > chip, &fusb302_debug_fops); > > @@ -223,7 +220,6 @@ static void fusb302_debugfs_init(struct fusb302_chip *chip) > > static void fusb302_debugfs_exit(struct fusb302_chip *chip) > > { > > debugfs_remove(chip->dentry); > > - debugfs_remove(rootdir); > > } > > #else > > @@ -1863,7 +1859,24 @@ static struct i2c_driver fusb302_driver = { > > .remove = fusb302_remove, > > .id_table = fusb302_i2c_device_id, > > }; > > -module_i2c_driver(fusb302_driver); > > + > > +static int __init fusb302_init(void) > > +{ > > + rootdir = debugfs_create_dir("fusb302", NULL); > > + if (IS_ERR(rootdir)) > > + return PTR_ERR(rootdir); > > + > > + return i2c_add_driver(&fusb302_driver); > > +} > > + > > +static void __exit fusb302_exit(void) > > +{ > > + i2c_del_driver(&fusb302_driver); > > + debugfs_remove(rootdir); > > +} > > + > > +module_init(fusb302_init); > > +module_exit(fusb302_exit); > > MODULE_AUTHOR("Yueyao Zhu <yueyao.zhu@gmail.com>"); > > MODULE_DESCRIPTION("Fairchild FUSB302 Type-C Chip Driver"); > > > > > > If it's OK, could you include that into this patch? > > I agree that we should do something about the rootdir leakage > on rmmod; and your patch seems like a good solution; and I can > test your patch if you want. > > But the rootdir leakage is a pre-existing problem, even without > my patch to call fusb302_debugfs_init earlier, we would still leak > it after a rmmod, assuming probe() has succeeded at least once. > > As for -EPROBE_DEFER, then eventually probe should succeed and > the "if (!rootdir)" in: > > if (!rootdir) > rootdir = debugfs_create_dir("fusb302", NULL); > > Ensures that we only do it once, so the only change to the rootdir > leak my patch makes is that it gets leaked on rmmod even if > probe() never succeeded. > > As said I agree that we should do something about it; and your > suggestion seems like a good solution, but it seems like an > almost orthogonal problem, so IMHO this should be fixed in a > separate patch, not squashed into the > "Call fusb302_debugfs_init earlier" patch. > > Do you want me to turn your POC code into a proper patch with > you as author and your S-o-b added (with your permission) and > that I then test it and add my Tested-by? If you could handle the patch, I would much appreciate, but please don't make me the author of the patch (and use my SoB) in that case. Just use Suggested-by tag. thanks,
On 8/13/19 3:52 AM, Heikki Krogerus wrote: > Hi Hans, > > On Tue, Aug 13, 2019 at 12:15:24PM +0200, Hans de Goede wrote: >> tcpm_register_port() will call some of the fusb302 code's callbacks >> wich in turn will call fusb302_log(). So we need to call >> fusb302_debugfs_init() before we call tcpm_register_port(). >> >> This fixes the following warning, which was caused by the logbuffer_lock >> not yet being initialized (which is done by fusb302_debugfs_init): >> >> DEBUG_LOCKS_WARN_ON(lock->magic != lock) >> WARNING: CPU: 0 PID: 1306 at kernel/locking/mutex.c:912 __mutex_lock+0x978/0x9a0 >> Modules linked in: fusb302(+) tcpm pi3usb30532 typec bq24190_charger snd_soc_sst_cht_bsw_rt5645 mei_hdcp dwc3 intel_rapl_msr udc_core ulpi gpio_keys intel_powerclamp coretemp kvm_intel brcmfmac kvm brcmutil joydev cfg80211 wdat_wdt irqbypass pcspkr intel_cstate extcon_intel_cht_wc i2c_cht_wc(E) snd_intel_sst_acpi snd_intel_sst_core snd_soc_rt5645 snd_soc_sst_atom_hifi2_platform snd_soc_acpi_intel_match snd_soc_rl6231 snd_soc_acpi intel_xhci_usb_role_switch roles hci_uart snd_soc_core btqca mei_txe btrtl processor_thermal_device mei snd_hdmi_lpe_audio lpc_ich snd_compress btbcm intel_rapl_common ac97_bus dwc3_pci snd_pcm_dmaengine intel_soc_dts_iosf btintel snd_seq bluetooth snd_seq_device snd_pcm intel_cht_int33fe_musb snd_timer intel_cht_int33fe_typec intel_hid intel_cht_int33fe_common sparse_keymap snd ecdh_generic goodix rfkill soundcore ecc spi_pxa2xx_platform max17042_battery dw_dmac int3406_thermal dptf_power acpi_pad soc_button_array int3400_thermal int3403_thermal >> gpd_pocket_fan intel_int0002_vgpio int340x_thermal_zone acpi_thermal_rel dm_crypt mmc_block i915 crct10dif_pclmul crc32_pclmul crc32c_intel ghash_clmulni_intel i2c_algo_bit drm_kms_helper drm video sdhci_acpi sdhci mmc_core pwm_lpss_platform pwm_lpss i2c_dev >> CPU: 0 PID: 1306 Comm: systemd-udevd Tainted: G E 5.3.0-rc4+ #83 >> Hardware name: Default string Default string/Default string, BIOS 5.11 06/28/2017 >> RIP: 0010:__mutex_lock+0x978/0x9a0 >> Code: c0 0f 84 26 f7 ff ff 44 8b 05 24 25 c8 00 45 85 c0 0f 85 16 f7 ff ff 48 c7 c6 da 55 2f ae 48 c7 c7 98 8c 2d ae e8 a0 f9 5c ff <0f> 0b e9 fc f6 ff ff 4c 89 f0 4d 89 fe 49 89 c7 e9 cf fa ff ff e8 >> RSP: 0018:ffffb7a8c0523800 EFLAGS: 00010286 >> RAX: 0000000000000000 RBX: 0000000000000000 RCX: 0000000000000000 >> RDX: 0000000000000002 RSI: 0000000000000001 RDI: 0000000000000246 >> RBP: ffffb7a8c05238c0 R08: 0000000000000000 R09: 0000000000000000 >> R10: ffffb7a8c0523648 R11: 0000000000000030 R12: 0000000000000000 >> R13: ffffb7a8c0523990 R14: ffff9bf22f70c028 R15: ffff9bf22f70c360 >> FS: 00007f39ca234940(0000) GS:ffff9bf237400000(0000) knlGS:0000000000000000 >> CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 >> CR2: 00007f1f108481a0 CR3: 0000000271f28000 CR4: 00000000001006f0 >> Call Trace: >> ? find_held_lock+0x39/0x90 >> ? _fusb302_log+0x81/0x1d0 [fusb302] >> ? vsnprintf+0x3aa/0x4f0 >> ? _fusb302_log+0x81/0x1d0 [fusb302] >> _fusb302_log+0x81/0x1d0 [fusb302] >> ... >> >> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >> --- >> drivers/usb/typec/tcpm/fusb302.c | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/usb/typec/tcpm/fusb302.c b/drivers/usb/typec/tcpm/fusb302.c >> index ccfc7e91e7a3..04c76b9d0065 100644 >> --- a/drivers/usb/typec/tcpm/fusb302.c >> +++ b/drivers/usb/typec/tcpm/fusb302.c >> @@ -1759,6 +1759,7 @@ static int fusb302_probe(struct i2c_client *client, >> INIT_WORK(&chip->irq_work, fusb302_irq_work); >> INIT_DELAYED_WORK(&chip->bc_lvl_handler, fusb302_bc_lvl_handler_work); >> init_tcpc_dev(&chip->tcpc_dev); >> + fusb302_debugfs_init(chip); >> >> if (client->irq) { >> chip->gpio_int_n_irq = client->irq; >> @@ -1784,7 +1785,6 @@ 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); > > That leaves the rootdir variable pointing to something again for > example if a failure happens (like -EPROBE_AGAIN) during probe (the > "fusb302" directory is removed, but the rootdir static variable still > points to something). > > Let's just create that rootdir directory during driver init. I don't > really understand why should we only create it when/if the first > instance of fusb302 is registered. I think something like this would > work: > > diff --git a/drivers/usb/typec/tcpm/fusb302.c b/drivers/usb/typec/tcpm/fusb302.c > index c524088246ee..7a950a6e5f0d 100644 > --- a/drivers/usb/typec/tcpm/fusb302.c > +++ b/drivers/usb/typec/tcpm/fusb302.c > @@ -212,9 +212,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); > - > chip->dentry = debugfs_create_file(dev_name(chip->dev), > S_IFREG | 0444, rootdir, > chip, &fusb302_debug_fops); > @@ -223,7 +220,6 @@ static void fusb302_debugfs_init(struct fusb302_chip *chip) > static void fusb302_debugfs_exit(struct fusb302_chip *chip) > { > debugfs_remove(chip->dentry); > - debugfs_remove(rootdir); > } > > #else > @@ -1863,7 +1859,24 @@ static struct i2c_driver fusb302_driver = { > .remove = fusb302_remove, > .id_table = fusb302_i2c_device_id, > }; > -module_i2c_driver(fusb302_driver); > + > +static int __init fusb302_init(void) > +{ > + rootdir = debugfs_create_dir("fusb302", NULL); > + if (IS_ERR(rootdir)) > + return PTR_ERR(rootdir); GregKH keeps saying that errors returned from debugfs should be ignored. That doesn't even require a check in the remove function because debugfs_remove() has an IS_ERR_OR_NULL() check. Guenter > + > + return i2c_add_driver(&fusb302_driver); > +} > + > +static void __exit fusb302_exit(void) > +{ > + i2c_del_driver(&fusb302_driver); > + debugfs_remove(rootdir); > +} > + > +module_init(fusb302_init); > +module_exit(fusb302_exit); > > MODULE_AUTHOR("Yueyao Zhu <yueyao.zhu@gmail.com>"); > MODULE_DESCRIPTION("Fairchild FUSB302 Type-C Chip Driver"); > > > If it's OK, could you include that into this patch? > > thanks, >
On Tue, 2019-08-13 at 13:52 +0300, Heikki Krogerus wrote: > Hi Hans, > > On Tue, Aug 13, 2019 at 12:15:24PM +0200, Hans de Goede wrote: > > tcpm_register_port() will call some of the fusb302 code's callbacks > > wich in turn will call fusb302_log(). So we need to call > > fusb302_debugfs_init() before we call tcpm_register_port(). <...> > > > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > > --- > > drivers/usb/typec/tcpm/fusb302.c | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/usb/typec/tcpm/fusb302.c b/drivers/usb/typec/tcpm/fusb302.c > > index ccfc7e91e7a3..04c76b9d0065 100644 > > --- a/drivers/usb/typec/tcpm/fusb302.c > > +++ b/drivers/usb/typec/tcpm/fusb302.c > > @@ -1759,6 +1759,7 @@ static int fusb302_probe(struct i2c_client *client, > > INIT_WORK(&chip->irq_work, fusb302_irq_work); > > INIT_DELAYED_WORK(&chip->bc_lvl_handler, fusb302_bc_lvl_handler_work); > > init_tcpc_dev(&chip->tcpc_dev); > > + fusb302_debugfs_init(chip); > > > > if (client->irq) { > > chip->gpio_int_n_irq = client->irq; > > @@ -1784,7 +1785,6 @@ 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); > > That leaves the rootdir variable pointing to something again for > example if a failure happens (like -EPROBE_AGAIN) during probe (the > "fusb302" directory is removed, but the rootdir static variable still > points to something). > > Let's just create that rootdir directory during driver init. I don't > really understand why should we only create it when/if the first > instance of fusb302 is registered. I think something like this would > work: > > diff --git a/drivers/usb/typec/tcpm/fusb302.c b/drivers/usb/typec/tcpm/fusb302.c > index c524088246ee..7a950a6e5f0d 100644 > --- a/drivers/usb/typec/tcpm/fusb302.c > +++ b/drivers/usb/typec/tcpm/fusb302.c > @@ -212,9 +212,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); > - > chip->dentry = debugfs_create_file(dev_name(chip->dev), > S_IFREG | 0444, rootdir, > chip, &fusb302_debug_fops); > @@ -223,7 +220,6 @@ static void fusb302_debugfs_init(struct fusb302_chip *chip) > static void fusb302_debugfs_exit(struct fusb302_chip *chip) > { > debugfs_remove(chip->dentry); > - debugfs_remove(rootdir); > } > > #else > @@ -1863,7 +1859,24 @@ static struct i2c_driver fusb302_driver = { > .remove = fusb302_remove, > .id_table = fusb302_i2c_device_id, > }; > -module_i2c_driver(fusb302_driver); > + > +static int __init fusb302_init(void) > +{ > + rootdir = debugfs_create_dir("fusb302", NULL); > + if (IS_ERR(rootdir)) > + return PTR_ERR(rootdir); This doesn't support multi-instance? > + > + return i2c_add_driver(&fusb302_driver); > +} > + > +static void __exit fusb302_exit(void) > +{ > + i2c_del_driver(&fusb302_driver); > + debugfs_remove(rootdir); > +} > + > +module_init(fusb302_init); > +module_exit(fusb302_exit); > > MODULE_AUTHOR("Yueyao Zhu <yueyao.zhu@gmail.com>"); > MODULE_DESCRIPTION("Fairchild FUSB302 Type-C Chip Driver"); > > > If it's OK, could you include that into this patch? > > thanks, >
On Wed, Aug 14, 2019 at 10:17:14AM +0800, Chunfeng Yun wrote: > On Tue, 2019-08-13 at 13:52 +0300, Heikki Krogerus wrote: > > Hi Hans, > > > > On Tue, Aug 13, 2019 at 12:15:24PM +0200, Hans de Goede wrote: > > > tcpm_register_port() will call some of the fusb302 code's callbacks > > > wich in turn will call fusb302_log(). So we need to call > > > fusb302_debugfs_init() before we call tcpm_register_port(). > <...> > > > > > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > > > --- > > > drivers/usb/typec/tcpm/fusb302.c | 3 ++- > > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/usb/typec/tcpm/fusb302.c b/drivers/usb/typec/tcpm/fusb302.c > > > index ccfc7e91e7a3..04c76b9d0065 100644 > > > --- a/drivers/usb/typec/tcpm/fusb302.c > > > +++ b/drivers/usb/typec/tcpm/fusb302.c > > > @@ -1759,6 +1759,7 @@ static int fusb302_probe(struct i2c_client *client, > > > INIT_WORK(&chip->irq_work, fusb302_irq_work); > > > INIT_DELAYED_WORK(&chip->bc_lvl_handler, fusb302_bc_lvl_handler_work); > > > init_tcpc_dev(&chip->tcpc_dev); > > > + fusb302_debugfs_init(chip); > > > > > > if (client->irq) { > > > chip->gpio_int_n_irq = client->irq; > > > @@ -1784,7 +1785,6 @@ 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); > > > > That leaves the rootdir variable pointing to something again for > > example if a failure happens (like -EPROBE_AGAIN) during probe (the > > "fusb302" directory is removed, but the rootdir static variable still > > points to something). > > > > Let's just create that rootdir directory during driver init. I don't > > really understand why should we only create it when/if the first > > instance of fusb302 is registered. I think something like this would > > work: > > > > diff --git a/drivers/usb/typec/tcpm/fusb302.c b/drivers/usb/typec/tcpm/fusb302.c > > index c524088246ee..7a950a6e5f0d 100644 > > --- a/drivers/usb/typec/tcpm/fusb302.c > > +++ b/drivers/usb/typec/tcpm/fusb302.c > > @@ -212,9 +212,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); > > - > > chip->dentry = debugfs_create_file(dev_name(chip->dev), > > S_IFREG | 0444, rootdir, > > chip, &fusb302_debug_fops); > > @@ -223,7 +220,6 @@ static void fusb302_debugfs_init(struct fusb302_chip *chip) > > static void fusb302_debugfs_exit(struct fusb302_chip *chip) > > { > > debugfs_remove(chip->dentry); > > - debugfs_remove(rootdir); > > } > > > > #else > > @@ -1863,7 +1859,24 @@ static struct i2c_driver fusb302_driver = { > > .remove = fusb302_remove, > > .id_table = fusb302_i2c_device_id, > > }; > > -module_i2c_driver(fusb302_driver); > > + > > +static int __init fusb302_init(void) > > +{ > > + rootdir = debugfs_create_dir("fusb302", NULL); > > + if (IS_ERR(rootdir)) > > + return PTR_ERR(rootdir); > This doesn't support multi-instance? Yes it does. That only creates the root directory "fusb302". For every instance of fusb302 on the system that is registered and probed by the driver, you will still have a separate file added to that directory, just like before. The only difference is that now we don't wait for the first instance of fusb302 to be registered before we create that "fusb302" directory. Instead, the directory is simply created the moment the driver is loaded. On a system that does not have fusb302 controllers, the directory will now stay empty, but that is not a problem. thanks,
On Tue, Aug 13, 2019 at 01:52:16PM +0300, Heikki Krogerus wrote: > Hi Hans, > > On Tue, Aug 13, 2019 at 12:15:24PM +0200, Hans de Goede wrote: > > tcpm_register_port() will call some of the fusb302 code's callbacks > > wich in turn will call fusb302_log(). So we need to call > > fusb302_debugfs_init() before we call tcpm_register_port(). > > > > This fixes the following warning, which was caused by the logbuffer_lock > > not yet being initialized (which is done by fusb302_debugfs_init): > > > > DEBUG_LOCKS_WARN_ON(lock->magic != lock) > > WARNING: CPU: 0 PID: 1306 at kernel/locking/mutex.c:912 __mutex_lock+0x978/0x9a0 > > Modules linked in: fusb302(+) tcpm pi3usb30532 typec bq24190_charger snd_soc_sst_cht_bsw_rt5645 mei_hdcp dwc3 intel_rapl_msr udc_core ulpi gpio_keys intel_powerclamp coretemp kvm_intel brcmfmac kvm brcmutil joydev cfg80211 wdat_wdt irqbypass pcspkr intel_cstate extcon_intel_cht_wc i2c_cht_wc(E) snd_intel_sst_acpi snd_intel_sst_core snd_soc_rt5645 snd_soc_sst_atom_hifi2_platform snd_soc_acpi_intel_match snd_soc_rl6231 snd_soc_acpi intel_xhci_usb_role_switch roles hci_uart snd_soc_core btqca mei_txe btrtl processor_thermal_device mei snd_hdmi_lpe_audio lpc_ich snd_compress btbcm intel_rapl_common ac97_bus dwc3_pci snd_pcm_dmaengine intel_soc_dts_iosf btintel snd_seq bluetooth snd_seq_device snd_pcm intel_cht_int33fe_musb snd_timer intel_cht_int33fe_typec intel_hid intel_cht_int33fe_common sparse_keymap snd ecdh_generic goodix rfkill soundcore ecc spi_pxa2xx_platform max17042_battery dw_dmac int3406_thermal dptf_power acpi_pad soc_button_array int3400_thermal int3403_thermal > > gpd_pocket_fan intel_int0002_vgpio int340x_thermal_zone acpi_thermal_rel dm_crypt mmc_block i915 crct10dif_pclmul crc32_pclmul crc32c_intel ghash_clmulni_intel i2c_algo_bit drm_kms_helper drm video sdhci_acpi sdhci mmc_core pwm_lpss_platform pwm_lpss i2c_dev > > CPU: 0 PID: 1306 Comm: systemd-udevd Tainted: G E 5.3.0-rc4+ #83 > > Hardware name: Default string Default string/Default string, BIOS 5.11 06/28/2017 > > RIP: 0010:__mutex_lock+0x978/0x9a0 > > Code: c0 0f 84 26 f7 ff ff 44 8b 05 24 25 c8 00 45 85 c0 0f 85 16 f7 ff ff 48 c7 c6 da 55 2f ae 48 c7 c7 98 8c 2d ae e8 a0 f9 5c ff <0f> 0b e9 fc f6 ff ff 4c 89 f0 4d 89 fe 49 89 c7 e9 cf fa ff ff e8 > > RSP: 0018:ffffb7a8c0523800 EFLAGS: 00010286 > > RAX: 0000000000000000 RBX: 0000000000000000 RCX: 0000000000000000 > > RDX: 0000000000000002 RSI: 0000000000000001 RDI: 0000000000000246 > > RBP: ffffb7a8c05238c0 R08: 0000000000000000 R09: 0000000000000000 > > R10: ffffb7a8c0523648 R11: 0000000000000030 R12: 0000000000000000 > > R13: ffffb7a8c0523990 R14: ffff9bf22f70c028 R15: ffff9bf22f70c360 > > FS: 00007f39ca234940(0000) GS:ffff9bf237400000(0000) knlGS:0000000000000000 > > CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 > > CR2: 00007f1f108481a0 CR3: 0000000271f28000 CR4: 00000000001006f0 > > Call Trace: > > ? find_held_lock+0x39/0x90 > > ? _fusb302_log+0x81/0x1d0 [fusb302] > > ? vsnprintf+0x3aa/0x4f0 > > ? _fusb302_log+0x81/0x1d0 [fusb302] > > _fusb302_log+0x81/0x1d0 [fusb302] > > ... > > > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > > --- > > drivers/usb/typec/tcpm/fusb302.c | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/usb/typec/tcpm/fusb302.c b/drivers/usb/typec/tcpm/fusb302.c > > index ccfc7e91e7a3..04c76b9d0065 100644 > > --- a/drivers/usb/typec/tcpm/fusb302.c > > +++ b/drivers/usb/typec/tcpm/fusb302.c > > @@ -1759,6 +1759,7 @@ static int fusb302_probe(struct i2c_client *client, > > INIT_WORK(&chip->irq_work, fusb302_irq_work); > > INIT_DELAYED_WORK(&chip->bc_lvl_handler, fusb302_bc_lvl_handler_work); > > init_tcpc_dev(&chip->tcpc_dev); > > + fusb302_debugfs_init(chip); > > > > if (client->irq) { > > chip->gpio_int_n_irq = client->irq; > > @@ -1784,7 +1785,6 @@ 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); > > That leaves the rootdir variable pointing to something again for > example if a failure happens (like -EPROBE_AGAIN) during probe (the > "fusb302" directory is removed, but the rootdir static variable still > points to something). > > Let's just create that rootdir directory during driver init. I don't > really understand why should we only create it when/if the first > instance of fusb302 is registered. I think something like this would > work: > > diff --git a/drivers/usb/typec/tcpm/fusb302.c b/drivers/usb/typec/tcpm/fusb302.c > index c524088246ee..7a950a6e5f0d 100644 > --- a/drivers/usb/typec/tcpm/fusb302.c > +++ b/drivers/usb/typec/tcpm/fusb302.c > @@ -212,9 +212,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); > - > chip->dentry = debugfs_create_file(dev_name(chip->dev), > S_IFREG | 0444, rootdir, > chip, &fusb302_debug_fops); > @@ -223,7 +220,6 @@ static void fusb302_debugfs_init(struct fusb302_chip *chip) > static void fusb302_debugfs_exit(struct fusb302_chip *chip) > { > debugfs_remove(chip->dentry); > - debugfs_remove(rootdir); > } > > #else > @@ -1863,7 +1859,24 @@ static struct i2c_driver fusb302_driver = { > .remove = fusb302_remove, > .id_table = fusb302_i2c_device_id, > }; > -module_i2c_driver(fusb302_driver); > + > +static int __init fusb302_init(void) > +{ > + rootdir = debugfs_create_dir("fusb302", NULL); > + if (IS_ERR(rootdir)) > + return PTR_ERR(rootdir); As Guenter points out, don't check this, just call it anb move on. But are you _SURE_ you want this to be the name, at the root of debugfs? Why not put it under the usb debugfs directory? thanks, greg k-h
On Thu, Aug 15, 2019 at 02:54:41PM +0200, Greg Kroah-Hartman wrote: > On Tue, Aug 13, 2019 at 01:52:16PM +0300, Heikki Krogerus wrote: > > Hi Hans, > > > > On Tue, Aug 13, 2019 at 12:15:24PM +0200, Hans de Goede wrote: > > > tcpm_register_port() will call some of the fusb302 code's callbacks > > > wich in turn will call fusb302_log(). So we need to call > > > fusb302_debugfs_init() before we call tcpm_register_port(). > > > > > > This fixes the following warning, which was caused by the logbuffer_lock > > > not yet being initialized (which is done by fusb302_debugfs_init): > > > > > > DEBUG_LOCKS_WARN_ON(lock->magic != lock) > > > WARNING: CPU: 0 PID: 1306 at kernel/locking/mutex.c:912 __mutex_lock+0x978/0x9a0 > > > Modules linked in: fusb302(+) tcpm pi3usb30532 typec bq24190_charger snd_soc_sst_cht_bsw_rt5645 mei_hdcp dwc3 intel_rapl_msr udc_core ulpi gpio_keys intel_powerclamp coretemp kvm_intel brcmfmac kvm brcmutil joydev cfg80211 wdat_wdt irqbypass pcspkr intel_cstate extcon_intel_cht_wc i2c_cht_wc(E) snd_intel_sst_acpi snd_intel_sst_core snd_soc_rt5645 snd_soc_sst_atom_hifi2_platform snd_soc_acpi_intel_match snd_soc_rl6231 snd_soc_acpi intel_xhci_usb_role_switch roles hci_uart snd_soc_core btqca mei_txe btrtl processor_thermal_device mei snd_hdmi_lpe_audio lpc_ich snd_compress btbcm intel_rapl_common ac97_bus dwc3_pci snd_pcm_dmaengine intel_soc_dts_iosf btintel snd_seq bluetooth snd_seq_device snd_pcm intel_cht_int33fe_musb snd_timer intel_cht_int33fe_typec intel_hid intel_cht_int33fe_common sparse_keymap snd ecdh_generic goodix rfkill soundcore ecc spi_pxa2xx_platform max17042_battery dw_dmac int3406_thermal dptf_power acpi_pad soc_button_array int3400_thermal int3403_ther mal > > > gpd_pocket_fan intel_int0002_vgpio int340x_thermal_zone acpi_thermal_rel dm_crypt mmc_block i915 crct10dif_pclmul crc32_pclmul crc32c_intel ghash_clmulni_intel i2c_algo_bit drm_kms_helper drm video sdhci_acpi sdhci mmc_core pwm_lpss_platform pwm_lpss i2c_dev > > > CPU: 0 PID: 1306 Comm: systemd-udevd Tainted: G E 5.3.0-rc4+ #83 > > > Hardware name: Default string Default string/Default string, BIOS 5.11 06/28/2017 > > > RIP: 0010:__mutex_lock+0x978/0x9a0 > > > Code: c0 0f 84 26 f7 ff ff 44 8b 05 24 25 c8 00 45 85 c0 0f 85 16 f7 ff ff 48 c7 c6 da 55 2f ae 48 c7 c7 98 8c 2d ae e8 a0 f9 5c ff <0f> 0b e9 fc f6 ff ff 4c 89 f0 4d 89 fe 49 89 c7 e9 cf fa ff ff e8 > > > RSP: 0018:ffffb7a8c0523800 EFLAGS: 00010286 > > > RAX: 0000000000000000 RBX: 0000000000000000 RCX: 0000000000000000 > > > RDX: 0000000000000002 RSI: 0000000000000001 RDI: 0000000000000246 > > > RBP: ffffb7a8c05238c0 R08: 0000000000000000 R09: 0000000000000000 > > > R10: ffffb7a8c0523648 R11: 0000000000000030 R12: 0000000000000000 > > > R13: ffffb7a8c0523990 R14: ffff9bf22f70c028 R15: ffff9bf22f70c360 > > > FS: 00007f39ca234940(0000) GS:ffff9bf237400000(0000) knlGS:0000000000000000 > > > CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 > > > CR2: 00007f1f108481a0 CR3: 0000000271f28000 CR4: 00000000001006f0 > > > Call Trace: > > > ? find_held_lock+0x39/0x90 > > > ? _fusb302_log+0x81/0x1d0 [fusb302] > > > ? vsnprintf+0x3aa/0x4f0 > > > ? _fusb302_log+0x81/0x1d0 [fusb302] > > > _fusb302_log+0x81/0x1d0 [fusb302] > > > ... > > > > > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > > > --- > > > drivers/usb/typec/tcpm/fusb302.c | 3 ++- > > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/usb/typec/tcpm/fusb302.c b/drivers/usb/typec/tcpm/fusb302.c > > > index ccfc7e91e7a3..04c76b9d0065 100644 > > > --- a/drivers/usb/typec/tcpm/fusb302.c > > > +++ b/drivers/usb/typec/tcpm/fusb302.c > > > @@ -1759,6 +1759,7 @@ static int fusb302_probe(struct i2c_client *client, > > > INIT_WORK(&chip->irq_work, fusb302_irq_work); > > > INIT_DELAYED_WORK(&chip->bc_lvl_handler, fusb302_bc_lvl_handler_work); > > > init_tcpc_dev(&chip->tcpc_dev); > > > + fusb302_debugfs_init(chip); > > > > > > if (client->irq) { > > > chip->gpio_int_n_irq = client->irq; > > > @@ -1784,7 +1785,6 @@ 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); > > > > That leaves the rootdir variable pointing to something again for > > example if a failure happens (like -EPROBE_AGAIN) during probe (the > > "fusb302" directory is removed, but the rootdir static variable still > > points to something). > > > > Let's just create that rootdir directory during driver init. I don't > > really understand why should we only create it when/if the first > > instance of fusb302 is registered. I think something like this would > > work: > > > > diff --git a/drivers/usb/typec/tcpm/fusb302.c b/drivers/usb/typec/tcpm/fusb302.c > > index c524088246ee..7a950a6e5f0d 100644 > > --- a/drivers/usb/typec/tcpm/fusb302.c > > +++ b/drivers/usb/typec/tcpm/fusb302.c > > @@ -212,9 +212,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); > > - > > chip->dentry = debugfs_create_file(dev_name(chip->dev), > > S_IFREG | 0444, rootdir, > > chip, &fusb302_debug_fops); > > @@ -223,7 +220,6 @@ static void fusb302_debugfs_init(struct fusb302_chip *chip) > > static void fusb302_debugfs_exit(struct fusb302_chip *chip) > > { > > debugfs_remove(chip->dentry); > > - debugfs_remove(rootdir); > > } > > > > #else > > @@ -1863,7 +1859,24 @@ static struct i2c_driver fusb302_driver = { > > .remove = fusb302_remove, > > .id_table = fusb302_i2c_device_id, > > }; > > -module_i2c_driver(fusb302_driver); > > + > > +static int __init fusb302_init(void) > > +{ > > + rootdir = debugfs_create_dir("fusb302", NULL); > > + if (IS_ERR(rootdir)) > > + return PTR_ERR(rootdir); > > As Guenter points out, don't check this, just call it anb move on. > > But are you _SURE_ you want this to be the name, at the root of debugfs? > Why not put it under the usb debugfs directory? That's a good point. Let's move it there while at it. thanks,
On Thu, Aug 15, 2019 at 04:31:59PM +0300, Heikki Krogerus wrote: > > > > As Guenter points out, don't check this, just call it anb move on. > > > > But are you _SURE_ you want this to be the name, at the root of debugfs? > > Why not put it under the usb debugfs directory? > > That's a good point. Let's move it there while at it. > Maybe we should move the tcpm root as well ? Guenter
Hi, On 15-08-19 17:20, Guenter Roeck wrote: > On Thu, Aug 15, 2019 at 04:31:59PM +0300, Heikki Krogerus wrote: >>> >>> As Guenter points out, don't check this, just call it anb move on. >>> >>> But are you _SURE_ you want this to be the name, at the root of debugfs? >>> Why not put it under the usb debugfs directory? >> >> That's a good point. Let's move it there while at it. >> > Maybe we should move the tcpm root as well ? Ack, I'm preparing a patch series that does both. Regards, Hans
diff --git a/drivers/usb/typec/tcpm/fusb302.c b/drivers/usb/typec/tcpm/fusb302.c index ccfc7e91e7a3..04c76b9d0065 100644 --- a/drivers/usb/typec/tcpm/fusb302.c +++ b/drivers/usb/typec/tcpm/fusb302.c @@ -1759,6 +1759,7 @@ static int fusb302_probe(struct i2c_client *client, INIT_WORK(&chip->irq_work, fusb302_irq_work); INIT_DELAYED_WORK(&chip->bc_lvl_handler, fusb302_bc_lvl_handler_work); init_tcpc_dev(&chip->tcpc_dev); + fusb302_debugfs_init(chip); if (client->irq) { chip->gpio_int_n_irq = client->irq; @@ -1784,7 +1785,6 @@ 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; @@ -1792,6 +1792,7 @@ static int fusb302_probe(struct i2c_client *client, tcpm_unregister_port: tcpm_unregister_port(chip->tcpm_port); destroy_workqueue: + fusb302_debugfs_exit(chip); destroy_workqueue(chip->wq); return ret;
tcpm_register_port() will call some of the fusb302 code's callbacks wich in turn will call fusb302_log(). So we need to call fusb302_debugfs_init() before we call tcpm_register_port(). This fixes the following warning, which was caused by the logbuffer_lock not yet being initialized (which is done by fusb302_debugfs_init): DEBUG_LOCKS_WARN_ON(lock->magic != lock) WARNING: CPU: 0 PID: 1306 at kernel/locking/mutex.c:912 __mutex_lock+0x978/0x9a0 Modules linked in: fusb302(+) tcpm pi3usb30532 typec bq24190_charger snd_soc_sst_cht_bsw_rt5645 mei_hdcp dwc3 intel_rapl_msr udc_core ulpi gpio_keys intel_powerclamp coretemp kvm_intel brcmfmac kvm brcmutil joydev cfg80211 wdat_wdt irqbypass pcspkr intel_cstate extcon_intel_cht_wc i2c_cht_wc(E) snd_intel_sst_acpi snd_intel_sst_core snd_soc_rt5645 snd_soc_sst_atom_hifi2_platform snd_soc_acpi_intel_match snd_soc_rl6231 snd_soc_acpi intel_xhci_usb_role_switch roles hci_uart snd_soc_core btqca mei_txe btrtl processor_thermal_device mei snd_hdmi_lpe_audio lpc_ich snd_compress btbcm intel_rapl_common ac97_bus dwc3_pci snd_pcm_dmaengine intel_soc_dts_iosf btintel snd_seq bluetooth snd_seq_device snd_pcm intel_cht_int33fe_musb snd_timer intel_cht_int33fe_typec intel_hid intel_cht_int33fe_common sparse_keymap snd ecdh_generic goodix rfkill soundcore ecc spi_pxa2xx_platform max17042_battery dw_dmac int3406_thermal dptf_power acpi_pad soc_button_array int3400_thermal int3403_thermal gpd_pocket_fan intel_int0002_vgpio int340x_thermal_zone acpi_thermal_rel dm_crypt mmc_block i915 crct10dif_pclmul crc32_pclmul crc32c_intel ghash_clmulni_intel i2c_algo_bit drm_kms_helper drm video sdhci_acpi sdhci mmc_core pwm_lpss_platform pwm_lpss i2c_dev CPU: 0 PID: 1306 Comm: systemd-udevd Tainted: G E 5.3.0-rc4+ #83 Hardware name: Default string Default string/Default string, BIOS 5.11 06/28/2017 RIP: 0010:__mutex_lock+0x978/0x9a0 Code: c0 0f 84 26 f7 ff ff 44 8b 05 24 25 c8 00 45 85 c0 0f 85 16 f7 ff ff 48 c7 c6 da 55 2f ae 48 c7 c7 98 8c 2d ae e8 a0 f9 5c ff <0f> 0b e9 fc f6 ff ff 4c 89 f0 4d 89 fe 49 89 c7 e9 cf fa ff ff e8 RSP: 0018:ffffb7a8c0523800 EFLAGS: 00010286 RAX: 0000000000000000 RBX: 0000000000000000 RCX: 0000000000000000 RDX: 0000000000000002 RSI: 0000000000000001 RDI: 0000000000000246 RBP: ffffb7a8c05238c0 R08: 0000000000000000 R09: 0000000000000000 R10: ffffb7a8c0523648 R11: 0000000000000030 R12: 0000000000000000 R13: ffffb7a8c0523990 R14: ffff9bf22f70c028 R15: ffff9bf22f70c360 FS: 00007f39ca234940(0000) GS:ffff9bf237400000(0000) knlGS:0000000000000000 CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 CR2: 00007f1f108481a0 CR3: 0000000271f28000 CR4: 00000000001006f0 Call Trace: ? find_held_lock+0x39/0x90 ? _fusb302_log+0x81/0x1d0 [fusb302] ? vsnprintf+0x3aa/0x4f0 ? _fusb302_log+0x81/0x1d0 [fusb302] _fusb302_log+0x81/0x1d0 [fusb302] ... Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- drivers/usb/typec/tcpm/fusb302.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)