On Tue, Feb 07, 2017 at 03:46:45PM -0600, Bjorn Helgaas wrote:
> On Mon, Feb 06, 2017 at 02:08:27PM +0000, Lorenzo Pieralisi wrote:
> > On Fri, Feb 03, 2017 at 02:45:30PM -0600, Bjorn Helgaas wrote:
> > > [+cc Tomasz, Lorenzo]
> > >
> > > On Thu, Jan 12, 2017 at 02:28:24PM +0800, Dongdong Liu wrote:
> > > > arch/arm64/pci.c pcibios_alloc_irq() has the same function as
> > > > pci_fixup_irqs(), so we add condition #ifndef CONFIG_ARM64 for
> > > > pci_fixup_irqs().
> > > >
> > > > Signed-off-by: Dongdong Liu <liudongdong3@xxxxxxxxxx>
> > > > Reviewed-by: Gabriele Paoloni <gabriele.paoloni@xxxxxxxxxx>
> > > > Reviewed-by: Zhou Wang <wangzhou1@xxxxxxxxxxxxx>
> > > > ---
> > > > drivers/pci/host/pci-host-common.c | 2 ++
> > > > 1 file changed, 2 insertions(+)
> > > >
> > > > diff --git a/drivers/pci/host/pci-host-common.c b/drivers/pci/host/pci-host-common.c
> > > > index e3c48b5..f160afc 100644
> > > > --- a/drivers/pci/host/pci-host-common.c
> > > > +++ b/drivers/pci/host/pci-host-common.c
> > > > @@ -145,7 +145,9 @@ int pci_host_common_probe(struct platform_device *pdev,
> > > > return -ENODEV;
> > > > }
> > > >
> > > > +#ifndef CONFIG_ARM64
> > > > pci_fixup_irqs(pci_common_swizzle, of_irq_parse_and_map_pci);
> > > > +#endif
> > >
> > > d8ed75d59332 ("ARM64: PCI: ACPI support for legacy IRQs parsing and
> > > consolidation with DT code") added pcibios_alloc_irq() for arm64.
> > >
> > > arm64 is the only arch that implements pcibios_alloc_irq(). And now
> > > you want to add an ifdef here so arm64 is the only arch that doesn't
> > > call pci_fixup_irqs().
> > >
> > > I don't remember the details of why arm64 is so special here.
> > > Obviously we'd prefer not to have the ifdef and not to have the
> > > arm64-specific pcibios_alloc_irq().
> >
> > Well, I am not sure ARM64 is more special than other architectures
> > in this respect, actually I think that what ARM64 does is what we
> > will end up doing when Matthew Minter's patches are merged and
> > that's my aim for v4.12, pci_fixup_irqs() should not be used
> > any longer, at least on ARM/ARM64 and I know what to do to make
> > it disappear.
>
> ARM64 is different from x86, even though both arches support both ACPI
> and DT: on ARM64 we currently call of_irq_parse_and_map_pci() when we
> call the driver's probe() method, whereas on x86 we don't call it
> until the driver calls pci_enable_device().
I can move the of_irq_parse_and_map_pci() call (along with the
acpi_pci_irq_enable() counterpart - that unfortunately has a
different behaviour because it _modifies_ the struct pci_dev*
pointer passed to it) at pci_enable_device() time, that's what
Matt's patchset was doing, the risk is always triggering
regressions by moving things around.
I suspect you want to get rid of pcibios_alloc_irq(), I will make
that part of the series I will post on top of Matt's code.
On top of that, through copy'n'paste pci_fixup_irqs() is spreading
like plague, it has to be removed, at least on all ARM host controllers.
> Obviously it would be better if this were done at the same point on
> both arches. I don't know what that point *is*. I could imagine
> enumeration-time, probe-time, or enable-time.
>
> OK, back to the patch at hand. I think we're agreed that having
> pci_host_common_probe() call pci_fixup_irqs() is wrong on all
> architectures (it doesn't cover hot-added devices and it may clobber
> an IRQ after a driver has claimed the device (since it "fixes" all PCI
> devices). And we hope to fix this in the next cycle using Matt's
> work.
>
> So I think I'm OK with this transient ugliness. Although
> pci-host-common.c is currently only available on ARM and ARM64, in
> theory, it should be usable on x86. I would probably use "#ifdef ARM"
> instead of "#ifndef ARM64" so that when it is made available on x86,
> we won't call pci_fixup_irqs() there.
>
> What do you think of the following?
>
>
> commit 4e3538fb84dcba2de5d5b0990ced7f3f881fda1e
> Author: Dongdong Liu <liudongdong3@xxxxxxxxxx>
> Date: Thu Jan 12 14:28:24 2017 +0800
>
> PCI: generic: Call pci_fixup_irqs() only on ARM
>
> pci_fixup_irqs() is problematic because:
>
> - it's called when we enumerate a host bridge, so we don't fixup IRQs for
> hot-added PCI devices, and
>
> - it fixes up IRQs for all PCI devices in the system, so if we call it
> multiple times, e.g., if we have several host controllers, we may
> reallocate an IRQ for a device after a driver has already claimed it.
>
> We plan to replace pci_fixup_irqs() soon, but we still need it on ARM
> because we don't have any other generic method for doing this.
>
> On ARM64, we don't need pci_fixup_irqs() because we do IRQ setup when we
> bind a driver to the device (in the pci_device_probe() ->
> pcibios_alloc_irq() path).
>
> pci-host-common.c is currently only used on ARM and ARM64. In principle,
> it could be used on x86, and we wouldn't want pci_fixup_irqs() there
> either, because x86 does IRQ setup in the pci_enable_device() path.
>
> [bhelgaas: changelog, use #ifdef ARM, not #ifndef ARM64]
> Signed-off-by: Dongdong Liu <liudongdong3@xxxxxxxxxx>
> Signed-off-by: Bjorn Helgaas <bhelgaas@xxxxxxxxxx>
> Reviewed-by: Gabriele Paoloni <gabriele.paoloni@xxxxxxxxxx>
> Reviewed-by: Zhou Wang <wangzhou1@xxxxxxxxxxxxx>
>
> diff --git a/drivers/pci/host/pci-host-common.c b/drivers/pci/host/pci-host-common.c
> index e3c48b5deb93..e9a53bae1c25 100644
> --- a/drivers/pci/host/pci-host-common.c
> +++ b/drivers/pci/host/pci-host-common.c
Acked-by: Lorenzo Pieralisi <lorenzo.pieralisi@xxxxxxx>
> @@ -145,7 +145,9 @@ int pci_host_common_probe(struct platform_device *pdev,
> return -ENODEV;
> }
>
> +#ifdef CONFIG_ARM
> pci_fixup_irqs(pci_common_swizzle, of_irq_parse_and_map_pci);
> +#endif
>
> /*
> * We insert PCI resources into the iomem_resource and
