RE: [PATCH v7 07/15] scsi: fnic: Add and integrate support for FDMI

[Karan Tilak Kumar (kartilak)]

On Tuesday, January 7, 2025 4:30 AM, Dan Carpenter <dan.carpenter@xxxxxxxxxx> wrote:
>
> On Wed, Dec 11, 2024 at 06:03:04PM -0800, Karan Tilak Kumar wrote:
> > @@ -612,6 +615,7 @@ static int fnic_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
> >     unsigned long flags;
> >     int hwq;
> >     char *desc, *subsys_desc;
> > +   int len;
>
> Do not introduce unnecessary levels of indirection.  Get rid of this len
> variable.

Thanks Dan. 
We will address this in a future patch.

> >
> >     /*
> >      * Allocate SCSI Host and set up association between host,
> > @@ -646,9 +650,17 @@ static int fnic_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
> >     fnic_stats_debugfs_init(fnic);
> >
> >     /* Find model name from PCIe subsys ID */
> > -   if (fnic_get_desc_by_devid(pdev, &desc, &subsys_desc) == 0)
> > +   if (fnic_get_desc_by_devid(pdev, &desc, &subsys_desc) == 0) {
> >             dev_info(&fnic->pdev->dev, "Model: %s\n", subsys_desc);
> > -   else {
> > +
> > +           /* Update FDMI model */
>
> This comment adds no information.  Delete it.

Thanks Dan. We will address this in a future patch.

> > +           fnic->subsys_desc_len = strlen(subsys_desc);
>
> Keep in mind that strlen() does not count the NUL-terminator.
>
> > +           len = ARRAY_SIZE(fnic->subsys_desc);
>
> Use sizeof() when you are talking about bytes or chars.  For snprintf() and
> other string functions, it's always sizeof() and never ARRAY_SIZE().
>
> > +           if (fnic->subsys_desc_len > len)
> > +                   fnic->subsys_desc_len = len;
> > +           memcpy(fnic->subsys_desc, subsys_desc, fnic->subsys_desc_len);
>
> So this is an 0-14 character buffer.  If fnic->subsys_desc_len is set to 14,
> then the string is not NUL terminated.  This is how the buffer is used in
> fdls_fdmi_register_hba()
>
> strscpy_pad(data, fnic->subsys_desc, FNIC_FDMI_MODEL_LEN);
> data[FNIC_FDMI_MODEL_LEN - 1] = 0;
>
> This suggests that fnic->subsys_desc is expected to be NUL-terminated.
> However FNIC_FDMI_MODEL_LEN is 12.  So in that case the last 3 characters
> are removed.  LOL.  It's harmless but so very annoying.
>
> Also strscpy_pad() will ensure that data[FNIC_FDMI_MODEL_LEN - 1] is set
> to zero so that line could be deleted.

Thanks Dan. Cisco VIC model names do not exceed 9 characters.
I understand your point about the theoretical case though.
The data init line can be removed in a future patch.

> regards,
> dan carpenter
>

Thanks for your time and effort in reviewing this patch Dan.
Appreciate your help.

Regards,
Karan