| 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
| ||
|
Message-ID: <6ba2221c-bbc9-a33c-7e62-85c2d87ceeed@linux.intel.com>
Date: Tue, 4 Oct 2022 11:18:54 +0300 (EEST)
From: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
To: Lizhi Hou <lizhi.hou@....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 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>
> ---
> +/* bits of the channel control register */
> +#define CHAN_CTRL_RUN_STOP BIT(0)
> +#define CHAN_CTRL_IE_DESC_STOPPED BIT(1)
> +#define CHAN_CTRL_IE_DESC_COMPLETED BIT(2)
> +#define CHAN_CTRL_IE_DESC_ALIGN_MISMATCH BIT(3)
> +#define CHAN_CTRL_IE_MAGIC_STOPPED BIT(4)
> +#define CHAN_CTRL_IE_IDLE_STOPPED BIT(6)
> +#define CHAN_CTRL_IE_READ_ERROR (0x1FUL << 9)
> +#define CHAN_CTRL_IE_DESC_ERROR (0x1FUL << 19)
Looks GENMASK()
> +/* bits of the SG DMA control register */
> +#define XDMA_CTRL_RUN_STOP BIT(0)
> +#define XDMA_CTRL_IE_DESC_STOPPED BIT(1)
> +#define XDMA_CTRL_IE_DESC_COMPLETED BIT(2)
> +#define XDMA_CTRL_IE_DESC_ALIGN_MISMATCH BIT(3)
> +#define XDMA_CTRL_IE_MAGIC_STOPPED BIT(4)
> +#define XDMA_CTRL_IE_IDLE_STOPPED BIT(6)
> +#define XDMA_CTRL_IE_READ_ERROR (0x1FUL << 9)
> +#define XDMA_CTRL_IE_DESC_ERROR (0x1FUL << 19)
Ditto.
> +/**
> + * xdma_config_channels - Detect and config DMA channels
> + * @xdev: DMA device pointer
> + * @dir: Channel direction
> + */
> +static int xdma_config_channels(struct xdma_device *xdev,
> + enum dma_transfer_direction dir)
> +{
> + struct xdma_platdata *pdata = dev_get_platdata(&xdev->pdev->dev);
> + u32 base, identifier, target;
> + struct xdma_chan **chans;
> + u32 *chan_num;
> + int i, j, ret;
> +
> + if (dir == DMA_MEM_TO_DEV) {
> + base = XDMA_CHAN_H2C_OFFSET;
> + target = XDMA_CHAN_H2C_TARGET;
> + chans = &xdev->h2c_chans;
> + chan_num = &xdev->h2c_chan_num;
> + } else if (dir == DMA_DEV_TO_MEM) {
> + base = XDMA_CHAN_C2H_OFFSET;
> + target = XDMA_CHAN_C2H_TARGET;
> + chans = &xdev->c2h_chans;
> + chan_num = &xdev->c2h_chan_num;
> + } else {
> + xdma_err(xdev, "invalid direction specified");
> + return -EINVAL;
> + }
> +
> + /* detect number of available DMA channels */
> + for (i = 0, *chan_num = 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;
> + }
> +
> + /* check if it is available DMA channel */
> + if (XDMA_CHAN_CHECK_TARGET(identifier, target))
> + (*chan_num)++;
> + }
> +
> + if (!*chan_num) {
> + xdma_err(xdev, "does not probe any channel");
> + return -EINVAL;
> + }
> +
> + *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?
> +
> + if (!XDMA_CHAN_CHECK_TARGET(identifier, target))
> + continue;
> +
> + 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;
> +}
> +static int xdma_irq_init(struct xdma_device *xdev)
> +{
> + u32 irq = xdev->irq_start;
> + int i, j, ret;
> +
> + /* return failure if there are not enough IRQs */
> + if (xdev->irq_num < xdev->h2c_chan_num + xdev->c2h_chan_num) {
> + xdma_err(xdev, "not enough irq");
> + return -EINVAL;
> + }
> +
> + /* setup H2C interrupt handler */
> + for (i = 0; i < xdev->h2c_chan_num; i++) {
> + ret = request_irq(irq, xdma_channel_isr, 0,
> + "xdma-h2c-channel", &xdev->h2c_chans[i]);
> + if (ret) {
> + xdma_err(xdev, "H2C channel%d request irq%d failed: %d",
> + i, irq, ret);
> + for (j = 0; j < i; j++) {
> + free_irq(xdev->h2c_chans[j].irq,
> + &xdev->h2c_chans[j]);
> + }
> + return ret;
Remove freeing for loop and just do
goto failed_init_c2h;
And reverse the iteration order down on the error path (from i to zero) by
using
while (i--)
> + }
> + xdev->h2c_chans[i].irq = irq;
> + irq++;
> + }
> +
> + /* setup C2H interrupt handler */
> + for (i = 0; i < xdev->c2h_chan_num; i++) {
> + ret = request_irq(irq, xdma_channel_isr, 0,
> + "xdma-c2h-channel", &xdev->c2h_chans[i]);
> + if (ret) {
> + xdma_err(xdev, "H2C channel%d request irq%d failed: %d",
> + i, irq, ret);
> + for (j = 0; j < i; j++) {
> + free_irq(xdev->c2h_chans[j].irq,
> + &xdev->c2h_chans[j]);
> + }
> + goto failed_init_c2h;
Ditto. But use j for this whole for loop so you can just do while (j--) on
the error path as the index variable won't clash with the previous loop.
> + }
> + xdev->c2h_chans[i].irq = irq;
> + irq++;
> + }
> +
> + /* config hardware IRQ registers */
> + ret = xdma_set_vector_reg(xdev, XDMA_IRQ_CHAN_VEC_NUM, 0,
> + xdev->h2c_chan_num + xdev->c2h_chan_num);
> + if (ret) {
> + xdma_err(xdev, "failed to set channel vectors: %d", ret);
> + goto failed;
> + }
> +
> + /* enable interrupt */
> + ret = xdma_enable_intr(xdev);
> + if (ret) {
> + xdma_err(xdev, "failed to enable interrupts: %d", ret);
> + goto failed;
> + }
> +
> + return 0;
> +
> +failed:
> + for (i = 0; i < xdev->c2h_chan_num; i++)
As mentioned above:
while (j--)
> + free_irq(xdev->c2h_chans[i].irq, &xdev->c2h_chans[i]);
> +failed_init_c2h:
> + for (i = 0; i < xdev->h2c_chan_num; i++)
while (i--)
> + free_irq(xdev->h2c_chans[i].irq, &xdev->h2c_chans[i]);
> +
> + return ret;
> +}
> +
> +static bool xdma_filter_fn(struct dma_chan *chan, void *param)
> +{
> + struct xdma_chan *xdma_chan = to_xdma_chan(chan);
> + struct xdma_chan_info *chan_info = param;
> +
> + if (chan_info->dir != xdma_chan->dir)
> + return false;
> +
> + return true;
return chan_info->dir == xdma_chan->dir;
--
i.
Powered by blists - more mailing lists