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

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

[Heiner Kallweit]

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;
 		/*
 		 * If arch hasn't set it explicitly yet, use the CLS
 		 * value shared by all PCI devices.  If there's a
-- 
2.29.2

Re: [PATCH] PCI: Don't try to read CLS from PCIe devices in pci_apply_final_quirks

[Bjorn Helgaas]

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
> 

Re: [PATCH] PCI: Don't try to read CLS from PCIe devices in pci_apply_final_quirks

[Heiner Kallweit]

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
>>

Re: [PATCH] PCI: Don't try to read CLS from PCIe devices in pci_apply_final_quirks

[Bjorn Helgaas]

On Thu, Apr 01, 2021 at 05:26:57PM +0200, Heiner Kallweit wrote:
> 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.

OK, I think I'll drop it just to avoid the cost of convincing
ourselves that there's no risk.  If we see "CLS mismatch" warnings or
if is a real performance cost, we should do something.  But I don't
think the extra PCI traffic will be measurable in this case.

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