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

On Fri, Aug 07, 2026 at 10:35:30AM +0530, Manikanta Maddireddy wrote:
> 
> 
> On 05/08/26 9:21 pm, Manivannan Sadhasivam wrote:
> > 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
> > 
> 
> Custom IRQ support for link up and down events is not available in this IP.
> For now, we have two options:
> 
> 1. Don’t set ICC BW for the hot-plug controller.
> 
> 2. Set ICC BW according to the Link Cap settings when the link is down and
> it is a hot-plug controller. When the link is up, use the Link Status
> settings.
> 
> We can get changes in link speed/width from the BW notification status.
> IIRC, the BW notification port driver has been reverted. If it comes back,
> we can consider integrating the ICC BW set logic into it instead of
> implementing it in each controller driver.
> 

Yeah sure. Option 2 sounds sensible to me.

- Mani

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