Re: [PATCH] qla2xxx: Protect access to qpair members with qpair->qp_lock
Yes, it¹s needed. We will post a patch. On 6/23/17, 9:34 AM, "Julia Lawall" <julia.law...@lip6.fr> wrote: >Please check on whether an unlock is neeed before line 1965. > >julia > >-- Forwarded message -- >Date: Fri, 23 Jun 2017 15:23:00 +0800 >From: kbuild test robot <fengguang...@intel.com> >To: kbu...@01.org >Cc: Julia Lawall <julia.law...@lip6.fr> >Subject: Re: [PATCH] qla2xxx: Protect access to qpair members with >qpair->qp_lock > >CC: kbuild-...@01.org >In-Reply-To: <20170622134325.26931-1-jthumsh...@suse.de> > >Hi Johannes, > >[auto build test WARNING on scsi/for-next] >[also build test WARNING on v4.12-rc6 next-20170622] >[if your patch is applied to the wrong git tree, please drop us a note to >help improve the system] > >url: >https://github.com/0day-ci/linux/commits/Johannes-Thumshirn/qla2xxx-Protec >t-access-to-qpair-members-with-qpair-qp_lock/20170623-123844 >base: https://git.kernel.org/pub/scm/linux/kernel/git/jejb/scsi.git >for-next >:: branch date: 3 hours ago >:: commit date: 3 hours ago > >>> drivers/scsi/qla2xxx/qla_iocb.c:1965:3-9: preceding lock on line 1952 > >git remote add linux-review https://github.com/0day-ci/linux >git remote update linux-review >git checkout 4a35d720268dbe9ac016957a3c4fc644398d68ba >vim +1965 drivers/scsi/qla2xxx/qla_iocb.c > >d74595278 Michael Hernandez 2016-12-12 1946 /* Only process >protection or >16 cdb in this routine */ >d74595278 Michael Hernandez 2016-12-12 1947 if >(scsi_get_prot_op(cmd) >== SCSI_PROT_NORMAL) { >d74595278 Michael Hernandez 2016-12-12 1948 if >(cmd->cmd_len <= 16) >d74595278 Michael Hernandez 2016-12-12 1949 return >qla2xxx_start_scsi_mq(sp); >d74595278 Michael Hernandez 2016-12-12 1950 } >d74595278 Michael Hernandez 2016-12-12 1951 >4a35d7202 Johannes Thumshirn 2017-06-22 @1952 > spin_lock_irqsave(>qp_lock, flags); >4a35d7202 Johannes Thumshirn 2017-06-22 1953 >d74595278 Michael Hernandez 2016-12-12 1954 /* Setup qpair pointers >*/ >d74595278 Michael Hernandez 2016-12-12 1955 rsp = qpair->rsp; >d74595278 Michael Hernandez 2016-12-12 1956 req = qpair->req; >d74595278 Michael Hernandez 2016-12-12 1957 >d74595278 Michael Hernandez 2016-12-12 1958 /* So we know we haven't >pci_map'ed anything yet */ >d74595278 Michael Hernandez 2016-12-12 1959 tot_dsds = 0; >d74595278 Michael Hernandez 2016-12-12 1960 >d74595278 Michael Hernandez 2016-12-12 1961 /* Send marker if >required */ >d74595278 Michael Hernandez 2016-12-12 1962 if (vha->marker_needed >!= >0) { >d74595278 Michael Hernandez 2016-12-12 1963 if >(qla2x00_marker(vha, >req, rsp, 0, 0, MK_SYNC_ALL) != >d74595278 Michael Hernandez 2016-12-12 1964 QLA_SUCCESS) >d74595278 Michael Hernandez 2016-12-12 @1965 return >QLA_FUNCTION_FAILED; >d74595278 Michael Hernandez 2016-12-12 1966 >vha->marker_needed = 0; >d74595278 Michael Hernandez 2016-12-12 1967 } >d74595278 Michael Hernandez 2016-12-12 1968 > >:: The code at line 1965 was first introduced by commit >:: d74595278f4ab192af66d9e60a9087464638beee scsi: qla2xxx: Add >multiple queue pair functionality. > >:: TO: Michael Hernandez <michael.hernan...@cavium.com> >:: CC: Martin K. Petersen <martin.peter...@oracle.com> > >--- >0-DAY kernel test infrastructureOpen Source Technology >Center >https://lists.01.org/pipermail/kbuild-all Intel >Corporation
Re: [PATCH] qla2xxx: Protect access to qpair members with qpair->qp_lock
Yes, it¹s needed. We will post a patch. On 6/23/17, 9:34 AM, "Julia Lawall" wrote: >Please check on whether an unlock is neeed before line 1965. > >julia > >-- Forwarded message -- >Date: Fri, 23 Jun 2017 15:23:00 +0800 >From: kbuild test robot >To: kbu...@01.org >Cc: Julia Lawall >Subject: Re: [PATCH] qla2xxx: Protect access to qpair members with >qpair->qp_lock > >CC: kbuild-...@01.org >In-Reply-To: <20170622134325.26931-1-jthumsh...@suse.de> > >Hi Johannes, > >[auto build test WARNING on scsi/for-next] >[also build test WARNING on v4.12-rc6 next-20170622] >[if your patch is applied to the wrong git tree, please drop us a note to >help improve the system] > >url: >https://github.com/0day-ci/linux/commits/Johannes-Thumshirn/qla2xxx-Protec >t-access-to-qpair-members-with-qpair-qp_lock/20170623-123844 >base: https://git.kernel.org/pub/scm/linux/kernel/git/jejb/scsi.git >for-next >:: branch date: 3 hours ago >:: commit date: 3 hours ago > >>> drivers/scsi/qla2xxx/qla_iocb.c:1965:3-9: preceding lock on line 1952 > >git remote add linux-review https://github.com/0day-ci/linux >git remote update linux-review >git checkout 4a35d720268dbe9ac016957a3c4fc644398d68ba >vim +1965 drivers/scsi/qla2xxx/qla_iocb.c > >d74595278 Michael Hernandez 2016-12-12 1946 /* Only process >protection or >16 cdb in this routine */ >d74595278 Michael Hernandez 2016-12-12 1947 if >(scsi_get_prot_op(cmd) >== SCSI_PROT_NORMAL) { >d74595278 Michael Hernandez 2016-12-12 1948 if >(cmd->cmd_len <= 16) >d74595278 Michael Hernandez 2016-12-12 1949 return >qla2xxx_start_scsi_mq(sp); >d74595278 Michael Hernandez 2016-12-12 1950 } >d74595278 Michael Hernandez 2016-12-12 1951 >4a35d7202 Johannes Thumshirn 2017-06-22 @1952 > spin_lock_irqsave(>qp_lock, flags); >4a35d7202 Johannes Thumshirn 2017-06-22 1953 >d74595278 Michael Hernandez 2016-12-12 1954 /* Setup qpair pointers >*/ >d74595278 Michael Hernandez 2016-12-12 1955 rsp = qpair->rsp; >d74595278 Michael Hernandez 2016-12-12 1956 req = qpair->req; >d74595278 Michael Hernandez 2016-12-12 1957 >d74595278 Michael Hernandez 2016-12-12 1958 /* So we know we haven't >pci_map'ed anything yet */ >d74595278 Michael Hernandez 2016-12-12 1959 tot_dsds = 0; >d74595278 Michael Hernandez 2016-12-12 1960 >d74595278 Michael Hernandez 2016-12-12 1961 /* Send marker if >required */ >d74595278 Michael Hernandez 2016-12-12 1962 if (vha->marker_needed >!= >0) { >d74595278 Michael Hernandez 2016-12-12 1963 if >(qla2x00_marker(vha, >req, rsp, 0, 0, MK_SYNC_ALL) != >d74595278 Michael Hernandez 2016-12-12 1964 QLA_SUCCESS) >d74595278 Michael Hernandez 2016-12-12 @1965 return >QLA_FUNCTION_FAILED; >d74595278 Michael Hernandez 2016-12-12 1966 >vha->marker_needed = 0; >d74595278 Michael Hernandez 2016-12-12 1967 } >d74595278 Michael Hernandez 2016-12-12 1968 > >:: The code at line 1965 was first introduced by commit >:: d74595278f4ab192af66d9e60a9087464638beee scsi: qla2xxx: Add >multiple queue pair functionality. > >:: TO: Michael Hernandez >:: CC: Martin K. Petersen > >--- >0-DAY kernel test infrastructureOpen Source Technology >Center >https://lists.01.org/pipermail/kbuild-all Intel >Corporation
Re: [PATCH] qla2xxx: Protect access to qpair members with qpair->qp_lock (fwd)
Please check on whether an unlock is neeed before line 1965. julia -- Forwarded message -- Date: Fri, 23 Jun 2017 15:23:00 +0800 From: kbuild test robot <fengguang...@intel.com> To: kbu...@01.org Cc: Julia Lawall <julia.law...@lip6.fr> Subject: Re: [PATCH] qla2xxx: Protect access to qpair members with qpair->qp_lock CC: kbuild-...@01.org In-Reply-To: <20170622134325.26931-1-jthumsh...@suse.de> Hi Johannes, [auto build test WARNING on scsi/for-next] [also build test WARNING on v4.12-rc6 next-20170622] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system] url: https://github.com/0day-ci/linux/commits/Johannes-Thumshirn/qla2xxx-Protect-access-to-qpair-members-with-qpair-qp_lock/20170623-123844 base: https://git.kernel.org/pub/scm/linux/kernel/git/jejb/scsi.git for-next :: branch date: 3 hours ago :: commit date: 3 hours ago >> drivers/scsi/qla2xxx/qla_iocb.c:1965:3-9: preceding lock on line 1952 git remote add linux-review https://github.com/0day-ci/linux git remote update linux-review git checkout 4a35d720268dbe9ac016957a3c4fc644398d68ba vim +1965 drivers/scsi/qla2xxx/qla_iocb.c d74595278 Michael Hernandez 2016-12-12 1946 /* Only process protection or >16 cdb in this routine */ d74595278 Michael Hernandez 2016-12-12 1947 if (scsi_get_prot_op(cmd) == SCSI_PROT_NORMAL) { d74595278 Michael Hernandez 2016-12-12 1948 if (cmd->cmd_len <= 16) d74595278 Michael Hernandez 2016-12-12 1949 return qla2xxx_start_scsi_mq(sp); d74595278 Michael Hernandez 2016-12-12 1950 } d74595278 Michael Hernandez 2016-12-12 1951 4a35d7202 Johannes Thumshirn 2017-06-22 @1952 spin_lock_irqsave(>qp_lock, flags); 4a35d7202 Johannes Thumshirn 2017-06-22 1953 d74595278 Michael Hernandez 2016-12-12 1954 /* Setup qpair pointers */ d74595278 Michael Hernandez 2016-12-12 1955 rsp = qpair->rsp; d74595278 Michael Hernandez 2016-12-12 1956 req = qpair->req; d74595278 Michael Hernandez 2016-12-12 1957 d74595278 Michael Hernandez 2016-12-12 1958 /* So we know we haven't pci_map'ed anything yet */ d74595278 Michael Hernandez 2016-12-12 1959 tot_dsds = 0; d74595278 Michael Hernandez 2016-12-12 1960 d74595278 Michael Hernandez 2016-12-12 1961 /* Send marker if required */ d74595278 Michael Hernandez 2016-12-12 1962 if (vha->marker_needed != 0) { d74595278 Michael Hernandez 2016-12-12 1963 if (qla2x00_marker(vha, req, rsp, 0, 0, MK_SYNC_ALL) != d74595278 Michael Hernandez 2016-12-12 1964 QLA_SUCCESS) d74595278 Michael Hernandez 2016-12-12 @1965 return QLA_FUNCTION_FAILED; d74595278 Michael Hernandez 2016-12-12 1966 vha->marker_needed = 0; d74595278 Michael Hernandez 2016-12-12 1967 } d74595278 Michael Hernandez 2016-12-12 1968 :: The code at line 1965 was first introduced by commit :: d74595278f4ab192af66d9e60a9087464638beee scsi: qla2xxx: Add multiple queue pair functionality. :: TO: Michael Hernandez <michael.hernan...@cavium.com> :: CC: Martin K. Petersen <martin.peter...@oracle.com> --- 0-DAY kernel test infrastructureOpen Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation
Re: [PATCH] qla2xxx: Protect access to qpair members with qpair->qp_lock (fwd)
Please check on whether an unlock is neeed before line 1965. julia -- Forwarded message -- Date: Fri, 23 Jun 2017 15:23:00 +0800 From: kbuild test robot To: kbu...@01.org Cc: Julia Lawall Subject: Re: [PATCH] qla2xxx: Protect access to qpair members with qpair->qp_lock CC: kbuild-...@01.org In-Reply-To: <20170622134325.26931-1-jthumsh...@suse.de> Hi Johannes, [auto build test WARNING on scsi/for-next] [also build test WARNING on v4.12-rc6 next-20170622] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system] url: https://github.com/0day-ci/linux/commits/Johannes-Thumshirn/qla2xxx-Protect-access-to-qpair-members-with-qpair-qp_lock/20170623-123844 base: https://git.kernel.org/pub/scm/linux/kernel/git/jejb/scsi.git for-next :: branch date: 3 hours ago :: commit date: 3 hours ago >> drivers/scsi/qla2xxx/qla_iocb.c:1965:3-9: preceding lock on line 1952 git remote add linux-review https://github.com/0day-ci/linux git remote update linux-review git checkout 4a35d720268dbe9ac016957a3c4fc644398d68ba vim +1965 drivers/scsi/qla2xxx/qla_iocb.c d74595278 Michael Hernandez 2016-12-12 1946 /* Only process protection or >16 cdb in this routine */ d74595278 Michael Hernandez 2016-12-12 1947 if (scsi_get_prot_op(cmd) == SCSI_PROT_NORMAL) { d74595278 Michael Hernandez 2016-12-12 1948 if (cmd->cmd_len <= 16) d74595278 Michael Hernandez 2016-12-12 1949 return qla2xxx_start_scsi_mq(sp); d74595278 Michael Hernandez 2016-12-12 1950 } d74595278 Michael Hernandez 2016-12-12 1951 4a35d7202 Johannes Thumshirn 2017-06-22 @1952 spin_lock_irqsave(>qp_lock, flags); 4a35d7202 Johannes Thumshirn 2017-06-22 1953 d74595278 Michael Hernandez 2016-12-12 1954 /* Setup qpair pointers */ d74595278 Michael Hernandez 2016-12-12 1955 rsp = qpair->rsp; d74595278 Michael Hernandez 2016-12-12 1956 req = qpair->req; d74595278 Michael Hernandez 2016-12-12 1957 d74595278 Michael Hernandez 2016-12-12 1958 /* So we know we haven't pci_map'ed anything yet */ d74595278 Michael Hernandez 2016-12-12 1959 tot_dsds = 0; d74595278 Michael Hernandez 2016-12-12 1960 d74595278 Michael Hernandez 2016-12-12 1961 /* Send marker if required */ d74595278 Michael Hernandez 2016-12-12 1962 if (vha->marker_needed != 0) { d74595278 Michael Hernandez 2016-12-12 1963 if (qla2x00_marker(vha, req, rsp, 0, 0, MK_SYNC_ALL) != d74595278 Michael Hernandez 2016-12-12 1964 QLA_SUCCESS) d74595278 Michael Hernandez 2016-12-12 @1965 return QLA_FUNCTION_FAILED; d74595278 Michael Hernandez 2016-12-12 1966 vha->marker_needed = 0; d74595278 Michael Hernandez 2016-12-12 1967 } d74595278 Michael Hernandez 2016-12-12 1968 :: The code at line 1965 was first introduced by commit :: d74595278f4ab192af66d9e60a9087464638beee scsi: qla2xxx: Add multiple queue pair functionality. :: TO: Michael Hernandez :: CC: Martin K. Petersen --- 0-DAY kernel test infrastructureOpen Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation
Re: [PATCH] qla2xxx: Protect access to qpair members with qpair->qp_lock
On 22/06/2017 14:43, Johannes Thumshirn wrote: In qla2xx_start_scsi_mq() and qla2xx_dif_start_scsi_mq() we grab the qpair->qp_lock but do access members of the qpair before having the lock. Re-order the locking sequence to have all read and write access to qpair members under the qpair->qp_lock. Signed-off-by: Johannes Thumshirn--- drivers/scsi/qla2xxx/qla_iocb.c | 20 +--- 1 file changed, 13 insertions(+), 7 deletions(-) diff --git a/drivers/scsi/qla2xxx/qla_iocb.c b/drivers/scsi/qla2xxx/qla_iocb.c index 8404f17f3c6c..425ca1646a9a 100644 --- a/drivers/scsi/qla2xxx/qla_iocb.c +++ b/drivers/scsi/qla2xxx/qla_iocb.c @@ -1770,10 +1770,6 @@ qla2xxx_start_scsi_mq(srb_t *sp) struct qla_hw_data *ha = vha->hw; struct qla_qpair *qpair = sp->qpair; - /* Setup qpair pointers */ - rsp = qpair->rsp; - req = qpair->req; Can you check the call to qla2x00_marker() before the spinlock grab, which takes rsp and req as parameters? - /* So we know we haven't pci_map'ed anything yet */ tot_dsds = 0; @@ -1788,6 +1784,10 @@ qla2xxx_start_scsi_mq(srb_t *sp) /* Acquire qpair specific lock */ spin_lock_irqsave(>qp_lock, flags); + /* Setup qpair pointers */ + rsp = qpair->rsp; + req = qpair->req; + /* Check for room in outstanding command list. */ handle = req->current_outstanding_cmd; for (index = 1; index < req->num_outstanding_cmds; index++) { @@ -1924,24 +1924,33 @@ qla2xxx_dif_start_scsi_mq(srb_t *sp) #define QDSS_GOT_Q_SPACE BIT_0 + /* Acquire ring specific lock */ + spin_lock_irqsave(>qp_lock, flags); + /* Check for host side state */ if (!qpair->online) { cmd->result = DID_NO_CONNECT << 16; + spin_unlock_irqrestore(>qp_lock, flags); return QLA_INTERFACE_ERROR; } if (!qpair->difdix_supported && scsi_get_prot_op(cmd) != SCSI_PROT_NORMAL) { cmd->result = DID_NO_CONNECT << 16; + spin_unlock_irqrestore(>qp_lock, flags); return QLA_INTERFACE_ERROR; } + spin_unlock_irqrestore(>qp_lock, flags); + /* Only process protection or >16 cdb in this routine */ if (scsi_get_prot_op(cmd) == SCSI_PROT_NORMAL) { if (cmd->cmd_len <= 16) return qla2xxx_start_scsi_mq(sp); } + spin_lock_irqsave(>qp_lock, flags); + /* Setup qpair pointers */ rsp = qpair->rsp; req = qpair->req; @@ -1957,9 +1966,6 @@ qla2xxx_dif_start_scsi_mq(srb_t *sp) vha->marker_needed = 0; } - /* Acquire ring specific lock */ - spin_lock_irqsave(>qp_lock, flags); - /* Check for room in outstanding command list. */ handle = req->current_outstanding_cmd; for (index = 1; index < req->num_outstanding_cmds; index++) {
Re: [PATCH] qla2xxx: Protect access to qpair members with qpair->qp_lock
On 22/06/2017 14:43, Johannes Thumshirn wrote: In qla2xx_start_scsi_mq() and qla2xx_dif_start_scsi_mq() we grab the qpair->qp_lock but do access members of the qpair before having the lock. Re-order the locking sequence to have all read and write access to qpair members under the qpair->qp_lock. Signed-off-by: Johannes Thumshirn --- drivers/scsi/qla2xxx/qla_iocb.c | 20 +--- 1 file changed, 13 insertions(+), 7 deletions(-) diff --git a/drivers/scsi/qla2xxx/qla_iocb.c b/drivers/scsi/qla2xxx/qla_iocb.c index 8404f17f3c6c..425ca1646a9a 100644 --- a/drivers/scsi/qla2xxx/qla_iocb.c +++ b/drivers/scsi/qla2xxx/qla_iocb.c @@ -1770,10 +1770,6 @@ qla2xxx_start_scsi_mq(srb_t *sp) struct qla_hw_data *ha = vha->hw; struct qla_qpair *qpair = sp->qpair; - /* Setup qpair pointers */ - rsp = qpair->rsp; - req = qpair->req; Can you check the call to qla2x00_marker() before the spinlock grab, which takes rsp and req as parameters? - /* So we know we haven't pci_map'ed anything yet */ tot_dsds = 0; @@ -1788,6 +1784,10 @@ qla2xxx_start_scsi_mq(srb_t *sp) /* Acquire qpair specific lock */ spin_lock_irqsave(>qp_lock, flags); + /* Setup qpair pointers */ + rsp = qpair->rsp; + req = qpair->req; + /* Check for room in outstanding command list. */ handle = req->current_outstanding_cmd; for (index = 1; index < req->num_outstanding_cmds; index++) { @@ -1924,24 +1924,33 @@ qla2xxx_dif_start_scsi_mq(srb_t *sp) #define QDSS_GOT_Q_SPACE BIT_0 + /* Acquire ring specific lock */ + spin_lock_irqsave(>qp_lock, flags); + /* Check for host side state */ if (!qpair->online) { cmd->result = DID_NO_CONNECT << 16; + spin_unlock_irqrestore(>qp_lock, flags); return QLA_INTERFACE_ERROR; } if (!qpair->difdix_supported && scsi_get_prot_op(cmd) != SCSI_PROT_NORMAL) { cmd->result = DID_NO_CONNECT << 16; + spin_unlock_irqrestore(>qp_lock, flags); return QLA_INTERFACE_ERROR; } + spin_unlock_irqrestore(>qp_lock, flags); + /* Only process protection or >16 cdb in this routine */ if (scsi_get_prot_op(cmd) == SCSI_PROT_NORMAL) { if (cmd->cmd_len <= 16) return qla2xxx_start_scsi_mq(sp); } + spin_lock_irqsave(>qp_lock, flags); + /* Setup qpair pointers */ rsp = qpair->rsp; req = qpair->req; @@ -1957,9 +1966,6 @@ qla2xxx_dif_start_scsi_mq(srb_t *sp) vha->marker_needed = 0; } - /* Acquire ring specific lock */ - spin_lock_irqsave(>qp_lock, flags); - /* Check for room in outstanding command list. */ handle = req->current_outstanding_cmd; for (index = 1; index < req->num_outstanding_cmds; index++) {
Re: [PATCH] qla2xxx: Protect access to qpair members with qpair->qp_lock
On Thu, Jun 22, 2017 at 03:19:27PM +0100, John Garry wrote: > On 22/06/2017 14:43, Johannes Thumshirn wrote: > >In qla2xx_start_scsi_mq() and qla2xx_dif_start_scsi_mq() we grab the > >qpair->qp_lock but do access members of the qpair before having the lock. > >Re-order the locking sequence to have all read and write access to qpair > >members under the qpair->qp_lock. > > > >Signed-off-by: Johannes Thumshirn> >--- > > drivers/scsi/qla2xxx/qla_iocb.c | 20 +--- > > 1 file changed, 13 insertions(+), 7 deletions(-) > > > >diff --git a/drivers/scsi/qla2xxx/qla_iocb.c > >b/drivers/scsi/qla2xxx/qla_iocb.c > >index 8404f17f3c6c..425ca1646a9a 100644 > >--- a/drivers/scsi/qla2xxx/qla_iocb.c > >+++ b/drivers/scsi/qla2xxx/qla_iocb.c > >@@ -1770,10 +1770,6 @@ qla2xxx_start_scsi_mq(srb_t *sp) > > struct qla_hw_data *ha = vha->hw; > > struct qla_qpair *qpair = sp->qpair; > > > >-/* Setup qpair pointers */ > >-rsp = qpair->rsp; > >-req = qpair->req; > > Can you check the call to qla2x00_marker() before the spinlock grab, which > takes rsp and req as parameters? Good catch, thanks. -- Johannes Thumshirn Storage jthumsh...@suse.de+49 911 74053 689 SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg GF: Felix Imendörffer, Jane Smithard, Graham Norton HRB 21284 (AG Nürnberg) Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850
Re: [PATCH] qla2xxx: Protect access to qpair members with qpair->qp_lock
On Thu, Jun 22, 2017 at 03:19:27PM +0100, John Garry wrote: > On 22/06/2017 14:43, Johannes Thumshirn wrote: > >In qla2xx_start_scsi_mq() and qla2xx_dif_start_scsi_mq() we grab the > >qpair->qp_lock but do access members of the qpair before having the lock. > >Re-order the locking sequence to have all read and write access to qpair > >members under the qpair->qp_lock. > > > >Signed-off-by: Johannes Thumshirn > >--- > > drivers/scsi/qla2xxx/qla_iocb.c | 20 +--- > > 1 file changed, 13 insertions(+), 7 deletions(-) > > > >diff --git a/drivers/scsi/qla2xxx/qla_iocb.c > >b/drivers/scsi/qla2xxx/qla_iocb.c > >index 8404f17f3c6c..425ca1646a9a 100644 > >--- a/drivers/scsi/qla2xxx/qla_iocb.c > >+++ b/drivers/scsi/qla2xxx/qla_iocb.c > >@@ -1770,10 +1770,6 @@ qla2xxx_start_scsi_mq(srb_t *sp) > > struct qla_hw_data *ha = vha->hw; > > struct qla_qpair *qpair = sp->qpair; > > > >-/* Setup qpair pointers */ > >-rsp = qpair->rsp; > >-req = qpair->req; > > Can you check the call to qla2x00_marker() before the spinlock grab, which > takes rsp and req as parameters? Good catch, thanks. -- Johannes Thumshirn Storage jthumsh...@suse.de+49 911 74053 689 SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg GF: Felix Imendörffer, Jane Smithard, Graham Norton HRB 21284 (AG Nürnberg) Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850
Re: [PATCH] qla2xxx: Protect access to qpair members with qpair->qp_lock
On 06/22/2017 03:43 PM, Johannes Thumshirn wrote: > In qla2xx_start_scsi_mq() and qla2xx_dif_start_scsi_mq() we grab the > qpair->qp_lock but do access members of the qpair before having the lock. > Re-order the locking sequence to have all read and write access to qpair > members under the qpair->qp_lock. > > Signed-off-by: Johannes Thumshirn> --- > drivers/scsi/qla2xxx/qla_iocb.c | 20 +--- > 1 file changed, 13 insertions(+), 7 deletions(-) > > diff --git a/drivers/scsi/qla2xxx/qla_iocb.c b/drivers/scsi/qla2xxx/qla_iocb.c > index 8404f17f3c6c..425ca1646a9a 100644 > --- a/drivers/scsi/qla2xxx/qla_iocb.c > +++ b/drivers/scsi/qla2xxx/qla_iocb.c > @@ -1770,10 +1770,6 @@ qla2xxx_start_scsi_mq(srb_t *sp) > struct qla_hw_data *ha = vha->hw; > struct qla_qpair *qpair = sp->qpair; > > - /* Setup qpair pointers */ > - rsp = qpair->rsp; > - req = qpair->req; > - > /* So we know we haven't pci_map'ed anything yet */ > tot_dsds = 0; > > @@ -1788,6 +1784,10 @@ qla2xxx_start_scsi_mq(srb_t *sp) > /* Acquire qpair specific lock */ > spin_lock_irqsave(>qp_lock, flags); > > + /* Setup qpair pointers */ > + rsp = qpair->rsp; > + req = qpair->req; > + > /* Check for room in outstanding command list. */ > handle = req->current_outstanding_cmd; > for (index = 1; index < req->num_outstanding_cmds; index++) { > @@ -1924,24 +1924,33 @@ qla2xxx_dif_start_scsi_mq(srb_t *sp) > > #define QDSS_GOT_Q_SPACE BIT_0 > > + /* Acquire ring specific lock */ > + spin_lock_irqsave(>qp_lock, flags); > + > /* Check for host side state */ > if (!qpair->online) { > cmd->result = DID_NO_CONNECT << 16; > + spin_unlock_irqrestore(>qp_lock, flags); > return QLA_INTERFACE_ERROR; > } > > if (!qpair->difdix_supported && > scsi_get_prot_op(cmd) != SCSI_PROT_NORMAL) { > cmd->result = DID_NO_CONNECT << 16; > + spin_unlock_irqrestore(>qp_lock, flags); > return QLA_INTERFACE_ERROR; > } > > + spin_unlock_irqrestore(>qp_lock, flags); > + > /* Only process protection or >16 cdb in this routine */ > if (scsi_get_prot_op(cmd) == SCSI_PROT_NORMAL) { > if (cmd->cmd_len <= 16) > return qla2xxx_start_scsi_mq(sp); > } > Not sure if that is required; at worst we're missing a device being online; difdix support is probably not changing that often to be warranted being protected with a lock here. So I'd rather omit spinlocks here. Cheers, Hannes -- Dr. Hannes ReineckeTeamlead Storage & Networking h...@suse.de +49 911 74053 688 SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton HRB 21284 (AG Nürnberg)
Re: [PATCH] qla2xxx: Protect access to qpair members with qpair->qp_lock
On 06/22/2017 03:43 PM, Johannes Thumshirn wrote: > In qla2xx_start_scsi_mq() and qla2xx_dif_start_scsi_mq() we grab the > qpair->qp_lock but do access members of the qpair before having the lock. > Re-order the locking sequence to have all read and write access to qpair > members under the qpair->qp_lock. > > Signed-off-by: Johannes Thumshirn > --- > drivers/scsi/qla2xxx/qla_iocb.c | 20 +--- > 1 file changed, 13 insertions(+), 7 deletions(-) > > diff --git a/drivers/scsi/qla2xxx/qla_iocb.c b/drivers/scsi/qla2xxx/qla_iocb.c > index 8404f17f3c6c..425ca1646a9a 100644 > --- a/drivers/scsi/qla2xxx/qla_iocb.c > +++ b/drivers/scsi/qla2xxx/qla_iocb.c > @@ -1770,10 +1770,6 @@ qla2xxx_start_scsi_mq(srb_t *sp) > struct qla_hw_data *ha = vha->hw; > struct qla_qpair *qpair = sp->qpair; > > - /* Setup qpair pointers */ > - rsp = qpair->rsp; > - req = qpair->req; > - > /* So we know we haven't pci_map'ed anything yet */ > tot_dsds = 0; > > @@ -1788,6 +1784,10 @@ qla2xxx_start_scsi_mq(srb_t *sp) > /* Acquire qpair specific lock */ > spin_lock_irqsave(>qp_lock, flags); > > + /* Setup qpair pointers */ > + rsp = qpair->rsp; > + req = qpair->req; > + > /* Check for room in outstanding command list. */ > handle = req->current_outstanding_cmd; > for (index = 1; index < req->num_outstanding_cmds; index++) { > @@ -1924,24 +1924,33 @@ qla2xxx_dif_start_scsi_mq(srb_t *sp) > > #define QDSS_GOT_Q_SPACE BIT_0 > > + /* Acquire ring specific lock */ > + spin_lock_irqsave(>qp_lock, flags); > + > /* Check for host side state */ > if (!qpair->online) { > cmd->result = DID_NO_CONNECT << 16; > + spin_unlock_irqrestore(>qp_lock, flags); > return QLA_INTERFACE_ERROR; > } > > if (!qpair->difdix_supported && > scsi_get_prot_op(cmd) != SCSI_PROT_NORMAL) { > cmd->result = DID_NO_CONNECT << 16; > + spin_unlock_irqrestore(>qp_lock, flags); > return QLA_INTERFACE_ERROR; > } > > + spin_unlock_irqrestore(>qp_lock, flags); > + > /* Only process protection or >16 cdb in this routine */ > if (scsi_get_prot_op(cmd) == SCSI_PROT_NORMAL) { > if (cmd->cmd_len <= 16) > return qla2xxx_start_scsi_mq(sp); > } > Not sure if that is required; at worst we're missing a device being online; difdix support is probably not changing that often to be warranted being protected with a lock here. So I'd rather omit spinlocks here. Cheers, Hannes -- Dr. Hannes ReineckeTeamlead Storage & Networking h...@suse.de +49 911 74053 688 SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton HRB 21284 (AG Nürnberg)