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

[<Kelvin.Cao@microchip.com>]

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@microchip.com>
> > Co-developed-by: George Ge <george.ge@microchip.com>
> > Signed-off-by: George Ge <george.ge@microchip.com>
> 
> 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