Re: [PATCH V6 1/1] dmaengine: amd: qdma: Add AMD QDMA driver

[Lizhi Hou]

On 9/29/23 13:35, Christophe JAILLET wrote:
> Le 29/09/2023 à 19:24, Lizhi Hou a écrit :
> > From: Nishad Saraf <nishads@xxxxxxx>
> >
> > 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
>
> > +};
>
> ...