| 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
| ||
|
Date: Tue, 4 Oct 2022 09:25:09 -0700
From: Lizhi Hou <lizhi.hou@....com>
To: Martin Tůma <tumic@...see.org>,
<vkoul@...nel.org>, <dmaengine@...r.kernel.org>,
<linux-kernel@...r.kernel.org>, <trix@...hat.com>
CC: <max.zhen@....com>, <sonal.santan@....com>, <larry.liu@....com>,
<brian.xu@....com>
Subject: Re: [PATCH V5 XDMA 2/2] dmaengine: xilinx: xdma: Add user logic
interrupt support
On 10/4/22 03:26, Martin Tůma wrote:
>
> On 03. 10. 22 17:52, Lizhi Hou wrote:
>>
>> On 10/3/22 06:59, Martin Tůma wrote:
>>> On 29. 09. 22 1:58, Lizhi Hou wrote:
>>>> The Xilinx DMA/Bridge Subsystem for PCIe (XDMA) provides up to 16 user
>>>> interrupt wires to user logic that generate interrupts to the host.
>>>> This patch adds APIs to enable/disable user logic interrupt for a
>>>> given
>>>> interrupt wire index.
>>>>
>>>> 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>
>>>> ---
>>>> MAINTAINERS | 1 +
>>>> drivers/dma/xilinx/xdma.c | 65
>>>> ++++++++++++++++++++++++++++++++++++
>>>> include/linux/dma/amd_xdma.h | 16 +++++++++
>>>> 3 files changed, 82 insertions(+)
>>>> create mode 100644 include/linux/dma/amd_xdma.h
>>>>
>>>> diff --git a/MAINTAINERS b/MAINTAINERS
>>>> index c1be0b2e378a..019d84b2b086 100644
>>>> --- a/MAINTAINERS
>>>> +++ b/MAINTAINERS
>>>> @@ -21691,6 +21691,7 @@ L: dmaengine@...r.kernel.org
>>>> S: Supported
>>>> F: drivers/dma/xilinx/xdma-regs.h
>>>> F: drivers/dma/xilinx/xdma.c
>>>> +F: include/linux/dma/amd_xdma.h
>>>> F: include/linux/platform_data/amd_xdma.h
>>>> XILINX ZYNQMP DPDMA DRIVER
>>>> diff --git a/drivers/dma/xilinx/xdma.c b/drivers/dma/xilinx/xdma.c
>>>> index 58a57e03bef5..13f627445c4a 100644
>>>> --- a/drivers/dma/xilinx/xdma.c
>>>> +++ b/drivers/dma/xilinx/xdma.c
>>>> @@ -25,6 +25,7 @@
>>>> #include <linux/dmapool.h>
>>>> #include <linux/regmap.h>
>>>> #include <linux/dmaengine.h>
>>>> +#include <linux/dma/amd_xdma.h>
>>>> #include <linux/platform_device.h>
>>>> #include <linux/platform_data/amd_xdma.h>
>>>> #include <linux/dma-mapping.h>
>>>> @@ -736,6 +737,7 @@ static int xdma_set_vector_reg(struct
>>>> xdma_device *xdev, u32 vec_tbl_start,
>>>> static int xdma_irq_init(struct xdma_device *xdev)
>>>> {
>>>> u32 irq = xdev->irq_start;
>>>> + u32 user_irq_start;
>>>> int i, j, ret;
>>>> /* return failure if there are not enough IRQs */
>>>> @@ -786,6 +788,18 @@ static int xdma_irq_init(struct xdma_device
>>>> *xdev)
>>>> goto failed;
>>>> }
>>>> + /* config user IRQ registers if needed */
>>>> + user_irq_start = xdev->h2c_chan_num + xdev->c2h_chan_num;
>>>> + if (xdev->irq_num > user_irq_start) {
>>>> + ret = xdma_set_vector_reg(xdev, XDMA_IRQ_USER_VEC_NUM,
>>>> + user_irq_start,
>>>> + xdev->irq_num - user_irq_start);
>>>> + if (ret) {
>>>> + xdma_err(xdev, "failed to set user vectors: %d", ret);
>>>> + goto failed;
>>>> + }
>>>> + }
>>>> +
>>>> /* enable interrupt */
>>>> ret = xdma_enable_intr(xdev);
>>>> if (ret) {
>>>> @@ -816,6 +830,57 @@ static bool xdma_filter_fn(struct dma_chan
>>>> *chan, void *param)
>>>> return true;
>>>> }
>>>> +/**
>>>> + * xdma_disable_user_irq - Disable user interrupt
>>>> + * @pdev: Pointer to the platform_device structure
>>>> + * @user_irq_index: User IRQ index
>>>> + */
>>>> +void xdma_disable_user_irq(struct platform_device *pdev, u32
>>>> user_irq_index)
>>>> +{
>>>> + struct xdma_device *xdev = platform_get_drvdata(pdev);
>>>> +
>>>> + if (xdev->h2c_chan_num + xdev->c2h_chan_num + user_irq_index >=
>>>> + xdev->irq_num) {
>>>> + xdma_err(xdev, "invalid user irq index");
>>>> + return;
>>>> + }
>>>> +
>>>> + xdma_write_reg(xdev, XDMA_IRQ_BASE, XDMA_IRQ_USER_INT_EN_W1C,
>>>> + (1 << user_irq_index));
>>>> +}
>>>> +EXPORT_SYMBOL(xdma_disable_user_irq);
>>>> +
>>>> +/**
>>>> + * xdma_enable_user_irq - Enable user logic interrupt
>>>> + * @pdev: Pointer to the platform_device structure
>>>> + * @user_irq_index: User logic IRQ wire index
>>>> + * @irq: Pointer to returning system IRQ number
>>>> + */
>>>> +int xdma_enable_user_irq(struct platform_device *pdev, u32
>>>> user_irq_index,
>>>> + u32 *irq)
>>>> +{
>>>> + struct xdma_device *xdev = platform_get_drvdata(pdev);
>>>> + u32 user_irq;
>>>> + int ret;
>>>> +
>>>> + user_irq = xdev->h2c_chan_num + xdev->c2h_chan_num +
>>>> user_irq_index;
>>>> + if (user_irq >= xdev->irq_num) {
>>>> + xdma_err(xdev, "invalid user irq index");
>>>> + return -EINVAL;
>>>> + }
>>>> +
>>>> + ret = xdma_write_reg(xdev, XDMA_IRQ_BASE,
>>>> XDMA_IRQ_USER_INT_EN_W1S,
>>>> + (1 << user_irq_index));
>>>> + if (ret) {
>>>> + xdma_err(xdev, "set user irq mask failed, %d", ret);
>>>> + return ret;
>>>> + }
>>>> + *irq = user_irq + xdev->irq_start;
>>>> +
>>>> + return 0;
>>>> +}
>>>> +EXPORT_SYMBOL(xdma_enable_user_irq);
>>>> +
>>>> /**
>>>> * xdma_remove - Driver remove function
>>>> * @pdev: Pointer to the platform_device structure
>>>> diff --git a/include/linux/dma/amd_xdma.h
>>>> b/include/linux/dma/amd_xdma.h
>>>> new file mode 100644
>>>> index 000000000000..91fb02ff93a7
>>>> --- /dev/null
>>>> +++ b/include/linux/dma/amd_xdma.h
>>>> @@ -0,0 +1,16 @@
>>>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
>>>> +/*
>>>> + * Copyright (C) 2022, Advanced Micro Devices, Inc.
>>>> + */
>>>> +
>>>> +#ifndef _DMAENGINE_AMD_XDMA_H
>>>> +#define _DMAENGINE_AMD_XDMA_H
>>>> +
>>>> +#include <linux/interrupt.h>
>>>> +#include <linux/platform_device.h>
>>>> +
>>>> +int xdma_enable_user_irq(struct platform_device *pdev, u32
>>>> user_irq_index,
>>>> + u32 *irq);
>>>> +void xdma_disable_user_irq(struct platform_device *pdev, u32
>>>> user_irq_index);
>>>> +
>>>> +#endif /* _DMAENGINE_AMD_XDMA_H */
>>>
>>> Hi,
>>> While rewriting our V4L2 driver to use your XDMA driver, I realized,
>>> that the API for the user interrupts is still not fully usable. If
>>> the expected outcome is that the "parent" driver using the xdma
>>> driver knows nothing about the IRQ allocation in XDMA, then the
>>> xdma_enable_user_irq() function must be split into two separate
>>> functions:
>>>
>>> int xdma_enable_user_irq(struct platform_device *pdev, u32
>>> user_irq_index)
>>>
>>> and
>>>
>>> int xdma_get_system_irq(u32 user_irq_index)
>>>
>>> that returns the system IRQ number for the given user_irq_index.
>>> Because without it you can not get the system IRQ number without
>>> enabling the irq at the same time which is usually not what you want.
>>
>> Is this because the user logic you are using does not have
>> disable/enable interrupt by itself? I thought that the user logic IP
>> would have its own register to enable/disable interrupt.
>>
> Yes. Our HW has no "own" registers to disable the IRQs. As such
> registers are already there in XDMA, there was no need for them.
>
>
>> And xdma_enable_user_irq() will only be called once for creating the
>> user logic platform device. Then, user logic IP driver would use its
>> own register to control interrupts. The existing platform driver does
>> not call any xdma_* to control interrupt.
>>
> As you can see, there is HW that was designed to use the XDMA
> registers during normal operation and separate enable/disable and irq
> mapping functions are required. Maybe it is not the best HW design,
> but it can evidently happen and then the driver author is screwed with
> the merged API.
Ok. To support this case, I will separate the API as you mentioned.
Thanks,
Lizhi
>
> M.
>
>>
>> Thanks,
>>
>> Lizhi
>>
>>>
>>> As a workaround, you can compute the system IRQ yourself with the
>>> knowledge of the XDMA internals (this is what I have to do at the
>>> moment in our driver), but then the "u32 *irq" parameter becomes
>>> useless and can be removed anyway.
>>>
>>> M.
Powered by blists - more mailing lists