Re: [PATCH v2] PCI: keystone: Fix ks_pcie_v3_65_add_bus() for AM654x SoC

[Siddharth Vadapalli]

On 18/10/23 17:45, Serge Semin wrote:
> On Wed, Oct 18, 2023 at 05:26:53PM +0530, Siddharth Vadapalli wrote:
>>
>>

...

>>
>> Even before commit 6ab15b5e7057, with support for AM654x (using version 4.90a
>> DWC PCIe IP-core) already present, ks_pcie_ops was being used for AM654x. This
>> indicates that prior to commit 6ab15b5e7057, the ks_pcie_ops was applicable to
>> AM654x and there's no problem. However, when commit 6ab15b5e7057 converted the
>> .scan_bus() method in ks_pcie_host_ops (which wasn't used for AM654x) to
>> .add_bus(), it added the method within the shared ks_pcie_ops structure which
>> implicitly, *mistakenly*, assumes that a function named "ks_pcie_v3_65_add_bus"
>> is also applicable to other controller versions (4.90a in this case). Paying
>> attention to the name of the function, it becomes clear that it was added for
>> controller version "3.65", hence the name "...v3_65...". The assumption that the
>> function/method is applicable to other controller versions as well, was
>> incorrect which led to the current issue that can be observed. The commit
>> 6ab15b5e7057 ended up adding a new method for a controller version which *never*
>> used the method. This therefore is a bug.
>>
> 
>> The simplest fix I see is that of exiting ks_pcie_v3_65_add_bus() if:
>> ks_pcie->is_am6 is set, since such checks have been scattered throughout the
>> same driver prior to the addition of commit 6ab15b5e7057 as well.
>>
>> I am open to other implementations of fixing this issue as well. Kindly let me
>> know in case of any suggestions.
> 
> A simplest fix isn't always the best one. As you said yourself the
> ks_pcie_v3_65_add_bus() implies having it called for v3.65. Calling it
> for the newer controllers was a mistake causing the bug. Thus having
> the ks_pcie_ops utilized for both old and new controllers was also a

At the point in time when support for the new controller was added, ks_pcie_ops
was defined as:
static struct pci_ops ks_pcie_ops = {
	.map_bus = dw_pcie_own_conf_map_bus,
	.read = pci_generic_config_read,
	.write = pci_generic_config_write,
};
which did not have anything version specific.

It is only after commit 6ab15b5e7057 that a version specific .add_bus method was
added to ks_pcie_ops, making ks_pcie_ops version-specific since then.

> mistake. Therefor my suggestion to fix the problem by defining the two
> pci_ops structure instances would be more correct at least from the
> code readability point of view. It would make the
> ks_pcie_v3_65_add_bus() function body and name coherent.

Sure. Thank you for the suggestion. I will leave ks_pcie_ops as-is for the older
3.65 controller while adding the ks_pcie_am6_ops without the .add_bus method for
the newer 4.90 controller. I assume this should be acceptable since the
pci-keystone.c driver only has two controller versions, namely 3.65a and 4.90a,
with the new 4.90a controller only applicable to AM654x SoC which is already
being distinguished in the driver using the is_am6 flag.

In the v3 patch, I will add the following:

static struct pci_ops ks_pcie_am6_ops = {
	.map_bus = dw_pcie_own_conf_map_bus,
	.read = pci_generic_config_read,
	.write = pci_generic_config_write,
};

and also update ks_pcie_host_init() to the following:
if(ks_pcie->is_am6)
	pp->bridge->ops = &ks_pcie_am6_ops;
else
	pp->bridge->ops = &ks_pcie_ops;

> 
> Meanwhile your fix look more like a workaround. The
> ks_pcie_v3_65_add_bus() function will be still called for the AM6x
> v4.90 controllers, which based on its semantic would be and will be
> wrong in any case. So instead of noop-ing the function it would be
> better to just drop it being called for the new controllers.

Yes, I will drop it for the new 4.90a controller rather than making it a no-op.

> 
> -Serge(y)
> 
>>
>>>
>>> -Serge(y)
>>>
>>>>  		return 0;
>>>>  
>>>>  	/* Configure and set up BAR0 */
>>>> -- 
>>>> 2.34.1
>>>>
>>
>> -- 
>> Regards,
>> Siddharth.

-- 
Regards,
Siddharth.