| Message ID | 20190108040034.14380-1-dgilbert@interlog.com (mailing list archive) |
|---|---|
| State | Deferred |
| Headers | show |
| Series | [v5] scsi: add debugfs directories | expand |
On 08/01/2019 04:00, Douglas Gilbert wrote: > Add a top level "scsi" directory in debugfs (usually at > /sys/kernel/debug/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 subdirectories like > "scsi/uld/sd" and "scsi/uld/st" as required. LLDs could follow a > similar pattern. > > Changes since v4: > - remove export for /sys/kernel/debug/scsi directory but leave > the exports for the uld and lld subdirectories > - put externs for scsi_debugfs_uld and scsi_debugfs_lld in > include/scsi/scsi_dbg.h . So ULDs and LLDs now need to > include <scsi/scsi_dbg.h> to use this facility > - clean-up whitespaces and redundant code [John Garry] > > 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> I think that this gives somewhat better organisation to the debugfs root, so, apart from comments below, FWIW: Reviewed-by: John Garry <john.garry@huawei.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 sg v4 > patchset. > > John Garry has indicated that he is prepared to modify debugfs > patches to the hisi_sas driver to use the proposed > /sys/kernel/debug/scsi/lld directory. I can send a patch for this (even if kbuild robot will complain), which you can pick up. But I think that we still need to see the user of uld. > > drivers/scsi/scsi.c | 3 +++ > drivers/scsi/scsi_debugfs.c | 32 +++++++++++++++++++++++++++++++- > drivers/scsi/scsi_priv.h | 12 ++++++++++++ > include/scsi/scsi_dbg.h | 5 +++++ > 4 files changed, 51 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..5b0c001c150f 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; I'm going to sound like a broken record here (and maybe I did not understand the response to v4 review comment): For now, this only seems to be accessed from this file, so better to make static until referenced from elsewhere. As I see, only scsi_debugfs_uld and scsi_debugfs_lld would be referenced externally. > + > +struct dentry *scsi_debugfs_uld; > +EXPORT_SYMBOL(scsi_debugfs_uld); > + > +struct dentry *scsi_debugfs_lld; > +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,23 @@ 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) > +{ > + 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; > diff --git a/include/scsi/scsi_dbg.h b/include/scsi/scsi_dbg.h > index e03bd9d41fa8..00d121bf5eb2 100644 > --- a/include/scsi/scsi_dbg.h > +++ b/include/scsi/scsi_dbg.h > @@ -2,6 +2,11 @@ > #ifndef _SCSI_SCSI_DBG_H > #define _SCSI_SCSI_DBG_H > > +#ifdef CONFIG_BLK_DEBUG_FS > +extern struct dentry *scsi_debugfs_uld; > +extern struct dentry *scsi_debugfs_lld; > +#endif > + > struct scsi_cmnd; > struct scsi_device; > struct scsi_sense_hdr; >
On 09/01/2019 09:17, John Garry wrote: > On 08/01/2019 04:00, Douglas Gilbert wrote: >> Add a top level "scsi" directory in debugfs (usually at >> /sys/kernel/debug/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 subdirectories like >> "scsi/uld/sd" and "scsi/uld/st" as required. LLDs could follow a >> similar pattern. >> >> Changes since v4: >> - remove export for /sys/kernel/debug/scsi directory but leave >> the exports for the uld and lld subdirectories >> - put externs for scsi_debugfs_uld and scsi_debugfs_lld in >> include/scsi/scsi_dbg.h . So ULDs and LLDs now need to >> include <scsi/scsi_dbg.h> to use this facility >> - clean-up whitespaces and redundant code [John Garry] >> >> 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> > > I think that this gives somewhat better organisation to the debugfs > root, so, apart from comments below, FWIW: > Reviewed-by: John Garry <john.garry@huawei.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 sg v4 >> patchset. >> >> John Garry has indicated that he is prepared to modify debugfs >> patches to the hisi_sas driver to use the proposed >> /sys/kernel/debug/scsi/lld directory. > > I can send a patch for this (even if kbuild robot will complain), which > you can pick up. But I think that we still need to see the user of uld. > >> >> drivers/scsi/scsi.c | 3 +++ >> drivers/scsi/scsi_debugfs.c | 32 +++++++++++++++++++++++++++++++- >> drivers/scsi/scsi_priv.h | 12 ++++++++++++ >> include/scsi/scsi_dbg.h | 5 +++++ >> 4 files changed, 51 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..5b0c001c150f 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; > > I'm going to sound like a broken record here (and maybe I did not > understand the response to v4 review comment): For now, this only seems > to be accessed from this file, so better to make static until referenced > from elsewhere. As I see, only scsi_debugfs_uld and scsi_debugfs_lld > would be referenced externally. > >> + >> +struct dentry *scsi_debugfs_uld; >> +EXPORT_SYMBOL(scsi_debugfs_uld); >> + >> +struct dentry *scsi_debugfs_lld; >> +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,23 @@ 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) >> +{ >> + 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; And I am not sure why we have these defined as extern twice. John >> +#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; >> diff --git a/include/scsi/scsi_dbg.h b/include/scsi/scsi_dbg.h >> index e03bd9d41fa8..00d121bf5eb2 100644 >> --- a/include/scsi/scsi_dbg.h >> +++ b/include/scsi/scsi_dbg.h >> @@ -2,6 +2,11 @@ >> #ifndef _SCSI_SCSI_DBG_H >> #define _SCSI_SCSI_DBG_H >> >> +#ifdef CONFIG_BLK_DEBUG_FS >> +extern struct dentry *scsi_debugfs_uld; >> +extern struct dentry *scsi_debugfs_lld; >> +#endif >> + >> struct scsi_cmnd; >> struct scsi_device; >> struct scsi_sense_hdr; >> > > > > . >
On 2019-01-09 4:17 a.m., John Garry wrote: > On 08/01/2019 04:00, Douglas Gilbert wrote: >> Add a top level "scsi" directory in debugfs (usually at >> /sys/kernel/debug/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 subdirectories like >> "scsi/uld/sd" and "scsi/uld/st" as required. LLDs could follow a >> similar pattern. >> >> Changes since v4: >> - remove export for /sys/kernel/debug/scsi directory but leave >> the exports for the uld and lld subdirectories >> - put externs for scsi_debugfs_uld and scsi_debugfs_lld in >> include/scsi/scsi_dbg.h . So ULDs and LLDs now need to >> include <scsi/scsi_dbg.h> to use this facility >> - clean-up whitespaces and redundant code [John Garry] >> >> 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> > > I think that this gives somewhat better organisation to the debugfs root, so, > apart from comments below, FWIW: > Reviewed-by: John Garry <john.garry@huawei.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 sg v4 >> patchset. >> >> John Garry has indicated that he is prepared to modify debugfs >> patches to the hisi_sas driver to use the proposed >> /sys/kernel/debug/scsi/lld directory. > > I can send a patch for this (even if kbuild robot will complain), which you can > pick up. But I think that we still need to see the user of uld. > >> >> drivers/scsi/scsi.c | 3 +++ >> drivers/scsi/scsi_debugfs.c | 32 +++++++++++++++++++++++++++++++- >> drivers/scsi/scsi_priv.h | 12 ++++++++++++ >> include/scsi/scsi_dbg.h | 5 +++++ >> 4 files changed, 51 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..5b0c001c150f 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; > > I'm going to sound like a broken record here (and maybe I did not understand the > response to v4 review comment): For now, this only seems to be accessed from > this file, so better to make static until referenced from elsewhere. As I see, > only scsi_debugfs_uld and scsi_debugfs_lld would be referenced externally. My thinking here is that other parts of the scsi mid-level and transports (basically the files that include scsi_priv.h (16 files **)) to put debug- appropriate pseudo files in /sys/kernel/debug/scsi . From a quick scan of the "knobs" associated with the mid-level /sys/kernel/debug/scsi/scsi_logging_level looks like a candidate. Doug Gilbert ** one of those 16 files is sd.c . Hmmm, so much for ULDs not using scsi_priv.h > >> + >> +struct dentry *scsi_debugfs_uld; >> +EXPORT_SYMBOL(scsi_debugfs_uld); >> + >> +struct dentry *scsi_debugfs_lld; >> +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,23 @@ 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) >> +{ >> + 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; >> diff --git a/include/scsi/scsi_dbg.h b/include/scsi/scsi_dbg.h >> index e03bd9d41fa8..00d121bf5eb2 100644 >> --- a/include/scsi/scsi_dbg.h >> +++ b/include/scsi/scsi_dbg.h >> @@ -2,6 +2,11 @@ >> #ifndef _SCSI_SCSI_DBG_H >> #define _SCSI_SCSI_DBG_H >> >> +#ifdef CONFIG_BLK_DEBUG_FS >> +extern struct dentry *scsi_debugfs_uld; >> +extern struct dentry *scsi_debugfs_lld; >> +#endif >> + >> struct scsi_cmnd; >> struct scsi_device; >> struct scsi_sense_hdr; >> > > >
On 2019-01-09 7:24 a.m., John Garry wrote: > On 09/01/2019 09:17, John Garry wrote: >> On 08/01/2019 04:00, Douglas Gilbert wrote: >>> Add a top level "scsi" directory in debugfs (usually at >>> /sys/kernel/debug/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 subdirectories like >>> "scsi/uld/sd" and "scsi/uld/st" as required. LLDs could follow a >>> similar pattern. >>> >>> Changes since v4: >>> - remove export for /sys/kernel/debug/scsi directory but leave >>> the exports for the uld and lld subdirectories >>> - put externs for scsi_debugfs_uld and scsi_debugfs_lld in >>> include/scsi/scsi_dbg.h . So ULDs and LLDs now need to >>> include <scsi/scsi_dbg.h> to use this facility >>> - clean-up whitespaces and redundant code [John Garry] >>> >>> 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> >> >> I think that this gives somewhat better organisation to the debugfs >> root, so, apart from comments below, FWIW: >> Reviewed-by: John Garry <john.garry@huawei.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 sg v4 >>> patchset. >>> >>> John Garry has indicated that he is prepared to modify debugfs >>> patches to the hisi_sas driver to use the proposed >>> /sys/kernel/debug/scsi/lld directory. >> >> I can send a patch for this (even if kbuild robot will complain), which >> you can pick up. But I think that we still need to see the user of uld. >> >>> >>> drivers/scsi/scsi.c | 3 +++ >>> drivers/scsi/scsi_debugfs.c | 32 +++++++++++++++++++++++++++++++- >>> drivers/scsi/scsi_priv.h | 12 ++++++++++++ >>> include/scsi/scsi_dbg.h | 5 +++++ >>> 4 files changed, 51 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..5b0c001c150f 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; >> >> I'm going to sound like a broken record here (and maybe I did not >> understand the response to v4 review comment): For now, this only seems >> to be accessed from this file, so better to make static until referenced >> from elsewhere. As I see, only scsi_debugfs_uld and scsi_debugfs_lld >> would be referenced externally. >> >>> + >>> +struct dentry *scsi_debugfs_uld; >>> +EXPORT_SYMBOL(scsi_debugfs_uld); >>> + >>> +struct dentry *scsi_debugfs_lld; >>> +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,23 @@ 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) >>> +{ >>> + 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; > > And I am not sure why we have these defined as extern twice. Once in "scsi_priv.h" for the mid-level and transports, once in <scsi/scsi_dbg.h> for ULDs and LLDs. There are some mid-level and transport files that include both (plus sd.c). I commented out "#include "scsi_priv.h" in sd.c to see what uses that header: async_schedule_domain() async_synchronize_full_domain() added by Dan Williams a while back. Doug Gilbert > John > >>> +#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; >>> diff --git a/include/scsi/scsi_dbg.h b/include/scsi/scsi_dbg.h >>> index e03bd9d41fa8..00d121bf5eb2 100644 >>> --- a/include/scsi/scsi_dbg.h >>> +++ b/include/scsi/scsi_dbg.h >>> @@ -2,6 +2,11 @@ >>> #ifndef _SCSI_SCSI_DBG_H >>> #define _SCSI_SCSI_DBG_H >>> >>> +#ifdef CONFIG_BLK_DEBUG_FS >>> +extern struct dentry *scsi_debugfs_uld; >>> +extern struct dentry *scsi_debugfs_lld; >>> +#endif >>> + >>> struct scsi_cmnd; >>> struct scsi_device; >>> struct scsi_sense_hdr; >>> >> >> >> >> . >> > > >
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..5b0c001c150f 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; +EXPORT_SYMBOL(scsi_debugfs_uld); + +struct dentry *scsi_debugfs_lld; +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,23 @@ 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) +{ + 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; diff --git a/include/scsi/scsi_dbg.h b/include/scsi/scsi_dbg.h index e03bd9d41fa8..00d121bf5eb2 100644 --- a/include/scsi/scsi_dbg.h +++ b/include/scsi/scsi_dbg.h @@ -2,6 +2,11 @@ #ifndef _SCSI_SCSI_DBG_H #define _SCSI_SCSI_DBG_H +#ifdef CONFIG_BLK_DEBUG_FS +extern struct dentry *scsi_debugfs_uld; +extern struct dentry *scsi_debugfs_lld; +#endif + struct scsi_cmnd; struct scsi_device; struct scsi_sense_hdr;
Add a top level "scsi" directory in debugfs (usually at /sys/kernel/debug/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 subdirectories like "scsi/uld/sd" and "scsi/uld/st" as required. LLDs could follow a similar pattern. Changes since v4: - remove export for /sys/kernel/debug/scsi directory but leave the exports for the uld and lld subdirectories - put externs for scsi_debugfs_uld and scsi_debugfs_lld in include/scsi/scsi_dbg.h . So ULDs and LLDs now need to include <scsi/scsi_dbg.h> to use this facility - clean-up whitespaces and redundant code [John Garry] 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 sg v4 patchset. John Garry has indicated that he is prepared to modify debugfs patches to the hisi_sas driver to use the proposed /sys/kernel/debug/scsi/lld directory. drivers/scsi/scsi.c | 3 +++ drivers/scsi/scsi_debugfs.c | 32 +++++++++++++++++++++++++++++++- drivers/scsi/scsi_priv.h | 12 ++++++++++++ include/scsi/scsi_dbg.h | 5 +++++ 4 files changed, 51 insertions(+), 1 deletion(-)