Skip to contentSkip to conversation

scsi: lpfc: Support dynamic unbounded SGL lists on G7 hardware.

bug report Dan Carpenter 2 participants 2 messages Aug 26, 2019 SCSI

lore
Share & tools
lore
#0 lore raw

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).

4108 if (!lpfc_ncmd)
4109 break;
4110 /*

  4111                   * Get memory from the pci pool to map the virt space to
  4112                   * pci bus space for an I/O. The DMA buffer includes the
  4113                   * number of SGE's necessary to support the sg_tablesize.
  4114                   */

4115 lpfc_ncmd->data = dma_pool_zalloc(phba->lpfc_sg_dma_buf_pool,
4116 GFP_KERNEL,
4117 &lpfc_ncmd->dma_handle);
4118 if (!lpfc_ncmd->data) {

regards,
dan carpenter

#1 lore raw
Quoted 57 lines · Dan Carpenter

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);
                         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Quoted 3 lines

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