Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 5 Oct 2022 12:47:42 +0300 (EEST)
From:   =?ISO-8859-15?Q?Ilpo_J=E4rvinen?= <ilpo.jarvinen@linux.intel.com>
To:     Lizhi Hou <lizhi.hou@amd.com>
cc:     vkoul@kernel.org, dmaengine@vger.kernel.org,
        LKML <linux-kernel@vger.kernel.org>, trix@redhat.com,
        tumic@gpxsee.org, max.zhen@amd.com, sonal.santan@amd.com,
        larry.liu@amd.com, brian.xu@amd.com
Subject: Re: [PATCH V5 XDMA 1/2] dmaengine: xilinx: xdma: Add xilinx xdma
 driver
In-Reply-To: <a0ab26b3-be2d-c1d6-e8ea-3ab9ad57962b@amd.com>
Message-ID: <ad41afcb-a7b1-bfd9-56af-6f5b7875de8d@linux.intel.com>
References: <1664409507-64079-1-git-send-email-lizhi.hou@amd.com> <1664409507-64079-2-git-send-email-lizhi.hou@amd.com> <6ba2221c-bbc9-a33c-7e62-85c2d87ceeed@linux.intel.com> <56f971da-5116-58dc-2df6-53ed465c8ec4@amd.com> <c86fb070-9d46-9558-f7ee-216fc657cf1a@linux.intel.com>
 <a0ab26b3-be2d-c1d6-e8ea-3ab9ad57962b@amd.com>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="8323329-886140070-1664963268=:1580"
Precedence: bulk

  This message is in MIME format.  The first part should be readable text,
  while the remaining parts are likely unreadable without MIME-aware tools.

--8323329-886140070-1664963268=:1580
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8BIT

On Tue, 4 Oct 2022, Lizhi Hou wrote:

> 
> On 10/4/22 09:43, Ilpo Järvinen wrote:
> > 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@amd.com>
> > > > > Signed-off-by: Sonal Santan <sonal.santan@amd.com>
> > > > > Signed-off-by: Max Zhen <max.zhen@amd.com>
> > > > > Signed-off-by: Brian Xu <brian.xu@amd.com>
> > > > > ---
> > > > > +	*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.
> 
> The memory does not need to be freed immediately. And it should not be memory
> leak because devm_* is used.

Ah, sorry. I think I checked exactly that there wasn't m in it but 
clearly there is now that I recheck.


-- 
 i.

--8323329-886140070-1664963268=:1580--