| Message ID | 20221220121231.20120-1-kvalo@kernel.org |
|---|---|
| State | New |
| Headers | show |
| Series | ath11k: debugfs: fix to work with multiple PCI devices | expand |
On 12/20/2022 5:42 PM, Kalle Valo wrote: > From: Kalle Valo <quic_kvalo@quicinc.com> > > ath11k fails to load if there are multiple ath11k PCI devices with same name: > > ath11k_pci 0000:01:00.0: Hardware name qcn9074 hw1.0 > debugfs: Directory 'ath11k' with parent '/' already present! > ath11k_pci 0000:01:00.0: failed to create ath11k debugfs > ath11k_pci 0000:01:00.0: failed to create soc core: -17 > ath11k_pci 0000:01:00.0: failed to init core: -17 > ath11k_pci: probe of 0000:01:00.0 failed with error -17 > > Fix this by creating a directory for each ath11k device using schema > <bus>-<devname>, for example "pci-0000:06:00.0". This directory created under > the top-level ath11k directory, for example /sys/kernel/debug/ath11k. > > The reference to the toplevel ath11k directory is not stored anymore within ath11k, instead > it's retrieved using debugfs_lookup(). If the directory does not exist it will > be created. After the last directory from the ath11k directory is removed, for > example when doing rmmod ath11k, the empty ath11k directory is left in place, > it's a minor cosmetic issue anyway. > > Here's an example hierarchy with one WCN6855: > > ath11k > `-- pci-0000:06:00.0 > |-- mac0 > | |-- dfs_block_radar_events > | |-- dfs_simulate_radar > | |-- ext_rx_stats > | |-- ext_tx_stats > | |-- fw_dbglog_config > | |-- fw_stats > | | |-- beacon_stats > | | |-- pdev_stats > | | `-- vdev_stats > | |-- htt_stats > | |-- htt_stats_reset > | |-- htt_stats_type > | `-- pktlog_filter > |-- simulate_fw_crash > `-- soc_dp_stats > > I didn't have a test setup where I could connect multiple ath11k devices to the > same the host, so I have only tested this with one device. > > Tested-on: WCN6855 hw2.0 PCI WLAN.HSP.1.1-03125-QCAHSPSWPL_V1_V2_SILICONZ_LITE-3.6510.9 > > Signed-off-by: Kalle Valo <quic_kvalo@quicinc.com> > --- > drivers/net/wireless/ath/ath11k/core.h | 1 - > drivers/net/wireless/ath/ath11k/debugfs.c | 48 +++++++++++++++++++---- > 2 files changed, 40 insertions(+), 9 deletions(-) > > diff --git a/drivers/net/wireless/ath/ath11k/core.h b/drivers/net/wireless/ath/ath11k/core.h > index a8acb8b7b8d5..beb552108ac3 100644 > --- a/drivers/net/wireless/ath/ath11k/core.h > +++ b/drivers/net/wireless/ath/ath11k/core.h > @@ -921,7 +921,6 @@ struct ath11k_base { > enum ath11k_dfs_region dfs_region; > #ifdef CONFIG_ATH11K_DEBUGFS > struct dentry *debugfs_soc; > - struct dentry *debugfs_ath11k; > #endif > struct ath11k_soc_dp_stats soc_stats; > > diff --git a/drivers/net/wireless/ath/ath11k/debugfs.c b/drivers/net/wireless/ath/ath11k/debugfs.c > index ccdf3d5ba1ab..5bb6fd17fdf6 100644 > --- a/drivers/net/wireless/ath/ath11k/debugfs.c > +++ b/drivers/net/wireless/ath/ath11k/debugfs.c > @@ -976,10 +976,6 @@ int ath11k_debugfs_pdev_create(struct ath11k_base *ab) > if (test_bit(ATH11K_FLAG_REGISTERED, &ab->dev_flags)) > return 0; > > - ab->debugfs_soc = debugfs_create_dir(ab->hw_params.name, ab->debugfs_ath11k); > - if (IS_ERR(ab->debugfs_soc)) > - return PTR_ERR(ab->debugfs_soc); > - > debugfs_create_file("simulate_fw_crash", 0600, ab->debugfs_soc, ab, > &fops_simulate_fw_crash); > > @@ -1001,15 +997,51 @@ void ath11k_debugfs_pdev_destroy(struct ath11k_base *ab) > > int ath11k_debugfs_soc_create(struct ath11k_base *ab) > { > - ab->debugfs_ath11k = debugfs_create_dir("ath11k", NULL); > + struct dentry *root; > + bool dput_needed; > + char name[64]; > + int ret; > + > + root = debugfs_lookup("ath11k", NULL); > + if (!root) { > + root = debugfs_create_dir("ath11k", NULL); > + if (IS_ERR_OR_NULL(root)) > + return PTR_ERR(root); > + > + dput_needed = false; > + } else { > + /* a dentry from lookup() needs dput() after we don't use it */ > + dput_needed = true; > + } > + > + scnprintf(name, sizeof(name), "%s-%s", ath11k_bus_str(ab->hif.bus), > + dev_name(ab->dev)); > + > + ab->debugfs_soc = debugfs_create_dir(name, root); > + if (IS_ERR_OR_NULL(ab->debugfs_soc)) { > + ret = PTR_ERR(ab->debugfs_soc); > + goto out; > + } > + > + ret = 0; > > - return PTR_ERR_OR_ZERO(ab->debugfs_ath11k); > +out: > + if (dput_needed) > + dput(root); > + > + return ret; > } > > void ath11k_debugfs_soc_destroy(struct ath11k_base *ab) > { > - debugfs_remove_recursive(ab->debugfs_ath11k); > - ab->debugfs_ath11k = NULL; > + debugfs_remove_recursive(ab->debugfs_soc); > + ab->debugfs_soc = NULL; > + > + /* We are not removing ath11k directory on purpose, even if it > + * would be empty. This simplifies the directory handling and it's > + * a minor cosmetic issue to leave an empty ath11k directory to > + * debugfs. > + */ > } > EXPORT_SYMBOL(ath11k_debugfs_soc_destroy); > > > base-commit: 922932ca02191a390f7f52fb6e21c44b50e14025 looks good to me Vasanth
On Tue, Dec 20, 2022 at 1:16 PM Kalle Valo <kvalo@kernel.org> wrote: > > From: Kalle Valo <quic_kvalo@quicinc.com> > > ath11k fails to load if there are multiple ath11k PCI devices with same name: > > ath11k_pci 0000:01:00.0: Hardware name qcn9074 hw1.0 > debugfs: Directory 'ath11k' with parent '/' already present! > ath11k_pci 0000:01:00.0: failed to create ath11k debugfs > ath11k_pci 0000:01:00.0: failed to create soc core: -17 > ath11k_pci 0000:01:00.0: failed to init core: -17 > ath11k_pci: probe of 0000:01:00.0 failed with error -17 > > Fix this by creating a directory for each ath11k device using schema > <bus>-<devname>, for example "pci-0000:06:00.0". This directory created under > the top-level ath11k directory, for example /sys/kernel/debug/ath11k. > > The reference to the toplevel ath11k directory is not stored anymore within ath11k, instead > it's retrieved using debugfs_lookup(). If the directory does not exist it will > be created. After the last directory from the ath11k directory is removed, for > example when doing rmmod ath11k, the empty ath11k directory is left in place, > it's a minor cosmetic issue anyway. > > Here's an example hierarchy with one WCN6855: > > ath11k > `-- pci-0000:06:00.0 > |-- mac0 > | |-- dfs_block_radar_events > | |-- dfs_simulate_radar > | |-- ext_rx_stats > | |-- ext_tx_stats > | |-- fw_dbglog_config > | |-- fw_stats > | | |-- beacon_stats > | | |-- pdev_stats > | | `-- vdev_stats > | |-- htt_stats > | |-- htt_stats_reset > | |-- htt_stats_type > | `-- pktlog_filter > |-- simulate_fw_crash > `-- soc_dp_stats > > I didn't have a test setup where I could connect multiple ath11k devices to the > same the host, so I have only tested this with one device. > > Tested-on: WCN6855 hw2.0 PCI WLAN.HSP.1.1-03125-QCAHSPSWPL_V1_V2_SILICONZ_LITE-3.6510.9 I can confirm that this works on a combination of IPQ8074 with an external QCN9074 radio which would previously clash and error out, so: Tested-on: IPQ8074 hw2.0 AHB WLAN.HK.2.5.0.1-01208-QCAHKSWPL_SILICONZ-1 Tested-on: QCN9074 hw1.0 PCI WLAN.HK.2.5.0.1-01208-QCAHKSWPL_SILICONZ-1 Tested-by: Robert Marko <robert.marko@sartura.hr> > > Signed-off-by: Kalle Valo <quic_kvalo@quicinc.com> > --- > drivers/net/wireless/ath/ath11k/core.h | 1 - > drivers/net/wireless/ath/ath11k/debugfs.c | 48 +++++++++++++++++++---- > 2 files changed, 40 insertions(+), 9 deletions(-) > > diff --git a/drivers/net/wireless/ath/ath11k/core.h b/drivers/net/wireless/ath/ath11k/core.h > index a8acb8b7b8d5..beb552108ac3 100644 > --- a/drivers/net/wireless/ath/ath11k/core.h > +++ b/drivers/net/wireless/ath/ath11k/core.h > @@ -921,7 +921,6 @@ struct ath11k_base { > enum ath11k_dfs_region dfs_region; > #ifdef CONFIG_ATH11K_DEBUGFS > struct dentry *debugfs_soc; > - struct dentry *debugfs_ath11k; > #endif > struct ath11k_soc_dp_stats soc_stats; > > diff --git a/drivers/net/wireless/ath/ath11k/debugfs.c b/drivers/net/wireless/ath/ath11k/debugfs.c > index ccdf3d5ba1ab..5bb6fd17fdf6 100644 > --- a/drivers/net/wireless/ath/ath11k/debugfs.c > +++ b/drivers/net/wireless/ath/ath11k/debugfs.c > @@ -976,10 +976,6 @@ int ath11k_debugfs_pdev_create(struct ath11k_base *ab) > if (test_bit(ATH11K_FLAG_REGISTERED, &ab->dev_flags)) > return 0; > > - ab->debugfs_soc = debugfs_create_dir(ab->hw_params.name, ab->debugfs_ath11k); > - if (IS_ERR(ab->debugfs_soc)) > - return PTR_ERR(ab->debugfs_soc); > - > debugfs_create_file("simulate_fw_crash", 0600, ab->debugfs_soc, ab, > &fops_simulate_fw_crash); > > @@ -1001,15 +997,51 @@ void ath11k_debugfs_pdev_destroy(struct ath11k_base *ab) > > int ath11k_debugfs_soc_create(struct ath11k_base *ab) > { > - ab->debugfs_ath11k = debugfs_create_dir("ath11k", NULL); > + struct dentry *root; > + bool dput_needed; > + char name[64]; > + int ret; > + > + root = debugfs_lookup("ath11k", NULL); > + if (!root) { > + root = debugfs_create_dir("ath11k", NULL); > + if (IS_ERR_OR_NULL(root)) > + return PTR_ERR(root); > + > + dput_needed = false; > + } else { > + /* a dentry from lookup() needs dput() after we don't use it */ > + dput_needed = true; > + } > + > + scnprintf(name, sizeof(name), "%s-%s", ath11k_bus_str(ab->hif.bus), > + dev_name(ab->dev)); > + > + ab->debugfs_soc = debugfs_create_dir(name, root); > + if (IS_ERR_OR_NULL(ab->debugfs_soc)) { > + ret = PTR_ERR(ab->debugfs_soc); > + goto out; > + } > + > + ret = 0; > > - return PTR_ERR_OR_ZERO(ab->debugfs_ath11k); > +out: > + if (dput_needed) > + dput(root); > + > + return ret; > } > > void ath11k_debugfs_soc_destroy(struct ath11k_base *ab) > { > - debugfs_remove_recursive(ab->debugfs_ath11k); > - ab->debugfs_ath11k = NULL; > + debugfs_remove_recursive(ab->debugfs_soc); > + ab->debugfs_soc = NULL; > + > + /* We are not removing ath11k directory on purpose, even if it > + * would be empty. This simplifies the directory handling and it's > + * a minor cosmetic issue to leave an empty ath11k directory to > + * debugfs. > + */ > } > EXPORT_SYMBOL(ath11k_debugfs_soc_destroy); > > > base-commit: 922932ca02191a390f7f52fb6e21c44b50e14025 > -- > 2.30.2 > > > -- > ath11k mailing list > ath11k@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/ath11k
Kalle Valo <kvalo@kernel.org> wrote: > ath11k fails to load if there are multiple ath11k PCI devices with same name: > > ath11k_pci 0000:01:00.0: Hardware name qcn9074 hw1.0 > debugfs: Directory 'ath11k' with parent '/' already present! > ath11k_pci 0000:01:00.0: failed to create ath11k debugfs > ath11k_pci 0000:01:00.0: failed to create soc core: -17 > ath11k_pci 0000:01:00.0: failed to init core: -17 > ath11k_pci: probe of 0000:01:00.0 failed with error -17 > > Fix this by creating a directory for each ath11k device using schema > <bus>-<devname>, for example "pci-0000:06:00.0". This directory created under > the top-level ath11k directory, for example /sys/kernel/debug/ath11k. > > The reference to the toplevel ath11k directory is not stored anymore within ath11k, instead > it's retrieved using debugfs_lookup(). If the directory does not exist it will > be created. After the last directory from the ath11k directory is removed, for > example when doing rmmod ath11k, the empty ath11k directory is left in place, > it's a minor cosmetic issue anyway. > > Here's an example hierarchy with one WCN6855: > > ath11k > `-- pci-0000:06:00.0 > |-- mac0 > | |-- dfs_block_radar_events > | |-- dfs_simulate_radar > | |-- ext_rx_stats > | |-- ext_tx_stats > | |-- fw_dbglog_config > | |-- fw_stats > | | |-- beacon_stats > | | |-- pdev_stats > | | `-- vdev_stats > | |-- htt_stats > | |-- htt_stats_reset > | |-- htt_stats_type > | `-- pktlog_filter > |-- simulate_fw_crash > `-- soc_dp_stats > > I didn't have a test setup where I could connect multiple ath11k devices to the > same the host, so I have only tested this with one device. > > Tested-on: WCN6855 hw2.0 PCI WLAN.HSP.1.1-03125-QCAHSPSWPL_V1_V2_SILICONZ_LITE-3.6510.9 > Tested-on: IPQ8074 hw2.0 AHB WLAN.HK.2.5.0.1-01208-QCAHKSWPL_SILICONZ-1 > Tested-on: QCN9074 hw1.0 PCI WLAN.HK.2.5.0.1-01208-QCAHKSWPL_SILICONZ-1 > > Tested-by: Robert Marko <robert.marko@sartura.hr> > Signed-off-by: Kalle Valo <quic_kvalo@quicinc.com> Patch applied to ath-next branch of ath.git, thanks. 323d91d4684d wifi: ath11k: debugfs: fix to work with multiple PCI devices
Robert Marko <robert.marko@sartura.hr> writes: > On Tue, Dec 20, 2022 at 1:16 PM Kalle Valo <kvalo@kernel.org> wrote: >> >> From: Kalle Valo <quic_kvalo@quicinc.com> >> >> ath11k fails to load if there are multiple ath11k PCI devices with same name: >> >> ath11k_pci 0000:01:00.0: Hardware name qcn9074 hw1.0 >> debugfs: Directory 'ath11k' with parent '/' already present! >> ath11k_pci 0000:01:00.0: failed to create ath11k debugfs >> ath11k_pci 0000:01:00.0: failed to create soc core: -17 >> ath11k_pci 0000:01:00.0: failed to init core: -17 >> ath11k_pci: probe of 0000:01:00.0 failed with error -17 >> >> Fix this by creating a directory for each ath11k device using schema >> <bus>-<devname>, for example "pci-0000:06:00.0". This directory created under >> the top-level ath11k directory, for example /sys/kernel/debug/ath11k. >> >> The reference to the toplevel ath11k directory is not stored anymore within ath11k, instead >> it's retrieved using debugfs_lookup(). If the directory does not exist it will >> be created. After the last directory from the ath11k directory is removed, for >> example when doing rmmod ath11k, the empty ath11k directory is left in place, >> it's a minor cosmetic issue anyway. >> >> Here's an example hierarchy with one WCN6855: >> >> ath11k >> `-- pci-0000:06:00.0 >> |-- mac0 >> | |-- dfs_block_radar_events >> | |-- dfs_simulate_radar >> | |-- ext_rx_stats >> | |-- ext_tx_stats >> | |-- fw_dbglog_config >> | |-- fw_stats >> | | |-- beacon_stats >> | | |-- pdev_stats >> | | `-- vdev_stats >> | |-- htt_stats >> | |-- htt_stats_reset >> | |-- htt_stats_type >> | `-- pktlog_filter >> |-- simulate_fw_crash >> `-- soc_dp_stats >> >> I didn't have a test setup where I could connect multiple ath11k devices to the >> same the host, so I have only tested this with one device. >> >> Tested-on: WCN6855 hw2.0 PCI WLAN.HSP.1.1-03125-QCAHSPSWPL_V1_V2_SILICONZ_LITE-3.6510.9 > > > I can confirm that this works on a combination of IPQ8074 with an > external QCN9074 radio which would previously clash and error out, so: > > Tested-on: IPQ8074 hw2.0 AHB WLAN.HK.2.5.0.1-01208-QCAHKSWPL_SILICONZ-1 > Tested-on: QCN9074 hw1.0 PCI WLAN.HK.2.5.0.1-01208-QCAHKSWPL_SILICONZ-1 > Tested-by: Robert Marko <robert.marko@sartura.hr> Great, thank you for testing this. Very much appreciated. I added the tags tags to the commit.
diff --git a/drivers/net/wireless/ath/ath11k/core.h b/drivers/net/wireless/ath/ath11k/core.h index a8acb8b7b8d5..beb552108ac3 100644 --- a/drivers/net/wireless/ath/ath11k/core.h +++ b/drivers/net/wireless/ath/ath11k/core.h @@ -921,7 +921,6 @@ struct ath11k_base { enum ath11k_dfs_region dfs_region; #ifdef CONFIG_ATH11K_DEBUGFS struct dentry *debugfs_soc; - struct dentry *debugfs_ath11k; #endif struct ath11k_soc_dp_stats soc_stats; diff --git a/drivers/net/wireless/ath/ath11k/debugfs.c b/drivers/net/wireless/ath/ath11k/debugfs.c index ccdf3d5ba1ab..5bb6fd17fdf6 100644 --- a/drivers/net/wireless/ath/ath11k/debugfs.c +++ b/drivers/net/wireless/ath/ath11k/debugfs.c @@ -976,10 +976,6 @@ int ath11k_debugfs_pdev_create(struct ath11k_base *ab) if (test_bit(ATH11K_FLAG_REGISTERED, &ab->dev_flags)) return 0; - ab->debugfs_soc = debugfs_create_dir(ab->hw_params.name, ab->debugfs_ath11k); - if (IS_ERR(ab->debugfs_soc)) - return PTR_ERR(ab->debugfs_soc); - debugfs_create_file("simulate_fw_crash", 0600, ab->debugfs_soc, ab, &fops_simulate_fw_crash); @@ -1001,15 +997,51 @@ void ath11k_debugfs_pdev_destroy(struct ath11k_base *ab) int ath11k_debugfs_soc_create(struct ath11k_base *ab) { - ab->debugfs_ath11k = debugfs_create_dir("ath11k", NULL); + struct dentry *root; + bool dput_needed; + char name[64]; + int ret; + + root = debugfs_lookup("ath11k", NULL); + if (!root) { + root = debugfs_create_dir("ath11k", NULL); + if (IS_ERR_OR_NULL(root)) + return PTR_ERR(root); + + dput_needed = false; + } else { + /* a dentry from lookup() needs dput() after we don't use it */ + dput_needed = true; + } + + scnprintf(name, sizeof(name), "%s-%s", ath11k_bus_str(ab->hif.bus), + dev_name(ab->dev)); + + ab->debugfs_soc = debugfs_create_dir(name, root); + if (IS_ERR_OR_NULL(ab->debugfs_soc)) { + ret = PTR_ERR(ab->debugfs_soc); + goto out; + } + + ret = 0; - return PTR_ERR_OR_ZERO(ab->debugfs_ath11k); +out: + if (dput_needed) + dput(root); + + return ret; } void ath11k_debugfs_soc_destroy(struct ath11k_base *ab) { - debugfs_remove_recursive(ab->debugfs_ath11k); - ab->debugfs_ath11k = NULL; + debugfs_remove_recursive(ab->debugfs_soc); + ab->debugfs_soc = NULL; + + /* We are not removing ath11k directory on purpose, even if it + * would be empty. This simplifies the directory handling and it's + * a minor cosmetic issue to leave an empty ath11k directory to + * debugfs. + */ } EXPORT_SYMBOL(ath11k_debugfs_soc_destroy);