| Message ID | 1455630825-27253-8-git-send-email-tn@semihalf.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Tomasz, Lorenzo, On Tue, Feb 16, 2016 at 7:23 PM, Tomasz Nowicki <tn@semihalf.com> 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. In my patchset, I had a slightly different and I think better approach for this without calling the _SEG method again. Please see http://www.spinics.net/lists/arm-kernel/msg478167.html at the last part of http://www.spinics.net/lists/arm-kernel/msg478169.html JC.
On 17.02.2016 14:44, Jayachandran Chandrashekaran Nair wrote: > Tomasz, Lorenzo, > > On Tue, Feb 16, 2016 at 7:23 PM, Tomasz Nowicki<tn@semihalf.com> 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. > In my patchset, I had a slightly different and I think better approach for > this without calling the _SEG method again. Please see > http://www.spinics.net/lists/arm-kernel/msg478167.html > at the last part ofhttp://www.spinics.net/lists/arm-kernel/msg478169.html Relying on NULL parent device to make decision on boot method is really ugly way. This may hit us again once we want to obtain another firmware specific info e.g. numa node. IMO we need to fix it this way. Tomasz
On Wed, Feb 17, 2016 at 7:37 PM, Tomasz Nowicki <tn@semihalf.com> wrote: > On 17.02.2016 14:44, Jayachandran Chandrashekaran Nair wrote: >> >> Tomasz, Lorenzo, >> >> On Tue, Feb 16, 2016 at 7:23 PM, Tomasz Nowicki<tn@semihalf.com> 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. >> >> In my patchset, I had a slightly different and I think better approach for >> this without calling the _SEG method again. Please see >> http://www.spinics.net/lists/arm-kernel/msg478167.html >> at the last part ofhttp://www.spinics.net/lists/arm-kernel/msg478169.html > > Relying on NULL parent device to make decision on boot method is really ugly > way. This may hit us again once we want to obtain another firmware specific > info e.g. numa node. IMO we need to fix it this way. I am not relying on NULL there, in the current code parent is NULL in case of ACPI, and the check is needed not to crash (unless that has changed). The main part was the macro acpi_pci_get_segment() and the use of acpi_pci_root_info from sysdata to do this. JC.
On 17.02.2016 15:21, Jayachandran Chandrashekaran Nair wrote: > On Wed, Feb 17, 2016 at 7:37 PM, Tomasz Nowicki <tn@semihalf.com> wrote: >> On 17.02.2016 14:44, Jayachandran Chandrashekaran Nair wrote: >>> >>> Tomasz, Lorenzo, >>> >>> On Tue, Feb 16, 2016 at 7:23 PM, Tomasz Nowicki<tn@semihalf.com> 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. >>> >>> In my patchset, I had a slightly different and I think better approach for >>> this without calling the _SEG method again. Please see >>> http://www.spinics.net/lists/arm-kernel/msg478167.html >>> at the last part ofhttp://www.spinics.net/lists/arm-kernel/msg478169.html >> >> Relying on NULL parent device to make decision on boot method is really ugly >> way. This may hit us again once we want to obtain another firmware specific >> info e.g. numa node. IMO we need to fix it this way. > > I am not relying on NULL there, in the current code parent is NULL > in case of ACPI, and the check is needed not to crash (unless that > has changed). This series passes down valid parent, see [PATCH V5 06/15]. > > The main part was the macro acpi_pci_get_segment() and the use > of acpi_pci_root_info from sysdata to do this. Since we can obtain related firmware specific data from valid parent device (without defining another accessors), I do not see the point to use sysdata. Let me know your opinion. Tomasz
On Wed, Feb 17, 2016 at 8:35 PM, Tomasz Nowicki <tn@semihalf.com> wrote: > On 17.02.2016 15:21, Jayachandran Chandrashekaran Nair wrote: >> >> On Wed, Feb 17, 2016 at 7:37 PM, Tomasz Nowicki <tn@semihalf.com> wrote: >>> >>> On 17.02.2016 14:44, Jayachandran Chandrashekaran Nair wrote: >>>> >>>> >>>> Tomasz, Lorenzo, >>>> >>>> On Tue, Feb 16, 2016 at 7:23 PM, Tomasz Nowicki<tn@semihalf.com> 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. >>>> >>>> >>>> In my patchset, I had a slightly different and I think better approach >>>> for >>>> this without calling the _SEG method again. Please see >>>> http://www.spinics.net/lists/arm-kernel/msg478167.html >>>> at the last part >>>> ofhttp://www.spinics.net/lists/arm-kernel/msg478169.html >>> >>> >>> Relying on NULL parent device to make decision on boot method is really >>> ugly >>> way. This may hit us again once we want to obtain another firmware >>> specific >>> info e.g. numa node. IMO we need to fix it this way. >> >> >> I am not relying on NULL there, in the current code parent is NULL >> in case of ACPI, and the check is needed not to crash (unless that >> has changed). > > > This series passes down valid parent, see [PATCH V5 06/15]. > >> >> The main part was the macro acpi_pci_get_segment() and the use >> of acpi_pci_root_info from sysdata to do this. > > > Since we can obtain related firmware specific data from valid parent device > (without defining another accessors), I do not see the point to use sysdata. > Let me know your opinion. In the patch, you use the parent info and call _SEG method again. The segment information is available in the ->root->segment of acpi_pci_root_info if you setup the sysdata like in my patch JC.
On 17.02.2016 16:21, Jayachandran Chandrashekaran Nair wrote: > On Wed, Feb 17, 2016 at 8:35 PM, Tomasz Nowicki <tn@semihalf.com> wrote: >> On 17.02.2016 15:21, Jayachandran Chandrashekaran Nair wrote: >>> >>> On Wed, Feb 17, 2016 at 7:37 PM, Tomasz Nowicki <tn@semihalf.com> wrote: >>>> >>>> On 17.02.2016 14:44, Jayachandran Chandrashekaran Nair wrote: >>>>> >>>>> >>>>> Tomasz, Lorenzo, >>>>> >>>>> On Tue, Feb 16, 2016 at 7:23 PM, Tomasz Nowicki<tn@semihalf.com> 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. >>>>> >>>>> >>>>> In my patchset, I had a slightly different and I think better approach >>>>> for >>>>> this without calling the _SEG method again. Please see >>>>> http://www.spinics.net/lists/arm-kernel/msg478167.html >>>>> at the last part >>>>> ofhttp://www.spinics.net/lists/arm-kernel/msg478169.html >>>> >>>> >>>> Relying on NULL parent device to make decision on boot method is really >>>> ugly >>>> way. This may hit us again once we want to obtain another firmware >>>> specific >>>> info e.g. numa node. IMO we need to fix it this way. >>> >>> >>> I am not relying on NULL there, in the current code parent is NULL >>> in case of ACPI, and the check is needed not to crash (unless that >>> has changed). >> >> >> This series passes down valid parent, see [PATCH V5 06/15]. >> >>> >>> The main part was the macro acpi_pci_get_segment() and the use >>> of acpi_pci_root_info from sysdata to do this. >> >> >> Since we can obtain related firmware specific data from valid parent device >> (without defining another accessors), I do not see the point to use sysdata. >> Let me know your opinion. > > In the patch, you use the parent info and call _SEG method again. > The segment information is available in the ->root->segment of > acpi_pci_root_info if you setup the sysdata like in my patch I know it is in sysdata->root->segment, but the way it is passed down is wrong. sysdata is the pointer to unknown content (void *) so we need to validate it before we can use it. If we merge this patch we can remove first _SEG call. Tomasz
Guys, On Wed, Feb 17, 2016 at 04:35:30PM +0100, Tomasz Nowicki wrote: [...] > >>>>>In my patchset, I had a slightly different and I think better approach > >>>>>for > >>>>>this without calling the _SEG method again. Please see > >>>>>http://www.spinics.net/lists/arm-kernel/msg478167.html > >>>>>at the last part > >>>>>ofhttp://www.spinics.net/lists/arm-kernel/msg478169.html > >>>> > >>>> > >>>>Relying on NULL parent device to make decision on boot method is really > >>>>ugly > >>>>way. This may hit us again once we want to obtain another firmware > >>>>specific > >>>>info e.g. numa node. IMO we need to fix it this way. > >>> > >>> > >>>I am not relying on NULL there, in the current code parent is NULL > >>>in case of ACPI, and the check is needed not to crash (unless that > >>>has changed). > >> > >> > >>This series passes down valid parent, see [PATCH V5 06/15]. > >> > >>> > >>>The main part was the macro acpi_pci_get_segment() and the use > >>>of acpi_pci_root_info from sysdata to do this. > >> > >> > >>Since we can obtain related firmware specific data from valid parent device > >>(without defining another accessors), I do not see the point to use sysdata. > >>Let me know your opinion. > > > >In the patch, you use the parent info and call _SEG method again. > >The segment information is available in the ->root->segment of > >acpi_pci_root_info if you setup the sysdata like in my patch > > I know it is in sysdata->root->segment, but the way it is passed > down is wrong. sysdata is the pointer to unknown content (void *) so > we need to validate it before we can use it. If we merge this patch > we can remove first _SEG call. I personally do not think there is such a significant difference, both solutions have pros and cons, it is worth keeping in mind though that reading _SEG again to set the bus domain number works only if the value we stash in acpi_pci_root.segment is not overridden, if it is (ie see x86 - agreed that's to fix a FW bug) we have a disconnect. On the other hand Tomasz's code allows removing some IA64 code in the process (code that sets the bridge companion, so part of the patch should be kept regardless). So, there are two things to do: - Assign the bridge companion in PCI core code - Decide where to get the domain number from (acpi_pci_root.segment vs calling _SEG again). At present they are equivalent so I do not see any compelling reason to change this patch. Side note: there is already a function (pci_domain_nr()) that you can implement in ACPI PCI host generic (by deselecting PCI_DOMAINS_GENERIC if ACPI) so there is no need for acpi_pci_get_segment() in case we have to override _SEG value in the future, at present there is no need, comments appreciated. Lorenzo
diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c index c2bd6dd..3b284dc 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 602eb42..d6c768e 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> @@ -4769,7 +4770,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 = of_get_pci_domain_nr(parent->of_node); @@ -4811,7 +4812,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 94d8f38..b4f87ba9 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) @@ -143,6 +144,7 @@ extern struct list_head pci_mmcfg_list; #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