diff mbox series

PCI: Don't try to read CLS from PCIe devices in pci_apply_final_quirks

Message ID b5b1456c-d5ff-d1d1-ba95-b9a12eda8ae7@gmail.com (mailing list archive)
State New, archived
Delegated to: Bjorn Helgaas
Headers show
Series PCI: Don't try to read CLS from PCIe devices in pci_apply_final_quirks | expand

Commit Message

Heiner Kallweit Dec. 8, 2020, 1:26 p.m. UTC
Don't try to read CLS from PCIe devices in pci_apply_final_quirks().
This value has no meaning for PCIe.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/pci/quirks.c | 3 +++
 1 file changed, 3 insertions(+)

Comments

Bjorn Helgaas March 31, 2021, 9 p.m. UTC | #1
On Tue, Dec 08, 2020 at 02:26:46PM +0100, Heiner Kallweit wrote:
> Don't try to read CLS from PCIe devices in pci_apply_final_quirks().
> This value has no meaning for PCIe.
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
>  drivers/pci/quirks.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> index d9cbe69b8..ac8ce9118 100644
> --- a/drivers/pci/quirks.c
> +++ b/drivers/pci/quirks.c
> @@ -163,6 +163,9 @@ static int __init pci_apply_final_quirks(void)
>  	pci_apply_fixup_final_quirks = true;
>  	for_each_pci_dev(dev) {
>  		pci_fixup_device(pci_fixup_final, dev);
> +
> +		if (pci_is_pcie(dev))
> +			continue;

This loop tries to deduce the platform's cache line size by looking at
the CLS of every PCI device.  It doesn't *write* CLS for any devices.

IIUC skipping PCIe devices would only make a difference if a PCIe
device had a non-zero CLS different from the CLS of other devices.
In that case we would print a "CLS mismatch" message and fall back to
pci_dfl_cache_line_size.

The power-up value is zero, so if we read a non-zero CLS, it means
firmware set it to something.  It would be strange if firmware set it
to something other than the platform's cache line size.

Skipping PCIe devices probably doesn't hurt anything, but I don't
really see a benefit either.  What do you think?  In general I think
we should add code to check PCI vs PCIe only if it makes a difference.

>  		/*
>  		 * If arch hasn't set it explicitly yet, use the CLS
>  		 * value shared by all PCI devices.  If there's a
> -- 
> 2.29.2
>
Heiner Kallweit April 1, 2021, 3:26 p.m. UTC | #2
On 31.03.2021 23:00, Bjorn Helgaas wrote:
> On Tue, Dec 08, 2020 at 02:26:46PM +0100, Heiner Kallweit wrote:
>> Don't try to read CLS from PCIe devices in pci_apply_final_quirks().
>> This value has no meaning for PCIe.
>>
>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>> ---
>>  drivers/pci/quirks.c | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
>> index d9cbe69b8..ac8ce9118 100644
>> --- a/drivers/pci/quirks.c
>> +++ b/drivers/pci/quirks.c
>> @@ -163,6 +163,9 @@ static int __init pci_apply_final_quirks(void)
>>  	pci_apply_fixup_final_quirks = true;
>>  	for_each_pci_dev(dev) {
>>  		pci_fixup_device(pci_fixup_final, dev);
>> +
>> +		if (pci_is_pcie(dev))
>> +			continue;
> 
> This loop tries to deduce the platform's cache line size by looking at
> the CLS of every PCI device.  It doesn't *write* CLS for any devices.
> 
> IIUC skipping PCIe devices would only make a difference if a PCIe
> device had a non-zero CLS different from the CLS of other devices.
> In that case we would print a "CLS mismatch" message and fall back to
> pci_dfl_cache_line_size.
> 
> The power-up value is zero, so if we read a non-zero CLS, it means
> firmware set it to something.  It would be strange if firmware set it
> to something other than the platform's cache line size.
> 
> Skipping PCIe devices probably doesn't hurt anything, but I don't
> really see a benefit either.  What do you think?  In general I think
> we should add code to check PCI vs PCIe only if it makes a difference.
> 
There is no functional change, right. The benefit is just that we
avoid some unnecessary traffic on the PCI bus.
If you think that this is not really worth a patch, then this is also
fine with me.


>>  		/*
>>  		 * If arch hasn't set it explicitly yet, use the CLS
>>  		 * value shared by all PCI devices.  If there's a
>> -- 
>> 2.29.2
>>
diff mbox series

Patch

diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index d9cbe69b8..ac8ce9118 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -163,6 +163,9 @@  static int __init pci_apply_final_quirks(void)
 	pci_apply_fixup_final_quirks = true;
 	for_each_pci_dev(dev) {
 		pci_fixup_device(pci_fixup_final, dev);
+
+		if (pci_is_pcie(dev))
+			continue;
 		/*
 		 * If arch hasn't set it explicitly yet, use the CLS
 		 * value shared by all PCI devices.  If there's a