Re: [PATCH] powerpc/pci: Add config option for using OF 'reg' for PCI domain

From: Christophe Leroy <christophe.leroy@csgroup.eu>
To: "Pali Rohár" <pali@kernel.org>,
	"Michael Ellerman" <mpe@ellerman.id.au>,
	"Benjamin Herrenschmidt" <benh@kernel.crashing.org>,
	"Paul Mackerras" <paulus@samba.org>,
	"Bjorn Helgaas" <bhelgaas@google.com>
Cc: "linuxppc-dev@lists.ozlabs.org" <linuxppc-dev@lists.ozlabs.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] powerpc/pci: Add config option for using OF 'reg' for PCI domain
Date: Thu, 5 May 2022 07:16:40 +0000	[thread overview]
Message-ID: <8ffa0287-de5e-4308-07d8-204ac2e7f63a@csgroup.eu> (raw)
In-Reply-To: <20220504175718.29011-1-pali@kernel.org>

Le 04/05/2022 à 19:57, Pali Rohár a écrit :
> Since commit 63a72284b159 ("powerpc/pci: Assign fixed PHB number based on
> device-tree properties"), powerpc kernel always fallback to PCI domain
> assignment from OF / Device Tree 'reg' property of the PCI controller.
> 
> PCI code for other Linux architectures use increasing assignment of the PCI
> domain for individual controllers (assign the first free number), like it
> was also for powerpc prior mentioned commit.
> 
> Upgrading powerpc kernels from LTS 4.4 version (which does not contain
> mentioned commit) to new LTS versions brings a regression in domain
> assignment.

Can you elaborate why it is a regression ?

That commit says 'no functionnal changes', I'm having hard time 
understanding how a nochange can be a regression.

Usually we don't commit regressions to mainline ...

> 
> Fix this issue by introducing a new option CONFIG_PPC_PCI_DOMAIN_FROM_OF_REG
> When this options is disabled then powerpc kernel would assign PCI domains
> in the similar way like it is doing kernel for other architectures and also
> how it was done prior that commit.

You don't define CONFIG_PPC_PCI_DOMAIN_FROM_OF_REG on by default, it 
means this commit will change the behaviour. Is that expected ?

Is that really worth a user selectable option ? Is the user able to 
decide what he needs ?

> 
> Fixes: 63a72284b159 ("powerpc/pci: Assign fixed PHB number based on device-tree properties")

Is that really a fix ? What is the problem really ?

> Signed-off-by: Pali Rohár <pali@kernel.org>
> ---
>   arch/powerpc/Kconfig             | 10 ++++++++++
>   arch/powerpc/kernel/pci-common.c |  4 ++--
>   2 files changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
> index 174edabb74fa..4dd3e3acddda 100644
> --- a/arch/powerpc/Kconfig
> +++ b/arch/powerpc/Kconfig
> @@ -375,6 +375,16 @@ config PPC_OF_PLATFORM_PCI
>   	depends on PCI
>   	depends on PPC64 # not supported on 32 bits yet
>   
> +config PPC_PCI_DOMAIN_FROM_OF_REG
> +	bool "Use OF reg property for PCI domain"
> +	depends on PCI

Should it depend on PPC_OF_PLATFORM_PCI instead ?

> +	help
> +	  By default PCI domain for host bridge during its registration is
> +	  chosen as the lowest unused PCI domain number.
> +
> +	  When this option is enabled then PCI domain is determined from
> +	  the OF / Device Tree 'reg' property.
> +
>   config ARCH_SUPPORTS_UPROBES
>   	def_bool y
>   
> diff --git a/arch/powerpc/kernel/pci-common.c b/arch/powerpc/kernel/pci-common.c
> index 8bc9cf62cd93..8cb6fc5302ae 100644
> --- a/arch/powerpc/kernel/pci-common.c
> +++ b/arch/powerpc/kernel/pci-common.c
> @@ -74,7 +74,6 @@ void __init set_pci_dma_ops(const struct dma_map_ops *dma_ops)
>   static int get_phb_number(struct device_node *dn)
>   {
>   	int ret, phb_id = -1;
> -	u32 prop_32;
>   	u64 prop;
>   
>   	/*
> @@ -83,7 +82,8 @@ static int get_phb_number(struct device_node *dn)
>   	 * reading "ibm,opal-phbid", only present in OPAL environment.
>   	 */
>   	ret = of_property_read_u64(dn, "ibm,opal-phbid", &prop);

This looks like very specific, it is not reflected in the commit log.

> -	if (ret) {
> +	if (ret && IS_ENABLED(CONFIG_PPC_PCI_DOMAIN_FROM_OF_REG)) {
> +		u32 prop_32;
>   		ret = of_property_read_u32_index(dn, "reg", 1, &prop_32);
>   		prop = prop_32;
>   	}