Re: [PATCH v2] PCI/portdrv: Use link bandwidth notification capability bit

[stuart hayes <stuart.w.hayes@gmail.com>]

On 7/16/2021 4:56 PM, Bjorn Helgaas wrote:
> On Thu, May 13, 2021 at 03:03:14AM +0530, Stuart Hayes wrote:
>> The pcieport driver can fail to attach to a downstream port that doesn't
>> support bandwidth notification.  This can happen when, in
>> pcie_port_device_register(), pci_init_service_irqs() tries (and fails) to
>> set up a bandwidth notification IRQ for a device that doesn't support it.
> 
> I'm trying to figure out exactly how this fails.  The only failure
> path in pcie_init_service_irqs() is when
> pci_alloc_irq_vectors(PCI_IRQ_LEGACY) fails, which I guess means the
> port's dev->irq was zero?
> 

Yes... dev->irq is zero.  Here's "lspci -v "for the device in case it helps:

66:00.0 PCI bridge: Broadcom / LSI PEX880xx PCIe Gen 4 Switch (rev b0) 
(prog-if 00 [Normal decode])
         Flags: bus master, fast devsel, latency 0, NUMA node 0
         Bus: primary=66, secondary=67, subordinate=70, sec-latency=0
         I/O behind bridge: [disabled]
         Memory behind bridge: be000000-bf4fffff [size=21M]
         Prefetchable memory behind bridge: 
00000d0000000000-00000d00017fffff [size=24M]
         Capabilities: [40] Power Management version 3
         Capabilities: [68] Express Downstream Port (Slot-), MSI 00
         Capabilities: [a4] Subsystem: Broadcom / LSI Device a064
         Capabilities: [100] Device Serial Number 00-80-5e-10-56-39-53-50
         Capabilities: [fb4] Advanced Error Reporting
         Capabilities: [148] Virtual Channel
         Capabilities: [b70] Vendor Specific Information: ID=0001 Rev=0 
Len=010 <?>
         Capabilities: [e00] Multicast
         Capabilities: [f24] Access Control Services

> And to even attempt legacy IRQs, either we had pcie_pme_no_msi() or
> pcie_port_enable_irq_vec() failed?
> 

pcie_port_enable_irq_vec() failed.  I don't believe this device has the 
ability to generate interrupts.

>> This patch changes get_port_device_capability() to look at the link
>> bandwidth notification capability bit in the link capabilities register of
>> the port, instead of assuming that all downstream ports have that
>> capability.
> 
> I think this needs:
> 
>    Fixes: e8303bb7a75c ("PCI/LINK: Report degraded links via link bandwidth notification")
> 
> because even though b4c7d2076b4e ("PCI/LINK: Remove bandwidth
> notification") removed *most* of e8303bb7a75c, it did not remove the
> code in get_port_device_capability() that you're fixing here.
> 
> I can fix this up locally, no need to resend.  I think the patch
> itself is fine, just trying to understand the commit log.
> 

Thank you!


>> Signed-off-by: Stuart Hayes <stuart.w.hayes@gmail.com>
>> Reviewed-by: Lukas Wunner <lukas@wunner.de>
>> ---
>>
>> changes from v1:
>> 	- corrected spelling errors in commit message
>> 	- added Lukas' reviewed-by:
>>
>> ---
>>   drivers/pci/pcie/portdrv_core.c | 9 +++++++--
>>   1 file changed, 7 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/pci/pcie/portdrv_core.c b/drivers/pci/pcie/portdrv_core.c
>> index e1fed6649c41..3ee63968deaa 100644
>> --- a/drivers/pci/pcie/portdrv_core.c
>> +++ b/drivers/pci/pcie/portdrv_core.c
>> @@ -257,8 +257,13 @@ static int get_port_device_capability(struct pci_dev *dev)
>>   		services |= PCIE_PORT_SERVICE_DPC;
>>   
>>   	if (pci_pcie_type(dev) == PCI_EXP_TYPE_DOWNSTREAM ||
>> -	    pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT)
>> -		services |= PCIE_PORT_SERVICE_BWNOTIF;
>> +	    pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT) {
>> +		u32 linkcap;
>> +
>> +		pcie_capability_read_dword(dev, PCI_EXP_LNKCAP, &linkcap);
>> +		if (linkcap & PCI_EXP_LNKCAP_LBNC)
>> +			services |= PCIE_PORT_SERVICE_BWNOTIF;
>> +	}
>>   
>>   	return services;
>>   }
>> -- 
>> 2.27.0
>>