Re: [PATCH 04/17] lpfc: NVME Initiator: Base modifications Part C
On Wed, Jan 18, 2017 at 06:54:37PM -0800, James Smart wrote: > > > On 1/18/2017 3:03 AM, Johannes Thumshirn wrote: > > > >>+ /* maximum number of xris available for nvme buffers */ > >>+ els_xri_cnt = lpfc_sli4_get_els_iocb_cnt(phba); > >>+ phba->sli4_hba.nvme_xri_max = phba->sli4_hba.max_cfg_param.max_xri - > >>+ els_xri_cnt; > >>+ phba->sli4_hba.nvme_xri_max -= phba->sli4_hba.scsi_xri_max; > > nvme_xri_max = phba->sli4_hba.max_cfg_param.max_xri - els_xri_cnt; > > nvme_xri_max -= phba->sli4_hba.scsi_xri_max; > > phba->sli4_hba.nvme_xri_max = nvme_xri_max; > > > >Low hanging anti line-break fruit. > > ok - but I didn't think that a style change like this is a mandate. As I'm > addressing your other comments, I'll do so. I don't think it's a mandate, but line wrappings are always bad to read. Code is written once but read a lot of times. I know I'm nitpicking a lot here but hard to read code makes errors hard to spot. Thanks for your patience, Johannes -- 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 -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 04/17] lpfc: NVME Initiator: Base modifications Part C
On 1/18/2017 3:03 AM, Johannes Thumshirn wrote: + /* maximum number of xris available for nvme buffers */ + els_xri_cnt = lpfc_sli4_get_els_iocb_cnt(phba); + phba->sli4_hba.nvme_xri_max = phba->sli4_hba.max_cfg_param.max_xri - + els_xri_cnt; + phba->sli4_hba.nvme_xri_max -= phba->sli4_hba.scsi_xri_max; nvme_xri_max = phba->sli4_hba.max_cfg_param.max_xri - els_xri_cnt; nvme_xri_max -= phba->sli4_hba.scsi_xri_max; phba->sli4_hba.nvme_xri_max = nvme_xri_max; Low hanging anti line-break fruit. ok - but I didn't think that a style change like this is a mandate. As I'm addressing your other comments, I'll do so. } @@ -4273,13 +4489,13 @@ lpfc_sli4_async_sli_evt(struct lpfc_hba *phba, struct lpfc_acqe_sli *acqe_sli) sprintf(message, "Unqualified optics - Replace with " "Avago optics for Warranty and Technical " Is Avago still correct, or should it read Broadcom? It's right - Avago. @@ -4854,17 +5070,20 @@ static int lpfc_enable_pci_dev(struct lpfc_hba *phba) { struct pci_dev *pdev; + int bars = 0; /* Obtain PCI device reference */ if (!phba->pcidev) goto out_error; else pdev = phba->pcidev; + /* Select PCI BARs */ + bars = pci_select_bars(pdev, IORESOURCE_MEM); /* Enable PCI device */ if (pci_enable_device_mem(pdev)) goto out_error; /* Request PCI resource for the device */ - if (pci_request_mem_regions(pdev, LPFC_DRIVER_NAME)) + if (pci_request_selected_regions(pdev, bars, LPFC_DRIVER_NAME)) goto out_disable_device; /* Set up device as PCI master and save state for EEH */ pci_set_master(pdev); @@ -4881,7 +5100,7 @@ lpfc_enable_pci_dev(struct lpfc_hba *phba) pci_disable_device(pdev); out_error: lpfc_printf_log(phba, KERN_ERR, LOG_INIT, - "1401 Failed to enable pci device\n"); + "1401 Failed to enable pci device, bars:x%x\n", bars); return -ENODEV; } I don't get this change. pci_request_mem_regions does pci_request_selected_regions(pdev, pci_select_bars(pdev, IORESOURCE_MEM), name); if you want to have the bars in the error log please do: lpfc_printf_log(phba, KERN_ERR, LOG_INIT, "1401 Failed to enable pci device, bars:x%x\n", pci_select_regions(pdev, IORESOURCE_MEM)); I agree - this is weird. I'll track why it was ever changed and address it. Other comments are good. I'll address them. -- james -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 04/17] lpfc: NVME Initiator: Base modifications Part C
On 1/17/2017 11:17 PM, Hannes Reinecke wrote: @@ -3315,16 +3421,121 @@ lpfc_sli4_xri_sgl_update(struct lpfc_hba *phba) [ .. ] Unsafe. 'nvme_xri_cnt' is updated under the lock, but not tested with the lock held. Please fix. Yep - will fix. -- james -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 04/17] lpfc: NVME Initiator: Base modifications Part C
On Tue, Jan 17, 2017 at 05:20:47PM -0800, James Smart wrote: > > NVME Initiator: Base modifications > > This is part C of parts A..F. > > Part C is the 1st half of the mods to lpfc_init.c. This is the location > of most of changes for the following: > - sli3 ring vs sli4 wq splits > - buffer pools are allocated/freed > - sgl pools allocated/freed > - adapter resources split up > - queue config decided and enacted > - receive buffer management > > * > > Refer to Part A for a description of base modifications > > Signed-off-by: Dick Kennedy > Signed-off-by: James Smart > --- [...] > @@ -925,32 +926,43 @@ static void > lpfc_hba_clean_txcmplq(struct lpfc_hba *phba) > { > struct lpfc_sli *psli = &phba->sli; > + struct lpfc_queue *qp = NULL; > struct lpfc_sli_ring *pring; > LIST_HEAD(completions); > int i; > > - for (i = 0; i < psli->num_rings; i++) { > - pring = &psli->ring[i]; > - if (phba->sli_rev >= LPFC_SLI_REV4) > - spin_lock_irq(&pring->ring_lock); > - else > + if (phba->sli_rev != LPFC_SLI_REV4) { > + for (i = 0; i < psli->num_rings; i++) { > + pring = &psli->sli3_ring[i]; > spin_lock_irq(&phba->hbalock); > - /* At this point in time the HBA is either reset or DOA. Either > - * way, nothing should be on txcmplq as it will NEVER complete. > - */ > - list_splice_init(&pring->txcmplq, &completions); > - pring->txcmplq_cnt = 0; > - > - if (phba->sli_rev >= LPFC_SLI_REV4) > - spin_unlock_irq(&pring->ring_lock); > - else > + /* At this point in time the HBA is either reset or DOA > + * Nothing should be on txcmplq as it will > + * NEVER complete. > + */ > + list_splice_init(&pring->txcmplq, &completions); > + pring->txcmplq_cnt = 0; > spin_unlock_irq(&phba->hbalock); > > + lpfc_sli_abort_iocb_ring(phba, pring); > + } > /* Cancel all the IOCBs from the completions list */ > - lpfc_sli_cancel_iocbs(phba, &completions, IOSTAT_LOCAL_REJECT, > - IOERR_SLI_ABORTED); > + lpfc_sli_cancel_iocbs(phba, &completions, > + IOSTAT_LOCAL_REJECT, IOERR_SLI_ABORTED); > + return; > + } And another great opportunity to factor a block into a helper function. [...] > /** > + * lpfc_sli4_nvme_sgl_update - update xri-sgl sizing and mapping > + * @phba: pointer to lpfc hba data structure. > + * > + * This routine first calculates the sizes of the current els and allocated > + * scsi sgl lists, and then goes through all sgls to updates the physical > + * XRIs assigned due to port function reset. During port initialization, the > + * current els and allocated scsi sgl lists are 0s. > + * > + * Return codes > + * 0 - successful (for now, it always returns 0) > + **/ > +int > +lpfc_sli4_nvme_sgl_update(struct lpfc_hba *phba) > +{ > + struct lpfc_nvme_buf *lpfc_ncmd = NULL, *lpfc_ncmd_next = NULL; > + uint16_t i, lxri, els_xri_cnt; > + uint16_t nvme_xri_cnt; > + LIST_HEAD(nvme_sgl_list); > + int rc; > + > + phba->total_nvme_bufs = 0; > + > + if (!(phba->cfg_enable_fc4_type & LPFC_ENABLE_NVME)) > + return 0; > + /* > + * update on pci function's allocated nvme xri-sgl list > + */ > + > + /* maximum number of xris available for nvme buffers */ > + els_xri_cnt = lpfc_sli4_get_els_iocb_cnt(phba); > + phba->sli4_hba.nvme_xri_max = phba->sli4_hba.max_cfg_param.max_xri - > + els_xri_cnt; > + phba->sli4_hba.nvme_xri_max -= phba->sli4_hba.scsi_xri_max; nvme_xri_max = phba->sli4_hba.max_cfg_param.max_xri - els_xri_cnt; nvme_xri_max -= phba->sli4_hba.scsi_xri_max; phba->sli4_hba.nvme_xri_max = nvme_xri_max; Low hanging anti line-break fruit. [...] > @@ -4240,9 +4456,9 @@ lpfc_sli4_async_sli_evt(struct lpfc_hba *phba, struct > lpfc_acqe_sli *acqe_sli) > break; > default: > lpfc_printf_log(phba, KERN_ERR, LOG_SLI, > - "3296 " > - "LPFC_SLI_EVENT_TYPE_MISCONFIGURED " > - "event: Invalid link %d", > + "3296 LPFC_SLI_EVENT_TYPE_" > + "MISCONFIGURED event: " > + "Invalid link %d\n", > phba->sli4_hba.lnk_info.lnk_no); > return; > } > @@ -4273,13 +4489,13 @@ lpfc_sli4_async_sli_evt(struct lpfc_hba *phba, struct >
Re: [PATCH 04/17] lpfc: NVME Initiator: Base modifications Part C
On 01/18/2017 02:20 AM, James Smart wrote: > > NVME Initiator: Base modifications > > This is part C of parts A..F. > > Part C is the 1st half of the mods to lpfc_init.c. This is the location > of most of changes for the following: > - sli3 ring vs sli4 wq splits > - buffer pools are allocated/freed > - sgl pools allocated/freed > - adapter resources split up > - queue config decided and enacted > - receive buffer management > > * > > Refer to Part A for a description of base modifications > > Signed-off-by: Dick Kennedy > Signed-off-by: James Smart > --- [ .. ] > @@ -3315,16 +3421,121 @@ lpfc_sli4_xri_sgl_update(struct lpfc_hba *phba) [ .. ] > + if (phba->sli4_hba.nvme_xri_cnt > phba->sli4_hba.nvme_xri_max) { > + /* max nvme xri shrunk below the allocated nvme buffers */ > + nvme_xri_cnt = phba->sli4_hba.nvme_xri_cnt - > + phba->sli4_hba.nvme_xri_max; > + /* release the extra allocated nvme buffers */ > + for (i = 0; i < nvme_xri_cnt; i++) { > + list_remove_head(&nvme_sgl_list, lpfc_ncmd, > + struct lpfc_nvme_buf, list); > + if (lpfc_ncmd) { > + pci_pool_free(phba->lpfc_sg_dma_buf_pool, > + lpfc_ncmd->data, > + lpfc_ncmd->dma_handle); > + kfree(lpfc_ncmd); > + } > + } > + spin_lock_irq(&phba->nvme_buf_list_get_lock); > + phba->sli4_hba.nvme_xri_cnt -= nvme_xri_cnt; > + spin_unlock_irq(&phba->nvme_buf_list_get_lock); > + } > + Unsafe. 'nvme_xri_cnt' is updated under the lock, but not tested with the lock held. Please fix. 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) -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 04/17] lpfc: NVME Initiator: Base modifications Part C
NVME Initiator: Base modifications This is part C of parts A..F. Part C is the 1st half of the mods to lpfc_init.c. This is the location of most of changes for the following: - sli3 ring vs sli4 wq splits - buffer pools are allocated/freed - sgl pools allocated/freed - adapter resources split up - queue config decided and enacted - receive buffer management * Refer to Part A for a description of base modifications Signed-off-by: Dick Kennedy Signed-off-by: James Smart --- drivers/scsi/lpfc/lpfc_init.c | 1086 +++-- 1 file changed, 724 insertions(+), 362 deletions(-) diff --git a/drivers/scsi/lpfc/lpfc_init.c b/drivers/scsi/lpfc/lpfc_init.c index 7a17bd0..ca54beb 100644 --- a/drivers/scsi/lpfc/lpfc_init.c +++ b/drivers/scsi/lpfc/lpfc_init.c @@ -34,6 +34,7 @@ #include #include #include +#include #include #include @@ -46,8 +47,9 @@ #include "lpfc_sli4.h" #include "lpfc_nl.h" #include "lpfc_disc.h" -#include "lpfc_scsi.h" #include "lpfc.h" +#include "lpfc_scsi.h" +#include "lpfc_nvme.h" #include "lpfc_logmsg.h" #include "lpfc_crtn.h" #include "lpfc_vport.h" @@ -87,6 +89,7 @@ static struct scsi_transport_template *lpfc_transport_template = NULL; static struct scsi_transport_template *lpfc_vport_transport_template = NULL; static DEFINE_IDR(lpfc_hba_index); + /** * lpfc_config_port_prep - Perform lpfc initialization prior to config port * @phba: pointer to lpfc hba data structure. @@ -499,12 +502,10 @@ lpfc_config_port_post(struct lpfc_hba *phba) phba->link_state = LPFC_LINK_DOWN; /* Only process IOCBs on ELS ring till hba_state is READY */ - if (psli->ring[psli->extra_ring].sli.sli3.cmdringaddr) - psli->ring[psli->extra_ring].flag |= LPFC_STOP_IOCB_EVENT; - if (psli->ring[psli->fcp_ring].sli.sli3.cmdringaddr) - psli->ring[psli->fcp_ring].flag |= LPFC_STOP_IOCB_EVENT; - if (psli->ring[psli->next_ring].sli.sli3.cmdringaddr) - psli->ring[psli->next_ring].flag |= LPFC_STOP_IOCB_EVENT; + if (psli->sli3_ring[LPFC_EXTRA_RING].sli.sli3.cmdringaddr) + psli->sli3_ring[LPFC_EXTRA_RING].flag |= LPFC_STOP_IOCB_EVENT; + if (psli->sli3_ring[LPFC_FCP_RING].sli.sli3.cmdringaddr) + psli->sli3_ring[LPFC_FCP_RING].flag |= LPFC_STOP_IOCB_EVENT; /* Post receive buffers for desired rings */ if (phba->sli_rev != 3) @@ -892,7 +893,7 @@ lpfc_hba_free_post_buf(struct lpfc_hba *phba) lpfc_sli_hbqbuf_free_all(phba); else { /* Cleanup preposted buffers on the ELS ring */ - pring = &psli->ring[LPFC_ELS_RING]; + pring = &psli->sli3_ring[LPFC_ELS_RING]; spin_lock_irq(&phba->hbalock); list_splice_init(&pring->postbufq, &buflist); spin_unlock_irq(&phba->hbalock); @@ -925,32 +926,43 @@ static void lpfc_hba_clean_txcmplq(struct lpfc_hba *phba) { struct lpfc_sli *psli = &phba->sli; + struct lpfc_queue *qp = NULL; struct lpfc_sli_ring *pring; LIST_HEAD(completions); int i; - for (i = 0; i < psli->num_rings; i++) { - pring = &psli->ring[i]; - if (phba->sli_rev >= LPFC_SLI_REV4) - spin_lock_irq(&pring->ring_lock); - else + if (phba->sli_rev != LPFC_SLI_REV4) { + for (i = 0; i < psli->num_rings; i++) { + pring = &psli->sli3_ring[i]; spin_lock_irq(&phba->hbalock); - /* At this point in time the HBA is either reset or DOA. Either -* way, nothing should be on txcmplq as it will NEVER complete. -*/ - list_splice_init(&pring->txcmplq, &completions); - pring->txcmplq_cnt = 0; - - if (phba->sli_rev >= LPFC_SLI_REV4) - spin_unlock_irq(&pring->ring_lock); - else + /* At this point in time the HBA is either reset or DOA +* Nothing should be on txcmplq as it will +* NEVER complete. +*/ + list_splice_init(&pring->txcmplq, &completions); + pring->txcmplq_cnt = 0; spin_unlock_irq(&phba->hbalock); + lpfc_sli_abort_iocb_ring(phba, pring); + } /* Cancel all the IOCBs from the completions list */ - lpfc_sli_cancel_iocbs(phba, &completions, IOSTAT_LOCAL_REJECT, - IOERR_SLI_ABORTED); + lpfc_sli_cancel_iocbs(phba, &completions, + IOSTAT_LOCAL_REJECT, IOERR_SLI_ABORTED); + return; + } + list_for_each_entry(qp, &phba->sli4_hba.lpfc_wq_list, wq_list) { + pring = qp->pring; + if (!pring) +