Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.35 as permitted sender) client-ip=23.128.96.35;
Message-ID: <0750d8b2-b624-3293-23ac-a034c88d8e0e@wanadoo.fr>
Date:   Fri, 29 Sep 2023 22:35:27 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.15.1
Subject: Re: [PATCH V6 1/1] dmaengine: amd: qdma: Add AMD QDMA driver
To:     Lizhi Hou <lizhi.hou@amd.com>, 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>
Content-Language: fr
From:   Christophe JAILLET <christophe.jaillet@wanadoo.fr>
In-Reply-To: <1696008263-42937-2-git-send-email-lizhi.hou@amd.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk

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.

> +
> +	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

> +};

...