Re: [PATCH V5 XDMA 1/2] dmaengine: xilinx: xdma: Add xilinx xdma driver

[Ilpo Järvinen]

On Tue, 4 Oct 2022, Lizhi Hou wrote:

> 
> On 10/4/22 01:18, Ilpo Järvinen wrote:
> > On Wed, 28 Sep 2022, Lizhi Hou wrote:
> > 
> > > Add driver to enable PCIe board which uses XDMA (the DMA/Bridge Subsystem
> > > for PCI Express). For example, Xilinx Alveo PCIe devices.
> > >      https://www.xilinx.com/products/boards-and-kits/alveo.html
> > > 
> > > The XDMA engine support up to 4 Host to Card (H2C) and 4 Card to Host
> > > (C2H)
> > > channels. Memory transfers are specified on a per-channel basis in
> > > descriptor linked lists, which the DMA fetches from host memory and
> > > processes. Events such as descriptor completion and errors are signaled
> > > using interrupts. The hardware detail is provided by
> > >      https://docs.xilinx.com/r/en-US/pg195-pcie-dma/Introduction
> > > 
> > > This driver implements dmaengine APIs.
> > >      - probe the available DMA channels
> > >      - use dma_slave_map for channel lookup
> > >      - use virtual channel to manage dmaengine tx descriptors
> > >      - implement device_prep_slave_sg callback to handle host scatter
> > > gather
> > >        list
> > >      - implement device_config to config device address for DMA transfer
> > > 
> > > Signed-off-by: Lizhi Hou <lizhi.hou@xxxxxxx>
> > > Signed-off-by: Sonal Santan <sonal.santan@xxxxxxx>
> > > Signed-off-by: Max Zhen <max.zhen@xxxxxxx>
> > > Signed-off-by: Brian Xu <brian.xu@xxxxxxx>
> > > ---

> > > +	*chans = devm_kzalloc(&xdev->pdev->dev, sizeof(**chans) * (*chan_num),
> > > +			      GFP_KERNEL);
> > > +	if (!*chans)
> > > +		return -ENOMEM;
> > > +
> > > +	for (i = 0, j = 0; i < pdata->max_dma_channels; i++) {
> > > +		ret = xdma_read_reg(xdev, base + i * XDMA_CHAN_STRIDE,
> > > +				    XDMA_CHAN_IDENTIFIER, &identifier);
> > > +		if (ret) {
> > > +			xdma_err(xdev, "failed to read channel id: %d", ret);
> > > +			return ret;
> > > +		}
> > Is it ok to not rollback the allocation in case of an error occurs?
> 
> In this loop, the failures are returned by read/write registers. The
> read/write register failure indicates serious hardware issue and the hardware
> may not be rollback in this situation.

What I meant is that you allocated memory above (to *chans, see above). 
Shouldn't that memory be free in case the hw is not working before you 
return the error from this function?

Check also the other returns below for the same problemx.

> > > +
> > > +		if (j == *chan_num) {
> > > +			xdma_err(xdev, "invalid channel number");
> > > +			return -EIO;
> > > +		}
> > Same here?
> > 
> > > +
> > > +		/* init channel structure and hardware */
> > > +		(*chans)[j].xdev_hdl = xdev;
> > > +		(*chans)[j].base = base + i * XDMA_CHAN_STRIDE;
> > > +		(*chans)[j].dir = dir;
> > > +
> > > +		ret = xdma_channel_init(&(*chans)[j]);
> > > +		if (ret)
> > > +			return ret;
> > And here.
> > 
> > > +		(*chans)[j].vchan.desc_free = xdma_free_desc;
> > > +		vchan_init(&(*chans)[j].vchan, &xdev->dma_dev);
> > > +
> > > +		j++;
> > > +	}
> > > +
> > > +	dev_info(&xdev->pdev->dev, "configured %d %s channels", j,
> > > +		 (dir == DMA_MEM_TO_DEV) ? "H2C" : "C2H");
> > > +
> > > +	return 0;
> > > +}


-- 
 i.