[PATCH 1/3] usb: typec: tcpm: Use usb_debug_root as root for our debugfs entry

[PATCH 1/3] usb: typec: tcpm: Use usb_debug_root as root for our debugfs entry

[Hans de Goede]

Use usb_debug_root as root for our debugfs entry instead of creating our
own subdirectory under the debugfs root.

Another patch in this series will make the same change to the fusb302
driver, which also uses dev_name() (on the same device) for the debugfs
entry name. So we also prefix dev_name() with "tcpm-" here to avoid a
name conflict.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/usb/typec/tcpm/tcpm.c | 17 +++++------------
 1 file changed, 5 insertions(+), 12 deletions(-)

diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
index 15abe1d9958f..5241d17c3399 100644
--- a/drivers/usb/typec/tcpm/tcpm.c
+++ b/drivers/usb/typec/tcpm/tcpm.c
@@ -19,6 +19,7 @@
 #include <linux/seq_file.h>
 #include <linux/slab.h>
 #include <linux/spinlock.h>
+#include <linux/usb.h>
 #include <linux/usb/pd.h>
 #include <linux/usb/pd_ado.h>
 #include <linux/usb/pd_bdo.h>
@@ -571,17 +572,13 @@ static int tcpm_debug_show(struct seq_file *s, void *v)
 }
 DEFINE_SHOW_ATTRIBUTE(tcpm_debug);
 
-static struct dentry *rootdir;
-
 static void tcpm_debugfs_init(struct tcpm_port *port)
 {
-	mutex_init(&port->logbuffer_lock);
-	/* /sys/kernel/debug/tcpm/usbcX */
-	if (!rootdir)
-		rootdir = debugfs_create_dir("tcpm", NULL);
+	char name[NAME_MAX];
 
-	port->dentry = debugfs_create_file(dev_name(port->dev),
-					   S_IFREG | 0444, rootdir,
+	mutex_init(&port->logbuffer_lock);
+	snprintf(name, NAME_MAX, "tcpm-%s", dev_name(port->dev));
+	port->dentry = debugfs_create_file(name, S_IFREG | 0444, usb_debug_root,
 					   port, &tcpm_debug_fops);
 }
 
@@ -597,10 +594,6 @@ static void tcpm_debugfs_exit(struct tcpm_port *port)
 	mutex_unlock(&port->logbuffer_lock);
 
 	debugfs_remove(port->dentry);
-	if (list_empty(&rootdir->d_subdirs)) {
-		debugfs_remove(rootdir);
-		rootdir = NULL;
-	}
 }
 
 #else
-- 
2.23.0.rc2

[PATCH 2/3] usb: typec: fusb: Use usb_debug_root as root for our debugfs entry

[Hans de Goede]

Use usb_debug_root as root for our debugfs entry instead of creating our
own subdirectory under the debugfs root.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/usb/typec/tcpm/fusb302.c | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/drivers/usb/typec/tcpm/fusb302.c b/drivers/usb/typec/tcpm/fusb302.c
index 93244d6c4bff..69a2afaf8f68 100644
--- a/drivers/usb/typec/tcpm/fusb302.c
+++ b/drivers/usb/typec/tcpm/fusb302.c
@@ -26,6 +26,7 @@
 #include <linux/spinlock.h>
 #include <linux/string.h>
 #include <linux/types.h>
+#include <linux/usb.h>
 #include <linux/usb/typec.h>
 #include <linux/usb/tcpm.h>
 #include <linux/usb/pd.h>
@@ -206,23 +207,17 @@ static int fusb302_debug_show(struct seq_file *s, void *v)
 }
 DEFINE_SHOW_ATTRIBUTE(fusb302_debug);
 
-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,
+					   S_IFREG | 0444, usb_debug_root,
 					   chip, &fusb302_debug_fops);
 }
 
 static void fusb302_debugfs_exit(struct fusb302_chip *chip)
 {
 	debugfs_remove(chip->dentry);
-	debugfs_remove(rootdir);
 }
 
 #else
-- 
2.23.0.rc2

[PATCH 3/3] usb: typec: fusb302: Call fusb302_debugfs_init earlier

[Hans de Goede]

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 69a2afaf8f68..c7769fa73148 100644
--- a/drivers/usb/typec/tcpm/fusb302.c
+++ b/drivers/usb/typec/tcpm/fusb302.c
@@ -1726,6 +1726,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;
@@ -1758,7 +1759,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;
@@ -1767,6 +1767,7 @@ static int fusb302_probe(struct i2c_client *client,
 	tcpm_unregister_port(chip->tcpm_port);
 	fwnode_handle_put(chip->tcpc_dev.fwnode);
 destroy_workqueue:
+	fusb302_debugfs_exit(chip);
 	destroy_workqueue(chip->wq);
 
 	return ret;
-- 
2.23.0.rc2

Re: [PATCH 1/3] usb: typec: tcpm: Use usb_debug_root as root for our debugfs entry

[Guenter Roeck]

On Thu, Aug 15, 2019 at 09:18:13PM +0200, Hans de Goede wrote:
> Use usb_debug_root as root for our debugfs entry instead of creating our
> own subdirectory under the debugfs root.
> 
> Another patch in this series will make the same change to the fusb302
> driver, which also uses dev_name() (on the same device) for the debugfs
> entry name. So we also prefix dev_name() with "tcpm-" here to avoid a
> name conflict.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

Reviewed-by: Guenter Roeck <linux@roeck-us.net>

> ---
>  drivers/usb/typec/tcpm/tcpm.c | 17 +++++------------
>  1 file changed, 5 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
> index 15abe1d9958f..5241d17c3399 100644
> --- a/drivers/usb/typec/tcpm/tcpm.c
> +++ b/drivers/usb/typec/tcpm/tcpm.c
> @@ -19,6 +19,7 @@
>  #include <linux/seq_file.h>
>  #include <linux/slab.h>
>  #include <linux/spinlock.h>
> +#include <linux/usb.h>
>  #include <linux/usb/pd.h>
>  #include <linux/usb/pd_ado.h>
>  #include <linux/usb/pd_bdo.h>
> @@ -571,17 +572,13 @@ static int tcpm_debug_show(struct seq_file *s, void *v)
>  }
>  DEFINE_SHOW_ATTRIBUTE(tcpm_debug);
>  
> -static struct dentry *rootdir;
> -
>  static void tcpm_debugfs_init(struct tcpm_port *port)
>  {
> -	mutex_init(&port->logbuffer_lock);
> -	/* /sys/kernel/debug/tcpm/usbcX */
> -	if (!rootdir)
> -		rootdir = debugfs_create_dir("tcpm", NULL);
> +	char name[NAME_MAX];
>  
> -	port->dentry = debugfs_create_file(dev_name(port->dev),
> -					   S_IFREG | 0444, rootdir,
> +	mutex_init(&port->logbuffer_lock);
> +	snprintf(name, NAME_MAX, "tcpm-%s", dev_name(port->dev));
> +	port->dentry = debugfs_create_file(name, S_IFREG | 0444, usb_debug_root,
>  					   port, &tcpm_debug_fops);
>  }
>  
> @@ -597,10 +594,6 @@ static void tcpm_debugfs_exit(struct tcpm_port *port)
>  	mutex_unlock(&port->logbuffer_lock);
>  
>  	debugfs_remove(port->dentry);
> -	if (list_empty(&rootdir->d_subdirs)) {
> -		debugfs_remove(rootdir);
> -		rootdir = NULL;
> -	}
>  }
>  
>  #else
> -- 
> 2.23.0.rc2
> 

Re: [PATCH 2/3] usb: typec: fusb: Use usb_debug_root as root for our debugfs entry

[Guenter Roeck]

On Thu, Aug 15, 2019 at 09:18:14PM +0200, Hans de Goede wrote:
> Use usb_debug_root as root for our debugfs entry instead of creating our
> own subdirectory under the debugfs root.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

Reviewed-by: Guenter Roeck <linux@roeck-us.net>

> ---
>  drivers/usb/typec/tcpm/fusb302.c | 9 ++-------
>  1 file changed, 2 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/usb/typec/tcpm/fusb302.c b/drivers/usb/typec/tcpm/fusb302.c
> index 93244d6c4bff..69a2afaf8f68 100644
> --- a/drivers/usb/typec/tcpm/fusb302.c
> +++ b/drivers/usb/typec/tcpm/fusb302.c
> @@ -26,6 +26,7 @@
>  #include <linux/spinlock.h>
>  #include <linux/string.h>
>  #include <linux/types.h>
> +#include <linux/usb.h>
>  #include <linux/usb/typec.h>
>  #include <linux/usb/tcpm.h>
>  #include <linux/usb/pd.h>
> @@ -206,23 +207,17 @@ static int fusb302_debug_show(struct seq_file *s, void *v)
>  }
>  DEFINE_SHOW_ATTRIBUTE(fusb302_debug);
>  
> -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,
> +					   S_IFREG | 0444, usb_debug_root,
>  					   chip, &fusb302_debug_fops);
>  }
>  
>  static void fusb302_debugfs_exit(struct fusb302_chip *chip)
>  {
>  	debugfs_remove(chip->dentry);
> -	debugfs_remove(rootdir);
>  }
>  
>  #else
> -- 
> 2.23.0.rc2
> 

Re: [PATCH 3/3] usb: typec: fusb302: Call fusb302_debugfs_init earlier

[Guenter Roeck]

On Thu, Aug 15, 2019 at 09:18:15PM +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>

Reviewed-by: Guenter Roeck <linux@roeck-us.net>

> ---
>  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 69a2afaf8f68..c7769fa73148 100644
> --- a/drivers/usb/typec/tcpm/fusb302.c
> +++ b/drivers/usb/typec/tcpm/fusb302.c
> @@ -1726,6 +1726,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;
> @@ -1758,7 +1759,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;
> @@ -1767,6 +1767,7 @@ static int fusb302_probe(struct i2c_client *client,
>  	tcpm_unregister_port(chip->tcpm_port);
>  	fwnode_handle_put(chip->tcpc_dev.fwnode);
>  destroy_workqueue:
> +	fusb302_debugfs_exit(chip);
>  	destroy_workqueue(chip->wq);
>  
>  	return ret;
> -- 
> 2.23.0.rc2
> 

Re: [PATCH 3/3] usb: typec: fusb302: Call fusb302_debugfs_init earlier

[Heikki Krogerus]

On Thu, Aug 15, 2019 at 09:18:15PM +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>

Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.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 69a2afaf8f68..c7769fa73148 100644
> --- a/drivers/usb/typec/tcpm/fusb302.c
> +++ b/drivers/usb/typec/tcpm/fusb302.c
> @@ -1726,6 +1726,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;
> @@ -1758,7 +1759,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;
> @@ -1767,6 +1767,7 @@ static int fusb302_probe(struct i2c_client *client,
>  	tcpm_unregister_port(chip->tcpm_port);
>  	fwnode_handle_put(chip->tcpc_dev.fwnode);
>  destroy_workqueue:
> +	fusb302_debugfs_exit(chip);
>  	destroy_workqueue(chip->wq);
>  
>  	return ret;
> -- 
> 2.23.0.rc2

thanks,

-- 
heikki

Re: [PATCH 2/3] usb: typec: fusb: Use usb_debug_root as root for our debugfs entry

[Heikki Krogerus]

On Thu, Aug 15, 2019 at 09:18:14PM +0200, Hans de Goede wrote:
> Use usb_debug_root as root for our debugfs entry instead of creating our
> own subdirectory under the debugfs root.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

I have one question below. Otherwise:

Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>

> ---
>  drivers/usb/typec/tcpm/fusb302.c | 9 ++-------
>  1 file changed, 2 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/usb/typec/tcpm/fusb302.c b/drivers/usb/typec/tcpm/fusb302.c
> index 93244d6c4bff..69a2afaf8f68 100644
> --- a/drivers/usb/typec/tcpm/fusb302.c
> +++ b/drivers/usb/typec/tcpm/fusb302.c
> @@ -26,6 +26,7 @@
>  #include <linux/spinlock.h>
>  #include <linux/string.h>
>  #include <linux/types.h>
> +#include <linux/usb.h>
>  #include <linux/usb/typec.h>
>  #include <linux/usb/tcpm.h>
>  #include <linux/usb/pd.h>
> @@ -206,23 +207,17 @@ static int fusb302_debug_show(struct seq_file *s, void *v)
>  }
>  DEFINE_SHOW_ATTRIBUTE(fusb302_debug);
>  
> -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,
> +					   S_IFREG | 0444, usb_debug_root,
>  					   chip, &fusb302_debug_fops);

In tcpm.c you named the entries "tcpm-%s", dev_name(...

Shouldn't we do something similar with these as well? I mean,
even though this is just debugfs, shouldn't we give some hint for the
user how to identify these entries?

How about if we still continue grouping the entries under the
"fusb302" directory, but just place that under usb_debug_root?


thanks,

-- 
heikki

Re: [PATCH 1/3] usb: typec: tcpm: Use usb_debug_root as root for our debugfs entry

[Heikki Krogerus]

On Thu, Aug 15, 2019 at 09:18:13PM +0200, Hans de Goede wrote:
> Use usb_debug_root as root for our debugfs entry instead of creating our
> own subdirectory under the debugfs root.
> 
> Another patch in this series will make the same change to the fusb302
> driver, which also uses dev_name() (on the same device) for the debugfs
> entry name. So we also prefix dev_name() with "tcpm-" here to avoid a
> name conflict.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>

> ---
>  drivers/usb/typec/tcpm/tcpm.c | 17 +++++------------
>  1 file changed, 5 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
> index 15abe1d9958f..5241d17c3399 100644
> --- a/drivers/usb/typec/tcpm/tcpm.c
> +++ b/drivers/usb/typec/tcpm/tcpm.c
> @@ -19,6 +19,7 @@
>  #include <linux/seq_file.h>
>  #include <linux/slab.h>
>  #include <linux/spinlock.h>
> +#include <linux/usb.h>
>  #include <linux/usb/pd.h>
>  #include <linux/usb/pd_ado.h>
>  #include <linux/usb/pd_bdo.h>
> @@ -571,17 +572,13 @@ static int tcpm_debug_show(struct seq_file *s, void *v)
>  }
>  DEFINE_SHOW_ATTRIBUTE(tcpm_debug);
>  
> -static struct dentry *rootdir;
> -
>  static void tcpm_debugfs_init(struct tcpm_port *port)
>  {
> -	mutex_init(&port->logbuffer_lock);
> -	/* /sys/kernel/debug/tcpm/usbcX */
> -	if (!rootdir)
> -		rootdir = debugfs_create_dir("tcpm", NULL);
> +	char name[NAME_MAX];
>  
> -	port->dentry = debugfs_create_file(dev_name(port->dev),
> -					   S_IFREG | 0444, rootdir,
> +	mutex_init(&port->logbuffer_lock);
> +	snprintf(name, NAME_MAX, "tcpm-%s", dev_name(port->dev));
> +	port->dentry = debugfs_create_file(name, S_IFREG | 0444, usb_debug_root,
>  					   port, &tcpm_debug_fops);
>  }
>  
> @@ -597,10 +594,6 @@ static void tcpm_debugfs_exit(struct tcpm_port *port)
>  	mutex_unlock(&port->logbuffer_lock);
>  
>  	debugfs_remove(port->dentry);
> -	if (list_empty(&rootdir->d_subdirs)) {
> -		debugfs_remove(rootdir);
> -		rootdir = NULL;
> -	}
>  }

I guess one option here as well would be to still group the entries
under the "tcpm" directry, but just place that under the
usb_debug_root directory.

thanks,

-- 
heikki

Re: [PATCH 2/3] usb: typec: fusb: Use usb_debug_root as root for our debugfs entry

[Hans de Goede]

Hi,

On 16-08-19 09:51, Heikki Krogerus wrote:
> On Thu, Aug 15, 2019 at 09:18:14PM +0200, Hans de Goede wrote:
>> Use usb_debug_root as root for our debugfs entry instead of creating our
>> own subdirectory under the debugfs root.
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> 
> I have one question below. Otherwise:
> 
> Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> 
>> ---
>>   drivers/usb/typec/tcpm/fusb302.c | 9 ++-------
>>   1 file changed, 2 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/usb/typec/tcpm/fusb302.c b/drivers/usb/typec/tcpm/fusb302.c
>> index 93244d6c4bff..69a2afaf8f68 100644
>> --- a/drivers/usb/typec/tcpm/fusb302.c
>> +++ b/drivers/usb/typec/tcpm/fusb302.c
>> @@ -26,6 +26,7 @@
>>   #include <linux/spinlock.h>
>>   #include <linux/string.h>
>>   #include <linux/types.h>
>> +#include <linux/usb.h>
>>   #include <linux/usb/typec.h>
>>   #include <linux/usb/tcpm.h>
>>   #include <linux/usb/pd.h>
>> @@ -206,23 +207,17 @@ static int fusb302_debug_show(struct seq_file *s, void *v)
>>   }
>>   DEFINE_SHOW_ATTRIBUTE(fusb302_debug);
>>   
>> -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,
>> +					   S_IFREG | 0444, usb_debug_root,
>>   					   chip, &fusb302_debug_fops);
> 
> In tcpm.c you named the entries "tcpm-%s", dev_name(...
> 
> Shouldn't we do something similar with these as well? I mean,
> even though this is just debugfs, shouldn't we give some hint for the
> user how to identify these entries?

Well on my test-machinw te dev_name is i2c-fusb302, but I realize the
dev_name is not always going to be so descriptive, so I will send
out a v2 adding a "fusb302-" prefix, like how we are doing for the
tcpm.c debug entry already.

> How about if we still continue grouping the entries under the
> "fusb302" directory, but just place that under usb_debug_root?

I would rather go with a prefix, the whole purpose of dropping the
subdir is so that we do not have a resource (the dir) shared between
multiple instances of the driver.

Regards,

Hans