Re: [PATCH v3 01/10] PCI: Keep pci_fixup_irqs() around after init

[Thierry Reding <thierry.reding-RM9K5IK7kjKj5M59NBduVrNAH6kLmebB@public.gmane.org>]

[-- Attachment #1: Type: text/plain, Size: 5165 bytes --]

On Fri, Sep 07, 2012 at 10:22:28AM -0700, Bjorn Helgaas wrote:
> On Fri, Sep 7, 2012 at 10:00 AM, Thierry Reding
> <thierry.reding-RM9K5IK7kjKj5M59NBduVrNAH6kLmebB@public.gmane.org> wrote:
> > On Fri, Sep 07, 2012 at 10:19:46AM -0600, Stephen Warren wrote:
> >> On 08/15/2012 01:42 PM, Bjorn Helgaas wrote:
> >> > On Wed, Aug 15, 2012 at 1:28 PM, Thierry Reding
> >> > <thierry.reding-RM9K5IK7kjKj5M59NBduVrNAH6kLmebB@public.gmane.org> wrote:
> >> >> On Wed, Aug 15, 2012 at 10:06:27AM -0700, Bjorn Helgaas wrote:
> >> >>> On Thu, Jul 26, 2012 at 12:55 PM, Thierry Reding
> >> >>> <thierry.reding-RM9K5IK7kjKj5M59NBduVrNAH6kLmebB@public.gmane.org> wrote:
> >> >>>> When using deferred driver probing, PCI host controller drivers may
> >> >>>> actually require this function after the init stage.
> >> >>>>
> >> >>>> Signed-off-by: Thierry Reding <thierry.reding-RM9K5IK7kjKj5M59NBduVrNAH6kLmebB@public.gmane.org>
> >> >>>> ---
> >> >>>> Changes in v3:
> >> >>>> - none
> >> >>>>
> >> >>>> Changes in v2:
> >> >>>> - use __devinit annotations
> >> >>>
> >> >>> Your original patch removed __init completely.  Here you change it to
> >> >>> __devinit.  That means we decide whether to discard the function based
> >> >>> on whether CONFIG_HOTPLUG is supported.  But I think your point is not
> >> >>> about hotplug; it's merely that we should be able to scan a PCI bus
> >> >>> after init-time.  We ought to be able to do a late PCI scan even if
> >> >>> hotplug is not supported.
> >> >>>
> >> >>> Therefore, I'd be inclined to remove __init completely unless you have
> >> >>> another reason for preferring __devinit.
> >> >>
> >> >> I thought __devinit would resolve to nothing if HOTPLUG is defined and
> >> >> __init otherwise. That seemed more appropriate. However you are right
> >> >> that it is useful to always have it available, so I'm fine with removing
> >> >> the annotations altogether. Do you want me to follow up with a patch? Or
> >> >> can you just take the first version? I'm not sure if it still applies.
> >> >
> >> > You're right about how __devinit works.  It's just that I don't think
> >> > hotplug is actually relevant here.  We're trying to make
> >> > pci_fixup_irqs() work after init, whether it's because of hotplug or
> >> > simply because the arch scans host bridges after init.
> >> >
> >> > I applied this to my "next" branch.  Thanks!
> >>
> >> Bjorn, I don't see this patch in next-20120907. Did it get dropped for
> >> some reason?
> >
> > Yes, it turns out that dropping the annotations causes lots of section
> > mismatches on other architectures. See here[0] for the details. I think
> > the solution to the issue would be to either remove HOTPLUG altogether
> > and drop __devinit and __devexit annotations or update all architectures
> > to fix these warnings. I think Bjorn and I settled on the latter because
> > it's obviously less intrusive. I've been busy building toolchains for
> > all the PCI architectures and I think I have all of them. I'll just need
> > some more time to build, find and fix any remaining section mismatches.
> 
> Greg KH is actively removing CONFIG_HOTPLUG altogether -- see
> https://lkml.org/lkml/2012/9/4/489
> 
> That will make __devinit resolve to nothing in all cases, and we'll
> eventually remove __devinit completely.  So we don't want to convert
> __init to __devinit; we have to remove the __init altogether.  I think
> that means we have to update all arches first to avoid the section
> mismatches.
> 
> So I think Thierry is on the right track:
>   1) Change all arch pcibios_update_irq() implementations (and
> probably a few other things) to be non-__init
>   2) Change pci_fixup_irqs() to be non-__init

Okay this wasn't as much work as I had thought. It turns out that there
very few dependencies other than pcibios_update_irq(). Actually none at
all. In addition some of the architectures already had these annotated
with __devinit. Luckily those were the ones I wasn't able to build a
cross-compiler for. Note that I resolved to converting all annotations
from __init to __devinit first. I'll try to find out what exactly Greg
is planning to do. If it turns out that the plan is to just remove the
__devinit incrementally I could just drop all of these. Otherwise maybe
a better option would be to leave them in and remove them all at once
when the HOTPLUG symbol is removed.

Furthermore it seems like almost all implementations are the same, so I
was going to include a patch that moves the common implementation to the
PCI core and make it a weak symbol so architectures would still have the
possibility to override it.

The only exception to this is 64-bit SPARC, which apparently doesn't do
anything in pcibios_update_irq(). I wonder if it would hurt to just use
the generic implementation anyway, which just sets a byte in the
configuration space. That should work regardless of architecture, right?

Unicore32 and ARM also output a debugging message and maybe it would be
okay to include that with the generic implementation as well. Do you
have any objections?

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]