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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <a0ab26b3-be2d-c1d6-e8ea-3ab9ad57962b@amd.com>
Date:   Tue, 4 Oct 2022 10:38:07 -0700
From:   Lizhi Hou <lizhi.hou@....com>
To:     Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
CC:     <vkoul@...nel.org>, <dmaengine@...r.kernel.org>,
        LKML <linux-kernel@...r.kernel.org>, <trix@...hat.com>,
        <tumic@...see.org>, <max.zhen@....com>, <sonal.santan@....com>,
        <larry.liu@....com>, <brian.xu@....com>
Subject: Re: [PATCH V5 XDMA 1/2] dmaengine: xilinx: xdma: Add xilinx xdma
 driver


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@....com>
>>>> Signed-off-by: Sonal Santan <sonal.santan@....com>
>>>> Signed-off-by: Max Zhen <max.zhen@....com>
>>>> Signed-off-by: Brian Xu <brian.xu@....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.

Thanks,

Lizhi

>>>> +
>>>> +		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;
>>>> +}
>