| Message ID | 20190105015254.4606-1-dgilbert@interlog.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v4] scsi: add debugfs directories | expand |
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@interlog.com> > --- > > 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? > +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. > + 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. > + 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. > @@ -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" + #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, John
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@interlog.com> >> --- >> >> 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
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; +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; + scsi_debugfs_uld = debugfs_create_dir("uld", scsi_debugfs_root); + if (!scsi_debugfs_uld) { + scsi_debugfs_exit(); + return; + } + 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) + 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 @@ -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;
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. 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@interlog.com> --- 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(-)