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] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 29 Sep 2023 22:35:27 +0200
From:   Christophe JAILLET <christophe.jaillet@...adoo.fr>
To:     Lizhi Hou <lizhi.hou@....com>, 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

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.

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

> +};

...