| Message ID | 20190104161836.8276-1-dgilbert@interlog.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v3] scsi: add debugfs directories | expand |
On Fri, 2019-01-04 at 11:18 -0500, 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. > > 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 A patch like this doesn't make sense on it's own. Please repost it together with the patches that use the directories created by this patch. Please also leave out the debugfs directories that are not used by the patches in the series that you will post. Thanks, Bart.
On Fri, 2019-01-04 at 11:18 -0500, 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. I'm really not sure this is useful, but just in case it is, I suppose the first question should be why this isn't all simply conditioned on BLK_DEBUG_FS? Is there a solid use case where someone would use this in a !BLK_DEBUG_FS situation. [...] > --- a/drivers/scsi/scsi.c > +++ b/drivers/scsi/scsi.c > @@ -68,6 +68,7 @@ > > #include "scsi_priv.h" > #include "scsi_logging.h" > +#include "scsi_debugfs.h" > > #define CREATE_TRACE_POINTS > #include <trace/events/scsi.h> > @@ -812,9 +813,27 @@ static int __init init_scsi(void) > scsi_netlink_init(); > > +#ifdef CONFIG_DEBUG_FS > + scsi_debugfs_root = debugfs_create_dir("scsi", NULL); > + if (!scsi_debugfs_root) > + goto cleanup_netlink; > + scsi_debugfs_uld = debugfs_create_dir("uld", > scsi_debugfs_root); > + if (!scsi_debugfs_uld) > + goto cleanup_debugfs; > + scsi_debugfs_lld = debugfs_create_dir("lld", > scsi_debugfs_root); > + if (!scsi_debugfs_lld) > + goto cleanup_debugfs; > +#endif Chunks of conditionally compiled code cause major problems (primarily because you need multiple Kconfig runs to check them all. The way to avoid this is either to use the IF_ENABLED() etc machinery, or simply do it in header files. In this case, like all init functions, I think there needs to be a scsi_debugfs_init() scsi_debugfs_exit() function like there is for netlink ... in fact, do it precisely how netlink does it. [...] > --- a/drivers/scsi/scsi_debugfs.c > +++ b/drivers/scsi/scsi_debugfs.c > @@ -4,6 +4,20 @@ > #include <scsi/scsi_dbg.h> > #include "scsi_debugfs.h" > > +#ifdef CONFIG_DEBUG_FS This is already conditionally compiled on CONFIG_DEBUG_FS ... you don't need another check of the condition here. > +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); > + > +#endif > + > + > +#ifdef CONFIG_BLK_DEBUG_FS So rather than doing this, it might be better to have two conditionally compiled files, one for just DEBUG_FS and the other for BLK_DEBUG_FS ... assuming theres a good reason not to condition it all on BLK_DEBUG_FS. [...] > --- a/drivers/scsi/scsi_debugfs.h > +++ b/drivers/scsi/scsi_debugfs.h > @@ -1,4 +1,14 @@ > +#include <linux/debugfs.h> > + > +#ifdef CONFIG_DEBUG_FS This file didn't have any CONFIG guards before and it doesn't need them now. James
On 04/01/2019 16:45, Bart Van Assche wrote: > On Fri, 2019-01-04 at 11:18 -0500, 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. >> >> 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 > > A patch like this doesn't make sense on it's own. Please repost it > together with the patches that use the directories created by this patch. > Please also leave out the debugfs directories that are not used by the > patches in the series that you will post. > I have already asked my colleagues if any objection with relocating hisi_sas lld debugfs directory to this new location (should be ok, since not a stable ABI). If not, then we can add that to the patch. <snip> > +#ifdef CONFIG_DEBUG_FS > +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); > + > +#endif Do uld and root dentrys need to be exported also? I was only concerned about lld since drivers are generally modules. Thanks, John > Thanks, > > Bart. > >
On 2019-01-04 11:52 a.m., James Bottomley wrote: > On Fri, 2019-01-04 at 11:18 -0500, 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. > > I'm really not sure this is useful, but just in case it is, I suppose > the first question should be why this isn't all simply conditioned on > BLK_DEBUG_FS? Is there a solid use case where someone would use this > in a !BLK_DEBUG_FS situation. Logically this extension should only depend on CONFIG_DEBUG_FS as that is the minimum requirement. Also it is not blindingly obviously why someone debugging the sg, st and ses driver (all char devices) needs to select CONFIG_BLK_DEBUG_FS. Why not have CONFIG_SCSI_DEBUG_FS ? The patch evolved this way due to the kbuild bot splitting the BLK_DEBUG_FS and DEBUG_FS cases, causing a compile error. In review comments on my sg v4 driver rewrite, reviewers comment that I should not be using the existing SCSI_LOGGING infrastructure since it is deprecated and "maintain-only". [IMO it works fine]. However there seems no or very little support within the SCSI subsystem for any other approach. So this patch is an attempt to make some progress on that front. When I'm debugging (and I do that a lot lately), I assume that the calls to the mid-level, block and other layers do what they are supposed to do. So I'm not interested in their debug or a breakdown of their objects. So, for example, I have never used scsi_debugfs and I expect that if I moved /proc/scsi/sg/debug to /sys/kernel/debug/scsi/sg/debug then I still would not want or need however scsi_show_rq() is used. Does debugging the hisi_sas driver need to access scsi_show_rq() ? >> --- a/drivers/scsi/scsi.c >> +++ b/drivers/scsi/scsi.c >> @@ -68,6 +68,7 @@ >> >> #include "scsi_priv.h" >> #include "scsi_logging.h" >> +#include "scsi_debugfs.h" >> >> #define CREATE_TRACE_POINTS >> #include <trace/events/scsi.h> >> @@ -812,9 +813,27 @@ static int __init init_scsi(void) >> scsi_netlink_init(); >> >> +#ifdef CONFIG_DEBUG_FS >> + scsi_debugfs_root = debugfs_create_dir("scsi", NULL); >> + if (!scsi_debugfs_root) >> + goto cleanup_netlink; >> + scsi_debugfs_uld = debugfs_create_dir("uld", >> scsi_debugfs_root); >> + if (!scsi_debugfs_uld) >> + goto cleanup_debugfs; >> + scsi_debugfs_lld = debugfs_create_dir("lld", >> scsi_debugfs_root); >> + if (!scsi_debugfs_lld) >> + goto cleanup_debugfs; >> +#endif > > Chunks of conditionally compiled code cause major problems (primarily > because you need multiple Kconfig runs to check them all. The way to > avoid this is either to use the IF_ENABLED() etc machinery, or simply > do it in header files. In this case, like all init functions, I think > there needs to be a scsi_debugfs_init() scsi_debugfs_exit() function > like there is for netlink ... in fact, do it precisely how netlink does > it. Fine. > [...] >> --- a/drivers/scsi/scsi_debugfs.c >> +++ b/drivers/scsi/scsi_debugfs.c >> @@ -4,6 +4,20 @@ >> #include <scsi/scsi_dbg.h> >> #include "scsi_debugfs.h" >> >> +#ifdef CONFIG_DEBUG_FS > > This is already conditionally compiled on CONFIG_DEBUG_FS ... you don't > need another check of the condition here. > >> +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); >> + >> +#endif >> + >> + >> +#ifdef CONFIG_BLK_DEBUG_FS > > So rather than doing this, it might be better to have two conditionally > compiled files, one for just DEBUG_FS and the other for BLK_DEBUG_FS > ... assuming theres a good reason not to condition it all on > BLK_DEBUG_FS. Can do. Awaiting a response to my CONFIG_SCSI_DEBUG_FS idea. Doug Gilbert > [...] >> --- a/drivers/scsi/scsi_debugfs.h >> +++ b/drivers/scsi/scsi_debugfs.h >> @@ -1,4 +1,14 @@ >> +#include <linux/debugfs.h> >> + >> +#ifdef CONFIG_DEBUG_FS > > This file didn't have any CONFIG guards before and it doesn't need them > now. > > > James > >
On Fri, 2019-01-04 at 13:34 -0500, Douglas Gilbert wrote: > On 2019-01-04 11:52 a.m., James Bottomley wrote: > > On Fri, 2019-01-04 at 11:18 -0500, 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. > > > > I'm really not sure this is useful, but just in case it is, I > > suppose the first question should be why this isn't all simply > > conditioned on BLK_DEBUG_FS? Is there a solid use case where > > someone would use this in a !BLK_DEBUG_FS situation. > > Logically this extension should only depend on CONFIG_DEBUG_FS as > that is the minimum requirement. Yes, but it's not a reason .. you can use that to justify all sorts of micro config options. However to be useful, config options must represent kernels people would actually build, so the pressure is on to reduce the number. > Also it is not blindingly obviously why someone debugging the sg, st > and ses driver (all char devices) needs to select > CONFIG_BLK_DEBUG_FS. Why not have CONFIG_SCSI_DEBUG_FS ? The reason I could see them being tied together is that if you're after debug information for a SCSI command, part of it might be what it went into block as ... i.e. the current request flags which are SCSI specific and enabled on BLK_DEBUG_FS. What I can't see is a reason to build a special kernel that has SCSI debug stuff but not block. > The patch evolved this way due to the kbuild bot splitting the > BLK_DEBUG_FS and DEBUG_FS cases, causing a compile error. Right, but that can be fixed by unifying them. > In review comments on my sg v4 driver rewrite, reviewers comment that > I should not be using the existing SCSI_LOGGING infrastructure since > it is deprecated and "maintain-only". [IMO it works fine]. However > there seems no or very little support within the SCSI subsystem for > any other approach. So this patch is an attempt to make some progress > on that front. That's kind of orthogonal. One of the best ways of getting debugging information out of the kernel is tracepoints and if we're doing that we should probably do it in a way that it plugs nicely into blktrace, so if that's the genesis, we should probably have the discussion first about how we want to best extract debug information about command flow. > When I'm debugging (and I do that a lot lately), I assume that the > calls to the mid-level, block and other layers do what they are > supposed to do. So I'm not interested in their debug or a breakdown > of their objects. So, for example, I have never used scsi_debugfs and > I expect that if I moved /proc/scsi/sg/debug to > /sys/kernel/debug/scsi/sg/debug then I still would not want or need > however scsi_show_rq() is used. but would you actually build a kernel specially with the sole object of not allowing the information to be seen? That's the point: this is a Kconfig option there has to be a good reason to build every possible kernel or else it's not a good option to have. > Does debugging the hisi_sas driver need to access scsi_show_rq() ? It's not about need to, it's about need not. Is there a specific reason not to build with it ... likelihood of looking at it isn't really the best measure. The kernel emits tons of stuff I don't particularly care about ... it even annoys me when it gets in the way of stuff I do care about but it doesn't annoy me enough to want to build a special kernel without it. James
diff --git a/drivers/scsi/Makefile b/drivers/scsi/Makefile index fcb41ae329c4..49835e2e5f2d 100644 --- a/drivers/scsi/Makefile +++ b/drivers/scsi/Makefile @@ -163,7 +163,7 @@ scsi_mod-y += scsi_scan.o scsi_sysfs.o scsi_devinfo.o scsi_mod-$(CONFIG_SCSI_NETLINK) += scsi_netlink.o scsi_mod-$(CONFIG_SYSCTL) += scsi_sysctl.o scsi_mod-$(CONFIG_SCSI_PROC_FS) += scsi_proc.o -scsi_mod-$(CONFIG_BLK_DEBUG_FS) += scsi_debugfs.o +scsi_mod-$(CONFIG_DEBUG_FS) += scsi_debugfs.o scsi_mod-y += scsi_trace.o scsi_logging.o scsi_mod-$(CONFIG_PM) += scsi_pm.o scsi_mod-$(CONFIG_SCSI_DH) += scsi_dh.o diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c index fc1356d101b0..01d4d0686699 100644 --- a/drivers/scsi/scsi.c +++ b/drivers/scsi/scsi.c @@ -68,6 +68,7 @@ #include "scsi_priv.h" #include "scsi_logging.h" +#include "scsi_debugfs.h" #define CREATE_TRACE_POINTS #include <trace/events/scsi.h> @@ -812,9 +813,27 @@ static int __init init_scsi(void) scsi_netlink_init(); +#ifdef CONFIG_DEBUG_FS + scsi_debugfs_root = debugfs_create_dir("scsi", NULL); + if (!scsi_debugfs_root) + goto cleanup_netlink; + scsi_debugfs_uld = debugfs_create_dir("uld", scsi_debugfs_root); + if (!scsi_debugfs_uld) + goto cleanup_debugfs; + scsi_debugfs_lld = debugfs_create_dir("lld", scsi_debugfs_root); + if (!scsi_debugfs_lld) + goto cleanup_debugfs; +#endif + printk(KERN_NOTICE "SCSI subsystem initialized\n"); return 0; +#ifdef CONFIG_DEBUG_FS +cleanup_debugfs: + debugfs_remove_recursive(scsi_debugfs_root); +cleanup_netlink: + scsi_netlink_exit(); +#endif cleanup_sysctl: scsi_exit_sysctl(); cleanup_hosts: @@ -832,6 +851,10 @@ static int __init init_scsi(void) static void __exit exit_scsi(void) { + +#ifdef CONFIG_DEBUG_FS + debugfs_remove_recursive(scsi_debugfs_root); +#endif 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..8d680656da38 100644 --- a/drivers/scsi/scsi_debugfs.c +++ b/drivers/scsi/scsi_debugfs.c @@ -4,6 +4,20 @@ #include <scsi/scsi_dbg.h> #include "scsi_debugfs.h" +#ifdef CONFIG_DEBUG_FS +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); + +#endif + + +#ifdef CONFIG_BLK_DEBUG_FS + #define SCSI_CMD_FLAG_NAME(name)[const_ilog2(SCMD_##name)] = #name static const char *const scsi_cmd_flags[] = { SCSI_CMD_FLAG_NAME(TAGGED), @@ -50,3 +64,5 @@ void scsi_show_rq(struct seq_file *m, struct request *rq) timeout_ms / 1000, timeout_ms % 1000, alloc_ms / 1000, alloc_ms % 1000); } + +#endif diff --git a/drivers/scsi/scsi_debugfs.h b/drivers/scsi/scsi_debugfs.h index 951b043e82d0..6de9b4691c82 100644 --- a/drivers/scsi/scsi_debugfs.h +++ b/drivers/scsi/scsi_debugfs.h @@ -1,4 +1,14 @@ +#include <linux/debugfs.h> + +#ifdef CONFIG_DEBUG_FS +extern struct dentry *scsi_debugfs_root; +extern struct dentry *scsi_debugfs_uld; +extern struct dentry *scsi_debugfs_lld; +#endif + +#ifdef CONFIG_BLK_DEBUG_FS struct request; struct seq_file; void scsi_show_rq(struct seq_file *m, struct request *rq); +#endif
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 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/debugfs/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/Makefile | 2 +- drivers/scsi/scsi.c | 23 +++++++++++++++++++++++ drivers/scsi/scsi_debugfs.c | 16 ++++++++++++++++ drivers/scsi/scsi_debugfs.h | 10 ++++++++++ 4 files changed, 50 insertions(+), 1 deletion(-)