[V6,02/13] pci, acpi: Provide generic way to assign bus domain number.

Commit Message

Tomasz Nowicki April 15, 2016, 5:06 p.m. UTC

As we now have valid PCI host bridge device reference we can
introduce code that is going to find its bus domain number using
ACPI _SEG method.

Note that _SEG method is optional, therefore _SEG absence means
that all PCI buses belong to domain 0.

While at it, for the sake of code clarity we put ACPI and DT domain
assign methods into the corresponding helpers.

Signed-off-by: Tomasz Nowicki <[e-mail]>
Reviewed-by: Liviu Dudau <[e-mail]>
Tested-by: Suravee Suthikulpanit <[e-mail]>
Tested-by: Jeremy Linton <[e-mail]>
Tested-by: Duc Dang <[e-mail]>
Tested-by: Dongdong Liu <[e-mail]>
Tested-by: Hanjun Guo <[e-mail]>
Tested-by: Graeme Gregory <[e-mail]>
Tested-by: Sinan Kaya <[e-mail]>
---
 drivers/acpi/pci_root.c  | 18 ++++++++++++++++++
 drivers/pci/pci.c        | 11 +++++++++--
 include/linux/pci-acpi.h |  2 ++
 3 files changed, 29 insertions(+), 2 deletions(-)

Comments

Bjorn Helgaas April 27, 2016, 2:26 a.m. UTC | #1

On Fri, Apr 15, 2016 at 07:06:37PM +0200, Tomasz Nowicki wrote:
> As we now have valid PCI host bridge device reference we can
> introduce code that is going to find its bus domain number using
> ACPI _SEG method.
> 
> Note that _SEG method is optional, therefore _SEG absence means
> that all PCI buses belong to domain 0.
> 
> While at it, for the sake of code clarity we put ACPI and DT domain
> assign methods into the corresponding helpers.
> 
> Signed-off-by: Tomasz Nowicki <[e-mail]>
> Reviewed-by: Liviu Dudau <[e-mail]>
> Tested-by: Suravee Suthikulpanit <[e-mail]>
> Tested-by: Jeremy Linton <[e-mail]>
> Tested-by: Duc Dang <[e-mail]>
> Tested-by: Dongdong Liu <[e-mail]>
> Tested-by: Hanjun Guo <[e-mail]>
> Tested-by: Graeme Gregory <[e-mail]>
> Tested-by: Sinan Kaya <[e-mail]>
> ---
>  drivers/acpi/pci_root.c  | 18 ++++++++++++++++++
>  drivers/pci/pci.c        | 11 +++++++++--
>  include/linux/pci-acpi.h |  2 ++
>  3 files changed, 29 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
> index 4581e0e..d9a70c4 100644
> --- a/drivers/acpi/pci_root.c
> +++ b/drivers/acpi/pci_root.c
> @@ -419,6 +419,24 @@ out:
>  }
>  EXPORT_SYMBOL(acpi_pci_osc_control_set);
>  
> +int acpi_pci_bus_domain_nr(struct device *parent)
> +{
> +	struct acpi_device *acpi_dev = to_acpi_device(parent);
> +	unsigned long long segment = 0;
> +	acpi_status status;
> +
> +	/*
> +	 * If _SEG method does not exist, following ACPI spec (6.5.6)
> +	 * all PCI buses belong to domain 0.
> +	 */
> +	status = acpi_evaluate_integer(acpi_dev->handle, METHOD_NAME__SEG, NULL,
> +				       &segment);

We already have code in acpi_pci_root_add() to evaluate _SEG.  We
don't want to evaluate it *twice*, do we?

I was sort of expecting that if you added it here, we'd remove the
existing call, but it looks like you're keeping both?

> +	if (ACPI_FAILURE(status) && status != AE_NOT_FOUND)
> +		dev_err(&acpi_dev->dev, "can't evaluate _SEG\n");
> +
> +	return segment;
> +}
> +
>  static void negotiate_os_control(struct acpi_pci_root *root, int *no_aspm)
>  {
>  	u32 support, control, requested;
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 25e0327..1a74e87 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -19,6 +19,7 @@
>  #include <linux/spinlock.h>
>  #include <linux/string.h>
>  #include <linux/log2.h>
> +#include <linux/pci-acpi.h>
>  #include <linux/pci-aspm.h>
>  #include <linux/pm_wakeup.h>
>  #include <linux/interrupt.h>
> @@ -4779,7 +4780,7 @@ int pci_get_new_domain_nr(void)
>  }
>  
>  #ifdef CONFIG_PCI_DOMAINS_GENERIC
> -void pci_bus_assign_domain_nr(struct pci_bus *bus, struct device *parent)
> +static int of_pci_bus_domain_nr(struct device *parent)
>  {
>  	static int use_dt_domains = -1;
>  	int domain = -1;
> @@ -4823,7 +4824,13 @@ void pci_bus_assign_domain_nr(struct pci_bus *bus, struct device *parent)
>  		domain = -1;
>  	}
>  
> -	bus->domain_nr = domain;
> +	return domain;
> +}
> +
> +void pci_bus_assign_domain_nr(struct pci_bus *bus, struct device *parent)
> +{
> +	bus->domain_nr = acpi_disabled ? of_pci_bus_domain_nr(parent) :
> +					 acpi_pci_bus_domain_nr(parent);
>  }
>  #endif
>  #endif
> diff --git a/include/linux/pci-acpi.h b/include/linux/pci-acpi.h
> index 89ab057..a72e22d 100644
> --- a/include/linux/pci-acpi.h
> +++ b/include/linux/pci-acpi.h
> @@ -22,6 +22,7 @@ static inline acpi_status pci_acpi_remove_pm_notifier(struct acpi_device *dev)
>  {
>  	return acpi_remove_pm_notifier(dev);
>  }
> +extern int acpi_pci_bus_domain_nr(struct device *parent);
>  extern phys_addr_t acpi_pci_root_get_mcfg_addr(acpi_handle handle);
>  
>  static inline acpi_handle acpi_find_root_bridge_handle(struct pci_dev *pdev)
> @@ -109,6 +110,7 @@ extern const u8 pci_acpi_dsm_uuid[];
>  #else	/* CONFIG_ACPI */
>  static inline void acpi_pci_add_bus(struct pci_bus *bus) { }
>  static inline void acpi_pci_remove_bus(struct pci_bus *bus) { }
> +static inline int acpi_pci_bus_domain_nr(struct device *parent) { return -1; }
>  #endif	/* CONFIG_ACPI */
>  
>  #ifdef CONFIG_ACPI_APEI
> -- 
> 1.9.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to [e-mail]
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Lorenzo Pieralisi April 27, 2016, 11:17 a.m. UTC | #2

On Tue, Apr 26, 2016 at 09:26:49PM -0500, Bjorn Helgaas wrote:
> On Fri, Apr 15, 2016 at 07:06:37PM +0200, Tomasz Nowicki wrote:
> > As we now have valid PCI host bridge device reference we can
> > introduce code that is going to find its bus domain number using
> > ACPI _SEG method.
> > 
> > Note that _SEG method is optional, therefore _SEG absence means
> > that all PCI buses belong to domain 0.
> > 
> > While at it, for the sake of code clarity we put ACPI and DT domain
> > assign methods into the corresponding helpers.
> > 
> > Signed-off-by: Tomasz Nowicki <[e-mail]>
> > Reviewed-by: Liviu Dudau <[e-mail]>
> > Tested-by: Suravee Suthikulpanit <[e-mail]>
> > Tested-by: Jeremy Linton <[e-mail]>
> > Tested-by: Duc Dang <[e-mail]>
> > Tested-by: Dongdong Liu <[e-mail]>
> > Tested-by: Hanjun Guo <[e-mail]>
> > Tested-by: Graeme Gregory <[e-mail]>
> > Tested-by: Sinan Kaya <[e-mail]>
> > ---
> >  drivers/acpi/pci_root.c  | 18 ++++++++++++++++++
> >  drivers/pci/pci.c        | 11 +++++++++--
> >  include/linux/pci-acpi.h |  2 ++
> >  3 files changed, 29 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
> > index 4581e0e..d9a70c4 100644
> > --- a/drivers/acpi/pci_root.c
> > +++ b/drivers/acpi/pci_root.c
> > @@ -419,6 +419,24 @@ out:
> >  }
> >  EXPORT_SYMBOL(acpi_pci_osc_control_set);
> >  
> > +int acpi_pci_bus_domain_nr(struct device *parent)
> > +{
> > +	struct acpi_device *acpi_dev = to_acpi_device(parent);
> > +	unsigned long long segment = 0;
> > +	acpi_status status;
> > +
> > +	/*
> > +	 * If _SEG method does not exist, following ACPI spec (6.5.6)
> > +	 * all PCI buses belong to domain 0.
> > +	 */
> > +	status = acpi_evaluate_integer(acpi_dev->handle, METHOD_NAME__SEG, NULL,
> > +				       &segment);
> 
> We already have code in acpi_pci_root_add() to evaluate _SEG.  We
> don't want to evaluate it *twice*, do we?
> 
> I was sort of expecting that if you added it here, we'd remove the
> existing call, but it looks like you're keeping both?

We can't remove the existing call, since it is used on X86 and IA64
to store the segment number that, in the process, is used in their
pci_domain_nr() arch specific callback to retrieve the domain nr.

On ARM64, that selects PCI_DOMAINS_GENERIC, we have to find a way
to retrieve the domain number that is not arch dependent, since
this is generic code, we can't rely on any bus->sysdata format (unless
we do something like JC did below), therefore the only way is to call
the _SEG method *again* here, which also forced Tomasz to go through
the ACPI_COMPANION setting song and dance and pass the parent pointer
to pci_create_root_bus() (see patch 1), which BTW is a source of
trouble on its own as you noticed.

JC solved it differently, via sysdata and pseudo-generic code:

http://www.spinics.net/lists/arm-kernel/msg478167.html
http://www.spinics.net/lists/arm-kernel/msg478169.html

I like neither, we need the lesser of two evils though.

Lorenzo

> > +	if (ACPI_FAILURE(status) && status != AE_NOT_FOUND)
> > +		dev_err(&acpi_dev->dev, "can't evaluate _SEG\n");
> > +
> > +	return segment;
> > +}
> > +
> >  static void negotiate_os_control(struct acpi_pci_root *root, int *no_aspm)
> >  {
> >  	u32 support, control, requested;
> > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > index 25e0327..1a74e87 100644
> > --- a/drivers/pci/pci.c
> > +++ b/drivers/pci/pci.c
> > @@ -19,6 +19,7 @@
> >  #include <linux/spinlock.h>
> >  #include <linux/string.h>
> >  #include <linux/log2.h>
> > +#include <linux/pci-acpi.h>
> >  #include <linux/pci-aspm.h>
> >  #include <linux/pm_wakeup.h>
> >  #include <linux/interrupt.h>
> > @@ -4779,7 +4780,7 @@ int pci_get_new_domain_nr(void)
> >  }
> >  
> >  #ifdef CONFIG_PCI_DOMAINS_GENERIC
> > -void pci_bus_assign_domain_nr(struct pci_bus *bus, struct device *parent)
> > +static int of_pci_bus_domain_nr(struct device *parent)
> >  {
> >  	static int use_dt_domains = -1;
> >  	int domain = -1;
> > @@ -4823,7 +4824,13 @@ void pci_bus_assign_domain_nr(struct pci_bus *bus, struct device *parent)
> >  		domain = -1;
> >  	}
> >  
> > -	bus->domain_nr = domain;
> > +	return domain;
> > +}
> > +
> > +void pci_bus_assign_domain_nr(struct pci_bus *bus, struct device *parent)
> > +{
> > +	bus->domain_nr = acpi_disabled ? of_pci_bus_domain_nr(parent) :
> > +					 acpi_pci_bus_domain_nr(parent);
> >  }
> >  #endif
> >  #endif
> > diff --git a/include/linux/pci-acpi.h b/include/linux/pci-acpi.h
> > index 89ab057..a72e22d 100644
> > --- a/include/linux/pci-acpi.h
> > +++ b/include/linux/pci-acpi.h
> > @@ -22,6 +22,7 @@ static inline acpi_status pci_acpi_remove_pm_notifier(struct acpi_device *dev)
> >  {
> >  	return acpi_remove_pm_notifier(dev);
> >  }
> > +extern int acpi_pci_bus_domain_nr(struct device *parent);
> >  extern phys_addr_t acpi_pci_root_get_mcfg_addr(acpi_handle handle);
> >  
> >  static inline acpi_handle acpi_find_root_bridge_handle(struct pci_dev *pdev)
> > @@ -109,6 +110,7 @@ extern const u8 pci_acpi_dsm_uuid[];
> >  #else	/* CONFIG_ACPI */
> >  static inline void acpi_pci_add_bus(struct pci_bus *bus) { }
> >  static inline void acpi_pci_remove_bus(struct pci_bus *bus) { }
> > +static inline int acpi_pci_bus_domain_nr(struct device *parent) { return -1; }
> >  #endif	/* CONFIG_ACPI */
> >  
> >  #ifdef CONFIG_ACPI_APEI
> > -- 
> > 1.9.1
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> > the body of a message to [e-mail]
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Tomasz Nowicki April 27, 2016, 11:59 a.m. UTC | #3

On 27.04.2016 04:26, Bjorn Helgaas wrote:
> On Fri, Apr 15, 2016 at 07:06:37PM +0200, Tomasz Nowicki wrote:
>> As we now have valid PCI host bridge device reference we can
>> introduce code that is going to find its bus domain number using
>> ACPI _SEG method.
>>
>> Note that _SEG method is optional, therefore _SEG absence means
>> that all PCI buses belong to domain 0.
>>
>> While at it, for the sake of code clarity we put ACPI and DT domain
>> assign methods into the corresponding helpers.
>>
>> Signed-off-by: Tomasz Nowicki <[e-mail]>
>> Reviewed-by: Liviu Dudau <[e-mail]>
>> Tested-by: Suravee Suthikulpanit <[e-mail]>
>> Tested-by: Jeremy Linton <[e-mail]>
>> Tested-by: Duc Dang <[e-mail]>
>> Tested-by: Dongdong Liu <[e-mail]>
>> Tested-by: Hanjun Guo <[e-mail]>
>> Tested-by: Graeme Gregory <[e-mail]>
>> Tested-by: Sinan Kaya <[e-mail]>
>> ---
>>   drivers/acpi/pci_root.c  | 18 ++++++++++++++++++
>>   drivers/pci/pci.c        | 11 +++++++++--
>>   include/linux/pci-acpi.h |  2 ++
>>   3 files changed, 29 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
>> index 4581e0e..d9a70c4 100644
>> --- a/drivers/acpi/pci_root.c
>> +++ b/drivers/acpi/pci_root.c
>> @@ -419,6 +419,24 @@ out:
>>   }
>>   EXPORT_SYMBOL(acpi_pci_osc_control_set);
>>
>> +int acpi_pci_bus_domain_nr(struct device *parent)
>> +{
>> +	struct acpi_device *acpi_dev = to_acpi_device(parent);
>> +	unsigned long long segment = 0;
>> +	acpi_status status;
>> +
>> +	/*
>> +	 * If _SEG method does not exist, following ACPI spec (6.5.6)
>> +	 * all PCI buses belong to domain 0.
>> +	 */
>> +	status = acpi_evaluate_integer(acpi_dev->handle, METHOD_NAME__SEG, NULL,
>> +				       &segment);
>
> We already have code in acpi_pci_root_add() to evaluate _SEG.  We
> don't want to evaluate it *twice*, do we?

Ideally we do not want.

The main intention here was to avoid using "void *sysdata" to retrieve 
domain number. sysdata means something different for each architectures. 
This would be common way for all arch using PCI_DOMAINS_GENERIC option 
such as ARM64.

>
> I was sort of expecting that if you added it here, we'd remove the
> existing call, but it looks like you're keeping both?

I leave _SEG evaluation in acpi_pci_root_add to keep support for IA64 
and x86 which are still using arch-specific sysdata (struct 
pci_sysdata->domain for x86 and struct pci_controller->segment for IA64) 
to retrieve domain number.

ARM64 uses PCI_DOMAINS_GENERIC for DT boot method, it would be 
consistent to keep it for ACPI too.

I am open to suggestions, do you think we should use sysdata for ARM64?

Thanks,
Tomasz

Bjorn Helgaas April 27, 2016, 4:44 p.m. UTC | #4

On Wed, Apr 27, 2016 at 12:17:58PM +0100, Lorenzo Pieralisi wrote:
> On Tue, Apr 26, 2016 at 09:26:49PM -0500, Bjorn Helgaas wrote:
> > On Fri, Apr 15, 2016 at 07:06:37PM +0200, Tomasz Nowicki wrote:
> > > As we now have valid PCI host bridge device reference we can
> > > introduce code that is going to find its bus domain number using
> > > ACPI _SEG method.
> > > 
> > > Note that _SEG method is optional, therefore _SEG absence means
> > > that all PCI buses belong to domain 0.
> > > 
> > > While at it, for the sake of code clarity we put ACPI and DT domain
> > > assign methods into the corresponding helpers.
> > > 
> > > Signed-off-by: Tomasz Nowicki <[e-mail]>
> > > Reviewed-by: Liviu Dudau <[e-mail]>
> > > Tested-by: Suravee Suthikulpanit <[e-mail]>
> > > Tested-by: Jeremy Linton <[e-mail]>
> > > Tested-by: Duc Dang <[e-mail]>
> > > Tested-by: Dongdong Liu <[e-mail]>
> > > Tested-by: Hanjun Guo <[e-mail]>
> > > Tested-by: Graeme Gregory <[e-mail]>
> > > Tested-by: Sinan Kaya <[e-mail]>
> > > ---
> > >  drivers/acpi/pci_root.c  | 18 ++++++++++++++++++
> > >  drivers/pci/pci.c        | 11 +++++++++--
> > >  include/linux/pci-acpi.h |  2 ++
> > >  3 files changed, 29 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
> > > index 4581e0e..d9a70c4 100644
> > > --- a/drivers/acpi/pci_root.c
> > > +++ b/drivers/acpi/pci_root.c
> > > @@ -419,6 +419,24 @@ out:
> > >  }
> > >  EXPORT_SYMBOL(acpi_pci_osc_control_set);
> > >  
> > > +int acpi_pci_bus_domain_nr(struct device *parent)

It looks like acpi_pci_bus_domain_nr() could be under #ifdef
CONFIG_PCI_DOMAINS_GENERIC, right?

> > > +{
> > > +	struct acpi_device *acpi_dev = to_acpi_device(parent);
> > > +	unsigned long long segment = 0;
> > > +	acpi_status status;
> > > +
> > > +	/*
> > > +	 * If _SEG method does not exist, following ACPI spec (6.5.6)
> > > +	 * all PCI buses belong to domain 0.
> > > +	 */
> > > +	status = acpi_evaluate_integer(acpi_dev->handle, METHOD_NAME__SEG, NULL,
> > > +				       &segment);
> > 
> > We already have code in acpi_pci_root_add() to evaluate _SEG.  We
> > don't want to evaluate it *twice*, do we?
> > 
> > I was sort of expecting that if you added it here, we'd remove the
> > existing call, but it looks like you're keeping both?
> 
> We can't remove the existing call, since it is used on X86 and IA64
> to store the segment number that, in the process, is used in their
> pci_domain_nr() arch specific callback to retrieve the domain nr.
> 
> On ARM64, that selects PCI_DOMAINS_GENERIC, we have to find a way
> to retrieve the domain number that is not arch dependent, since
> this is generic code, we can't rely on any bus->sysdata format (unless
> we do something like JC did below), therefore the only way is to call
> the _SEG method *again* here, which also forced Tomasz to go through
> the ACPI_COMPANION setting song and dance and pass the parent pointer
> to pci_create_root_bus() (see patch 1), which BTW is a source of
> trouble on its own as you noticed.
> 
> JC solved it differently, via sysdata and pseudo-generic code:
> 
> http://www.spinics.net/lists/arm-kernel/msg478167.html

The thing I don't like about this is the special case of checking
parent and parent->of_node to figure out whether we should use the
segment from ACPI and the fragility of depending on the fact that the
companion hasn't been set yet.

> http://www.spinics.net/lists/arm-kernel/msg478169.html
> 
> I like neither, we need the lesser of two evils though.

Today we call pci_bus_assign_domain_nr() from the PCI core (from
pci_create_root_bus()).  This is only implemented for
PCI_DOMAINS_GENERIC, but even so, it fiddles around to figure out
whether to get the domain from DT or to assign a new one.

That seems backwards to me.  The host bridge drivers already know
where the domain should come from (ACPI _SEG, DT, etc.) and in the
long term, I think they should be responsible for looking up or
assigning a domain number *before* they call pci_create_root_bus().

> > > +	if (ACPI_FAILURE(status) && status != AE_NOT_FOUND)
> > > +		dev_err(&acpi_dev->dev, "can't evaluate _SEG\n");
> > > +
> > > +	return segment;
> > > +}
> > > +
> > >  static void negotiate_os_control(struct acpi_pci_root *root, int *no_aspm)
> > >  {
> > >  	u32 support, control, requested;
> > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > > index 25e0327..1a74e87 100644
> > > --- a/drivers/pci/pci.c
> > > +++ b/drivers/pci/pci.c
> > > @@ -19,6 +19,7 @@
> > >  #include <linux/spinlock.h>
> > >  #include <linux/string.h>
> > >  #include <linux/log2.h>
> > > +#include <linux/pci-acpi.h>
> > >  #include <linux/pci-aspm.h>
> > >  #include <linux/pm_wakeup.h>
> > >  #include <linux/interrupt.h>
> > > @@ -4779,7 +4780,7 @@ int pci_get_new_domain_nr(void)
> > >  }
> > >  
> > >  #ifdef CONFIG_PCI_DOMAINS_GENERIC
> > > -void pci_bus_assign_domain_nr(struct pci_bus *bus, struct device *parent)
> > > +static int of_pci_bus_domain_nr(struct device *parent)
> > >  {
> > >  	static int use_dt_domains = -1;
> > >  	int domain = -1;
> > > @@ -4823,7 +4824,13 @@ void pci_bus_assign_domain_nr(struct pci_bus *bus, struct device *parent)
> > >  		domain = -1;
> > >  	}
> > >  
> > > -	bus->domain_nr = domain;
> > > +	return domain;
> > > +}
> > > +
> > > +void pci_bus_assign_domain_nr(struct pci_bus *bus, struct device *parent)
> > > +{
> > > +	bus->domain_nr = acpi_disabled ? of_pci_bus_domain_nr(parent) :
> > > +					 acpi_pci_bus_domain_nr(parent);

We have the pci_bus * here, so to_pci_host_bridge(bus->bridge) gives
us the struct pci_host_bridge.  I can't remember why we put domain_nr
in the struct pci_bus instead of in the struct pci_host_bridge.  It
seems like pci_host_bridge is the more logical place for it, because
every bus below the host bridge must have the same domain by
definition.

Would it be feasible to either (a) move domain_nr to the
pci_host_bridge, or (b) change acpi_pci_bus_domain_nr() so it uses the
struct pci_bus * or the struct device * to find the struct
acpi_pci_root where segment has already been stored by
acpi_pci_root_add()?

Another wrinkle is the quirk added by 1f09b09b4de0 ("x86/PCI: Ignore
_SEG on HP xw9300").  x86 doesn't use PCI_DOMAINS_GENERIC yet, so this
patch wouldn't break it, but I hope x86 can use PCI_DOMAINS_GENERIC in
the future, and then it will be a problem if we evaluate _SEG again.

> > >  }
> > >  #endif
> > >  #endif
> > > diff --git a/include/linux/pci-acpi.h b/include/linux/pci-acpi.h
> > > index 89ab057..a72e22d 100644
> > > --- a/include/linux/pci-acpi.h
> > > +++ b/include/linux/pci-acpi.h
> > > @@ -22,6 +22,7 @@ static inline acpi_status pci_acpi_remove_pm_notifier(struct acpi_device *dev)
> > >  {
> > >  	return acpi_remove_pm_notifier(dev);
> > >  }
> > > +extern int acpi_pci_bus_domain_nr(struct device *parent);
> > >  extern phys_addr_t acpi_pci_root_get_mcfg_addr(acpi_handle handle);
> > >  
> > >  static inline acpi_handle acpi_find_root_bridge_handle(struct pci_dev *pdev)
> > > @@ -109,6 +110,7 @@ extern const u8 pci_acpi_dsm_uuid[];
> > >  #else	/* CONFIG_ACPI */
> > >  static inline void acpi_pci_add_bus(struct pci_bus *bus) { }
> > >  static inline void acpi_pci_remove_bus(struct pci_bus *bus) { }
> > > +static inline int acpi_pci_bus_domain_nr(struct device *parent) { return -1; }
> > >  #endif	/* CONFIG_ACPI */
> > >  
> > >  #ifdef CONFIG_ACPI_APEI
> > > -- 
> > > 1.9.1
> > > 
> > > --
> > > To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> > > the body of a message to [e-mail]
> > > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> >

Lorenzo Pieralisi April 27, 2016, 5:31 p.m. UTC | #5

On Wed, Apr 27, 2016 at 11:44:53AM -0500, Bjorn Helgaas wrote:
> On Wed, Apr 27, 2016 at 12:17:58PM +0100, Lorenzo Pieralisi wrote:
> > On Tue, Apr 26, 2016 at 09:26:49PM -0500, Bjorn Helgaas wrote:
> > > On Fri, Apr 15, 2016 at 07:06:37PM +0200, Tomasz Nowicki wrote:
> > > > As we now have valid PCI host bridge device reference we can
> > > > introduce code that is going to find its bus domain number using
> > > > ACPI _SEG method.
> > > > 
> > > > Note that _SEG method is optional, therefore _SEG absence means
> > > > that all PCI buses belong to domain 0.
> > > > 
> > > > While at it, for the sake of code clarity we put ACPI and DT domain
> > > > assign methods into the corresponding helpers.
> > > > 
> > > > Signed-off-by: Tomasz Nowicki <[e-mail]>
> > > > Reviewed-by: Liviu Dudau <[e-mail]>
> > > > Tested-by: Suravee Suthikulpanit <[e-mail]>
> > > > Tested-by: Jeremy Linton <[e-mail]>
> > > > Tested-by: Duc Dang <[e-mail]>
> > > > Tested-by: Dongdong Liu <[e-mail]>
> > > > Tested-by: Hanjun Guo <[e-mail]>
> > > > Tested-by: Graeme Gregory <[e-mail]>
> > > > Tested-by: Sinan Kaya <[e-mail]>
> > > > ---
> > > >  drivers/acpi/pci_root.c  | 18 ++++++++++++++++++
> > > >  drivers/pci/pci.c        | 11 +++++++++--
> > > >  include/linux/pci-acpi.h |  2 ++
> > > >  3 files changed, 29 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
> > > > index 4581e0e..d9a70c4 100644
> > > > --- a/drivers/acpi/pci_root.c
> > > > +++ b/drivers/acpi/pci_root.c
> > > > @@ -419,6 +419,24 @@ out:
> > > >  }
> > > >  EXPORT_SYMBOL(acpi_pci_osc_control_set);
> > > >  
> > > > +int acpi_pci_bus_domain_nr(struct device *parent)
> 
> It looks like acpi_pci_bus_domain_nr() could be under #ifdef
> CONFIG_PCI_DOMAINS_GENERIC, right?

Yes it should.

> > > > +{
> > > > +	struct acpi_device *acpi_dev = to_acpi_device(parent);
> > > > +	unsigned long long segment = 0;
> > > > +	acpi_status status;
> > > > +
> > > > +	/*
> > > > +	 * If _SEG method does not exist, following ACPI spec (6.5.6)
> > > > +	 * all PCI buses belong to domain 0.
> > > > +	 */
> > > > +	status = acpi_evaluate_integer(acpi_dev->handle, METHOD_NAME__SEG, NULL,
> > > > +				       &segment);
> > > 
> > > We already have code in acpi_pci_root_add() to evaluate _SEG.  We
> > > don't want to evaluate it *twice*, do we?
> > > 
> > > I was sort of expecting that if you added it here, we'd remove the
> > > existing call, but it looks like you're keeping both?
> > 
> > We can't remove the existing call, since it is used on X86 and IA64
> > to store the segment number that, in the process, is used in their
> > pci_domain_nr() arch specific callback to retrieve the domain nr.
> > 
> > On ARM64, that selects PCI_DOMAINS_GENERIC, we have to find a way
> > to retrieve the domain number that is not arch dependent, since
> > this is generic code, we can't rely on any bus->sysdata format (unless
> > we do something like JC did below), therefore the only way is to call
> > the _SEG method *again* here, which also forced Tomasz to go through
> > the ACPI_COMPANION setting song and dance and pass the parent pointer
> > to pci_create_root_bus() (see patch 1), which BTW is a source of
> > trouble on its own as you noticed.
> > 
> > JC solved it differently, via sysdata and pseudo-generic code:
> > 
> > http://www.spinics.net/lists/arm-kernel/msg478167.html
> 
> The thing I don't like about this is the special case of checking
> parent and parent->of_node to figure out whether we should use the
> segment from ACPI and the fragility of depending on the fact that the
> companion hasn't been set yet.
> 
> > http://www.spinics.net/lists/arm-kernel/msg478169.html
> > 
> > I like neither, we need the lesser of two evils though.
> 
> Today we call pci_bus_assign_domain_nr() from the PCI core (from
> pci_create_root_bus()).  This is only implemented for
> PCI_DOMAINS_GENERIC, but even so, it fiddles around to figure out
> whether to get the domain from DT or to assign a new one.
> 
> That seems backwards to me.  The host bridge drivers already know
> where the domain should come from (ACPI _SEG, DT, etc.) and in the
> long term, I think they should be responsible for looking up or
> assigning a domain number *before* they call pci_create_root_bus().

Yes, the question still is how pci_create_root_bus() can get that
value (I am pretty certain this was heavily debated in the past, which
does not mean we can't give it another try).

> > > > +	if (ACPI_FAILURE(status) && status != AE_NOT_FOUND)
> > > > +		dev_err(&acpi_dev->dev, "can't evaluate _SEG\n");
> > > > +
> > > > +	return segment;
> > > > +}
> > > > +
> > > >  static void negotiate_os_control(struct acpi_pci_root *root, int *no_aspm)
> > > >  {
> > > >  	u32 support, control, requested;
> > > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > > > index 25e0327..1a74e87 100644
> > > > --- a/drivers/pci/pci.c
> > > > +++ b/drivers/pci/pci.c
> > > > @@ -19,6 +19,7 @@
> > > >  #include <linux/spinlock.h>
> > > >  #include <linux/string.h>
> > > >  #include <linux/log2.h>
> > > > +#include <linux/pci-acpi.h>
> > > >  #include <linux/pci-aspm.h>
> > > >  #include <linux/pm_wakeup.h>
> > > >  #include <linux/interrupt.h>
> > > > @@ -4779,7 +4780,7 @@ int pci_get_new_domain_nr(void)
> > > >  }
> > > >  
> > > >  #ifdef CONFIG_PCI_DOMAINS_GENERIC
> > > > -void pci_bus_assign_domain_nr(struct pci_bus *bus, struct device *parent)
> > > > +static int of_pci_bus_domain_nr(struct device *parent)
> > > >  {
> > > >  	static int use_dt_domains = -1;
> > > >  	int domain = -1;
> > > > @@ -4823,7 +4824,13 @@ void pci_bus_assign_domain_nr(struct pci_bus *bus, struct device *parent)
> > > >  		domain = -1;
> > > >  	}
> > > >  
> > > > -	bus->domain_nr = domain;
> > > > +	return domain;
> > > > +}
> > > > +
> > > > +void pci_bus_assign_domain_nr(struct pci_bus *bus, struct device *parent)
> > > > +{
> > > > +	bus->domain_nr = acpi_disabled ? of_pci_bus_domain_nr(parent) :
> > > > +					 acpi_pci_bus_domain_nr(parent);
> 
> We have the pci_bus * here, so to_pci_host_bridge(bus->bridge) gives
> us the struct pci_host_bridge.  I can't remember why we put domain_nr
> in the struct pci_bus instead of in the struct pci_host_bridge.  It
> seems like pci_host_bridge is the more logical place for it, because
> every bus below the host bridge must have the same domain by
> definition.
> 
> Would it be feasible to either (a) move domain_nr to the
> pci_host_bridge, or (b) change acpi_pci_bus_domain_nr() so it uses the
> struct pci_bus * or the struct device * to find the struct
> acpi_pci_root where segment has already been stored by
> acpi_pci_root_add()?

(b) is what JC implemented even though it works differently for
different hosts since it all depends on what's in bus->sysdata.

It can certainly be done in a generic way (that works on X86 and IA64
too), let's give it more thought.

> Another wrinkle is the quirk added by 1f09b09b4de0 ("x86/PCI: Ignore
> _SEG on HP xw9300").  x86 doesn't use PCI_DOMAINS_GENERIC yet, so this
> patch wouldn't break it, but I hope x86 can use PCI_DOMAINS_GENERIC in
> the future, and then it will be a problem if we evaluate _SEG again.

Yes, I share your concern here and I thought about that, if that's the
end goal let's find a solution that works across arches (or we temporarily
use JC's code and we then generalize it).

Thanks,
Lorenzo

> 
> > > >  }
> > > >  #endif
> > > >  #endif
> > > > diff --git a/include/linux/pci-acpi.h b/include/linux/pci-acpi.h
> > > > index 89ab057..a72e22d 100644
> > > > --- a/include/linux/pci-acpi.h
> > > > +++ b/include/linux/pci-acpi.h
> > > > @@ -22,6 +22,7 @@ static inline acpi_status pci_acpi_remove_pm_notifier(struct acpi_device *dev)
> > > >  {
> > > >  	return acpi_remove_pm_notifier(dev);
> > > >  }
> > > > +extern int acpi_pci_bus_domain_nr(struct device *parent);
> > > >  extern phys_addr_t acpi_pci_root_get_mcfg_addr(acpi_handle handle);
> > > >  
> > > >  static inline acpi_handle acpi_find_root_bridge_handle(struct pci_dev *pdev)
> > > > @@ -109,6 +110,7 @@ extern const u8 pci_acpi_dsm_uuid[];
> > > >  #else	/* CONFIG_ACPI */
> > > >  static inline void acpi_pci_add_bus(struct pci_bus *bus) { }
> > > >  static inline void acpi_pci_remove_bus(struct pci_bus *bus) { }
> > > > +static inline int acpi_pci_bus_domain_nr(struct device *parent) { return -1; }
> > > >  #endif	/* CONFIG_ACPI */
> > > >  
> > > >  #ifdef CONFIG_ACPI_APEI
> > > > -- 
> > > > 1.9.1
> > > > 
> > > > --
> > > > To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> > > > the body of a message to [e-mail]
> > > > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > > 
>

Liviu Dudau April 28, 2016, 8:13 a.m. UTC | #6

On Wed, Apr 27, 2016 at 06:31:29PM +0100, Lorenzo Pieralisi wrote:
> On Wed, Apr 27, 2016 at 11:44:53AM -0500, Bjorn Helgaas wrote:
> > On Wed, Apr 27, 2016 at 12:17:58PM +0100, Lorenzo Pieralisi wrote:
> > > On Tue, Apr 26, 2016 at 09:26:49PM -0500, Bjorn Helgaas wrote:
> > > > On Fri, Apr 15, 2016 at 07:06:37PM +0200, Tomasz Nowicki wrote:
> > > > > As we now have valid PCI host bridge device reference we can
> > > > > introduce code that is going to find its bus domain number using
> > > > > ACPI _SEG method.
> > > > > 
> > > > > Note that _SEG method is optional, therefore _SEG absence means
> > > > > that all PCI buses belong to domain 0.
> > > > > 
> > > > > While at it, for the sake of code clarity we put ACPI and DT domain
> > > > > assign methods into the corresponding helpers.
> > > > > 
> > > > > Signed-off-by: Tomasz Nowicki <[e-mail]>
> > > > > Reviewed-by: Liviu Dudau <[e-mail]>
> > > > > Tested-by: Suravee Suthikulpanit <[e-mail]>
> > > > > Tested-by: Jeremy Linton <[e-mail]>
> > > > > Tested-by: Duc Dang <[e-mail]>
> > > > > Tested-by: Dongdong Liu <[e-mail]>
> > > > > Tested-by: Hanjun Guo <[e-mail]>
> > > > > Tested-by: Graeme Gregory <[e-mail]>
> > > > > Tested-by: Sinan Kaya <[e-mail]>
> > > > > ---
> > > > >  drivers/acpi/pci_root.c  | 18 ++++++++++++++++++
> > > > >  drivers/pci/pci.c        | 11 +++++++++--
> > > > >  include/linux/pci-acpi.h |  2 ++
> > > > >  3 files changed, 29 insertions(+), 2 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
> > > > > index 4581e0e..d9a70c4 100644
> > > > > --- a/drivers/acpi/pci_root.c
> > > > > +++ b/drivers/acpi/pci_root.c
> > > > > @@ -419,6 +419,24 @@ out:
> > > > >  }
> > > > >  EXPORT_SYMBOL(acpi_pci_osc_control_set);
> > > > >  
> > > > > +int acpi_pci_bus_domain_nr(struct device *parent)
> > 
> > It looks like acpi_pci_bus_domain_nr() could be under #ifdef
> > CONFIG_PCI_DOMAINS_GENERIC, right?
> 
> Yes it should.
> 
> > > > > +{
> > > > > +	struct acpi_device *acpi_dev = to_acpi_device(parent);
> > > > > +	unsigned long long segment = 0;
> > > > > +	acpi_status status;
> > > > > +
> > > > > +	/*
> > > > > +	 * If _SEG method does not exist, following ACPI spec (6.5.6)
> > > > > +	 * all PCI buses belong to domain 0.
> > > > > +	 */
> > > > > +	status = acpi_evaluate_integer(acpi_dev->handle, METHOD_NAME__SEG, NULL,
> > > > > +				       &segment);
> > > > 
> > > > We already have code in acpi_pci_root_add() to evaluate _SEG.  We
> > > > don't want to evaluate it *twice*, do we?
> > > > 
> > > > I was sort of expecting that if you added it here, we'd remove the
> > > > existing call, but it looks like you're keeping both?
> > > 
> > > We can't remove the existing call, since it is used on X86 and IA64
> > > to store the segment number that, in the process, is used in their
> > > pci_domain_nr() arch specific callback to retrieve the domain nr.
> > > 
> > > On ARM64, that selects PCI_DOMAINS_GENERIC, we have to find a way
> > > to retrieve the domain number that is not arch dependent, since
> > > this is generic code, we can't rely on any bus->sysdata format (unless
> > > we do something like JC did below), therefore the only way is to call
> > > the _SEG method *again* here, which also forced Tomasz to go through
> > > the ACPI_COMPANION setting song and dance and pass the parent pointer
> > > to pci_create_root_bus() (see patch 1), which BTW is a source of
> > > trouble on its own as you noticed.
> > > 
> > > JC solved it differently, via sysdata and pseudo-generic code:
> > > 
> > > http://www.spinics.net/lists/arm-kernel/msg478167.html
> > 
> > The thing I don't like about this is the special case of checking
> > parent and parent->of_node to figure out whether we should use the
> > segment from ACPI and the fragility of depending on the fact that the
> > companion hasn't been set yet.
> > 
> > > http://www.spinics.net/lists/arm-kernel/msg478169.html
> > > 
> > > I like neither, we need the lesser of two evils though.
> > 
> > Today we call pci_bus_assign_domain_nr() from the PCI core (from
> > pci_create_root_bus()).  This is only implemented for
> > PCI_DOMAINS_GENERIC, but even so, it fiddles around to figure out
> > whether to get the domain from DT or to assign a new one.
> > 
> > That seems backwards to me.  The host bridge drivers already know
> > where the domain should come from (ACPI _SEG, DT, etc.) and in the
> > long term, I think they should be responsible for looking up or
> > assigning a domain number *before* they call pci_create_root_bus().
> 
> Yes, the question still is how pci_create_root_bus() can get that
> value (I am pretty certain this was heavily debated in the past, which
> does not mean we can't give it another try).

The main issue is that pci_create_root_bus() does a weird dance trying to
figure out if the root bus hasn't been already allocated. It allocates a
new bus, assigns a domain number and then it tries to find it in the list
of already allocated busses. Because pci_alloc_bus() does not pass any
additional information, pci_bus_assign_domain_nr() needs to try to guess
where the barely initialised bus should live and give you back a number.

Simplifying the creation of root busses to be the job of the host bridges
would greatly simplify the code as well.

Best regards,
Liviu

> 
> > > > > +	if (ACPI_FAILURE(status) && status != AE_NOT_FOUND)
> > > > > +		dev_err(&acpi_dev->dev, "can't evaluate _SEG\n");
> > > > > +
> > > > > +	return segment;
> > > > > +}
> > > > > +
> > > > >  static void negotiate_os_control(struct acpi_pci_root *root, int *no_aspm)
> > > > >  {
> > > > >  	u32 support, control, requested;
> > > > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > > > > index 25e0327..1a74e87 100644
> > > > > --- a/drivers/pci/pci.c
> > > > > +++ b/drivers/pci/pci.c
> > > > > @@ -19,6 +19,7 @@
> > > > >  #include <linux/spinlock.h>
> > > > >  #include <linux/string.h>
> > > > >  #include <linux/log2.h>
> > > > > +#include <linux/pci-acpi.h>
> > > > >  #include <linux/pci-aspm.h>
> > > > >  #include <linux/pm_wakeup.h>
> > > > >  #include <linux/interrupt.h>
> > > > > @@ -4779,7 +4780,7 @@ int pci_get_new_domain_nr(void)
> > > > >  }
> > > > >  
> > > > >  #ifdef CONFIG_PCI_DOMAINS_GENERIC
> > > > > -void pci_bus_assign_domain_nr(struct pci_bus *bus, struct device *parent)
> > > > > +static int of_pci_bus_domain_nr(struct device *parent)
> > > > >  {
> > > > >  	static int use_dt_domains = -1;
> > > > >  	int domain = -1;
> > > > > @@ -4823,7 +4824,13 @@ void pci_bus_assign_domain_nr(struct pci_bus *bus, struct device *parent)
> > > > >  		domain = -1;
> > > > >  	}
> > > > >  
> > > > > -	bus->domain_nr = domain;
> > > > > +	return domain;
> > > > > +}
> > > > > +
> > > > > +void pci_bus_assign_domain_nr(struct pci_bus *bus, struct device *parent)
> > > > > +{
> > > > > +	bus->domain_nr = acpi_disabled ? of_pci_bus_domain_nr(parent) :
> > > > > +					 acpi_pci_bus_domain_nr(parent);
> > 
> > We have the pci_bus * here, so to_pci_host_bridge(bus->bridge) gives
> > us the struct pci_host_bridge.  I can't remember why we put domain_nr
> > in the struct pci_bus instead of in the struct pci_host_bridge.  It
> > seems like pci_host_bridge is the more logical place for it, because
> > every bus below the host bridge must have the same domain by
> > definition.
> > 
> > Would it be feasible to either (a) move domain_nr to the
> > pci_host_bridge, or (b) change acpi_pci_bus_domain_nr() so it uses the
> > struct pci_bus * or the struct device * to find the struct
> > acpi_pci_root where segment has already been stored by
> > acpi_pci_root_add()?
> 
> (b) is what JC implemented even though it works differently for
> different hosts since it all depends on what's in bus->sysdata.
> 
> It can certainly be done in a generic way (that works on X86 and IA64
> too), let's give it more thought.
> 
> > Another wrinkle is the quirk added by 1f09b09b4de0 ("x86/PCI: Ignore
> > _SEG on HP xw9300").  x86 doesn't use PCI_DOMAINS_GENERIC yet, so this
> > patch wouldn't break it, but I hope x86 can use PCI_DOMAINS_GENERIC in
> > the future, and then it will be a problem if we evaluate _SEG again.
> 
> Yes, I share your concern here and I thought about that, if that's the
> end goal let's find a solution that works across arches (or we temporarily
> use JC's code and we then generalize it).
> 
> Thanks,
> Lorenzo
> 
> > 
> > > > >  }
> > > > >  #endif
> > > > >  #endif
> > > > > diff --git a/include/linux/pci-acpi.h b/include/linux/pci-acpi.h
> > > > > index 89ab057..a72e22d 100644
> > > > > --- a/include/linux/pci-acpi.h
> > > > > +++ b/include/linux/pci-acpi.h
> > > > > @@ -22,6 +22,7 @@ static inline acpi_status pci_acpi_remove_pm_notifier(struct acpi_device *dev)
> > > > >  {
> > > > >  	return acpi_remove_pm_notifier(dev);
> > > > >  }
> > > > > +extern int acpi_pci_bus_domain_nr(struct device *parent);
> > > > >  extern phys_addr_t acpi_pci_root_get_mcfg_addr(acpi_handle handle);
> > > > >  
> > > > >  static inline acpi_handle acpi_find_root_bridge_handle(struct pci_dev *pdev)
> > > > > @@ -109,6 +110,7 @@ extern const u8 pci_acpi_dsm_uuid[];
> > > > >  #else	/* CONFIG_ACPI */
> > > > >  static inline void acpi_pci_add_bus(struct pci_bus *bus) { }
> > > > >  static inline void acpi_pci_remove_bus(struct pci_bus *bus) { }
> > > > > +static inline int acpi_pci_bus_domain_nr(struct device *parent) { return -1; }
> > > > >  #endif	/* CONFIG_ACPI */
> > > > >  
> > > > >  #ifdef CONFIG_ACPI_APEI
> > > > > -- 
> > > > > 1.9.1
> > > > > 
> > > > > --
> > > > > To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> > > > > the body of a message to [e-mail]
> > > > > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > > > 
> > 
>

Bjorn Helgaas April 28, 2016, 3:12 p.m. UTC | #7

On Wed, Apr 27, 2016 at 06:31:29PM +0100, Lorenzo Pieralisi wrote:
> On Wed, Apr 27, 2016 at 11:44:53AM -0500, Bjorn Helgaas wrote:
> > On Wed, Apr 27, 2016 at 12:17:58PM +0100, Lorenzo Pieralisi wrote:
> > > On Tue, Apr 26, 2016 at 09:26:49PM -0500, Bjorn Helgaas wrote:
> > > > On Fri, Apr 15, 2016 at 07:06:37PM +0200, Tomasz Nowicki wrote:


> > Today we call pci_bus_assign_domain_nr() from the PCI core (from
> > pci_create_root_bus()).  This is only implemented for
> > PCI_DOMAINS_GENERIC, but even so, it fiddles around to figure out
> > whether to get the domain from DT or to assign a new one.
> > 
> > That seems backwards to me.  The host bridge drivers already know
> > where the domain should come from (ACPI _SEG, DT, etc.) and in the
> > long term, I think they should be responsible for looking up or
> > assigning a domain number *before* they call pci_create_root_bus().
> 
> Yes, the question still is how pci_create_root_bus() can get that
> value (I am pretty certain this was heavily debated in the past, which
> does not mean we can't give it another try).

Right, we don't have a good mechanism for passing more info into
pci_create_root_bus().  Maybe the caller could fill in a struct so we
have a chance to extend it without having to change all the existing
callers.

I wonder if there's a design pattern we can copy, e.g., would
something like the scsi_host_alloc(), scsi_add_host(),
scsi_scan_host() model work here?

> > > > > +void pci_bus_assign_domain_nr(struct pci_bus *bus, struct device *parent)
> > > > > +{
> > > > > +	bus->domain_nr = acpi_disabled ? of_pci_bus_domain_nr(parent) :
> > > > > +					 acpi_pci_bus_domain_nr(parent);
> > 
> > We have the pci_bus * here, so to_pci_host_bridge(bus->bridge) gives
> > us the struct pci_host_bridge.  I can't remember why we put domain_nr
> > in the struct pci_bus instead of in the struct pci_host_bridge.  It
> > seems like pci_host_bridge is the more logical place for it, because
> > every bus below the host bridge must have the same domain by
> > definition.
> > 
> > Would it be feasible to either (a) move domain_nr to the
> > pci_host_bridge, or (b) change acpi_pci_bus_domain_nr() so it uses the
> > struct pci_bus * or the struct device * to find the struct
> > acpi_pci_root where segment has already been stored by
> > acpi_pci_root_add()?
> 
> (b) is what JC implemented even though it works differently for
> different hosts since it all depends on what's in bus->sysdata.
> 
> It can certainly be done in a generic way (that works on X86 and IA64
> too), let's give it more thought.
> 
> > Another wrinkle is the quirk added by 1f09b09b4de0 ("x86/PCI: Ignore
> > _SEG on HP xw9300").  x86 doesn't use PCI_DOMAINS_GENERIC yet, so this
> > patch wouldn't break it, but I hope x86 can use PCI_DOMAINS_GENERIC in
> > the future, and then it will be a problem if we evaluate _SEG again.
> 
> Yes, I share your concern here and I thought about that, if that's the
> end goal let's find a solution that works across arches (or we temporarily
> use JC's code and we then generalize it).

I would ultimately like all arches to use PCI_DOMAINS_GENERIC, because
I don't think there's anything intrisically arch-specific about where
we store the domain number.  The means of discovering or assigning a
domain number might be arch-specific, but I think it would be cleanest
if the host bridge driver handled that.

Bjorn

Arnd Bergmann April 28, 2016, 3:34 p.m. UTC | #8

On Thursday 28 April 2016 10:12:12 Bjorn Helgaas wrote:
> On Wed, Apr 27, 2016 at 06:31:29PM +0100, Lorenzo Pieralisi wrote:
> > On Wed, Apr 27, 2016 at 11:44:53AM -0500, Bjorn Helgaas wrote:
> > > On Wed, Apr 27, 2016 at 12:17:58PM +0100, Lorenzo Pieralisi wrote:
> > > > On Tue, Apr 26, 2016 at 09:26:49PM -0500, Bjorn Helgaas wrote:
> > > > > On Fri, Apr 15, 2016 at 07:06:37PM +0200, Tomasz Nowicki wrote:
> 
> 
> > > Today we call pci_bus_assign_domain_nr() from the PCI core (from
> > > pci_create_root_bus()).  This is only implemented for
> > > PCI_DOMAINS_GENERIC, but even so, it fiddles around to figure out
> > > whether to get the domain from DT or to assign a new one.
> > > 
> > > That seems backwards to me.  The host bridge drivers already know
> > > where the domain should come from (ACPI _SEG, DT, etc.) and in the
> > > long term, I think they should be responsible for looking up or
> > > assigning a domain number *before* they call pci_create_root_bus().
> > 
> > Yes, the question still is how pci_create_root_bus() can get that
> > value (I am pretty certain this was heavily debated in the past, which
> > does not mean we can't give it another try).
> 
> Right, we don't have a good mechanism for passing more info into
> pci_create_root_bus().  Maybe the caller could fill in a struct so we
> have a chance to extend it without having to change all the existing
> callers.
> 
> I wonder if there's a design pattern we can copy, e.g., would
> something like the scsi_host_alloc(), scsi_add_host(),
> scsi_scan_host() model work here?

Yes, I think that is a good idea in general. Especially
now that we have separate the ARM code from pci_common_init_dev
and pci_sys_data, that can help with cleanups in the other drivers
as well.

I see two common variations in other subsystems: some use a
special alloc() function that you pass the size of the private
data into, while others just expect you to embed a structure
inside of the driver specific one allocate that separately to
have the generic registration function fill out the common fields.

I have a slight preference for the second, but they are really
the same thing basically.

	Arnd

Arnd Bergmann April 29, 2016, 10:50 p.m. UTC | #9

On Thursday 28 April 2016 17:34:10 Arnd Bergmann wrote:
> On Thursday 28 April 2016 10:12:12 Bjorn Helgaas wrote:
> > Right, we don't have a good mechanism for passing more info into
> > pci_create_root_bus().  Maybe the caller could fill in a struct so we
> > have a chance to extend it without having to change all the existing
> > callers.
> > 
> > I wonder if there's a design pattern we can copy, e.g., would
> > something like the scsi_host_alloc(), scsi_add_host(),
> > scsi_scan_host() model work here?
> 
> Yes, I think that is a good idea in general. Especially
> now that we have separate the ARM code from pci_common_init_dev
> and pci_sys_data, that can help with cleanups in the other drivers
> as well.
> 
> I see two common variations in other subsystems: some use a
> special alloc() function that you pass the size of the private
> data into, while others just expect you to embed a structure
> inside of the driver specific one allocate that separately to
> have the generic registration function fill out the common fields.
> 
> I have a slight preference for the second, but they are really
> the same thing basically.

I've tried this out now, and will follow up with a separate patch
series. Overall, I think it works out well, though I haven't gotten
to the point of actually saving code yet. I've converted two
drivers for demonstration.

	Arnd

Tomasz Nowicki May 2, 2016, 12:43 p.m. UTC | #10

On 04/27/2016 01:17 PM, Lorenzo Pieralisi wrote:
> On Tue, Apr 26, 2016 at 09:26:49PM -0500, Bjorn Helgaas wrote:
>> On Fri, Apr 15, 2016 at 07:06:37PM +0200, Tomasz Nowicki wrote:
[...]
>>   
>> +int acpi_pci_bus_domain_nr(struct device *parent)
>> +{
>> +	struct acpi_device *acpi_dev = to_acpi_device(parent);
>> +	unsigned long long segment = 0;
>> +	acpi_status status;
>> +
>> +	/*
>> +	 * If _SEG method does not exist, following ACPI spec (6.5.6)
>> +	 * all PCI buses belong to domain 0.
>> +	 */
>> +	status = acpi_evaluate_integer(acpi_dev->handle, METHOD_NAME__SEG, NULL,
>> +				       &segment);
>> We already have code in acpi_pci_root_add() to evaluate _SEG.  We
>> don't want to evaluate it *twice*, do we?
>>
>> I was sort of expecting that if you added it here, we'd remove the
>> existing call, but it looks like you're keeping both?
> We can't remove the existing call, since it is used on X86 and IA64
> to store the segment number that, in the process, is used in their
> pci_domain_nr() arch specific callback to retrieve the domain nr.
>
> On ARM64, that selects PCI_DOMAINS_GENERIC, we have to find a way
> to retrieve the domain number that is not arch dependent, since
> this is generic code, we can't rely on any bus->sysdata format (unless
> we do something like JC did below), therefore the only way is to call
> the _SEG method *again* here, which also forced Tomasz to go through
> the ACPI_COMPANION setting song and dance and pass the parent pointer
> to pci_create_root_bus() (see patch 1), which BTW is a source of
> trouble on its own as you noticed.
What trouble in patch 1 do you mean? I may miss something.

I agree that patch 1 is not necessary if we decide to use sysdata or 
rework root bus scanning to move domain to host bridge. Nevertheless, 
patch 1 is still a cleanup IMO.

Thanks,
Tomasz

Jayachandran C. May 2, 2016, 1:26 p.m. UTC | #11

On Mon, May 2, 2016 at 6:13 PM, Tomasz Nowicki <[e-mail]> wrote:
> On 04/27/2016 01:17 PM, Lorenzo Pieralisi wrote:
>>
>> On Tue, Apr 26, 2016 at 09:26:49PM -0500, Bjorn Helgaas wrote:
>>>
>>> On Fri, Apr 15, 2016 at 07:06:37PM +0200, Tomasz Nowicki wrote:
>
> [...]
>
>>>   +int acpi_pci_bus_domain_nr(struct device *parent)
>>> +{
>>> +       struct acpi_device *acpi_dev = to_acpi_device(parent);
>>> +       unsigned long long segment = 0;
>>> +       acpi_status status;
>>> +
>>> +       /*
>>> +        * If _SEG method does not exist, following ACPI spec (6.5.6)
>>> +        * all PCI buses belong to domain 0.
>>> +        */
>>> +       status = acpi_evaluate_integer(acpi_dev->handle,
>>> METHOD_NAME__SEG, NULL,
>>> +                                      &segment);
>>> We already have code in acpi_pci_root_add() to evaluate _SEG.  We
>>> don't want to evaluate it *twice*, do we?
>>>
>>> I was sort of expecting that if you added it here, we'd remove the
>>> existing call, but it looks like you're keeping both?
>>
>> We can't remove the existing call, since it is used on X86 and IA64
>> to store the segment number that, in the process, is used in their
>> pci_domain_nr() arch specific callback to retrieve the domain nr.
>>
>> On ARM64, that selects PCI_DOMAINS_GENERIC, we have to find a way
>> to retrieve the domain number that is not arch dependent, since
>> this is generic code, we can't rely on any bus->sysdata format (unless
>> we do something like JC did below), therefore the only way is to call
>> the _SEG method *again* here, which also forced Tomasz to go through
>> the ACPI_COMPANION setting song and dance and pass the parent pointer
>> to pci_create_root_bus() (see patch 1), which BTW is a source of
>> trouble on its own as you noticed.
>
> What trouble in patch 1 do you mean? I may miss something.
>
> I agree that patch 1 is not necessary if we decide to use sysdata or rework
> root bus scanning to move domain to host bridge. Nevertheless, patch 1 is
> still a cleanup IMO.

In this case, getting the domain should be trivial since the ACPI
companion on parent is already set, this should work

int acpi_pci_bus_domain_nr(struct device *parent)
{
        struct acpi_device *acpi_dev = to_acpi_device(parent);
        struct acpi_pci_root *root = acpi_dev->driver_data;

        return root->segment;
}

Or am I missing something here?

JC.

Lorenzo Pieralisi May 3, 2016, 11:02 a.m. UTC | #12

On Mon, May 02, 2016 at 06:56:13PM +0530, Jayachandran C wrote:
> On Mon, May 2, 2016 at 6:13 PM, Tomasz Nowicki <[e-mail]> wrote:
> > On 04/27/2016 01:17 PM, Lorenzo Pieralisi wrote:
> >>
> >> On Tue, Apr 26, 2016 at 09:26:49PM -0500, Bjorn Helgaas wrote:
> >>>
> >>> On Fri, Apr 15, 2016 at 07:06:37PM +0200, Tomasz Nowicki wrote:
> >
> > [...]
> >
> >>>   +int acpi_pci_bus_domain_nr(struct device *parent)
> >>> +{
> >>> +       struct acpi_device *acpi_dev = to_acpi_device(parent);
> >>> +       unsigned long long segment = 0;
> >>> +       acpi_status status;
> >>> +
> >>> +       /*
> >>> +        * If _SEG method does not exist, following ACPI spec (6.5.6)
> >>> +        * all PCI buses belong to domain 0.
> >>> +        */
> >>> +       status = acpi_evaluate_integer(acpi_dev->handle,
> >>> METHOD_NAME__SEG, NULL,
> >>> +                                      &segment);
> >>> We already have code in acpi_pci_root_add() to evaluate _SEG.  We
> >>> don't want to evaluate it *twice*, do we?
> >>>
> >>> I was sort of expecting that if you added it here, we'd remove the
> >>> existing call, but it looks like you're keeping both?
> >>
> >> We can't remove the existing call, since it is used on X86 and IA64
> >> to store the segment number that, in the process, is used in their
> >> pci_domain_nr() arch specific callback to retrieve the domain nr.
> >>
> >> On ARM64, that selects PCI_DOMAINS_GENERIC, we have to find a way
> >> to retrieve the domain number that is not arch dependent, since
> >> this is generic code, we can't rely on any bus->sysdata format (unless
> >> we do something like JC did below), therefore the only way is to call
> >> the _SEG method *again* here, which also forced Tomasz to go through
> >> the ACPI_COMPANION setting song and dance and pass the parent pointer
> >> to pci_create_root_bus() (see patch 1), which BTW is a source of
> >> trouble on its own as you noticed.
> >
> > What trouble in patch 1 do you mean? I may miss something.
> >
> > I agree that patch 1 is not necessary if we decide to use sysdata or rework
> > root bus scanning to move domain to host bridge. Nevertheless, patch 1 is
> > still a cleanup IMO.
> 
> In this case, getting the domain should be trivial since the ACPI
> companion on parent is already set, this should work
> 
> int acpi_pci_bus_domain_nr(struct device *parent)
> {
>         struct acpi_device *acpi_dev = to_acpi_device(parent);
>         struct acpi_pci_root *root = acpi_dev->driver_data;
> 
>         return root->segment;
> }
> 
> Or am I missing something here?

Well, I thought that the whole idea behind this exercise was to move
the domain number into struct pci_host_bridge (Arnd did not do it with
his first set but this does not mean we can't add it as Bjorn suggested),
so that the domain number could be read from there straight away in an
arch (and FW) independent manner, right ?

Lorenzo

Jayachandran C. May 3, 2016, 2:22 p.m. UTC | #13

On Tue, May 3, 2016 at 4:32 PM, Lorenzo Pieralisi
<[e-mail]> wrote:
> On Mon, May 02, 2016 at 06:56:13PM +0530, Jayachandran C wrote:
>> On Mon, May 2, 2016 at 6:13 PM, Tomasz Nowicki <[e-mail]> wrote:
>> > On 04/27/2016 01:17 PM, Lorenzo Pieralisi wrote:
>> >>
>> >> On Tue, Apr 26, 2016 at 09:26:49PM -0500, Bjorn Helgaas wrote:
>> >>>
>> >>> On Fri, Apr 15, 2016 at 07:06:37PM +0200, Tomasz Nowicki wrote:
>> >
>> > [...]
>> >
>> >>>   +int acpi_pci_bus_domain_nr(struct device *parent)
>> >>> +{
>> >>> +       struct acpi_device *acpi_dev = to_acpi_device(parent);
>> >>> +       unsigned long long segment = 0;
>> >>> +       acpi_status status;
>> >>> +
>> >>> +       /*
>> >>> +        * If _SEG method does not exist, following ACPI spec (6.5.6)
>> >>> +        * all PCI buses belong to domain 0.
>> >>> +        */
>> >>> +       status = acpi_evaluate_integer(acpi_dev->handle,
>> >>> METHOD_NAME__SEG, NULL,
>> >>> +                                      &segment);
>> >>> We already have code in acpi_pci_root_add() to evaluate _SEG.  We
>> >>> don't want to evaluate it *twice*, do we?
>> >>>
>> >>> I was sort of expecting that if you added it here, we'd remove the
>> >>> existing call, but it looks like you're keeping both?
>> >>
>> >> We can't remove the existing call, since it is used on X86 and IA64
>> >> to store the segment number that, in the process, is used in their
>> >> pci_domain_nr() arch specific callback to retrieve the domain nr.
>> >>
>> >> On ARM64, that selects PCI_DOMAINS_GENERIC, we have to find a way
>> >> to retrieve the domain number that is not arch dependent, since
>> >> this is generic code, we can't rely on any bus->sysdata format (unless
>> >> we do something like JC did below), therefore the only way is to call
>> >> the _SEG method *again* here, which also forced Tomasz to go through
>> >> the ACPI_COMPANION setting song and dance and pass the parent pointer
>> >> to pci_create_root_bus() (see patch 1), which BTW is a source of
>> >> trouble on its own as you noticed.
>> >
>> > What trouble in patch 1 do you mean? I may miss something.
>> >
>> > I agree that patch 1 is not necessary if we decide to use sysdata or rework
>> > root bus scanning to move domain to host bridge. Nevertheless, patch 1 is
>> > still a cleanup IMO.
>>
>> In this case, getting the domain should be trivial since the ACPI
>> companion on parent is already set, this should work
>>
>> int acpi_pci_bus_domain_nr(struct device *parent)
>> {
>>         struct acpi_device *acpi_dev = to_acpi_device(parent);
>>         struct acpi_pci_root *root = acpi_dev->driver_data;
>>
>>         return root->segment;
>> }
>>
>> Or am I missing something here?
>
> Well, I thought that the whole idea behind this exercise was to move
> the domain number into struct pci_host_bridge (Arnd did not do it with
> his first set but this does not mean we can't add it as Bjorn suggested),
> so that the domain number could be read from there straight away in an
> arch (and FW) independent manner, right ?

The original issue was using _SEG call again instead of using
the value from acpi_pci_root, and the solution for that problem
is very simple.

The pci host bridge work is of course very useful, but creating a
dependency with that work for an issue that can be so easily solved
is unnecessary.

JC.

Lorenzo Pieralisi May 3, 2016, 2:55 p.m. UTC | #14

On Tue, May 03, 2016 at 07:52:52PM +0530, Jayachandran C wrote:
> On Tue, May 3, 2016 at 4:32 PM, Lorenzo Pieralisi
> <[e-mail]> wrote:
> > On Mon, May 02, 2016 at 06:56:13PM +0530, Jayachandran C wrote:
> >> On Mon, May 2, 2016 at 6:13 PM, Tomasz Nowicki <[e-mail]> wrote:
> >> > On 04/27/2016 01:17 PM, Lorenzo Pieralisi wrote:
> >> >>
> >> >> On Tue, Apr 26, 2016 at 09:26:49PM -0500, Bjorn Helgaas wrote:
> >> >>>
> >> >>> On Fri, Apr 15, 2016 at 07:06:37PM +0200, Tomasz Nowicki wrote:
> >> >
> >> > [...]
> >> >
> >> >>>   +int acpi_pci_bus_domain_nr(struct device *parent)
> >> >>> +{
> >> >>> +       struct acpi_device *acpi_dev = to_acpi_device(parent);
> >> >>> +       unsigned long long segment = 0;
> >> >>> +       acpi_status status;
> >> >>> +
> >> >>> +       /*
> >> >>> +        * If _SEG method does not exist, following ACPI spec (6.5.6)
> >> >>> +        * all PCI buses belong to domain 0.
> >> >>> +        */
> >> >>> +       status = acpi_evaluate_integer(acpi_dev->handle,
> >> >>> METHOD_NAME__SEG, NULL,
> >> >>> +                                      &segment);
> >> >>> We already have code in acpi_pci_root_add() to evaluate _SEG.  We
> >> >>> don't want to evaluate it *twice*, do we?
> >> >>>
> >> >>> I was sort of expecting that if you added it here, we'd remove the
> >> >>> existing call, but it looks like you're keeping both?
> >> >>
> >> >> We can't remove the existing call, since it is used on X86 and IA64
> >> >> to store the segment number that, in the process, is used in their
> >> >> pci_domain_nr() arch specific callback to retrieve the domain nr.
> >> >>
> >> >> On ARM64, that selects PCI_DOMAINS_GENERIC, we have to find a way
> >> >> to retrieve the domain number that is not arch dependent, since
> >> >> this is generic code, we can't rely on any bus->sysdata format (unless
> >> >> we do something like JC did below), therefore the only way is to call
> >> >> the _SEG method *again* here, which also forced Tomasz to go through
> >> >> the ACPI_COMPANION setting song and dance and pass the parent pointer
> >> >> to pci_create_root_bus() (see patch 1), which BTW is a source of
> >> >> trouble on its own as you noticed.
> >> >
> >> > What trouble in patch 1 do you mean? I may miss something.
> >> >
> >> > I agree that patch 1 is not necessary if we decide to use sysdata or rework
> >> > root bus scanning to move domain to host bridge. Nevertheless, patch 1 is
> >> > still a cleanup IMO.
> >>
> >> In this case, getting the domain should be trivial since the ACPI
> >> companion on parent is already set, this should work
> >>
> >> int acpi_pci_bus_domain_nr(struct device *parent)
> >> {
> >>         struct acpi_device *acpi_dev = to_acpi_device(parent);
> >>         struct acpi_pci_root *root = acpi_dev->driver_data;
> >>
> >>         return root->segment;
> >> }
> >>
> >> Or am I missing something here?
> >
> > Well, I thought that the whole idea behind this exercise was to move
> > the domain number into struct pci_host_bridge (Arnd did not do it with
> > his first set but this does not mean we can't add it as Bjorn suggested),
> > so that the domain number could be read from there straight away in an
> > arch (and FW) independent manner, right ?
> 
> The original issue was using _SEG call again instead of using
> the value from acpi_pci_root, and the solution for that problem
> is very simple.
> 
> The pci host bridge work is of course very useful, but creating a
> dependency with that work for an issue that can be so easily solved
> is unnecessary.

It can be easily solved if we do not drop patch 1 (I understood the
additional call to _SEG was only part of the problem). If we keep patch
1 the code above is fine by me, if we have to drop patch 1 (IIRC by
passing the parent pointer to pci_create_root_bus() we are affecting
IA64 and X86 code, if that's acceptable that's fine by me) I do not
think we can use the code above anymore, that's what my comment was
all about because Bjorn was concerned about the fragility of the code
setting the ACPI companion.

Lorenzo

Patch

diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
index 4581e0e..d9a70c4 100644
--- a/drivers/acpi/pci_root.c
+++ b/drivers/acpi/pci_root.c
@@ -419,6 +419,24 @@  out:
 }
 EXPORT_SYMBOL(acpi_pci_osc_control_set);
 
+int acpi_pci_bus_domain_nr(struct device *parent)
+{
+	struct acpi_device *acpi_dev = to_acpi_device(parent);
+	unsigned long long segment = 0;
+	acpi_status status;
+
+	/*
+	 * If _SEG method does not exist, following ACPI spec (6.5.6)
+	 * all PCI buses belong to domain 0.
+	 */
+	status = acpi_evaluate_integer(acpi_dev->handle, METHOD_NAME__SEG, NULL,
+				       &segment);
+	if (ACPI_FAILURE(status) && status != AE_NOT_FOUND)
+		dev_err(&acpi_dev->dev, "can't evaluate _SEG\n");
+
+	return segment;
+}
+
 static void negotiate_os_control(struct acpi_pci_root *root, int *no_aspm)
 {
 	u32 support, control, requested;
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 25e0327..1a74e87 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -19,6 +19,7 @@ 
 #include <linux/spinlock.h>
 #include <linux/string.h>
 #include <linux/log2.h>
+#include <linux/pci-acpi.h>
 #include <linux/pci-aspm.h>
 #include <linux/pm_wakeup.h>
 #include <linux/interrupt.h>
@@ -4779,7 +4780,7 @@  int pci_get_new_domain_nr(void)
 }
 
 #ifdef CONFIG_PCI_DOMAINS_GENERIC
-void pci_bus_assign_domain_nr(struct pci_bus *bus, struct device *parent)
+static int of_pci_bus_domain_nr(struct device *parent)
 {
 	static int use_dt_domains = -1;
 	int domain = -1;
@@ -4823,7 +4824,13 @@  void pci_bus_assign_domain_nr(struct pci_bus *bus, struct device *parent)
 		domain = -1;
 	}
 
-	bus->domain_nr = domain;
+	return domain;
+}
+
+void pci_bus_assign_domain_nr(struct pci_bus *bus, struct device *parent)
+{
+	bus->domain_nr = acpi_disabled ? of_pci_bus_domain_nr(parent) :
+					 acpi_pci_bus_domain_nr(parent);
 }
 #endif
 #endif
diff --git a/include/linux/pci-acpi.h b/include/linux/pci-acpi.h
index 89ab057..a72e22d 100644
--- a/include/linux/pci-acpi.h
+++ b/include/linux/pci-acpi.h
@@ -22,6 +22,7 @@  static inline acpi_status pci_acpi_remove_pm_notifier(struct acpi_device *dev)
 {
 	return acpi_remove_pm_notifier(dev);
 }
+extern int acpi_pci_bus_domain_nr(struct device *parent);
 extern phys_addr_t acpi_pci_root_get_mcfg_addr(acpi_handle handle);
 
 static inline acpi_handle acpi_find_root_bridge_handle(struct pci_dev *pdev)
@@ -109,6 +110,7 @@  extern const u8 pci_acpi_dsm_uuid[];
 #else	/* CONFIG_ACPI */
 static inline void acpi_pci_add_bus(struct pci_bus *bus) { }
 static inline void acpi_pci_remove_bus(struct pci_bus *bus) { }
+static inline int acpi_pci_bus_domain_nr(struct device *parent) { return -1; }
 #endif	/* CONFIG_ACPI */
 
 #ifdef CONFIG_ACPI_APEI