linux-kernel - Re: [PATCH] PCI: j721e: Set .map_irq and .swizzle

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <be3e3c5f-0d48-41b0-87f4-2210f13b9460@ti.com>
Date: Thu, 25 Jul 2024 11:23:43 +0530
From: Siddharth Vadapalli <s-vadapalli@...com>
To: Bjorn Helgaas <helgaas@...nel.org>
CC: Siddharth Vadapalli <s-vadapalli@...com>, <bhelgaas@...gle.com>,
        <lpieralisi@...nel.org>, <kw@...ux.com>, <robh@...nel.org>,
        <vigneshr@...com>, <kishon@...nel.org>, <linux-pci@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>, <linux-arm-kernel@...ts.infradead.org>,
        <stable@...r.kernel.org>, <ahalaney@...hat.com>, <srk@...com>
Subject: Re: [PATCH] PCI: j721e: Set .map_irq and .swizzle_irq to NULL

On Wed, Jul 24, 2024 at 11:23:04AM -0500, Bjorn Helgaas wrote:
> Subject should say something about why this change is needed, not just
> translate the C code to English.

My intent was to summarize the change to make it easy for anyone to find
out what's being done. The commit message below explains in detail as to
why they are set to NULL. As an alternative, I could change the $subject
to:
PCI: j721e: Disable INTx mapping and swizzling
where the "Disable" is equivalent to NULL. Kindly let me know if this is
acceptable or needs to be improved further.

> 
> On Wed, Jul 24, 2024 at 12:20:48PM +0530, Siddharth Vadapalli wrote:
> > Since the configuration of Legacy Interrupts (INTx) is not supported, set
> 
> I assume you mean J721E doesn't support INTx?

Yes, the driver support doesn't exist and the device-tree also doesn't
have the required nodes. Kishon had posted a series to enable it for
J721E, J7200 and AM64 on the 4th of August 2021:
https://lore.kernel.org/r/20210804132912.30685-1-kishon@ti.com/
and based on the discussion on the series, an Errata was discovered for
J721E:
https://www.ti.com/lit/er/sprz455d/sprz455d.pdf
i2094: PCIe: End of Interrupt (EOI) Not Enabled for PCIe Legacy Interrupts

Thus, there is no support for Legacy Interrupts (INTx) in the pci-j721e.c
driver and as a consequence, no device-tree nodes either.

> 
> > the .map_irq and .swizzle_irq callbacks to NULL. This fixes the error:
> >   of_irq_parse_pci: failed with rc=-22
> 
> I guess this happens because devm_of_pci_bridge_init() initializes
> .map_irq and .swizzle_irq unconditionally?

Yes. The PCIe Controller on J721E and other SoCs is the Cadence PCIe
Controller. In the commit which added driver support for the Cadence
PCIe Controller's Host functionality:
https://github.com/torvalds/linux/commit/1b79c5284439
'map_irq' and 'swizzle_irq' were set to the same functions:
'of_irq_parse_and_map_pci()' and 'pci_common_swizzle()'
respectively. Commit:
https://github.com/torvalds/linux/commit/b64aa11eb2dd
extracted out the shared defaults from multiple Host Controller drivers
and moved them into the common 'devm_of_pci_bridge_init()' function.

While 'map_irq' and 'swizzle_irq' are unconditionally set to the
defaults in 'devm_of_pci_bridge_init()', those Host Controller driver
which haven't been using the shared defaults do have the choice of
overriding them in the driver (which is already being done).

Similarly, for pci-j721e.c, since the shared defaults inherited from the
Cadence PCIe Controller are not applicable (due to the Errata), the
pci-j721e.c driver should override them to NULL as done in this patch.

> 
> I'm not sure the default assumption should be that a host bridge
> supports INTx.
> 
> Maybe .map_irq and .swizzle_irq should only be set when we discover
> some information about INTx routing, e.g., an ACPI _PRT or the
> corresponding DT properties.

I believe that the defaults were set since most of the Host drivers were
using them in their respective drivers as indicated in the commit:
https://github.com/torvalds/linux/commit/b64aa11eb2dd

[...]

> > 
> >  drivers/pci/controller/cadence/pci-j721e.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> > 
> > diff --git a/drivers/pci/controller/cadence/pci-j721e.c b/drivers/pci/controller/cadence/pci-j721e.c
> > index 85718246016b..5372218849a8 100644
> > --- a/drivers/pci/controller/cadence/pci-j721e.c
> > +++ b/drivers/pci/controller/cadence/pci-j721e.c
> > @@ -417,6 +417,10 @@ static int j721e_pcie_probe(struct platform_device *pdev)
> >  		if (!bridge)
> >  			return -ENOMEM;
> >  
> > +		/* Legacy interrupts are not supported */
> 
> Say "INTx" explicitly here instead of assuming "legacy" == "INTx".

Sure. I will update it in the v2 patch.

Thank you for reviewing this patch and sharing your feedback.

Regards,
Siddharth.