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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <299a9a54-89c4-75d6-67d4-3aebfc6fd414@amd.com>
Date:   Wed, 4 Oct 2023 09:03:03 -0700
From:   Lizhi Hou <lizhi.hou@....com>
To:     Christophe JAILLET <christophe.jaillet@...adoo.fr>,
        <vkoul@...nel.org>, <dmaengine@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>
CC:     Nishad Saraf <nishads@....com>, <nishad.saraf@....com>,
        <sonal.santan@....com>, <max.zhen@....com>
Subject: Re: [PATCH V6 1/1] dmaengine: amd: qdma: Add AMD QDMA driver


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