Message-ID: <b0a07e5a-fa0b-40fb-9d85-5f316f94a226@amd.com>
Date: Sun, 26 Jan 2025 11:56:17 -0800
From: Yidong Zhang <yidong.zhang@....com>
To: Christophe JAILLET <christophe.jaillet@...adoo.fr>,
	<linux-kernel@...r.kernel.org>, <linux-fpga@...r.kernel.org>,
	<mdf@...nel.org>, <hao.wu@...el.com>, <yilun.xu@...el.com>
CC: <lizhi.hou@....com>, DMG Karthik <Karthik.DMG@....com>, Nishad Saraf
	<nishads@....com>, Prapul Krishnamurthy <prapulk@....com>, Hayden Laccabue
	<hayden.laccabue@....com>
Subject: Re: [PATCH V2 1/4] drivers/fpga/amd: Add new driver amd versal-pci



On 1/26/25 02:12, Christophe JAILLET wrote:
> Caution: This message originated from an External Source. Use proper 
> caution when opening attachments, clicking links, or responding.
> 
> 
> Le 10/12/2024 à 19:37, Yidong Zhang a écrit :
>> AMD Versal based PCIe card, including V70, is designed for AI inference
>> efficiency and is tuned for video analytics and natural language 
>> processing
>> applications.
> 
> ...
> 
>> +static void versal_pci_uuid_parse(struct versal_pci_device *vdev, 
>> uuid_t *uuid)
>> +{
>> +     char str[UUID_STRING_LEN];
>> +     u8 i, j;
>> +
>> +     /* parse uuid into a valid uuid string format */
>> +     for (i  = 0, j = 0; i < strlen(vdev->fw_id) && i < sizeof(str); 
>> i++) {
> 
> Unneeded extra space in "i  = 0"

Great catch! I will fix this.

> 
> I think that the compiler already does it on its own, but the strlen
> could be computed before the for loop.

I will change the code.

> 
>> +             str[j++] = vdev->fw_id[i];
>> +             if (j == 8 || j == 13 || j == 18 || j == 23)
>> +                     str[j++] = '-';
>> +     }
>> +
>> +     uuid_parse(str, uuid);
>> +     vdev_info(vdev, "Interface uuid %pU", uuid);
>> +}
>> +
>> +static struct fpga_device *versal_pci_fpga_init(struct 
>> versal_pci_device *vdev)
>> +{
>> +     struct device *dev = &vdev->pdev->dev;
>> +     struct fpga_manager_info info = { 0 };
> 
> Is the { 0 } needed?
> Isn't the assigment below enough?

Right. I will remove the unnecessary { 0 }.

> 
>> +     struct fpga_device *fdev;
>> +     int ret;
>> +
>> +     fdev = devm_kzalloc(dev, sizeof(*fdev), GFP_KERNEL);
>> +     if (!fdev)
>> +             return ERR_PTR(-ENOMEM);
>> +
>> +     fdev->vdev = vdev;
>> +
>> +     info = (struct fpga_manager_info) {
>> +             .name = "AMD Versal FPGA Manager",
>> +             .mops = &versal_pci_fpga_ops,
>> +             .priv = fdev,
>> +     };
>> +
>> +     fdev->mgr = fpga_mgr_register_full(dev, &info);
>> +     if (IS_ERR(fdev->mgr)) {
>> +             ret = PTR_ERR(fdev->mgr);
>> +             vdev_err(vdev, "Failed to register FPGA manager, err 
>> %d", ret);
>> +             return ERR_PTR(ret);
>> +     }
>> +
>> +     /* Place holder for rm_queue_get_fw_id(vdev->rdev) */
>> +     versal_pci_uuid_parse(vdev, &vdev->intf_uuid);
>> +
>> +     return fdev;
>> +}
> 
> ...
> 
>> +static struct firmware_device *versal_pci_fw_upload_init(struct 
>> versal_pci_device *vdev)
>> +{
>> +     struct device *dev = &vdev->pdev->dev;
>> +     struct firmware_device *fwdev;
>> +     u32 devid;
>> +
>> +     fwdev = devm_kzalloc(dev, sizeof(*fwdev), GFP_KERNEL);
>> +     if (!fwdev)
>> +             return ERR_PTR(-ENOMEM);
>> +
>> +     devid = versal_pci_devid(vdev);
>> +     fwdev->name = kasprintf(GFP_KERNEL, "%s%x", DRV_NAME, devid);
> 
> Why is fwdev managed, and not fwdev->name?
> It looks ok as-is, but using devm_kasprintf() would save a few lines of
> code.

I will change the code. Great suggestion.

> 
>> +     if (!fwdev->name)
>> +             return ERR_PTR(-ENOMEM);
>> +
>> +     fwdev->fw = firmware_upload_register(THIS_MODULE, dev, fwdev->name,
>> +                                          &versal_pci_fw_ops, fwdev);
>> +     if (IS_ERR(fwdev->fw)) {
>> +             kfree(fwdev->name);
>> +             return ERR_CAST(fwdev->fw);
>> +     }
>> +
>> +     fwdev->vdev = vdev;
>> +
>> +     return fwdev;
>> +}
> 
> ...
> 
>> +static int versal_pci_probe(struct pci_dev *pdev, const struct 
>> pci_device_id *pdev_id)
>> +{
>> +     struct versal_pci_device *vdev;
>> +     int ret;
>> +
>> +     vdev = devm_kzalloc(&pdev->dev, sizeof(*vdev), GFP_KERNEL);
>> +     if (!vdev)
>> +             return -ENOMEM;
>> +
>> +     pci_set_drvdata(pdev, vdev);
>> +     vdev->pdev = pdev;
>> +
>> +     ret = pcim_enable_device(pdev);
>> +     if (ret) {
>> +             vdev_err(vdev, "Failed to enable device %d", ret);
>> +             return ret;
>> +     }
>> +
>> +     vdev->io_regs = pcim_iomap_region(vdev->pdev, MGMT_BAR, DRV_NAME);
>> +     if (IS_ERR(vdev->io_regs)) {
>> +             vdev_err(vdev, "Failed to map RM shared memory BAR%d", 
>> MGMT_BAR);
>> +             return PTR_ERR(vdev->io_regs);
>> +     }
>> +
>> +     ret = versal_pci_device_setup(vdev);
>> +     if (ret) {
>> +             vdev_err(vdev, "Failed to setup Versal device %d", ret);
>> +             return ret;
>> +     }
>> +
>> +     vdev_dbg(vdev, "Successfully probed %s driver!", DRV_NAME);
> 
> Usually, such debug messages are not needed.
> No strong opinion about it.

I will remove this. The dbg only enable in the debug kernel, but this 
line isn't necessary. I will fix it.

> 
>> +     return 0;
>> +}
> 
> ...
> 
> CJ

Powered by blists - more mailing lists