[v5] scsi: add debugfs directories
diff mbox series

Message ID 20190108040034.14380-1-dgilbert@interlog.com
State New
Headers show
Series
  • [v5] scsi: add debugfs directories
Related show

Commit Message

Douglas Gilbert Jan. 8, 2019, 4 a.m. UTC
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(-)

Comments

John Garry Jan. 9, 2019, 9:17 a.m. UTC | #1
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;
>
John Garry Jan. 9, 2019, 12:24 p.m. UTC | #2
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;
>>
>
>
>
> .
>
Douglas Gilbert Jan. 9, 2019, 3:15 p.m. UTC | #3
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;
>>
> 
> 
>
Douglas Gilbert Jan. 9, 2019, 3:25 p.m. UTC | #4
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;
>>>
>>
>>
>>
>> .
>>
> 
> 
>

Patch
diff mbox series

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;