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

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.

-- 
nvpublic