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
