John,
See response below.
On 2019-01-07 10:38 a.m., John Garry wrote:
On 05/01/2019 01:52, Douglas Gilbert wrote:
Add a top level "scsi" directory in debugfs (usually at
/sys/kernel/debugfs/scsi) with two subdirectories: "uld" and "lld".
The idea is to place mid-level related 'knobs' in the "scsi"
directory, and for the ULDs to make subsirectories like
"scsi/uld/sd" and "scsi/uld/st" as required. LLDs could follow a
similar pattern.
Hi Doug,
Some comments, below:
Changes since v3:
- re-arrange as per scsi netlink interface [James Bottomley]
- make all debugfs code conditional on CONFIG_BLK_DEBUG_FS
Changes since v2:
- export symbols so other driver can use them [John Garry]
- make prior code conditional on CONFIG_BLK_DEBUG_FS
Changes since v1:
- tweak Makefile to keep kbuild test robot happier
Signed-off-by: Douglas Gilbert <dgilbert@xxxxxxxxxxxx>
---
My intention is to add a /sys/kernel/debug/scsi/uld/sg
directory containing at least a pseudo file called debug to have
the same actions as /proc/scsi/sg/debug , as part of my v4
patchset.
drivers/scsi/scsi.c | 3 +++
drivers/scsi/scsi_debugfs.c | 33 ++++++++++++++++++++++++++++++++-
drivers/scsi/scsi_priv.h | 12 ++++++++++++
3 files changed, 47 insertions(+), 1 deletion(-)
diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
index fc1356d101b0..e8676a19ba6e 100644
--- a/drivers/scsi/scsi.c
+++ b/drivers/scsi/scsi.c
@@ -812,6 +812,8 @@ static int __init init_scsi(void)
scsi_netlink_init();
+ scsi_debugfs_init();
+
printk(KERN_NOTICE "SCSI subsystem initialized\n");
return 0;
@@ -832,6 +834,7 @@ static int __init init_scsi(void)
static void __exit exit_scsi(void)
{
+ scsi_debugfs_exit();
scsi_netlink_exit();
scsi_sysfs_unregister();
scsi_exit_sysctl();
diff --git a/drivers/scsi/scsi_debugfs.c b/drivers/scsi/scsi_debugfs.c
index c5a8756384bc..20d4cb0fa58b 100644
--- a/drivers/scsi/scsi_debugfs.c
+++ b/drivers/scsi/scsi_debugfs.c
@@ -1,8 +1,18 @@
// SPDX-License-Identifier: GPL-2.0
#include <linux/seq_file.h>
+#include <linux/debugfs.h>
#include <scsi/scsi_cmnd.h>
#include <scsi/scsi_dbg.h>
-#include "scsi_debugfs.h"
+#include "scsi_priv.h"
+
+struct dentry *scsi_debugfs_root;
Can this be static, i.e. not exported?
Yes. I'll make in global, non-exported so the mid-level (and
transports ?) can add stuff in the /sys/kernel/debug/scsi
directory.
+struct dentry *scsi_debugfs_uld;
+struct dentry *scsi_debugfs_lld;
+
+EXPORT_SYMBOL(scsi_debugfs_root);
+EXPORT_SYMBOL(scsi_debugfs_uld);
+EXPORT_SYMBOL(scsi_debugfs_lld);
+
#define SCSI_CMD_FLAG_NAME(name)[const_ilog2(SCMD_##name)] = #name
static const char *const scsi_cmd_flags[] = {
@@ -50,3 +60,24 @@ void scsi_show_rq(struct seq_file *m, struct request *rq)
timeout_ms / 1000, timeout_ms % 1000,
alloc_ms / 1000, alloc_ms % 1000);
}
+
+void scsi_debugfs_init(void)
+{
+ scsi_debugfs_root = debugfs_create_dir("scsi", NULL);
+ if (!scsi_debugfs_root)
+ return;
This seems to be indented with whitespaces.
My bad. How could I have forgotten to use my favourite whitespace checker?
+ scsi_debugfs_uld = debugfs_create_dir("uld", scsi_debugfs_root);
+ if (!scsi_debugfs_uld) {
+ scsi_debugfs_exit();
+ return;
+ }
Strange indentation.
+ scsi_debugfs_lld = debugfs_create_dir("lld", scsi_debugfs_root);
+ if (!scsi_debugfs_lld)
+ scsi_debugfs_exit();
+}
+
+void scsi_debugfs_exit(void)
+{
+ if (scsi_debugfs_root)
I think debugfs_remove_recursive() can safely handle NULL as an argument.
checkpatch.pl picked that one up as well.
+ debugfs_remove_recursive(scsi_debugfs_root);
+}
diff --git a/drivers/scsi/scsi_priv.h b/drivers/scsi/scsi_priv.h
index 99f1db5e467e..e24835e8fa4f 100644
--- a/drivers/scsi/scsi_priv.h
+++ b/drivers/scsi/scsi_priv.h
I am suspicious that this header should not be included outside driver/scsi,
where some scsi LLDs exist.
Yes, it only seems to be included in mid-level and transport function files;
not in ULDs or LLDs. So I add this:
#ifdef CONFIG_BLK_DEBUG_FS
extern struct dentry *scsi_debugfs_uld;
extern struct dentry *scsi_debugfs_lld;
#endif
in include/scsi/scsi_dbg.h
@@ -160,6 +160,18 @@ static inline void scsi_netlink_init(void) {}
static inline void scsi_netlink_exit(void) {}
#endif
+/* scsi_debugfs.c */
+#ifdef CONFIG_BLK_DEBUG_FS
+extern void scsi_debugfs_init(void);
+extern void scsi_debugfs_exit(void);
+extern struct dentry *scsi_debugfs_root;
+extern struct dentry *scsi_debugfs_uld;
+extern struct dentry *scsi_debugfs_lld;
+#else
+static inline void scsi_debugfs_init(void) {}
+static inline void scsi_debugfs_exit(void) {}
+#endif
+
/* scsi_pm.c */
#ifdef CONFIG_PM
extern const struct dev_pm_ops scsi_bus_pm_ops;
You can include this change as a user of the lld folder if you want:
diff --git a/drivers/scsi/hisi_sas/hisi_sas.h b/drivers/scsi/hisi_sas/hisi_sas.h
index 6a1a5ad..964d34d8 100644
--- a/drivers/scsi/hisi_sas/hisi_sas.h
+++ b/drivers/scsi/hisi_sas/hisi_sas.h
@@ -28,6 +28,8 @@
#include <scsi/sas_ata.h>
#include <scsi/libsas.h>
+#include "../scsi_priv.h"
So now the above line will be replaced by:
#include <scsi/scsi_dbg.h>
That seems cleaner.
+
#define HISI_SAS_MAX_PHYS 9
#define HISI_SAS_MAX_QUEUES 32
#define HISI_SAS_QUEUE_SLOTS 512
diff --git a/drivers/scsi/hisi_sas/hisi_sas_main.c
b/drivers/scsi/hisi_sas/hisi_sas_main.c
index 5fe13e9..bc8f014 100644
--- a/drivers/scsi/hisi_sas/hisi_sas_main.c
+++ b/drivers/scsi/hisi_sas/hisi_sas_main.c
@@ -3018,7 +3018,8 @@ static __init int hisi_sas_init(void)
return -ENOMEM;
if (hisi_sas_debugfs_enable)
- hisi_sas_debugfs_dir = debugfs_create_dir("hisi_sas", NULL);
+ hisi_sas_debugfs_dir = debugfs_create_dir("hisi_sas",
+ scsi_debugfs_lld);
return 0;
}
Thanks for the review and testing; version 5 coming.
Doug Gilbert
