diff mbox series

[05/15] qla2xxx: Don't check for fw_started while posting nvme command

Message ID 20201201082730.24158-6-njavali@marvell.com (mailing list archive)
State Superseded
Headers show
Series qla2xxx bug fixes | expand

Commit Message

Nilesh Javali Dec. 1, 2020, 8:27 a.m. UTC 
From: Saurav Kashyap <skashyap@marvell.com>

NVMe commands can come only after successful addition of rport and nvme
connect, and rport is only registered after FW started bit is set. Remove the
redundant check.

Signed-off-by: Saurav Kashyap <skashyap@marvell.com>
Signed-off-by: Nilesh Javali <njavali@marvell.com>
---
 drivers/scsi/qla2xxx/qla_nvme.c | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

Comments

Daniel Wagner Dec. 1, 2020, 9:01 a.m. UTC | #1 
On Tue, Dec 01, 2020 at 12:27:20AM -0800, Nilesh Javali wrote:
> From: Saurav Kashyap <skashyap@marvell.com>
> 
> NVMe commands can come only after successful addition of rport and nvme
> connect, and rport is only registered after FW started bit is set. Remove the
> redundant check.
> 
> Signed-off-by: Saurav Kashyap <skashyap@marvell.com>
> Signed-off-by: Nilesh Javali <njavali@marvell.com>
> ---
>  drivers/scsi/qla2xxx/qla_nvme.c | 12 ++++--------
>  1 file changed, 4 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/scsi/qla2xxx/qla_nvme.c b/drivers/scsi/qla2xxx/qla_nvme.c
> index b7a1dc24db38..d4159d5a4ffd 100644
> --- a/drivers/scsi/qla2xxx/qla_nvme.c
> +++ b/drivers/scsi/qla2xxx/qla_nvme.c
> @@ -554,19 +554,15 @@ static int qla_nvme_post_cmd(struct nvme_fc_local_port *lport,
>  
>  	fcport = qla_rport->fcport;
>  
> -	if (!qpair || !fcport)
> -		return -ENODEV;
> -
> -	if (!qpair->fw_started || fcport->deleted)
> +	if (unlikely(!qpair || !fcport || fcport->deleted))
>  		return -EBUSY;

This reverts the fix from patch #1 in this series. What's the reasoning
that needs to return EBUSY when !qpair || !fcport is true?
Saurav Kashyap Dec. 1, 2020, 9:39 a.m. UTC | #2 
Hi Daniel,
Comments inline..

> -----Original Message-----
> From: Daniel Wagner <dwagner@suse.de>
> Sent: Tuesday, December 1, 2020 2:32 PM
> To: Nilesh Javali <njavali@marvell.com>
> Cc: martin.petersen@oracle.com; linux-scsi@vger.kernel.org; GR-QLogic-
> Storage-Upstream <GR-QLogic-Storage-Upstream@marvell.com>
> Subject: Re: [PATCH 05/15] qla2xxx: Don't check for fw_started while posting
> nvme command
> 
> On Tue, Dec 01, 2020 at 12:27:20AM -0800, Nilesh Javali wrote:
> > From: Saurav Kashyap <skashyap@marvell.com>
> >
> > NVMe commands can come only after successful addition of rport and nvme
> > connect, and rport is only registered after FW started bit is set. Remove the
> > redundant check.
> >
> > Signed-off-by: Saurav Kashyap <skashyap@marvell.com>
> > Signed-off-by: Nilesh Javali <njavali@marvell.com>
> > ---
> >  drivers/scsi/qla2xxx/qla_nvme.c | 12 ++++--------
> >  1 file changed, 4 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/scsi/qla2xxx/qla_nvme.c
> b/drivers/scsi/qla2xxx/qla_nvme.c
> > index b7a1dc24db38..d4159d5a4ffd 100644
> > --- a/drivers/scsi/qla2xxx/qla_nvme.c
> > +++ b/drivers/scsi/qla2xxx/qla_nvme.c
> > @@ -554,19 +554,15 @@ static int qla_nvme_post_cmd(struct
> nvme_fc_local_port *lport,
> >
> >  	fcport = qla_rport->fcport;
> >
> > -	if (!qpair || !fcport)
> > -		return -ENODEV;
> > -
> > -	if (!qpair->fw_started || fcport->deleted)
> > +	if (unlikely(!qpair || !fcport || fcport->deleted))
> >  		return -EBUSY;
> 
> This reverts the fix from patch #1 in this series. What's the reasoning
> that needs to return EBUSY when !qpair || !fcport is true?
<SK> Ideally driver should not hit (!qpair || !fcport) case.  The patch was to remove fw_started flag and consolidate other checks.
We want IO to retry until remote port is deleted and below condition is hit.
----------<condition>--------
        if (!(fcport->nvme_flag & NVME_FLAG_REGISTERED))
                return rval;
----------<condition>---------

Thanks,
~Saurav
Daniel Wagner Dec. 1, 2020, 9:53 a.m. UTC | #3 
Hi Saurav

On Tue, Dec 01, 2020 at 09:39:05AM +0000, Saurav Kashyap wrote:
> > > --- a/drivers/scsi/qla2xxx/qla_nvme.c
> > > +++ b/drivers/scsi/qla2xxx/qla_nvme.c
> > > @@ -554,19 +554,15 @@ static int qla_nvme_post_cmd(struct
> > nvme_fc_local_port *lport,
> > >
> > >  	fcport = qla_rport->fcport;
> > >
> > > -	if (!qpair || !fcport)
> > > -		return -ENODEV;
> > > -
> > > -	if (!qpair->fw_started || fcport->deleted)
> > > +	if (unlikely(!qpair || !fcport || fcport->deleted))
> > >  		return -EBUSY;
> > 
> > This reverts the fix from patch #1 in this series. What's the reasoning
> > that needs to return EBUSY when !qpair || !fcport is true?
>
> Ideally driver should not hit (!qpair || !fcport) case.  The patch was
> to remove fw_started flag and consolidate other checks.

Looking again on the patch I think I got confused.

> We want IO to retry until remote port is deleted and below condition is hit.

The result of this patch is that in EBUSY will be returned in all the
cases, not just for the case of fcport->deleted. So all is good from my
point of view. Thanks for explaining.

Sorry for the noise.

Thanks,
Daniel
Saurav Kashyap Dec. 1, 2020, 9:57 a.m. UTC | #4 
Hi Daniel,

> -----Original Message-----
> From: Daniel Wagner <dwagner@suse.de>
> Sent: Tuesday, December 1, 2020 3:24 PM
> To: Saurav Kashyap <skashyap@marvell.com>
> Cc: Nilesh Javali <njavali@marvell.com>; martin.petersen@oracle.com; linux-
> scsi@vger.kernel.org; GR-QLogic-Storage-Upstream <GR-QLogic-Storage-
> Upstream@marvell.com>
> Subject: [EXT] Re: [PATCH 05/15] qla2xxx: Don't check for fw_started while
> posting nvme command
> 
> External Email
> 
> ----------------------------------------------------------------------
> Hi Saurav
> 
> On Tue, Dec 01, 2020 at 09:39:05AM +0000, Saurav Kashyap wrote:
> > > > --- a/drivers/scsi/qla2xxx/qla_nvme.c
> > > > +++ b/drivers/scsi/qla2xxx/qla_nvme.c
> > > > @@ -554,19 +554,15 @@ static int qla_nvme_post_cmd(struct
> > > nvme_fc_local_port *lport,
> > > >
> > > >  	fcport = qla_rport->fcport;
> > > >
> > > > -	if (!qpair || !fcport)
> > > > -		return -ENODEV;
> > > > -
> > > > -	if (!qpair->fw_started || fcport->deleted)
> > > > +	if (unlikely(!qpair || !fcport || fcport->deleted))
> > > >  		return -EBUSY;
> > >
> > > This reverts the fix from patch #1 in this series. What's the reasoning
> > > that needs to return EBUSY when !qpair || !fcport is true?
> >
> > Ideally driver should not hit (!qpair || !fcport) case.  The patch was
> > to remove fw_started flag and consolidate other checks.
> 
> Looking again on the patch I think I got confused.
> 
> > We want IO to retry until remote port is deleted and below condition is hit.
> 
> The result of this patch is that in EBUSY will be returned in all the
> cases, not just for the case of fcport->deleted. So all is good from my
> point of view. Thanks for explaining.
> 
> Sorry for the noise.

No problem, most welcome.

Thanks,
~Saurav
> 
> Thanks,
> Daniel
Himanshu Madhani Dec. 1, 2020, 3:50 p.m. UTC | #5 
> On Dec 1, 2020, at 2:27 AM, Nilesh Javali <njavali@marvell.com> wrote:
> 
> From: Saurav Kashyap <skashyap@marvell.com>
> 
> NVMe commands can come only after successful addition of rport and nvme
> connect, and rport is only registered after FW started bit is set. Remove the
> redundant check.
> 
> Signed-off-by: Saurav Kashyap <skashyap@marvell.com>
> Signed-off-by: Nilesh Javali <njavali@marvell.com>
> ---
> drivers/scsi/qla2xxx/qla_nvme.c | 12 ++++--------
> 1 file changed, 4 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/scsi/qla2xxx/qla_nvme.c b/drivers/scsi/qla2xxx/qla_nvme.c
> index b7a1dc24db38..d4159d5a4ffd 100644
> --- a/drivers/scsi/qla2xxx/qla_nvme.c
> +++ b/drivers/scsi/qla2xxx/qla_nvme.c
> @@ -554,19 +554,15 @@ static int qla_nvme_post_cmd(struct nvme_fc_local_port *lport,
> 
> 	fcport = qla_rport->fcport;
> 
> -	if (!qpair || !fcport)
> -		return -ENODEV;
> -
> -	if (!qpair->fw_started || fcport->deleted)
> +	if (unlikely(!qpair || !fcport || fcport->deleted))
> 		return -EBUSY;
> 
> -	vha = fcport->vha;
> -
> 	if (!(fcport->nvme_flag & NVME_FLAG_REGISTERED))
> 		return -ENODEV;
> 
> -	if (test_bit(ABORT_ISP_ACTIVE, &vha->dpc_flags) ||
> -	    (qpair && !qpair->fw_started) || fcport->deleted)
> +	vha = fcport->vha;
> +
> +	if (test_bit(ABORT_ISP_ACTIVE, &vha->dpc_flags))
> 		return -EBUSY;
> 
> 	/*
> -- 
> 2.19.0.rc0
> 

Looks Good

Reviewed-by: Himanshu Madhani <himanshu.madhani@oracle.com>

--
Himanshu Madhani	 Oracle Linux Engineering
diff mbox series

Patch

diff --git a/drivers/scsi/qla2xxx/qla_nvme.c b/drivers/scsi/qla2xxx/qla_nvme.c
index b7a1dc24db38..d4159d5a4ffd 100644
--- a/drivers/scsi/qla2xxx/qla_nvme.c
+++ b/drivers/scsi/qla2xxx/qla_nvme.c
@@ -554,19 +554,15 @@  static int qla_nvme_post_cmd(struct nvme_fc_local_port *lport,
 
 	fcport = qla_rport->fcport;
 
-	if (!qpair || !fcport)
-		return -ENODEV;
-
-	if (!qpair->fw_started || fcport->deleted)
+	if (unlikely(!qpair || !fcport || fcport->deleted))
 		return -EBUSY;
 
-	vha = fcport->vha;
-
 	if (!(fcport->nvme_flag & NVME_FLAG_REGISTERED))
 		return -ENODEV;
 
-	if (test_bit(ABORT_ISP_ACTIVE, &vha->dpc_flags) ||
-	    (qpair && !qpair->fw_started) || fcport->deleted)
+	vha = fcport->vha;
+
+	if (test_bit(ABORT_ISP_ACTIVE, &vha->dpc_flags))
 		return -EBUSY;
 
 	/*