Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752399AbbLPXWz (ORCPT ); Wed, 16 Dec 2015 18:22:55 -0500 Received: from mail-pa0-f45.google.com ([209.85.220.45]:32828 "EHLO mail-pa0-f45.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751524AbbLPXWx (ORCPT ); Wed, 16 Dec 2015 18:22:53 -0500 Subject: Re: [RFC PATCH] lpfc: Add lockdep assertions To: Johannes Thumshirn , "James E.J. Bottomley" , Dick Kennedy , "Martin K. Petersen" References: <1448023037-43391-1-git-send-email-jthumshirn@suse.de> Cc: linux-scsi@vger.kernel.org, linux-kernel@vger.kernel.org From: James Smart Message-ID: <5671F24A.1020607@avagotech.com> Date: Wed, 16 Dec 2015 15:22:50 -0800 User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:38.0) Gecko/20100101 Thunderbird/38.4.0 MIME-Version: 1.0 In-Reply-To: <1448023037-43391-1-git-send-email-jthumshirn@suse.de> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 10522 Lines: 290 Johannes, Thank you for the time and effort on the patch. At this time, as it doesn't functionally change anything, I did not include the patch. I will consider it if we see additional issues it can help resolve. -- james s On 11/20/2015 4:37 AM, Johannes Thumshirn wrote: > Several functions in lpfc have comments stating that the function must be > called with the hbalock (or hostlock, or ringlock) held. Add > lockdep_assert_held() annotations to these functions, so one can actually > verify the locks are held. > > Signed-off-by: Johannes Thumshirn > --- > > I'm not sure if this is actually helpfull upstream but it helped me to debug a > downstream bug. > > drivers/scsi/lpfc/lpfc_hbadisc.c | 5 +++++ > drivers/scsi/lpfc/lpfc_sli.c | 43 ++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 48 insertions(+) > > diff --git a/drivers/scsi/lpfc/lpfc_hbadisc.c b/drivers/scsi/lpfc/lpfc_hbadisc.c > index bfc2442..96de2ab 100644 > --- a/drivers/scsi/lpfc/lpfc_hbadisc.c > +++ b/drivers/scsi/lpfc/lpfc_hbadisc.c > @@ -25,6 +25,7 @@ > #include > #include > #include > +#include > > #include > #include > @@ -1314,6 +1315,8 @@ __lpfc_update_fcf_record_pri(struct lpfc_hba *phba, uint16_t fcf_index, > { > struct lpfc_fcf_pri *fcf_pri; > > + lockdep_assert_held(&phba->hbalock); > + > fcf_pri = &phba->fcf.fcf_pri[fcf_index]; > fcf_pri->fcf_rec.fcf_index = fcf_index; > /* FCF record priority */ > @@ -1398,6 +1401,8 @@ __lpfc_update_fcf_record(struct lpfc_hba *phba, struct lpfc_fcf_rec *fcf_rec, > struct fcf_record *new_fcf_record, uint32_t addr_mode, > uint16_t vlan_id, uint32_t flag) > { > + lockdep_assert_held(&phba->hbalock); > + > /* Copy the fields from the HBA's FCF record */ > lpfc_copy_fcf_record(fcf_rec, new_fcf_record); > /* Update other fields of driver FCF record */ > diff --git a/drivers/scsi/lpfc/lpfc_sli.c b/drivers/scsi/lpfc/lpfc_sli.c > index f9585cd..6d84c77 100644 > --- a/drivers/scsi/lpfc/lpfc_sli.c > +++ b/drivers/scsi/lpfc/lpfc_sli.c > @@ -24,6 +24,7 @@ > #include > #include > #include > +#include > > #include > #include > @@ -576,6 +577,8 @@ __lpfc_sli_get_iocbq(struct lpfc_hba *phba) > struct list_head *lpfc_iocb_list = &phba->lpfc_iocb_list; > struct lpfc_iocbq * iocbq = NULL; > > + lockdep_assert_held(&phba->hbalock); > + > list_remove_head(lpfc_iocb_list, iocbq, struct lpfc_iocbq, list); > if (iocbq) > phba->iocb_cnt++; > @@ -797,6 +800,7 @@ int > lpfc_test_rrq_active(struct lpfc_hba *phba, struct lpfc_nodelist *ndlp, > uint16_t xritag) > { > + lockdep_assert_held(&phba->hbalock); > if (!ndlp) > return 0; > if (!ndlp->active_rrqs_xri_bitmap) > @@ -914,6 +918,8 @@ __lpfc_sli_get_sglq(struct lpfc_hba *phba, struct lpfc_iocbq *piocbq) > struct lpfc_nodelist *ndlp; > int found = 0; > > + lockdep_assert_held(&phba->hbalock); > + > if (piocbq->iocb_flag & LPFC_IO_FCP) { > lpfc_cmd = (struct lpfc_scsi_buf *) piocbq->context1; > ndlp = lpfc_cmd->rdata->pnode; > @@ -1003,6 +1009,8 @@ __lpfc_sli_release_iocbq_s4(struct lpfc_hba *phba, struct lpfc_iocbq *iocbq) > unsigned long iflag = 0; > struct lpfc_sli_ring *pring = &phba->sli.ring[LPFC_ELS_RING]; > > + lockdep_assert_held(&phba->hbalock); > + > if (iocbq->sli4_xritag == NO_XRI) > sglq = NULL; > else > @@ -1058,6 +1066,7 @@ __lpfc_sli_release_iocbq_s3(struct lpfc_hba *phba, struct lpfc_iocbq *iocbq) > { > size_t start_clean = offsetof(struct lpfc_iocbq, iocb); > > + lockdep_assert_held(&phba->hbalock); > > /* > * Clean all volatile data fields, preserve iotag and node struct. > @@ -1080,6 +1089,8 @@ __lpfc_sli_release_iocbq_s3(struct lpfc_hba *phba, struct lpfc_iocbq *iocbq) > static void > __lpfc_sli_release_iocbq(struct lpfc_hba *phba, struct lpfc_iocbq *iocbq) > { > + lockdep_assert_held(&phba->hbalock); > + > phba->__lpfc_sli_release_iocbq(phba, iocbq); > phba->iocb_cnt--; > } > @@ -1310,6 +1321,8 @@ static int > lpfc_sli_ringtxcmpl_put(struct lpfc_hba *phba, struct lpfc_sli_ring *pring, > struct lpfc_iocbq *piocb) > { > + lockdep_assert_held(&phba->hbalock); > + > list_add_tail(&piocb->list, &pring->txcmplq); > piocb->iocb_flag |= LPFC_IO_ON_TXCMPLQ; > > @@ -1344,6 +1357,8 @@ lpfc_sli_ringtx_get(struct lpfc_hba *phba, struct lpfc_sli_ring *pring) > { > struct lpfc_iocbq *cmd_iocb; > > + lockdep_assert_held(&phba->hbalock); > + > list_remove_head((&pring->txq), cmd_iocb, struct lpfc_iocbq, list); > return cmd_iocb; > } > @@ -1367,6 +1382,9 @@ lpfc_sli_next_iocb_slot (struct lpfc_hba *phba, struct lpfc_sli_ring *pring) > { > struct lpfc_pgp *pgp = &phba->port_gp[pring->ringno]; > uint32_t max_cmd_idx = pring->sli.sli3.numCiocb; > + > + lockdep_assert_held(&phba->hbalock); > + > if ((pring->sli.sli3.next_cmdidx == pring->sli.sli3.cmdidx) && > (++pring->sli.sli3.next_cmdidx >= max_cmd_idx)) > pring->sli.sli3.next_cmdidx = 0; > @@ -1497,6 +1515,7 @@ static void > lpfc_sli_submit_iocb(struct lpfc_hba *phba, struct lpfc_sli_ring *pring, > IOCB_t *iocb, struct lpfc_iocbq *nextiocb) > { > + lockdep_assert_held(&phba->hbalock); > /* > * Set up an iotag > */ > @@ -1606,6 +1625,8 @@ lpfc_sli_resume_iocb(struct lpfc_hba *phba, struct lpfc_sli_ring *pring) > IOCB_t *iocb; > struct lpfc_iocbq *nextiocb; > > + lockdep_assert_held(&phba->hbalock); > + > /* > * Check to see if: > * (a) there is anything on the txq to send > @@ -1647,6 +1668,8 @@ lpfc_sli_next_hbq_slot(struct lpfc_hba *phba, uint32_t hbqno) > { > struct hbq_s *hbqp = &phba->hbqs[hbqno]; > > + lockdep_assert_held(&phba->hbalock); > + > if (hbqp->next_hbqPutIdx == hbqp->hbqPutIdx && > ++hbqp->next_hbqPutIdx >= hbqp->entry_count) > hbqp->next_hbqPutIdx = 0; > @@ -1747,6 +1770,7 @@ static int > lpfc_sli_hbq_to_firmware(struct lpfc_hba *phba, uint32_t hbqno, > struct hbq_dmabuf *hbq_buf) > { > + lockdep_assert_held(&phba->hbalock); > return phba->lpfc_sli_hbq_to_firmware(phba, hbqno, hbq_buf); > } > > @@ -1768,6 +1792,7 @@ lpfc_sli_hbq_to_firmware_s3(struct lpfc_hba *phba, uint32_t hbqno, > struct lpfc_hbq_entry *hbqe; > dma_addr_t physaddr = hbq_buf->dbuf.phys; > > + lockdep_assert_held(&phba->hbalock); > /* Get next HBQ entry slot to use */ > hbqe = lpfc_sli_next_hbq_slot(phba, hbqno); > if (hbqe) { > @@ -1808,6 +1833,7 @@ lpfc_sli_hbq_to_firmware_s4(struct lpfc_hba *phba, uint32_t hbqno, > struct lpfc_rqe hrqe; > struct lpfc_rqe drqe; > > + lockdep_assert_held(&phba->hbalock); > hrqe.address_lo = putPaddrLow(hbq_buf->hbuf.phys); > hrqe.address_hi = putPaddrHigh(hbq_buf->hbuf.phys); > drqe.address_lo = putPaddrLow(hbq_buf->dbuf.phys); > @@ -1986,6 +2012,8 @@ lpfc_sli_hbqbuf_find(struct lpfc_hba *phba, uint32_t tag) > struct hbq_dmabuf *hbq_buf; > uint32_t hbqno; > > + lockdep_assert_held(&phba->hbalock); > + > hbqno = tag >> 16; > if (hbqno >= LPFC_MAX_HBQS) > return NULL; > @@ -2647,6 +2675,7 @@ lpfc_sli_iocbq_lookup(struct lpfc_hba *phba, > { > struct lpfc_iocbq *cmd_iocb = NULL; > uint16_t iotag; > + lockdep_assert_held(&phba->hbalock); > > iotag = prspiocb->iocb.ulpIoTag; > > @@ -2685,6 +2714,7 @@ lpfc_sli_iocbq_lookup_by_tag(struct lpfc_hba *phba, > { > struct lpfc_iocbq *cmd_iocb; > > + lockdep_assert_held(&phba->hbalock); > if (iotag != 0 && iotag <= phba->sli.last_iotag) { > cmd_iocb = phba->sli.iocbq_lookup[iotag]; > if (cmd_iocb->iocb_flag & LPFC_IO_ON_TXCMPLQ) { > @@ -3799,6 +3829,8 @@ void lpfc_reset_barrier(struct lpfc_hba *phba) > int i; > uint8_t hdrtype; > > + lockdep_assert_held(&phba->hbalock); > + > pci_read_config_byte(phba->pcidev, PCI_HEADER_TYPE, &hdrtype); > if (hdrtype != 0x80 || > (FC_JEDEC_ID(phba->vpd.rev.biuRev) != HELIOS_JEDEC_ID && > @@ -7861,6 +7893,7 @@ void > __lpfc_sli_ringtx_put(struct lpfc_hba *phba, struct lpfc_sli_ring *pring, > struct lpfc_iocbq *piocb) > { > + lockdep_assert_held(&phba->hbalock); > /* Insert the caller's iocb in the txq tail for later processing. */ > list_add_tail(&piocb->list, &pring->txq); > } > @@ -7888,6 +7921,8 @@ lpfc_sli_next_iocb(struct lpfc_hba *phba, struct lpfc_sli_ring *pring, > { > struct lpfc_iocbq * nextiocb; > > + lockdep_assert_held(&phba->hbalock); > + > nextiocb = lpfc_sli_ringtx_get(phba, pring); > if (!nextiocb) { > nextiocb = *piocb; > @@ -7927,6 +7962,8 @@ __lpfc_sli_issue_iocb_s3(struct lpfc_hba *phba, uint32_t ring_number, > IOCB_t *iocb; > struct lpfc_sli_ring *pring = &phba->sli.ring[ring_number]; > > + lockdep_assert_held(&phba->hbalock); > + > if (piocb->iocb_cmpl && (!piocb->vport) && > (piocb->iocb.ulpCommand != CMD_ABORT_XRI_CN) && > (piocb->iocb.ulpCommand != CMD_CLOSE_XRI_CN)) { > @@ -8642,6 +8679,8 @@ __lpfc_sli_issue_iocb_s4(struct lpfc_hba *phba, uint32_t ring_number, > struct lpfc_queue *wq; > struct lpfc_sli_ring *pring = &phba->sli.ring[ring_number]; > > + lockdep_assert_held(&phba->hbalock); > + > if (piocb->sli4_xritag == NO_XRI) { > if (piocb->iocb.ulpCommand == CMD_ABORT_XRI_CN || > piocb->iocb.ulpCommand == CMD_CLOSE_XRI_CN) > @@ -9752,6 +9791,8 @@ lpfc_sli_abort_iotag_issue(struct lpfc_hba *phba, struct lpfc_sli_ring *pring, > int retval; > unsigned long iflags; > > + lockdep_assert_held(&phba->hbalock); > + > /* > * There are certain command types we don't want to abort. And we > * don't want to abort commands that are already in the process of > @@ -9854,6 +9895,8 @@ lpfc_sli_issue_abort_iotag(struct lpfc_hba *phba, struct lpfc_sli_ring *pring, > int retval = IOCB_ERROR; > IOCB_t *icmd = NULL; > > + lockdep_assert_held(&phba->hbalock); > + > /* > * There are certain command types we don't want to abort. And we > * don't want to abort commands that are already in the process of -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/