diff mbox series

[03/12] qla2xxx: Allow dev_loss_tmo setting for FC-NVMe devices

Message ID 20200818123203.20361-4-njavali@marvell.com (mailing list archive)
State Superseded
Headers show
Series qla2xxx misc features and bug fixes | expand

Commit Message

Nilesh Javali Aug. 18, 2020, 12:31 p.m. UTC 
From: Arun Easi <aeasi@marvell.com>

Add a remote port debugfs entry to get/set dev_loss_tmo for NVMe
devices.

Signed-off-by: Arun Easi <aeasi@marvell.com>
Signed-off-by: Himanshu Madhani <hmadhani@marvell.com>
Signed-off-by: Nilesh Javali <njavali@marvell.com>
---
 drivers/scsi/qla2xxx/qla_dfs.c | 54 ++++++++++++++++++++++++++++++++++
 1 file changed, 54 insertions(+)

Comments

Roman Bolshakov Aug. 18, 2020, 1:24 p.m. UTC | #1 
On Tue, Aug 18, 2020 at 05:31:54AM -0700, Nilesh Javali wrote:
> From: Arun Easi <aeasi@marvell.com>
> 
> Add a remote port debugfs entry to get/set dev_loss_tmo for NVMe
> devices.
> 
> Signed-off-by: Arun Easi <aeasi@marvell.com>
> Signed-off-by: Himanshu Madhani <hmadhani@marvell.com>
> Signed-off-by: Nilesh Javali <njavali@marvell.com>
> ---
>  drivers/scsi/qla2xxx/qla_dfs.c | 54 ++++++++++++++++++++++++++++++++++
>  1 file changed, 54 insertions(+)
> 
> diff --git a/drivers/scsi/qla2xxx/qla_dfs.c b/drivers/scsi/qla2xxx/qla_dfs.c
> index 3c4b9b549b17..7482e6bf7f7f 100644
> --- a/drivers/scsi/qla2xxx/qla_dfs.c
> +++ b/drivers/scsi/qla2xxx/qla_dfs.c
> @@ -12,6 +12,57 @@
>  static struct dentry *qla2x00_dfs_root;
>  static atomic_t qla2x00_dfs_root_count;
>  
> +#define QLA_DFS_RPORT_DEVLOSS_TMO	1
> +
> +static int
> +qla_dfs_rport_get(struct fc_port *fp, int attr_id, u64 *val)
> +{
> +	switch (attr_id) {
> +	case QLA_DFS_RPORT_DEVLOSS_TMO:
> +		/* Only supported for FC-NVMe devices that are registered. */
> +		if (!(fp->nvme_flag & NVME_FLAG_REGISTERED))
> +			return -EIO;
> +		*val = fp->nvme_remote_port->dev_loss_tmo;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +	return 0;
> +}
> +
> +static int
> +qla_dfs_rport_set(struct fc_port *fp, int attr_id, u64 val)
> +{
> +	switch (attr_id) {
> +	case QLA_DFS_RPORT_DEVLOSS_TMO:
> +		/* Only supported for FC-NVMe devices that are registered. */
> +		if (!(fp->nvme_flag & NVME_FLAG_REGISTERED))
> +			return -EIO;
> +		return nvme_fc_set_remoteport_devloss(fp->nvme_remote_port,
> +						      val);
> +	default:
> +		return -EINVAL;
> +	}
> +	return 0;
> +}
> +
> +#define DEFINE_QLA_DFS_RPORT_RW_ATTR(_attr_id, _attr)		\
> +static int qla_dfs_rport_##_attr##_get(void *data, u64 *val)	\
> +{								\
> +	struct fc_port *fp = data;				\
> +	return qla_dfs_rport_get(fp, _attr_id, val);		\
> +}								\
> +static int qla_dfs_rport_##_attr##_set(void *data, u64 val)	\
> +{								\
> +	struct fc_port *fp = data;				\
> +	return qla_dfs_rport_set(fp, _attr_id, val);		\
> +}								\
> +DEFINE_DEBUGFS_ATTRIBUTE(qla_dfs_rport_##_attr##_fops,		\
> +		qla_dfs_rport_##_attr##_get,			\
> +		qla_dfs_rport_##_attr##_set, "%llu\n")
> +
> +DEFINE_QLA_DFS_RPORT_RW_ATTR(QLA_DFS_RPORT_DEVLOSS_TMO, dev_loss_tmo);
> +
>  void
>  qla2x00_dfs_create_rport(scsi_qla_host_t *vha, struct fc_port *fp)
>  {
> @@ -24,6 +75,9 @@ qla2x00_dfs_create_rport(scsi_qla_host_t *vha, struct fc_port *fp)
>  	fp->dfs_rport_dir = debugfs_create_dir(wwn, vha->dfs_rport_root);
>  	if (!fp->dfs_rport_dir)
>  		return;
> +	if (NVME_TARGET(vha->hw, fp))
> +		debugfs_create_file("dev_loss_tmo", 0600, fp->dfs_rport_dir,
> +				    fp, &qla_dfs_rport_dev_loss_tmo_fops);
>  }
>  
>  void
> -- 
> 2.19.0.rc0
> 

Hi Arun,

I don't think that the setting should be in debugfs. IMO there should be
separate fc_remote_ports entry for FCP and FC-NVMe, one per PRLI for
multi-protocol targets.

That'd be consistent with what exists for FCP and would allow
similar configuration of dev_loss_tmo from multipath. It'd also provide
semantics of per-rport PRLO to allow logout from either of the protocol
but leaving second rport/session online.

Thanks,
Roman
kernel test robot Aug. 18, 2020, 9:04 p.m. UTC | #2 
Hi Nilesh,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on dca93232b361d260413933903cd4bdbd92ebcc7f]

url:    https://github.com/0day-ci/linux/commits/Nilesh-Javali/qla2xxx-misc-features-and-bug-fixes/20200818-203851
base:    dca93232b361d260413933903cd4bdbd92ebcc7f
config: s390-randconfig-r034-20200818 (attached as .config)
compiler: clang version 12.0.0 (https://github.com/llvm/llvm-project 790878f291fa5dc58a1c560cb6cc76fd1bfd1c5a)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install s390 cross compiling tool for clang build
        # apt-get install binutils-s390x-linux-gnu
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=s390 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   s390x-linux-gnu-ld: drivers/scsi/qla2xxx/qla_dfs.o: in function `qla_dfs_rport_set':
>> drivers/scsi/qla2xxx/qla_dfs.c:41: undefined reference to `nvme_fc_set_remoteport_devloss'

# https://github.com/0day-ci/linux/commit/c8724abdb8c3a827186052c58054832ee10bd817
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Nilesh-Javali/qla2xxx-misc-features-and-bug-fixes/20200818-203851
git checkout c8724abdb8c3a827186052c58054832ee10bd817
vim +41 drivers/scsi/qla2xxx/qla_dfs.c

    32	
    33	static int
    34	qla_dfs_rport_set(struct fc_port *fp, int attr_id, u64 val)
    35	{
    36		switch (attr_id) {
    37		case QLA_DFS_RPORT_DEVLOSS_TMO:
    38			/* Only supported for FC-NVMe devices that are registered. */
    39			if (!(fp->nvme_flag & NVME_FLAG_REGISTERED))
    40				return -EIO;
  > 41			return nvme_fc_set_remoteport_devloss(fp->nvme_remote_port,
    42							      val);
    43		default:
    44			return -EINVAL;
    45		}
    46		return 0;
    47	}
    48	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
kernel test robot Aug. 18, 2020, 9:12 p.m. UTC | #3 
Hi Nilesh,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on dca93232b361d260413933903cd4bdbd92ebcc7f]

url:    https://github.com/0day-ci/linux/commits/Nilesh-Javali/qla2xxx-misc-features-and-bug-fixes/20200818-203851
base:    dca93232b361d260413933903cd4bdbd92ebcc7f
config: x86_64-kexec (attached as .config)
compiler: gcc-9 (Debian 9.3.0-15) 9.3.0
reproduce (this is a W=1 build):
        # save the attached .config to linux build tree
        make W=1 ARCH=x86_64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>, old ones prefixed by <<):

>> ERROR: modpost: "nvme_fc_set_remoteport_devloss" [drivers/scsi/qla2xxx/qla2xxx.ko] undefined!

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
Arun Easi Aug. 19, 2020, 10:09 p.m. UTC | #4 
+James S.

Hi Roman,

On Tue, 18 Aug 2020, 6:24am, Roman Bolshakov wrote:

> 
> On Tue, Aug 18, 2020 at 05:31:54AM -0700, Nilesh Javali wrote:
> > From: Arun Easi <aeasi@marvell.com>
> > 
> > Add a remote port debugfs entry to get/set dev_loss_tmo for NVMe
> > devices.
> > 
> > Signed-off-by: Arun Easi <aeasi@marvell.com>
> > Signed-off-by: Himanshu Madhani <hmadhani@marvell.com>
> > Signed-off-by: Nilesh Javali <njavali@marvell.com>
> > ---
> >  drivers/scsi/qla2xxx/qla_dfs.c | 54 ++++++++++++++++++++++++++++++++++
> >  1 file changed, 54 insertions(+)
> > 
> > diff --git a/drivers/scsi/qla2xxx/qla_dfs.c b/drivers/scsi/qla2xxx/qla_dfs.c
> > index 3c4b9b549b17..7482e6bf7f7f 100644
> > --- a/drivers/scsi/qla2xxx/qla_dfs.c
> > +++ b/drivers/scsi/qla2xxx/qla_dfs.c
> > @@ -12,6 +12,57 @@
> >  static struct dentry *qla2x00_dfs_root;
> >  static atomic_t qla2x00_dfs_root_count;
> >  
> > +#define QLA_DFS_RPORT_DEVLOSS_TMO	1
> > +
> > +static int
> > +qla_dfs_rport_get(struct fc_port *fp, int attr_id, u64 *val)
> > +{
> > +	switch (attr_id) {
> > +	case QLA_DFS_RPORT_DEVLOSS_TMO:
> > +		/* Only supported for FC-NVMe devices that are registered. */
> > +		if (!(fp->nvme_flag & NVME_FLAG_REGISTERED))
> > +			return -EIO;
> > +		*val = fp->nvme_remote_port->dev_loss_tmo;
> > +		break;
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +	return 0;
> > +}
> > +
> > +static int
> > +qla_dfs_rport_set(struct fc_port *fp, int attr_id, u64 val)
> > +{
> > +	switch (attr_id) {
> > +	case QLA_DFS_RPORT_DEVLOSS_TMO:
> > +		/* Only supported for FC-NVMe devices that are registered. */
> > +		if (!(fp->nvme_flag & NVME_FLAG_REGISTERED))
> > +			return -EIO;
> > +		return nvme_fc_set_remoteport_devloss(fp->nvme_remote_port,
> > +						      val);
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +	return 0;
> > +}
> > +
> > +#define DEFINE_QLA_DFS_RPORT_RW_ATTR(_attr_id, _attr)		\
> > +static int qla_dfs_rport_##_attr##_get(void *data, u64 *val)	\
> > +{								\
> > +	struct fc_port *fp = data;				\
> > +	return qla_dfs_rport_get(fp, _attr_id, val);		\
> > +}								\
> > +static int qla_dfs_rport_##_attr##_set(void *data, u64 val)	\
> > +{								\
> > +	struct fc_port *fp = data;				\
> > +	return qla_dfs_rport_set(fp, _attr_id, val);		\
> > +}								\
> > +DEFINE_DEBUGFS_ATTRIBUTE(qla_dfs_rport_##_attr##_fops,		\
> > +		qla_dfs_rport_##_attr##_get,			\
> > +		qla_dfs_rport_##_attr##_set, "%llu\n")
> > +
> > +DEFINE_QLA_DFS_RPORT_RW_ATTR(QLA_DFS_RPORT_DEVLOSS_TMO, dev_loss_tmo);
> > +
> >  void
> >  qla2x00_dfs_create_rport(scsi_qla_host_t *vha, struct fc_port *fp)
> >  {
> > @@ -24,6 +75,9 @@ qla2x00_dfs_create_rport(scsi_qla_host_t *vha, struct fc_port *fp)
> >  	fp->dfs_rport_dir = debugfs_create_dir(wwn, vha->dfs_rport_root);
> >  	if (!fp->dfs_rport_dir)
> >  		return;
> > +	if (NVME_TARGET(vha->hw, fp))
> > +		debugfs_create_file("dev_loss_tmo", 0600, fp->dfs_rport_dir,
> > +				    fp, &qla_dfs_rport_dev_loss_tmo_fops);
> >  }
> >  
> >  void
> > -- 
> > 2.19.0.rc0
> > 
> 
> Hi Arun,
> 
> I don't think that the setting should be in debugfs. IMO there should be
> separate fc_remote_ports entry for FCP and FC-NVMe, one per PRLI for
> multi-protocol targets.
> 
> That'd be consistent with what exists for FCP and would allow
> similar configuration of dev_loss_tmo from multipath. It'd also provide
> semantics of per-rport PRLO to allow logout from either of the protocol
> but leaving second rport/session online.
> 

Thanks for your comments. This debugfs way is sort of a crutch, but that 
is something a user could use to set for FC-NVME dev_loss_tmo until a 
FC-NVME sysfs node hierarchy for a FC initiator (similar to FCP) comes 
into place.

BTW, the above setting is specific for FC-NVME, and is separate from FCP; 
so PRLO semantics can still be met.

Copying James for his thoughts on the topic (on dev_loss_tmo setting). 
James, would you mind sharing your ideas you'd considered?

Regards,
-Arun
diff mbox series

Patch

diff --git a/drivers/scsi/qla2xxx/qla_dfs.c b/drivers/scsi/qla2xxx/qla_dfs.c
index 3c4b9b549b17..7482e6bf7f7f 100644
--- a/drivers/scsi/qla2xxx/qla_dfs.c
+++ b/drivers/scsi/qla2xxx/qla_dfs.c
@@ -12,6 +12,57 @@ 
 static struct dentry *qla2x00_dfs_root;
 static atomic_t qla2x00_dfs_root_count;
 
+#define QLA_DFS_RPORT_DEVLOSS_TMO	1
+
+static int
+qla_dfs_rport_get(struct fc_port *fp, int attr_id, u64 *val)
+{
+	switch (attr_id) {
+	case QLA_DFS_RPORT_DEVLOSS_TMO:
+		/* Only supported for FC-NVMe devices that are registered. */
+		if (!(fp->nvme_flag & NVME_FLAG_REGISTERED))
+			return -EIO;
+		*val = fp->nvme_remote_port->dev_loss_tmo;
+		break;
+	default:
+		return -EINVAL;
+	}
+	return 0;
+}
+
+static int
+qla_dfs_rport_set(struct fc_port *fp, int attr_id, u64 val)
+{
+	switch (attr_id) {
+	case QLA_DFS_RPORT_DEVLOSS_TMO:
+		/* Only supported for FC-NVMe devices that are registered. */
+		if (!(fp->nvme_flag & NVME_FLAG_REGISTERED))
+			return -EIO;
+		return nvme_fc_set_remoteport_devloss(fp->nvme_remote_port,
+						      val);
+	default:
+		return -EINVAL;
+	}
+	return 0;
+}
+
+#define DEFINE_QLA_DFS_RPORT_RW_ATTR(_attr_id, _attr)		\
+static int qla_dfs_rport_##_attr##_get(void *data, u64 *val)	\
+{								\
+	struct fc_port *fp = data;				\
+	return qla_dfs_rport_get(fp, _attr_id, val);		\
+}								\
+static int qla_dfs_rport_##_attr##_set(void *data, u64 val)	\
+{								\
+	struct fc_port *fp = data;				\
+	return qla_dfs_rport_set(fp, _attr_id, val);		\
+}								\
+DEFINE_DEBUGFS_ATTRIBUTE(qla_dfs_rport_##_attr##_fops,		\
+		qla_dfs_rport_##_attr##_get,			\
+		qla_dfs_rport_##_attr##_set, "%llu\n")
+
+DEFINE_QLA_DFS_RPORT_RW_ATTR(QLA_DFS_RPORT_DEVLOSS_TMO, dev_loss_tmo);
+
 void
 qla2x00_dfs_create_rport(scsi_qla_host_t *vha, struct fc_port *fp)
 {
@@ -24,6 +75,9 @@  qla2x00_dfs_create_rport(scsi_qla_host_t *vha, struct fc_port *fp)
 	fp->dfs_rport_dir = debugfs_create_dir(wwn, vha->dfs_rport_root);
 	if (!fp->dfs_rport_dir)
 		return;
+	if (NVME_TARGET(vha->hw, fp))
+		debugfs_create_file("dev_loss_tmo", 0600, fp->dfs_rport_dir,
+				    fp, &qla_dfs_rport_dev_loss_tmo_fops);
 }
 
 void