[PATCH v4 1/3] starfive: pci: Add StarFive JH7110 pcie driver

[Minda Chen]

On 2023/4/12 5:29, Pali Rohár wrote:
> Hello!
> 
> On Tuesday 11 April 2023 09:02:07 Minda Chen wrote:
>> +int starfive_pcie_config_write(struct udevice *udev, pci_dev_t bdf,
>> +			       uint offset, ulong value,
>> +			       enum pci_size_t size)
>> +{
>> +	struct starfive_pcie *priv = dev_get_priv(udev);
>> +	int ret;
>> +
>> +	ret = pci_generic_mmap_write_config(udev, starfive_pcie_conf_address,
>> +					    bdf, offset, value, size);
>> +
>> +	if (!ret && offset == PCI_SECONDARY_BUS) {
>> +		priv->sec_busno = value & 0xff;
>> +		debug("Secondary bus number was changed to %d\n",
>> +		      priv->sec_busno);
>> +	}
> 
> This block of code contains two issues:
> 
> 1) If secondary bus is changed by the 16-bit or 32-bit write operation
>    then this condition does not catch it.
> 
> 2) priv->sec_busno is used just for checking if driver is going to
>    access device on secondary bus of the Root Port. But this code
>    updates priv->sec_busno also for write to _any_ device on any bus,
>    not just when updating Root Port device. So it breaks support for
>    non-trivial PCIe hierarchy which contains e.g. PCIe switch (e.g. when
>    changing configuration of the virtual PCI-to-PCI bridge device of
>    PCIe switch, which is behind the secondary bus of the Root Port).
> 
> So you need something like this:
> 
>     if (!ret &&
>         PCI_BUS(bdf) == dev_seq(udev) && PCI_DEV(bdf) == 0 && PCI_FUNC(bdf) == 0 &&
>         (offset == PCI_SECONDARY_BUS || (offset == PCI_PRIMARY_BUS && size != PCI_SIZE_8)) {
>         priv->sec_busno = ((offset == PCI_PRIMARY_BUS) ? (value >> 8) : value) & 0xff;
>         debug("Secondary bus number was changed to %d\n", pcie->sec_busno);
>     }
> 
> You have to update priv->sec_busno only when write request is for the
> Root Port. And you need to catch also 16-bit or 32-bit write operation
> to the PCI_PRIMARY_BUS register. It is because PCI_SECONDARY_BUS reg
> is (PCI_PRIMARY_BUS+2) and (PCI_SECONDARY_BUS & ~3) == PCI_PRIMARY_BUS
> 
I will change like this. Thank you very much.
>> +	return ret;
>> +}