Re: [PATCH RFC 10/13] dmaengine: sdxi: Add PCI driver support

[Jonathan Cameron <jonathan.cameron@huawei.com>]

On Fri, 05 Sep 2025 13:48:33 -0500
Nathan Lynch via B4 Relay <devnull+nathan.lynch.amd.com@kernel.org> wrote:

> From: Nathan Lynch <nathan.lynch@amd.com>
> 
> Add support for binding to PCIe-hosted SDXI devices. SDXI requires
> MSI(-X) for PCI implementations, so this code will be gated by
> CONFIG_PCI_MSI in the Makefile.
> 
> Co-developed-by: Wei Huang <wei.huang2@amd.com>
> Signed-off-by: Wei Huang <wei.huang2@amd.com>
> Signed-off-by: Nathan Lynch <nathan.lynch@amd.com>

I've tried not to overlap too much with other reviewers.
A few comments inline.

> ---
>  drivers/dma/sdxi/pci.c | 216 +++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 216 insertions(+)
> 
> diff --git a/drivers/dma/sdxi/pci.c b/drivers/dma/sdxi/pci.c
> new file mode 100644
> index 0000000000000000000000000000000000000000..b7f74555395c605c4affffb198ee359accac8521
> --- /dev/null
> +++ b/drivers/dma/sdxi/pci.c

> + * SDXI devices signal message 0 on error conditions, see "Error
> + * Logging Control and Status Registers".
> + */
> +#define ERROR_IRQ_MSG 0
> +
> +/* MMIO BARs */
> +#define MMIO_CTL_REGS_BAR		0x0
> +#define MMIO_DOORBELL_BAR		0x2

> +
> +static void sdxi_pci_unmap(struct sdxi_dev *sdxi)
> +{
> +	struct pci_dev *pdev = sdxi_to_pci_dev(sdxi);
> +
> +	pcim_iounmap(pdev, sdxi->ctrl_regs);
> +	pcim_iounmap(pdev, sdxi->dbs);
> +}
> +
> +static int sdxi_pci_init(struct sdxi_dev *sdxi)
> +{
> +	struct pci_dev *pdev = sdxi_to_pci_dev(sdxi);
> +	struct device *dev = &pdev->dev;
> +	int dma_bits = 64;
> +	int ret;
> +
> +	ret = pcim_enable_device(pdev);
> +	if (ret) {
> +		sdxi_err(sdxi, "pcim_enbale_device failed\n");

enable.

> +		return ret;

For probe stuff I'd suggest not using the sdxi wrapper and instead
using return dev_err_probe(); I guess you could define and sdxi_err_probe()
if you want to.   

> +	}
> +
> +	pci_set_master(pdev);
> +	ret = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(dma_bits));
> +	if (ret) {
> +		sdxi_err(sdxi, "failed to set DMA mask & coherent bits\n");
> +		return ret;
> +	}
> +
> +	ret = sdxi_pci_map(sdxi);
> +	if (ret) {
> +		sdxi_err(sdxi, "failed to map device IO resources\n");
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static void sdxi_pci_exit(struct sdxi_dev *sdxi)
> +{
> +	sdxi_pci_unmap(sdxi);
> +}
> +
> +static struct sdxi_dev *sdxi_device_alloc(struct device *dev)
> +{
> +	struct sdxi_dev *sdxi;
> +
> +	sdxi = kzalloc(sizeof(*sdxi), GFP_KERNEL);
	sdxi = devm_kzalloc(dev, sizeof(*sdxi), GFP_KERNEL);
seems like it would be sufficient here

> +	if (!sdxi)
> +		return NULL;
> +
> +	sdxi->dev = dev;
> +
> +	mutex_init(&sdxi->cxt_lock);
For new code, nice to use
	rc = devm_mutex_init(&sdxi->ctx_lock);
	if (rc)
		return ERR_PTR(rc);

Benefit for lock debugging is small, but it's also not particularly
bad wrt to code complexity here.  Up to you.

> +
> +	return sdxi;
> +}
> +
> +static void sdxi_device_free(struct sdxi_dev *sdxi)
> +{
> +	kfree(sdxi);
> +}
If this doesn't get more complex later I'd just take the view
it's obvious that kfree(sdxi) is undoing something done in sdxi_device_alloc()
and just call that inline.  No need for the trivial wrapper.

Or just use devm_kzalloc() and let the automatic stuff clean it up
for you on error or remove.


> +static const struct pci_device_id sdxi_id_table[] = {
> +	{ PCI_DEVICE_CLASS(PCI_CLASS_ACCELERATOR_SDXI, 0xffffff) },
> +	{0, }
	{ }

is fine

> +};
> +MODULE_DEVICE_TABLE(pci, sdxi_id_table);

>