Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1314397 > unrolled thread

Re: [PATCH V3 13/21] pci, acpi: Provide generic way to assign bus domain number.

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH V3 13/21] pci, acpi: Provide generic way to assign bus  domain number. Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> - 2016-01-21 19:30 +0100
    Re: [PATCH V3 13/21] pci, acpi: Provide generic way to assign bus  domain number. Tomasz Nowicki <tn@semihalf.com> - 2016-01-21 19:50 +0100
      Re: [PATCH V3 13/21] pci, acpi: Provide generic way to assign bus  domain number. Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> - 2016-01-22 12:30 +0100

#1314397 — Re: [PATCH V3 13/21] pci, acpi: Provide generic way to assign bus domain number.

Hi Tomasz,

On Wed, Jan 13, 2016 at 02:20:59PM +0100, Tomasz Nowicki wrote:

[...]

> @@ -4796,14 +4797,34 @@ void pci_bus_assign_domain_nr(struct pci_bus *bus, struct device *parent)
>  	 * API and update the use_dt_domains value to keep track of method we
>  	 * are using to assign domain numbers (use_dt_domains = 0).
>  	 *
> +	 * IF ACPI, we expect non-DT method (use_dt_domains == -1)
> +	 * and call _SEG method for corresponding host bridge device.
> +	 * If _SEG method does not exist, following ACPI spec (6.5.6)
> +	 * all PCI buses belong to domain 0.
> +	 *
>  	 * All other combinations imply we have a platform that is trying
> -	 * to mix domain numbers obtained from DT and pci_get_new_domain_nr(),
> -	 * which is a recipe for domain mishandling and it is prevented by
> -	 * invalidating the domain value (domain = -1) and printing a
> -	 * corresponding error.
> +	 * to mix domain numbers obtained from DT, ACPI and
> +	 * pci_get_new_domain_nr(), which is a recipe for domain mishandling and
> +	 * it is prevented by invalidating the domain value (domain = -1) and
> +	 * printing a corresponding error.
>  	 */
> +
>  	if (domain >= 0 && use_dt_domains) {
>  		use_dt_domains = 1;
> +#ifdef CONFIG_ACPI
> +	} else if (!acpi_disabled && use_dt_domains == -1) {
> +		struct acpi_device *acpi_dev = to_acpi_device(parent);
> +		unsigned long long segment = 0;
> +		acpi_status status;
> +
> +		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");
> +
> +		domain = segment;
> +#endif

I think you can reshuffle a bit the code to make it easier to follow.

How about this ? (on top of mainline, I just compiled it do not
take it verbatim):

-- >8 --
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index d1a7105..467a316 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -7,6 +7,7 @@
  *	Copyright 1997 -- 2000 Martin Mares <mj@ucw.cz>
  */
 
+#include <linux/acpi.h>
 #include <linux/kernel.h>
 #include <linux/delay.h>
 #include <linux/init.h>
@@ -4769,7 +4770,27 @@ 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)
+/* Feel free to add this to drivers/acpi as a helper */
+#ifdef CONFIG_ACPI
+int pci_bus_acpi_domain_nr(struct device *parent)
+{
+	struct acpi_device *acpi_dev = to_acpi_device(parent);
+	unsigned long long segment = 0;
+	acpi_status status;
+
+	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;
+}
+#else
+int pci_bus_acpi_domain_nr(struct device *parent) { return -1; }
+#endif
+
+int pci_bus_of_domain_nr(struct device *parent)
 {
 	static int use_dt_domains = -1;
 	int domain = of_get_pci_domain_nr(parent->of_node);
@@ -4811,7 +4832,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 ? pci_bus_of_domain_nr(parent) :
+					 pci_bus_acpi_domain_nr(parent);
 }
 #endif
 #endif

[toc] | [next] | [standalone]

#1314410

On 21.01.2016 19:22, Lorenzo Pieralisi wrote:
> Hi Tomasz,
>
> On Wed, Jan 13, 2016 at 02:20:59PM +0100, Tomasz Nowicki wrote:
>
> [...]
>
>> @@ -4796,14 +4797,34 @@ void pci_bus_assign_domain_nr(struct pci_bus *bus, struct device *parent)
>>   	 * API and update the use_dt_domains value to keep track of method we
>>   	 * are using to assign domain numbers (use_dt_domains = 0).
>>   	 *
>> +	 * IF ACPI, we expect non-DT method (use_dt_domains == -1)
>> +	 * and call _SEG method for corresponding host bridge device.
>> +	 * If _SEG method does not exist, following ACPI spec (6.5.6)
>> +	 * all PCI buses belong to domain 0.
>> +	 *
>>   	 * All other combinations imply we have a platform that is trying
>> -	 * to mix domain numbers obtained from DT and pci_get_new_domain_nr(),
>> -	 * which is a recipe for domain mishandling and it is prevented by
>> -	 * invalidating the domain value (domain = -1) and printing a
>> -	 * corresponding error.
>> +	 * to mix domain numbers obtained from DT, ACPI and
>> +	 * pci_get_new_domain_nr(), which is a recipe for domain mishandling and
>> +	 * it is prevented by invalidating the domain value (domain = -1) and
>> +	 * printing a corresponding error.
>>   	 */
>> +
>>   	if (domain >= 0 && use_dt_domains) {
>>   		use_dt_domains = 1;
>> +#ifdef CONFIG_ACPI
>> +	} else if (!acpi_disabled && use_dt_domains == -1) {
>> +		struct acpi_device *acpi_dev = to_acpi_device(parent);
>> +		unsigned long long segment = 0;
>> +		acpi_status status;
>> +
>> +		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");
>> +
>> +		domain = segment;
>> +#endif
>
> I think you can reshuffle a bit the code to make it easier to follow.
>
> How about this ? (on top of mainline, I just compiled it do not
> take it verbatim):
>
> -- >8 --
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index d1a7105..467a316 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -7,6 +7,7 @@
>    *	Copyright 1997 -- 2000 Martin Mares <mj@ucw.cz>
>    */
>
> +#include <linux/acpi.h>
>   #include <linux/kernel.h>
>   #include <linux/delay.h>
>   #include <linux/init.h>
> @@ -4769,7 +4770,27 @@ 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)
> +/* Feel free to add this to drivers/acpi as a helper */
> +#ifdef CONFIG_ACPI
> +int pci_bus_acpi_domain_nr(struct device *parent)
> +{
> +	struct acpi_device *acpi_dev = to_acpi_device(parent);
> +	unsigned long long segment = 0;
> +	acpi_status status;
> +
> +	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;
> +}
> +#else
> +int pci_bus_acpi_domain_nr(struct device *parent) { return -1; }
> +#endif
> +
> +int pci_bus_of_domain_nr(struct device *parent)
>   {
>   	static int use_dt_domains = -1;
>   	int domain = of_get_pci_domain_nr(parent->of_node);
> @@ -4811,7 +4832,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 ? pci_bus_of_domain_nr(parent) :
> +					 pci_bus_acpi_domain_nr(parent);
>   }
>   #endif
>   #endif
>

It looks nice, I will integrate your patch. Thanks!

Tomasz

[toc] | [prev] | [next] | [standalone]

#1314915

On Thu, Jan 21, 2016 at 07:38:51PM +0100, Tomasz Nowicki wrote:

[...]

> >I think you can reshuffle a bit the code to make it easier to follow.
> >
> >How about this ? (on top of mainline, I just compiled it do not
> >take it verbatim):
> >
> >-- >8 --
> >diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> >index d1a7105..467a316 100644
> >--- a/drivers/pci/pci.c
> >+++ b/drivers/pci/pci.c
> >@@ -7,6 +7,7 @@
> >   *	Copyright 1997 -- 2000 Martin Mares <mj@ucw.cz>
> >   */
> >
> >+#include <linux/acpi.h>
> >  #include <linux/kernel.h>
> >  #include <linux/delay.h>
> >  #include <linux/init.h>
> >@@ -4769,7 +4770,27 @@ 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)
> >+/* Feel free to add this to drivers/acpi as a helper */
> >+#ifdef CONFIG_ACPI
> >+int pci_bus_acpi_domain_nr(struct device *parent)
> >+{
> >+	struct acpi_device *acpi_dev = to_acpi_device(parent);
> >+	unsigned long long segment = 0;
> >+	acpi_status status;
> >+
> >+	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;
> >+}
> >+#else
> >+int pci_bus_acpi_domain_nr(struct device *parent) { return -1; }
> >+#endif
> >+
> >+int pci_bus_of_domain_nr(struct device *parent)
> >  {
> >  	static int use_dt_domains = -1;
> >  	int domain = of_get_pci_domain_nr(parent->of_node);
> >@@ -4811,7 +4832,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 ? pci_bus_of_domain_nr(parent) :
> >+					 pci_bus_acpi_domain_nr(parent);
> >  }
> >  #endif
> >  #endif
> >
> 
> It looks nice, I will integrate your patch. Thanks!

While at it, as I mentioned, please move the ACPI helper function to
drivers/acpi, thanks.

Lorenzo

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web