Re: [PATCH] genirq/msi: Check null pointer before copying struct msi_msg

From: Marc Zyngier <maz@kernel.org>
To: Gustavo Pimentel <Gustavo.Pimentel@synopsys.com>,
	Alan Mikhak <alan.mikhak@sifive.com>
Cc: linux-kernel@vger.kernel.org, dmaengine@vger.kernel.org,
	linux-pci@vger.kernel.org, tglx@linutronix.de, kishon@ti.com,
	paul.walmsley@sifive.com
Subject: Re: [PATCH] genirq/msi: Check null pointer before copying struct msi_msg
Date: Mon, 20 Apr 2020 10:14:33 +0100	[thread overview]
Message-ID: <8a03b55223b118c6fc605d7204e01460@kernel.org> (raw)
In-Reply-To: <CY4PR12MB1271277CEE4F1FE06B71DDE8DAD60@CY4PR12MB1271.namprd12.prod.outlook.com>

On 2020-04-18 16:19, Gustavo Pimentel wrote:
> Hi Marc and Alan,
> 
>> I'm not convinced by this. If you know that, by construction, these
>> interrupts are not associated with an underlying MSI, why calling
>> get_cached_msi_msg() the first place?
>> 
>> There seem to be some assumptions in the DW EDMA driver that the
>> signaling would be MSI based, so maybe someone from Synopsys 
>> (Gustavo?)
>> could clarify that. From my own perspective, running on an endpoint
>> device means that it is *generating* interrupts, and I'm not sure what
>> the MSIs represent here.
> 
> Giving a little context to this topic.
> 
> The eDMA IP present on the Synopsys DesignWare PCIe Endpoints can be
> configured and triggered *remotely* as well *locally*.
> For the sake of simplicity let's assume for now the eDMA was 
> implemented
> on the EP and that is the IP that we want to configure and use.
> 
> When I say *remotely* I mean that this IP can be configurable through 
> the
> RC/CPU side, however, for that, it requires the eDMA registers to be
> exposed through a PCIe BAR on the EP. This will allow setting the SAR,
> DAR and other settings, also need(s) the interrupt(s) address(es) to be
> set as well (MSI or MSI-X only) so that it can signal through PCIe (to
> the RC and consecutively the associated EP driver) if the data transfer
> has been completed, aborted or if the Linked List consumer algorithm 
> has
> passed in some linked element marked with a watermark.
> 
> It was based on this case that the eDMA driver was exclusively 
> developed.
> 
> However, Alan, wants to expand a little more this, by being able to use
> this driver on the EP side (through
> pcitest/pci_endpoint_test/pci_epf_test) so that he can configure this 
> IP
> *locally*.
> In fact, when doing this, he doesn't need to configure the interrupt
> address (MSI or MSI-X), because this IP provides a local interrupt line
> so that be connected to other blocks on the EP side.

Right, so this confirms my hunch that the driver is being used in
a way that doesn't reflect the expected use case. Rather than
papering over the problem by hacking the core code, I'd rather see
the eDMA driver be updated to support both host and endpoint cases.
This probably boils down to a PCI vs non-PCI set of helpers.

Alan, could you confirm whether we got it right?

Thanks,

         M.
-- 
Jazz is not dead. It just smells funny...