Le 29/09/2023 à 19:24, Lizhi Hou a écrit :
> From: Nishad Saraf <[email protected]>
>
> Adds driver to enable PCIe board which uses AMD QDMA (the Queue-based
> Direct Memory Access) subsystem. For example, Xilinx Alveo V70 AI
> Accelerator devices.
> https://www.xilinx.com/applications/data-center/v70.html
>
> The QDMA subsystem is used in conjunction with the PCI Express IP block
> to provide high performance data transfer between host memory and the
> card's DMA subsystem.
>
...
> +static int amd_qdma_probe(struct platform_device *pdev)
> +{
> + struct qdma_platdata *pdata = dev_get_platdata(&pdev->dev);
> + struct qdma_device *qdev;
> + struct resource *res;
> + void __iomem *regs;
> + int ret;
> +
> + qdev = devm_kzalloc(&pdev->dev, sizeof(*qdev), GFP_KERNEL);
> + if (!qdev)
> + return -ENOMEM;
> +
> + platform_set_drvdata(pdev, qdev);
> + qdev->pdev = pdev;
> + mutex_init(&qdev->ctxt_lock);
If this was done later, it could simplify some error handling below.
> +
> + res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
> + if (!res) {
> + qdma_err(qdev, "Failed to get IRQ resource");
> + ret = -ENODEV;
> + goto failed;
> + }
> + qdev->err_irq_idx = pdata->irq_index;
> + qdev->queue_irq_start = res->start + 1;
> + qdev->queue_irq_num = res->end - res->start;
> +
> + regs = devm_platform_get_and_ioremap_resource(pdev, 0, NULL);
> + if (IS_ERR(regs)) {
> + ret = PTR_ERR(regs);
> + qdma_err(qdev, "Failed to map IO resource, err %d", ret);
> + goto failed;
> + }
> +
> + qdev->regmap = devm_regmap_init_mmio(&pdev->dev, regs,
> + &qdma_regmap_config);
> + if (IS_ERR(qdev->regmap)) {
> + ret = PTR_ERR(qdev->regmap);
> + qdma_err(qdev, "Regmap init failed, err %d", ret);
> + goto failed;
> + }
> +
> + ret = qdma_device_verify(qdev);
> + if (ret)
> + goto failed;
> +
> + ret = qdma_get_hw_info(qdev);
> + if (ret)
> + goto failed;
> +
> + INIT_LIST_HEAD(&qdev->dma_dev.channels);
> +
> + ret = qdma_device_setup(qdev);
> + if (ret)
> + goto failed;
> +
> + ret = qdma_intr_init(qdev);
> + if (ret) {
> + qdma_err(qdev, "Failed to initialize IRQs %d", ret);
> + return ret;
Should it be "goto failed"?
> + }
> + qdev->status |= QDMA_DEV_STATUS_INTR_INIT;
> +
> + dma_cap_set(DMA_SLAVE, qdev->dma_dev.cap_mask);
> + dma_cap_set(DMA_PRIVATE, qdev->dma_dev.cap_mask);
...
> +struct qdma_device {
> + struct platform_device *pdev;
> + struct dma_device dma_dev;
> + struct regmap *regmap;
> + struct mutex ctxt_lock; /* protect ctxt registers */
> + const struct qdma_reg_field *rfields;
> + const struct qdma_reg *roffs;
> + struct qdma_queue *h2c_queues;
> + struct qdma_queue *c2h_queues;
> + struct qdma_intr_ring *qintr_rings;
> + u32 qintr_ring_num;
> + u32 qintr_ring_idx;
> + u32 chan_num;
> + u32 queue_irq_start;
> + u32 queue_irq_num;
> + u32 err_irq_idx;
> + u32 fid;
> + u32 status;
Using such a mechanism with this 'status' in the probe and
amd_qdma_remove(), apparently only to simplify the error handling of the
probe looks really unusual to me.
CJ
> +};
...
On 9/29/23 13:35, Christophe JAILLET wrote:
> Le 29/09/2023 à 19:24, Lizhi Hou a écrit :
>> From: Nishad Saraf <[email protected]>
>>
>> Adds driver to enable PCIe board which uses AMD QDMA (the Queue-based
>> Direct Memory Access) subsystem. For example, Xilinx Alveo V70 AI
>> Accelerator devices.
>> https://www.xilinx.com/applications/data-center/v70.html
>>
>> The QDMA subsystem is used in conjunction with the PCI Express IP block
>> to provide high performance data transfer between host memory and the
>> card's DMA subsystem.
>>
>
> ...
>
>> +static int amd_qdma_probe(struct platform_device *pdev)
>> +{
>> + struct qdma_platdata *pdata = dev_get_platdata(&pdev->dev);
>> + struct qdma_device *qdev;
>> + struct resource *res;
>> + void __iomem *regs;
>> + int ret;
>> +
>> + qdev = devm_kzalloc(&pdev->dev, sizeof(*qdev), GFP_KERNEL);
>> + if (!qdev)
>> + return -ENOMEM;
>> +
>> + platform_set_drvdata(pdev, qdev);
>> + qdev->pdev = pdev;
>> + mutex_init(&qdev->ctxt_lock);
>
> If this was done later, it could simplify some error handling below.
This lock is used for sending indirect commands. And the probe function
needs to send indirect commands as well. Thus, the lock init can not be
moved to the end of the function to simplify error handling.
>
>> +
>> + res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
>> + if (!res) {
>> + qdma_err(qdev, "Failed to get IRQ resource");
>> + ret = -ENODEV;
>> + goto failed;
>> + }
>> + qdev->err_irq_idx = pdata->irq_index;
>> + qdev->queue_irq_start = res->start + 1;
>> + qdev->queue_irq_num = res->end - res->start;
>> +
>> + regs = devm_platform_get_and_ioremap_resource(pdev, 0, NULL);
>> + if (IS_ERR(regs)) {
>> + ret = PTR_ERR(regs);
>> + qdma_err(qdev, "Failed to map IO resource, err %d", ret);
>> + goto failed;
>> + }
>> +
>> + qdev->regmap = devm_regmap_init_mmio(&pdev->dev, regs,
>> + &qdma_regmap_config);
>> + if (IS_ERR(qdev->regmap)) {
>> + ret = PTR_ERR(qdev->regmap);
>> + qdma_err(qdev, "Regmap init failed, err %d", ret);
>> + goto failed;
>> + }
>> +
>> + ret = qdma_device_verify(qdev);
>> + if (ret)
>> + goto failed;
>> +
>> + ret = qdma_get_hw_info(qdev);
>> + if (ret)
>> + goto failed;
>> +
>> + INIT_LIST_HEAD(&qdev->dma_dev.channels);
>> +
>> + ret = qdma_device_setup(qdev);
>> + if (ret)
>> + goto failed;
>> +
>> + ret = qdma_intr_init(qdev);
>> + if (ret) {
>> + qdma_err(qdev, "Failed to initialize IRQs %d", ret);
>> + return ret;
>
> Should it be "goto failed"?
Yes. Will fix it.
>
>> + }
>> + qdev->status |= QDMA_DEV_STATUS_INTR_INIT;
>> +
>> + dma_cap_set(DMA_SLAVE, qdev->dma_dev.cap_mask);
>> + dma_cap_set(DMA_PRIVATE, qdev->dma_dev.cap_mask);
>
> ...
>
>> +struct qdma_device {
>> + struct platform_device *pdev;
>> + struct dma_device dma_dev;
>> + struct regmap *regmap;
>> + struct mutex ctxt_lock; /* protect ctxt registers */
>> + const struct qdma_reg_field *rfields;
>> + const struct qdma_reg *roffs;
>> + struct qdma_queue *h2c_queues;
>> + struct qdma_queue *c2h_queues;
>> + struct qdma_intr_ring *qintr_rings;
>> + u32 qintr_ring_num;
>> + u32 qintr_ring_idx;
>> + u32 chan_num;
>> + u32 queue_irq_start;
>> + u32 queue_irq_num;
>> + u32 err_irq_idx;
>> + u32 fid;
>> + u32 status;
>
> Using such a mechanism with this 'status' in the probe and
> amd_qdma_remove(), apparently only to simplify the error handling of
> the probe looks really unusual to me.
Ok. I will remove 'status' and add more 'goto' labels to do error handling.
Thanks,
Lizhi
>
> CJ
>
>> +};
>
> ...
>