Re: [RFC PATCH v1 3/3] PCI: vmd: Add WA for VMD PCH rootbus support

[Bjorn Helgaas <helgaas@kernel.org>]

In subject (and the code comment), spell out "WA" (I assume it means
workaround?)

Also make it more specific than "VMD PCH rootbus support," which
doesn't say anything about what the actual issue is.

On Fri, Oct 25, 2024 at 05:01:53PM +0200, Szymon Durawa wrote:
> VMD PCH rootbus primary number is 0x80 and pci_scan_bridge_extend()
> cannot assign it as "hard-wired to 0" and marks setup as broken. To
> avoid this, PCH bus number has to be the same as PCH primary number.

From the cover letter, I infer that whatever the issue is, it doesn't
happen when VMD is integrated into an IOC, but it does happen when VMD
is in a PCH.  These details should be in this commit log because they
are relevant to *this* patch, and the cover letter doesn't make it
into git.

Maybe the problem is that some root bus numbers are hardwired to fixed
non-zero values?  I thought we already handled that, but perhaps not.

What does the user see without this patch?  Some warning?  Failure to
enumerate some devices?  Include a hint here if possible so users can
find the fix to their issue.

> Suggested-by: Nirmal Patel <nirmal.patel@linux.intel.com>
> Reviewed-by: Mariusz Tkaczyk <mariusz.tkaczyk@linux.intel.com>
> Signed-off-by: Szymon Durawa <szymon.durawa@linux.intel.com>
> ---
>  drivers/pci/controller/vmd.c | 26 ++++++++++++++++++++++++--
>  1 file changed, 24 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pci/controller/vmd.c b/drivers/pci/controller/vmd.c
> index 842b70a21325..bb47e0a76c89 100755
> --- a/drivers/pci/controller/vmd.c
> +++ b/drivers/pci/controller/vmd.c
> @@ -404,8 +404,22 @@ static inline u8 vmd_has_pch_rootbus(struct vmd_dev *vmd)
>  static void __iomem *vmd_cfg_addr(struct vmd_dev *vmd, struct pci_bus *bus,
>  				  unsigned int devfn, int reg, int len)
>  {
> -	unsigned int busnr_ecam = bus->number - vmd->busn_start[VMD_BUS_0];
> -	u32 offset = PCIE_ECAM_OFFSET(busnr_ecam, devfn, reg);
> +	unsigned char bus_number;
> +	unsigned int busnr_ecam;
> +	u32 offset;
> +
> +	/*
> +	 * VMD WA: for PCH rootbus, bus number is set to VMD_PRIMARY_PCH_BUS
> +	 * (see comment in vmd_create_pch_bus()) but original value is 0xE1
> +	 * which is stored in vmd->busn_start[VMD_BUS_1].

Used 225 elsewhere.  Would be nice to at least use the same base (dec
vs hex), and preferably the same #define.

> +	 */
> +	if (vmd_has_pch_rootbus(vmd) && bus->number == VMD_PRIMARY_PCH_BUS)
> +		bus_number = vmd->busn_start[VMD_BUS_1];
> +	else
> +		bus_number = bus->number;
> +
> +	busnr_ecam = bus_number - vmd->busn_start[VMD_BUS_0];
> +	offset = PCIE_ECAM_OFFSET(busnr_ecam, devfn, reg);
>  
>  	if (offset + len >= resource_size(&vmd->dev->resource[VMD_CFGBAR]))
>  		return NULL;
> @@ -1023,6 +1037,14 @@ static int vmd_create_pch_bus(struct vmd_dev *vmd, struct pci_sysdata *sd,
>  	 */
>  	vmd->bus[VMD_BUS_1]->primary = VMD_PRIMARY_PCH_BUS;
>  
> +	/* This is a workaround for pci_scan_bridge_extend() code.

Use the prevailing comment style:

  /*
   * This is ...

> +	 * It assigns setup as broken when primary != bus->number and
> +	 * for PCH rootbus primary is not "hard-wired to 0".
> +	 * To avoid this, vmd->bus[VMD_BUS_1]->number and
> +	 * vmd->bus[VMD_BUS_1]->primary are updated to the same value.
> +	 */
> +	vmd->bus[VMD_BUS_1]->number = VMD_PRIMARY_PCH_BUS;
> +
>  	vmd_copy_host_bridge_flags(
>  		pci_find_host_bridge(vmd->dev->bus),
>  		to_pci_host_bridge(vmd->bus[VMD_BUS_1]->bridge));
> -- 
> 2.39.3
>