Re: [PATCH 1/1] dmaengine: switchtec-dma: Introduce Switchtec DMA engine PCI driver

[Logan Gunthorpe]

Hi Kelvin,

On 2023-03-27 19:11, Kelvin Cao wrote:
> Some Switchtec Switches can expose DMA engines via extra PCI functions
> on the upstream ports. At most one such function can be supported on
> each upstream port. Each function can have one or more DMA channels.
> 
> Implement core PCI driver skeleton and register DMA engine callbacks.
> 
> Signed-off-by: Kelvin Cao <kelvin.cao@xxxxxxxxxxxxx>
> Co-developed-by: George Ge <george.ge@xxxxxxxxxxxxx>
> Signed-off-by: George Ge <george.ge@xxxxxxxxxxxxx>

Looks largely good. I did a quick review on some of the code and have a
few questions and nits.

> +static void switchtec_dma_release(struct dma_device *dma_dev)
> +{
> +	int i;
> +	struct switchtec_dma_dev *swdma_dev =
> +		container_of(dma_dev, struct switchtec_dma_dev, dma_dev);
> +
> +	for (i = 0; i < swdma_dev->chan_cnt; i++)
> +		kfree(swdma_dev->swdma_chans[i]);
> +
> +	kfree(swdma_dev->swdma_chans);
> +	kfree(swdma_dev);
> +
> +	put_device(dma_dev->dev);

dma_dev is part of swdma_dev, but it was just kfree'd so this doesn't
look valid.

> +}
> +
> +static int switchtec_dma_create(struct pci_dev *pdev)
> +{
> +	struct switchtec_dma_dev *swdma_dev;
> +	struct dma_device *dma;
> +	struct dma_chan *chan;
> +	struct device *dev = &pdev->dev;
> +	void __iomem *bar;
> +	int chan_cnt;
> +	int nr_vecs;
> +	int irq;
> +	int rc;
> +	int i;
> +
> +	/*
> +	 * Create the switchtec dma device
> +	 */
> +	swdma_dev = kzalloc(sizeof(*swdma_dev), GFP_KERNEL);
> +	if (!swdma_dev)
> +		return -ENOMEM;
> +
> +	bar = pcim_iomap_table(pdev)[0];
> +	swdma_dev->bar = bar;
> +
> +	swdma_dev->mmio_dmac_ver = bar + SWITCHTEC_DMAC_VERSION_OFFSET;
> +	swdma_dev->mmio_dmac_cap = bar + SWITCHTEC_DMAC_CAPABILITY_OFFSET;
> +	swdma_dev->mmio_dmac_status = bar + SWITCHTEC_DMAC_STATUS_OFFSET;
> +	swdma_dev->mmio_dmac_ctrl = bar + SWITCHTEC_DMAC_CONTROL_OFFSET;
> +	swdma_dev->mmio_chan_hw_all = bar + SWITCHTEC_DMAC_CHAN_CTRL_OFFSET;
> +	swdma_dev->mmio_chan_fw_all = bar + SWITCHTEC_DMAC_CHAN_CFG_STS_OFFSET;
> +
> +	dev_info(dev, "Switchtec PSX/PFX DMA EP\n");

Other prints in this function use the pci_version and pdev directly. As
best as I can see 'dev' is unused except for these prints so could be
removed and converted to pci_ functions.


> +static void switchtec_dma_remove(struct pci_dev *pdev)
> +{
> +	struct switchtec_dma_dev *swdma_dev = pci_get_drvdata(pdev);
> +
> +	switchtec_dma_chans_release(swdma_dev);
> +
> +	tasklet_kill(&swdma_dev->chan_status_task);
> +
> +	rcu_assign_pointer(swdma_dev->pdev, NULL);
> +	synchronize_rcu();

switchtec_dma_remove() can be called while transactions are in progress.
I'm not seeing anything here that might abort transactions that have
been placed on the ring that might not have been finished by the
hardware. Maybe I'm not seeing it. But have you tested unbind while
active? Simply run a process that does dma transactions and then use
sysfs to unbind the pci device and ensure there are no kernel hangs,
panics or memory leaks.

> +#define SWITCHTEC_PCI_DEVICE(device_id, is_fabric) \
> +	{ \
> +		.vendor     = MICROSEMI_VENDOR_ID, \
> +		.device     = device_id, \
> +		.subvendor  = PCI_ANY_ID, \
> +		.subdevice  = PCI_ANY_ID, \
> +		.class      = PCI_CLASS_SYSTEM_OTHER << 8, \
> +		.class_mask = 0xFFFFFFFF, \
> +		.driver_data = is_fabric, \
> +	}

Doesn't look like the .driver_data is used anywhere. Seems like it can
be removed.

> +static const struct pci_device_id switchtec_dma_pci_tbl[] = {
> +	SWITCHTEC_PCI_DEVICE(0x4000, 0),  //PFX 100XG4
> +	SWITCHTEC_PCI_DEVICE(0x4084, 0),  //PFX 84XG4
> +	SWITCHTEC_PCI_DEVICE(0x4068, 0),  //PFX 68XG4
> +	SWITCHTEC_PCI_DEVICE(0x4052, 0),  //PFX 52XG4
> +	SWITCHTEC_PCI_DEVICE(0x4036, 0),  //PFX 36XG4
> +	SWITCHTEC_PCI_DEVICE(0x4028, 0),  //PFX 28XG4
> +	SWITCHTEC_PCI_DEVICE(0x4100, 0),  //PSX 100XG4
> +	SWITCHTEC_PCI_DEVICE(0x4184, 0),  //PSX 84XG4
> +	SWITCHTEC_PCI_DEVICE(0x4168, 0),  //PSX 68XG4
> +	SWITCHTEC_PCI_DEVICE(0x4152, 0),  //PSX 52XG4
> +	SWITCHTEC_PCI_DEVICE(0x4136, 0),  //PSX 36XG4
> +	SWITCHTEC_PCI_DEVICE(0x4128, 0),  //PSX 28XG4
> +	SWITCHTEC_PCI_DEVICE(0x4352, 0),  //PFXA 52XG4
> +	SWITCHTEC_PCI_DEVICE(0x4336, 0),  //PFXA 36XG4
> +	SWITCHTEC_PCI_DEVICE(0x4328, 0),  //PFXA 28XG4
> +	SWITCHTEC_PCI_DEVICE(0x4452, 0),  //PSXA 52XG4
> +	SWITCHTEC_PCI_DEVICE(0x4436, 0),  //PSXA 36XG4
> +	SWITCHTEC_PCI_DEVICE(0x4428, 0),  //PSXA 28XG4
> +	{0}
> +};
> +MODULE_DEVICE_TABLE(pci, switchtec_dma_pci_tbl);
> +
> +static struct pci_driver switchtec_dma_pci_driver = {
> +	.name           = KBUILD_MODNAME,
> +	.id_table       = switchtec_dma_pci_tbl,
> +	.probe          = switchtec_dma_probe,
> +	.remove		= switchtec_dma_remove,
> +};
> +module_pci_driver(switchtec_dma_pci_driver);


Logan