| 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 12:26:32 +0200
From: Martin Tůma <tumic@...see.org>
To: Lizhi Hou <lizhi.hou@....com>, 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 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.
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