Re: [PATCH v9 3/5] PCI: tegra: Add Tegra264 support

On Wed, Aug 05, 2026 at 12:10:38PM +0000, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [High] The driver incorrectly accesses the ECAM window via `pcie->cfg->win` to read the root port configuration space, causing an out-of-bounds heap read on 32-bit platforms.
> - [High] The driver incorrectly uses the Negotiated Link Width (NLW) to set interconnect bandwidth for hotplug-capable ports when the link is down, permanently setting it to 0.
> --
> 
> --- Patch [3]: [PATCH v9 3/5] PCI: tegra: Add Tegra264 support ---
> commit af63006f768e2991e49a48de9855ea58d20662ed
> Author: Thierry Reding <[email protected]>
> 
>     PCI: tegra: Add Tegra264 support
>     
>     Add a driver for the PCIe controller found on NVIDIA Tegra264 SoCs. The
>     driver is very small, with its main purpose being to set up the address
>     translation registers and then creating a standard PCI host using ECAM.
> 
> > diff --git a/drivers/pci/controller/pcie-tegra264.c b/drivers/pci/controller/pcie-tegra264.c
> > new file mode 100644
> > index 0000000000000..e0c5b9d0d20bf
> > --- /dev/null
> > +++ b/drivers/pci/controller/pcie-tegra264.c
> [ ... ]
> > +static void tegra264_pcie_icc_set(struct tegra264_pcie *pcie)
> > +{
> > +	u32 value, speed, width;
> > +	int err;
> > +
> > +	value = readw(pcie->ecam + XTL_RC_PCIE_CFG_LINK_STATUS);
> > +	speed = FIELD_GET(PCI_EXP_LNKSTA_CLS, value);
> > +	width = FIELD_GET(PCI_EXP_LNKSTA_NLW, value);
> > +
> > +	value = Mbps_to_icc(width * PCIE_SPEED2MBS_ENC(pcie_link_speed[speed]));
> 
> [Severity: High]
> Will this permanently set the interconnect bandwidth to 0 for hotplug-capable
> ports if the link is initially down?
> 
> When the link is down, the Negotiated Link Width (NLW) is 0, which means
> the bandwidth value will be calculated as 0. Since the generic pciehp driver
> handles hotplug but doesn't know about this driver's specific interconnect
> bandwidth requirements, it appears there is no way to dynamically increase
> the bandwidth later when a device is actually inserted.
> 

I think this is a valid concern. If the ICC BW is set to 0, then the hotplugged
device won't work properly.

> Should this read the Maximum Link Width and Speed from PCI_EXP_LNKCAP
> instead of PCI_EXP_LNKSTA to properly reserve bandwidth for a hotplug slot?
> 

If possible, BW should be updated once the device is added, but that requires
detecting Link Up using some custom IRQ if available.

- Mani

-- 
மணிவண்ணன் சதாசிவம்