Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.34 as permitted sender) client-ip=23.128.96.34;
Received-SPF: Pass (protection.outlook.com: domain of amd.com designates
 165.204.84.17 as permitted sender) receiver=protection.outlook.com;
 client-ip=165.204.84.17; helo=SATLEXMB03.amd.com; pr=C
Message-ID: <299a9a54-89c4-75d6-67d4-3aebfc6fd414@amd.com>
Date:   Wed, 4 Oct 2023 09:03:03 -0700
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.11.0
Subject: Re: [PATCH V6 1/1] dmaengine: amd: qdma: Add AMD QDMA driver
Content-Language: en-US
To:     Christophe JAILLET <christophe.jaillet@wanadoo.fr>,
        <vkoul@kernel.org>, <dmaengine@vger.kernel.org>,
        <linux-kernel@vger.kernel.org>
CC:     Nishad Saraf <nishads@amd.com>, <nishad.saraf@amd.com>,
        <sonal.santan@amd.com>, <max.zhen@amd.com>
References: <1696008263-42937-1-git-send-email-lizhi.hou@amd.com>
 <1696008263-42937-2-git-send-email-lizhi.hou@amd.com>
 <0750d8b2-b624-3293-23ac-a034c88d8e0e@wanadoo.fr>
From:   Lizhi Hou <lizhi.hou@amd.com>
In-Reply-To: <0750d8b2-b624-3293-23ac-a034c88d8e0e@wanadoo.fr>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 8bit
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 04 Oct 2023 16:03:10.1551
 (UTC)
X-MS-Exchange-CrossTenant-Network-Message-Id: bf2614cc-3874-4831-a42e-08dbc4f367df
X-MS-Exchange-CrossTenant-Id: 3dd8961f-e488-4e60-8e11-a82d994e183d
X-MS-Exchange-CrossTenant-OriginalAttributedTenantConnectingIp: TenantId=3dd8961f-e488-4e60-8e11-a82d994e183d;Ip=[165.204.84.17];Helo=[SATLEXMB03.amd.com]
X-MS-Exchange-CrossTenant-AuthSource: MN1PEPF0000ECD6.namprd02.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Anonymous
X-MS-Exchange-CrossTenant-FromEntityHeader: HybridOnPrem
X-MS-Exchange-Transport-CrossTenantHeadersStamped: IA1PR12MB7493
X-Spam-Status: No, score=-3.1 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FORGED_SPF_HELO,
        NICE_REPLY_A,RCVD_IN_DNSWL_BLOCKED,RCVD_IN_MSPIKE_H2,SPF_HELO_PASS,
        SPF_NONE,URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (howler.vger.email [0.0.0.0]); Wed, 04 Oct 2023 09:03:19 -0700 (PDT)


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