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

[<Kelvin.Cao@microchip.com>]

On Mon, 2023-04-03 at 22:26 +0200, Christophe JAILLET wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you
> know the content is safe
> 
> Le 03/04/2023 à 20:06, Kelvin Cao a écrit :
> > 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>
> > ---
> >   MAINTAINERS                 |    5 +
> >   drivers/dma/Kconfig         |    9 +
> >   drivers/dma/Makefile        |    1 +
> >   drivers/dma/switchtec_dma.c | 1734
> > +++++++++++++++++++++++++++++++++++
> >   4 files changed, 1749 insertions(+)
> >   create mode 100644 drivers/dma/switchtec_dma.c
> > 
> 
> Hi,
> just a few nit, should you find them useful.
> 
> [...]
> 
> > +static void switchtec_dma_process_desc(struct switchtec_dma_chan
> > *swdma_chan)
> > +{
> > +     struct device *chan_dev = to_chan_dev(swdma_chan);
> > +     struct dmaengine_result res;
> > +     struct switchtec_dma_desc *desc;
> > +     struct switchtec_dma_hw_ce *ce;
> > +     __le16 phase_tag;
> > +     int tail;
> > +     int cid;
> > +     int se_idx;
> > +     u32 sts_code;
> > +     int i = 0;
> 
> No need to init.
> 
> [...]
> 
> > +static int switchtec_dma_alloc_desc(struct switchtec_dma_chan
> > *swdma_chan)
> > +{
> > +     struct switchtec_dma_dev *swdma_dev = swdma_chan->swdma_dev;
> > +     struct pci_dev *pdev;
> > +     struct chan_fw_regs __iomem *chan_fw = swdma_chan-
> > >mmio_chan_fw;
> > +     size_t size;
> > +     struct switchtec_dma_desc *desc;
> > +     int rc;
> > +     int i;
> > +
> > +     swdma_chan->head = swdma_chan->tail = 0;
> > +     swdma_chan->cq_tail = 0;
> > +
> > +     size = SWITCHTEC_DMA_SQ_SIZE * sizeof(*swdma_chan->hw_sq);
> > +     swdma_chan->hw_sq = dma_alloc_coherent(swdma_dev-
> > >dma_dev.dev, size,
> > +                                            &swdma_chan-
> > >dma_addr_sq,
> > +                                            GFP_KERNEL);
> > +     if (!swdma_chan->hw_sq) {
> > +             rc = -ENOMEM;
> > +             goto free_and_exit;
> > +     }
> > +
> > +     size = SWITCHTEC_DMA_CQ_SIZE * sizeof(*swdma_chan->hw_cq);
> > +     swdma_chan->hw_cq = dma_alloc_coherent(swdma_dev-
> > >dma_dev.dev, size,
> > +                                            &swdma_chan-
> > >dma_addr_cq,
> > +                                            GFP_KERNEL);
> > +     if (!swdma_chan->hw_cq) {
> > +             rc = -ENOMEM;
> > +             goto free_and_exit;
> > +     }
> > +
> > +     memset(swdma_chan->hw_cq, 0, size);
> 
> The memory allocated with dma_alloc_coherent() is already zeroed.
> 
> > +
> > +     /* reset host phase tag */
> > +     swdma_chan->phase_tag = 0;
> > +
> > +     size = sizeof(*swdma_chan->desc_ring);
> > +     swdma_chan->desc_ring = kcalloc(SWITCHTEC_DMA_RING_SIZE,
> > size,
> > +                                     GFP_KERNEL);
> > +     if (!swdma_chan->desc_ring) {
> > +             rc = -ENOMEM;
> > +             goto free_and_exit;
> > +     }
> > +
> > +     memset(swdma_chan->desc_ring, 0, SWITCHTEC_DMA_RING_SIZE *
> > size);
> 
> This is already kcalloc()'ed.
> 
> CJ

Hi CJ,

Thanks for the review, will fix them in the next version.

Kelvin