Re: [PATCH V5 07/15] pci, acpi: Provide generic way to assign bus domain number.

[Lorenzo Pieralisi]

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