Re: [PATCH 04/17] lpfc: NVME Initiator: Base modifications Part C

[Johannes Thumshirn]

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

[James Smart]

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

[James Smart]

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

[Johannes Thumshirn]

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 = >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 = >ring[i];
> - if (phba->sli_rev >= LPFC_SLI_REV4)
> - spin_lock_irq(>ring_lock);
> - else
> + if (phba->sli_rev != LPFC_SLI_REV4) {
> + for (i = 0; i < psli->num_rings; i++) {
> + pring = >sli3_ring[i];
>   spin_lock_irq(>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(>txcmplq, );
> - pring->txcmplq_cnt = 0;
> -
> - if (phba->sli_rev >= LPFC_SLI_REV4)
> - spin_unlock_irq(>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(>txcmplq, );
> + pring->txcmplq_cnt = 0;
>   spin_unlock_irq(>hbalock);
>  
> + lpfc_sli_abort_iocb_ring(phba, pring);
> + }

>   /* Cancel all the IOCBs from the completions list */
> - lpfc_sli_cancel_iocbs(phba, , IOSTAT_LOCAL_REJECT,
> -   IOERR_SLI_ABORTED);
> + lpfc_sli_cancel_iocbs(phba, ,
> +   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 
> lpfc_acqe_sli *acqe_sli)
>   

Re: [PATCH 04/17] lpfc: NVME Initiator: Base modifications Part C

[Hannes Reinecke]

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(_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(>nvme_buf_list_get_lock);
> + phba->sli4_hba.nvme_xri_cnt -= nvme_xri_cnt;
> + spin_unlock_irq(>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