Re: [PATCH 1/1] dmaengine: switchtec-dma: Introduce Switchtec DMA engine PCI driver
From: Kelvin.Cao
Date: Fri Mar 31 2023 - 18:42:42 EST
On Thu, 2023-03-30 at 17:10 -0600, Logan Gunthorpe wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you
> know the content is safe
>
> 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.
I think I should move put_device(dma_dev->dev) ahead of
kfree(swdma_dev) to fix this issue.
>
> > +}
> > +
> > +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.
Will fix them.
>
>
> > +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.
In switchtec_dma_chans_release(), channels will be stopped so that no
new completions will be coming back after that point. I tested unbind
while transactions were running and no hangs, panics or memory leaks
observed.
However, some references to the channel in dev_ print functions after
dma_remove() caused some decode issues. Will fix them.
>
> > +#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.
Yes.
>
> > +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
Hi Logan,
Thanks for the review. Will send the 2nd batch soon.
Kelvin