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
