Re: [bug report] scsi: lpfc: Support dynamic unbounded SGL lists on G7 hardware.

[James Smart <jsmart2021@gmail.com>]

On 8/26/2019 6:40 AM, Dan Carpenter wrote:
> Hello James Smart,
> 
> The patch d79c9e9d4b3d: "scsi: lpfc: Support dynamic unbounded SGL
> lists on G7 hardware." from Aug 14, 2019, leads to the following
> static checker warning:
> 
> 	drivers/scsi/lpfc/lpfc_init.c:4107 lpfc_new_io_buf()
> 	error: not allocating enough data 784 vs 768
> 
> drivers/scsi/lpfc/lpfc_init.c
>    4071  /**
>    4072   * lpfc_new_io_buf - IO buffer allocator for HBA with SLI4 IF spec
>    4073   * @vport: The virtual port for which this call being executed.
>    4074   * @num_to_allocate: The requested number of buffers to allocate.
>    4075   *
>    4076   * This routine allocates nvme buffers for device with SLI-4 interface spec,
>    4077   * the nvme buffer contains all the necessary information needed to initiate
>    4078   * an I/O. After allocating up to @num_to_allocate IO buffers and put
>    4079   * them on a list, it post them to the port by using SGL block post.
>    4080   *
>    4081   * Return codes:
>    4082   *   int - number of IO buffers that were allocated and posted.
>    4083   *   0 = failure, less than num_to_alloc is a partial failure.
>    4084   **/
>    4085  int
>    4086  lpfc_new_io_buf(struct lpfc_hba *phba, int num_to_alloc)
>    4087  {
>    4088          struct lpfc_io_buf *lpfc_ncmd;
>    4089          struct lpfc_iocbq *pwqeq;
>    4090          uint16_t iotag, lxri = 0;
>    4091          int bcnt, num_posted;
>    4092          LIST_HEAD(prep_nblist);
>    4093          LIST_HEAD(post_nblist);
>    4094          LIST_HEAD(nvme_nblist);
>    4095
>    4096          /* Sanity check to ensure our sizing is right for both SCSI and NVME */
>    4097          if (sizeof(struct lpfc_io_buf) > LPFC_COMMON_IO_BUF_SZ) {
>                      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> 
> We made the lpfc_io_buf struct larger so now this check is more likely
> to trigger.  Why don't we make this condition a BUILD_BUG_ON()?
> 
>    4098                  lpfc_printf_log(phba, KERN_ERR, LOG_FCP,
>    4099                                  "6426 Common buffer size %zd exceeds %d\n",
>    4100                                  sizeof(struct lpfc_io_buf),
>    4101                                  LPFC_COMMON_IO_BUF_SZ);
>    4102                  return 0;
> 
> Zero means we're returning failure on this path.
> 
>    4103          }
>    4104
>    4105          phba->sli4_hba.io_xri_cnt = 0;
>    4106          for (bcnt = 0; bcnt < num_to_alloc; bcnt++) {
>    4107                  lpfc_ncmd = kzalloc(LPFC_COMMON_IO_BUF_SZ, GFP_KERNEL);
>                          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> 
> Smatch generates a warning here.  It's obviously not a problem, because
> of the earlier check.  I guess I don't really understand why
> LPFC_COMMON_IO_BUF_SZ is useful when it's so close to sizeof(*lpfc_ncmd).

Completely agree - I'll clean this up.

-- james