diff mbox series

[V2] scsi: ufs-debugfs: Add error counters

Message ID 20201216185145.25800-1-adrian.hunter@intel.com (mailing list archive)
State Superseded
Headers show
Series [V2] scsi: ufs-debugfs: Add error counters | expand

Commit Message

Adrian Hunter Dec. 16, 2020, 6:51 p.m. UTC 
People testing have a need to know how many errors might be occurring
over time. Add error counters and expose them via debugfs.

Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
---


Changes in V2:
	Add missing '#include "ufs-debugfs.h"' in ufs-debugfs.c
	Reported-by: kernel test robot <lkp@intel.com>


 drivers/scsi/ufs/Makefile      |  1 +
 drivers/scsi/ufs/ufs-debugfs.c | 56 ++++++++++++++++++++++++++++++++++
 drivers/scsi/ufs/ufs-debugfs.h | 22 +++++++++++++
 drivers/scsi/ufs/ufshcd.c      | 19 ++++++++++++
 drivers/scsi/ufs/ufshcd.h      |  5 +++
 5 files changed, 103 insertions(+)
 create mode 100644 drivers/scsi/ufs/ufs-debugfs.c
 create mode 100644 drivers/scsi/ufs/ufs-debugfs.h

Comments

Bean Huo Dec. 16, 2020, 8:16 p.m. UTC | #1 
On Wed, 2020-12-16 at 20:51 +0200, Adrian Hunter wrote:
>                 ufshcd_variant_hba_exit(hba);
>                 ufshcd_setup_vreg(hba, false);
>                 ufshcd_suspend_clkscaling(hba);
> @@ -9436,6 +9441,20 @@ int ufshcd_init(struct ufs_hba *hba, void
> __iomem *mmio_base, unsigned int irq)
>  }
>  EXPORT_SYMBOL_GPL(ufshcd_init);
>  
> +static int __init ufshcd_core_init(void)
> +{
> +       ufs_debugfs_init();
> +       return 0;
> +}
> +
> +static void __exit ufshcd_core_exit(void)
> +{
> +       ufs_debugfs_exit();
> +}

Hi, Adrian

The purpose of patch is acceptable, but I don't know why you choose
using ufshcd_core_* here. 
Also. I don't know if module_init()  is a proper way here.

thanks,
Bean

> 
> n
> +
> +module_init(ufshcd_core_init);
> +module_exit(ufshcd_core_exit)
Adrian Hunter Dec. 17, 2020, 9:49 a.m. UTC | #2 
On 16/12/20 10:16 pm, Bean Huo wrote:
> On Wed, 2020-12-16 at 20:51 +0200, Adrian Hunter wrote:
>>                 ufshcd_variant_hba_exit(hba);
>>                 ufshcd_setup_vreg(hba, false);
>>                 ufshcd_suspend_clkscaling(hba);
>> @@ -9436,6 +9441,20 @@ int ufshcd_init(struct ufs_hba *hba, void
>> __iomem *mmio_base, unsigned int irq)
>>  }
>>  EXPORT_SYMBOL_GPL(ufshcd_init);
>>  
>> +static int __init ufshcd_core_init(void)
>> +{
>> +       ufs_debugfs_init();
>> +       return 0;
>> +}
>> +
>> +static void __exit ufshcd_core_exit(void)
>> +{
>> +       ufs_debugfs_exit();
>> +}
> 
> Hi, Adrian

Thanks for looking at the patch.

> 
> The purpose of patch is acceptable, but I don't know why you choose
> using ufshcd_core_* here. 

Do you mean you would like a different function name?  'ufshcd_init' is used
already.  The module is called ufshcd-core, so ufshcd_core_* seems appropriate.

> Also. I don't know if module_init()  is a proper way here.

Can you be more specific?  It is normal to do module initialization in
module_init().

However I do see a problem.  When builtin, initcalls are ordered according
to link order, but the Makefile does not have ufshcd-core at the top i.e.

$ cat drivers/scsi/ufs/Makefile
# SPDX-License-Identifier: GPL-2.0
# UFSHCD makefile
obj-$(CONFIG_SCSI_UFS_DWC_TC_PCI) += tc-dwc-g210-pci.o ufshcd-dwc.o
tc-dwc-g210.o
obj-$(CONFIG_SCSI_UFS_DWC_TC_PLATFORM) += tc-dwc-g210-pltfrm.o ufshcd-dwc.o
tc-dwc-g210.o
obj-$(CONFIG_SCSI_UFS_CDNS_PLATFORM) += cdns-pltfrm.o
obj-$(CONFIG_SCSI_UFS_QCOM) += ufs_qcom.o
ufs_qcom-y += ufs-qcom.o
ufs_qcom-$(CONFIG_SCSI_UFS_CRYPTO) += ufs-qcom-ice.o
obj-$(CONFIG_SCSI_UFS_EXYNOS) += ufs-exynos.o
obj-$(CONFIG_SCSI_UFSHCD) += ufshcd-core.o
ufshcd-core-y                           += ufshcd.o ufs-sysfs.o
ufshcd-core-$(CONFIG_DEBUG_FS)          += ufs-debugfs.o
ufshcd-core-$(CONFIG_SCSI_UFS_BSG)      += ufs_bsg.o
ufshcd-core-$(CONFIG_SCSI_UFS_CRYPTO) += ufshcd-crypto.o
obj-$(CONFIG_SCSI_UFSHCD_PCI) += ufshcd-pci.o
obj-$(CONFIG_SCSI_UFSHCD_PLATFORM) += ufshcd-pltfrm.o
obj-$(CONFIG_SCSI_UFS_HISI) += ufs-hisi.o
obj-$(CONFIG_SCSI_UFS_MEDIATEK) += ufs-mediatek.o
obj-$(CONFIG_SCSI_UFS_TI_J721E) += ti-j721e-ufs.o

Should be:

# SPDX-License-Identifier: GPL-2.0
# UFSHCD makefile
# Order here is important. ufshcd-core must initialize first.
ufshcd-core-y                           += ufshcd.o ufs-sysfs.o
ufshcd-core-$(CONFIG_DEBUG_FS)          += ufs-debugfs.o
ufshcd-core-$(CONFIG_SCSI_UFS_BSG)      += ufs_bsg.o
ufshcd-core-$(CONFIG_SCSI_UFS_CRYPTO) += ufshcd-crypto.o
obj-$(CONFIG_SCSI_UFS_DWC_TC_PCI) += tc-dwc-g210-pci.o ufshcd-dwc.o
tc-dwc-g210.o
obj-$(CONFIG_SCSI_UFS_DWC_TC_PLATFORM) += tc-dwc-g210-pltfrm.o ufshcd-dwc.o
tc-dwc-g210.o
obj-$(CONFIG_SCSI_UFS_CDNS_PLATFORM) += cdns-pltfrm.o
obj-$(CONFIG_SCSI_UFS_QCOM) += ufs_qcom.o
ufs_qcom-y += ufs-qcom.o
ufs_qcom-$(CONFIG_SCSI_UFS_CRYPTO) += ufs-qcom-ice.o
obj-$(CONFIG_SCSI_UFS_EXYNOS) += ufs-exynos.o
obj-$(CONFIG_SCSI_UFSHCD) += ufshcd-core.o
obj-$(CONFIG_SCSI_UFSHCD_PCI) += ufshcd-pci.o
obj-$(CONFIG_SCSI_UFSHCD_PLATFORM) += ufshcd-pltfrm.o
obj-$(CONFIG_SCSI_UFS_HISI) += ufs-hisi.o
obj-$(CONFIG_SCSI_UFS_MEDIATEK) += ufs-mediatek.o
obj-$(CONFIG_SCSI_UFS_TI_J721E) += ti-j721e-ufs.o

What do you think?

> 
> thanks,
> Bean
> 
>>
>> n
>> +
>> +module_init(ufshcd_core_init);
>> +module_exit(ufshcd_core_exit)
>
Avri Altman Dec. 17, 2020, 11:06 a.m. UTC | #3 
Please add my reviewed-by once you fix that.
Thanks,
Avri

> However I do see a problem.  When builtin, initcalls are ordered according
> to link order, but the Makefile does not have ufshcd-core at the top i.e.
> 
> $ cat drivers/scsi/ufs/Makefile
> # SPDX-License-Identifier: GPL-2.0
> # UFSHCD makefile
> obj-$(CONFIG_SCSI_UFS_DWC_TC_PCI) += tc-dwc-g210-pci.o ufshcd-dwc.o
> tc-dwc-g210.o
> obj-$(CONFIG_SCSI_UFS_DWC_TC_PLATFORM) += tc-dwc-g210-pltfrm.o
> ufshcd-dwc.o
> tc-dwc-g210.o
> obj-$(CONFIG_SCSI_UFS_CDNS_PLATFORM) += cdns-pltfrm.o
> obj-$(CONFIG_SCSI_UFS_QCOM) += ufs_qcom.o
> ufs_qcom-y += ufs-qcom.o
> ufs_qcom-$(CONFIG_SCSI_UFS_CRYPTO) += ufs-qcom-ice.o
> obj-$(CONFIG_SCSI_UFS_EXYNOS) += ufs-exynos.o
> obj-$(CONFIG_SCSI_UFSHCD) += ufshcd-core.o
> ufshcd-core-y                           += ufshcd.o ufs-sysfs.o
> ufshcd-core-$(CONFIG_DEBUG_FS)          += ufs-debugfs.o
> ufshcd-core-$(CONFIG_SCSI_UFS_BSG)      += ufs_bsg.o
> ufshcd-core-$(CONFIG_SCSI_UFS_CRYPTO) += ufshcd-crypto.o
> obj-$(CONFIG_SCSI_UFSHCD_PCI) += ufshcd-pci.o
> obj-$(CONFIG_SCSI_UFSHCD_PLATFORM) += ufshcd-pltfrm.o
> obj-$(CONFIG_SCSI_UFS_HISI) += ufs-hisi.o
> obj-$(CONFIG_SCSI_UFS_MEDIATEK) += ufs-mediatek.o
> obj-$(CONFIG_SCSI_UFS_TI_J721E) += ti-j721e-ufs.o
> 
> Should be:
> 
> # SPDX-License-Identifier: GPL-2.0
> # UFSHCD makefile
> # Order here is important. ufshcd-core must initialize first.
> ufshcd-core-y                           += ufshcd.o ufs-sysfs.o
> ufshcd-core-$(CONFIG_DEBUG_FS)          += ufs-debugfs.o
> ufshcd-core-$(CONFIG_SCSI_UFS_BSG)      += ufs_bsg.o
> ufshcd-core-$(CONFIG_SCSI_UFS_CRYPTO) += ufshcd-crypto.o
> obj-$(CONFIG_SCSI_UFS_DWC_TC_PCI) += tc-dwc-g210-pci.o ufshcd-dwc.o
> tc-dwc-g210.o
> obj-$(CONFIG_SCSI_UFS_DWC_TC_PLATFORM) += tc-dwc-g210-pltfrm.o
> ufshcd-dwc.o
> tc-dwc-g210.o
> obj-$(CONFIG_SCSI_UFS_CDNS_PLATFORM) += cdns-pltfrm.o
> obj-$(CONFIG_SCSI_UFS_QCOM) += ufs_qcom.o
> ufs_qcom-y += ufs-qcom.o
> ufs_qcom-$(CONFIG_SCSI_UFS_CRYPTO) += ufs-qcom-ice.o
> obj-$(CONFIG_SCSI_UFS_EXYNOS) += ufs-exynos.o
> obj-$(CONFIG_SCSI_UFSHCD) += ufshcd-core.o
> obj-$(CONFIG_SCSI_UFSHCD_PCI) += ufshcd-pci.o
> obj-$(CONFIG_SCSI_UFSHCD_PLATFORM) += ufshcd-pltfrm.o
> obj-$(CONFIG_SCSI_UFS_HISI) += ufs-hisi.o
> obj-$(CONFIG_SCSI_UFS_MEDIATEK) += ufs-mediatek.o
> obj-$(CONFIG_SCSI_UFS_TI_J721E) += ti-j721e-ufs.o
> 
> What do you think?
> 
> >
> > thanks,
> > Bean
> >
> >>
> >> n
> >> +
> >> +module_init(ufshcd_core_init);
> >> +module_exit(ufshcd_core_exit)
> >
Bean Huo Dec. 17, 2020, 10:57 p.m. UTC | #4 
On Thu, 2020-12-17 at 11:49 +0200, Adrian Hunter wrote:
> > 
> > The purpose of patch is acceptable, but I don't know why you choose
> > using ufshcd_core_* here. 
> 
> Do you mean you would like a different function name?  'ufshcd_init'
> is used
> already.  The module is called ufshcd-core, so ufshcd_core_* seems
> appropriate.
> 
> > Also. I don't know if module_init()  is a proper way here.
> 
> Can you be more specific?  It is normal to do module initialization
> in
> module_init().

Hi Adrian
My concern that ufs_debugfs_init() is called in module_init(), but your
another debugfs initialization function ufs_debugfs_hba_init(hba)
called in the UFS host probe path. 

If these two (module_init() and module_platform_driver())
initializaiton sequence always as your expectation: ufs_debugfs_init()-
->ufs_debugfs_hba_init(), that is fine, otherwise, it is better just
group them, make it simpler.


Thanks,
Bean
Adrian Hunter Dec. 18, 2020, 12:17 p.m. UTC | #5 
On 18/12/20 12:57 am, Bean Huo wrote:
> On Thu, 2020-12-17 at 11:49 +0200, Adrian Hunter wrote:
>>>
>>> The purpose of patch is acceptable, but I don't know why you choose
>>> using ufshcd_core_* here. 
>>
>> Do you mean you would like a different function name?  'ufshcd_init'
>> is used
>> already.  The module is called ufshcd-core, so ufshcd_core_* seems
>> appropriate.
>>
>>> Also. I don't know if module_init()  is a proper way here.
>>
>> Can you be more specific?  It is normal to do module initialization
>> in
>> module_init().
> 
> Hi Adrian
> My concern that ufs_debugfs_init() is called in module_init(), but your
> another debugfs initialization function ufs_debugfs_hba_init(hba)
> called in the UFS host probe path. 

It is a good question, but module dependencies and initcall ordering means
that won't happen.  It is not unusual for modules to do initialization in
this way, that is completed before dependent modules initialize.

> 
> If these two (module_init() and module_platform_driver())
> initializaiton sequence always as your expectation: ufs_debugfs_init()-
> ->ufs_debugfs_hba_init(), that is fine, otherwise, it is better just
> group them, make it simpler.

Unfortunately, doing it that way, calls to debugsfs_create_dir("ufshcd",
NULL) could race. Losers of the race will get an error, and will not get the
dentry they need to proceed.  Preventing the race would require adding a
mutex.  So the other way is simpler.
diff mbox series

Patch

diff --git a/drivers/scsi/ufs/Makefile b/drivers/scsi/ufs/Makefile
index 4679af1b564e..9ca32b1143cb 100644
--- a/drivers/scsi/ufs/Makefile
+++ b/drivers/scsi/ufs/Makefile
@@ -9,6 +9,7 @@  ufs_qcom-$(CONFIG_SCSI_UFS_CRYPTO) += ufs-qcom-ice.o
 obj-$(CONFIG_SCSI_UFS_EXYNOS) += ufs-exynos.o
 obj-$(CONFIG_SCSI_UFSHCD) += ufshcd-core.o
 ufshcd-core-y				+= ufshcd.o ufs-sysfs.o
+ufshcd-core-$(CONFIG_DEBUG_FS)		+= ufs-debugfs.o
 ufshcd-core-$(CONFIG_SCSI_UFS_BSG)	+= ufs_bsg.o
 ufshcd-core-$(CONFIG_SCSI_UFS_CRYPTO) += ufshcd-crypto.o
 obj-$(CONFIG_SCSI_UFSHCD_PCI) += ufshcd-pci.o
diff --git a/drivers/scsi/ufs/ufs-debugfs.c b/drivers/scsi/ufs/ufs-debugfs.c
new file mode 100644
index 000000000000..e7e6e2dba346
--- /dev/null
+++ b/drivers/scsi/ufs/ufs-debugfs.c
@@ -0,0 +1,56 @@ 
+// SPDX-License-Identifier: GPL-2.0
+// Copyright (C) 2020 Intel Corporation
+
+#include <linux/debugfs.h>
+
+#include "ufs-debugfs.h"
+#include "ufshcd.h"
+
+static struct dentry *ufs_debugfs_root;
+
+void ufs_debugfs_init(void)
+{
+	ufs_debugfs_root = debugfs_create_dir("ufshcd", NULL);
+}
+
+void ufs_debugfs_exit(void)
+{
+	debugfs_remove_recursive(ufs_debugfs_root);
+}
+
+static int ufs_debugfs_stats_show(struct seq_file *s, void *data)
+{
+	struct ufs_hba *hba = s->private;
+	struct ufs_event_hist *e = hba->ufs_stats.event;
+
+#define PRT(fmt, typ) \
+	seq_printf(s, fmt, e[UFS_EVT_ ## typ].cnt)
+
+	PRT("PHY Adapter Layer errors (except LINERESET): %llu\n", PA_ERR);
+	PRT("Data Link Layer errors: %llu\n", DL_ERR);
+	PRT("Network Layer errors: %llu\n", NL_ERR);
+	PRT("Transport Layer errors: %llu\n", TL_ERR);
+	PRT("Generic DME errors: %llu\n", DME_ERR);
+	PRT("Auto-hibernate errors: %llu\n", AUTO_HIBERN8_ERR);
+	PRT("IS Fatal errors (CEFES, SBFES, HCFES, DFES): %llu\n", FATAL_ERR);
+	PRT("DME Link Startup errors: %llu\n", LINK_STARTUP_FAIL);
+	PRT("PM Resume errors: %llu\n", RESUME_ERR);
+	PRT("PM Suspend errors : %llu\n", SUSPEND_ERR);
+	PRT("Logical Unit Resets: %llu\n", DEV_RESET);
+	PRT("Host Resets: %llu\n", HOST_RESET);
+	PRT("SCSI command aborts: %llu\n", ABORT);
+#undef PRT
+	return 0;
+}
+DEFINE_SHOW_ATTRIBUTE(ufs_debugfs_stats);
+
+void ufs_debugfs_hba_init(struct ufs_hba *hba)
+{
+	hba->debugfs_root = debugfs_create_dir(dev_name(hba->dev), ufs_debugfs_root);
+	debugfs_create_file("stats", 0400, hba->debugfs_root, hba, &ufs_debugfs_stats_fops);
+}
+
+void ufs_debugfs_hba_exit(struct ufs_hba *hba)
+{
+	debugfs_remove_recursive(hba->debugfs_root);
+}
diff --git a/drivers/scsi/ufs/ufs-debugfs.h b/drivers/scsi/ufs/ufs-debugfs.h
new file mode 100644
index 000000000000..d5e744c7bd2d
--- /dev/null
+++ b/drivers/scsi/ufs/ufs-debugfs.h
@@ -0,0 +1,22 @@ 
+/* SPDX-License-Identifier: GPL-2.0 */
+/* Copyright (C) 2020 Intel Corporation
+ */
+
+#ifndef __UFS_DEBUGFS_H__
+#define __UFS_DEBUGFS_H__
+
+struct ufs_hba;
+
+#ifdef CONFIG_DEBUG_FS
+void ufs_debugfs_init(void);
+void ufs_debugfs_exit(void);
+void ufs_debugfs_hba_init(struct ufs_hba *hba);
+void ufs_debugfs_hba_exit(struct ufs_hba *hba);
+#else
+static inline void ufs_debugfs_init(void) {}
+static inline void ufs_debugfs_exit(void) {}
+static inline void ufs_debugfs_hba_init(struct ufs_hba *hba) {}
+static inline void ufs_debugfs_hba_exit(struct ufs_hba *hba) {}
+#endif
+
+#endif
diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 82ad31781bc9..d8a3cf0cd6d5 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -20,6 +20,7 @@ 
 #include "ufs_quirks.h"
 #include "unipro.h"
 #include "ufs-sysfs.h"
+#include "ufs-debugfs.h"
 #include "ufs_bsg.h"
 #include "ufshcd-crypto.h"
 #include <asm/unaligned.h>
@@ -4540,6 +4541,7 @@  void ufshcd_update_evt_hist(struct ufs_hba *hba, u32 id, u32 val)
 	e = &hba->ufs_stats.event[id];
 	e->val[e->pos] = val;
 	e->tstamp[e->pos] = ktime_get();
+	e->cnt += 1;
 	e->pos = (e->pos + 1) % UFS_EVENT_HIST_LENGTH;
 
 	ufshcd_vops_event_notify(hba, id, &val);
@@ -8334,6 +8336,8 @@  static int ufshcd_hba_init(struct ufs_hba *hba)
 	if (err)
 		goto out_disable_vreg;
 
+	ufs_debugfs_hba_init(hba);
+
 	hba->is_powered = true;
 	goto out;
 
@@ -8350,6 +8354,7 @@  static int ufshcd_hba_init(struct ufs_hba *hba)
 static void ufshcd_hba_exit(struct ufs_hba *hba)
 {
 	if (hba->is_powered) {
+		ufs_debugfs_hba_exit(hba);
 		ufshcd_variant_hba_exit(hba);
 		ufshcd_setup_vreg(hba, false);
 		ufshcd_suspend_clkscaling(hba);
@@ -9436,6 +9441,20 @@  int ufshcd_init(struct ufs_hba *hba, void __iomem *mmio_base, unsigned int irq)
 }
 EXPORT_SYMBOL_GPL(ufshcd_init);
 
+static int __init ufshcd_core_init(void)
+{
+	ufs_debugfs_init();
+	return 0;
+}
+
+static void __exit ufshcd_core_exit(void)
+{
+	ufs_debugfs_exit();
+}
+
+module_init(ufshcd_core_init);
+module_exit(ufshcd_core_exit);
+
 MODULE_AUTHOR("Santosh Yaragnavi <santosh.sy@samsung.com>");
 MODULE_AUTHOR("Vinayak Holikatti <h.vinayak@samsung.com>");
 MODULE_DESCRIPTION("Generic UFS host controller driver Core");
diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
index aa9ea3552323..8c51ce01517e 100644
--- a/drivers/scsi/ufs/ufshcd.h
+++ b/drivers/scsi/ufs/ufshcd.h
@@ -445,11 +445,13 @@  struct ufs_clk_scaling {
  * @pos: index to indicate cyclic buffer position
  * @reg: cyclic buffer for registers value
  * @tstamp: cyclic buffer for time stamp
+ * @cnt: error counter
  */
 struct ufs_event_hist {
 	int pos;
 	u32 val[UFS_EVENT_HIST_LENGTH];
 	ktime_t tstamp[UFS_EVENT_HIST_LENGTH];
+	unsigned long long cnt;
 };
 
 /**
@@ -817,6 +819,9 @@  struct ufs_hba {
 	u32 crypto_cfg_register;
 	struct blk_keyslot_manager ksm;
 #endif
+#ifdef CONFIG_DEBUG_FS
+	struct dentry *debugfs_root;
+#endif
 };
 
 /* Returns true if clocks can be gated. Otherwise false */