Re: [PATCH v4] scsi: add debugfs directories

[John Garry <john.garry@xxxxxxxxxx>]

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@xxxxxxxxxxxx>
> ---
>
> 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