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

Commit Message

Thierry Reding July 26, 2012, 7:55 p.m. UTC

When using deferred driver probing, PCI host controller drivers may
actually require this function after the init stage.

Signed-off-by: Thierry Reding <[e-mail]>
---
Changes in v3:
- none

Changes in v2:
- use __devinit annotations

 drivers/pci/setup-irq.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Bjorn Helgaas Aug. 14, 2012, 5:06 a.m. UTC | #1

On Thu, Jul 26, 2012 at 12:55 PM, Thierry Reding
<[e-mail]> wrote:
> When using deferred driver probing, PCI host controller drivers may
> actually require this function after the init stage.

Yes, this is a bug.  Actually, there's still another bug here: if we
hot-add a device, we won't do pdev_fixup_irq() for it.  But your
change is worthwhile even without fixing that other bug.

Normally I would include this in my PCI tree since it's under
drivers/pci.  That would make it easier for somebody to fix the
hotplug problem in this cycle.  Would having it in my PCI tree make
things harder for you?

> Signed-off-by: Thierry Reding <[e-mail]>

Acked-by: Bjorn Helgaas <[e-mail]>

> ---
> Changes in v3:
> - none
>
> Changes in v2:
> - use __devinit annotations
>
>  drivers/pci/setup-irq.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/pci/setup-irq.c b/drivers/pci/setup-irq.c
> index eb219a1..f0bcd56 100644
> --- a/drivers/pci/setup-irq.c
> +++ b/drivers/pci/setup-irq.c
> @@ -18,7 +18,7 @@
>  #include <linux/cache.h>
>
>
> -static void __init
> +static void __devinit
>  pdev_fixup_irq(struct pci_dev *dev,
>                u8 (*swizzle)(struct pci_dev *, u8 *),
>                int (*map_irq)(const struct pci_dev *, u8, u8))
> @@ -54,7 +54,7 @@ pdev_fixup_irq(struct pci_dev *dev,
>         pcibios_update_irq(dev, irq);
>  }
>
> -void __init
> +void __devinit
>  pci_fixup_irqs(u8 (*swizzle)(struct pci_dev *, u8 *),
>                int (*map_irq)(const struct pci_dev *, u8, u8))
>  {
> --
> 1.7.11.2
>
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Thierry Reding Aug. 14, 2012, 5:37 a.m. UTC | #2

On Mon, Aug 13, 2012 at 10:06:38PM -0700, Bjorn Helgaas wrote:
> On Thu, Jul 26, 2012 at 12:55 PM, Thierry Reding
> <[e-mail]> wrote:
> > When using deferred driver probing, PCI host controller drivers may
> > actually require this function after the init stage.
> 
> Yes, this is a bug.  Actually, there's still another bug here: if we
> hot-add a device, we won't do pdev_fixup_irq() for it.  But your
> change is worthwhile even without fixing that other bug.
> 
> Normally I would include this in my PCI tree since it's under
> drivers/pci.  That would make it easier for somebody to fix the
> hotplug problem in this cycle.  Would having it in my PCI tree make
> things harder for you?

No, not at all.

> > Signed-off-by: Thierry Reding <[e-mail]>
> 
> Acked-by: Bjorn Helgaas <[e-mail]>

Thierry

Bjorn Helgaas Aug. 15, 2012, 5:06 p.m. UTC | #3

On Thu, Jul 26, 2012 at 12:55 PM, Thierry Reding
<[e-mail]> wrote:
> When using deferred driver probing, PCI host controller drivers may
> actually require this function after the init stage.
>
> Signed-off-by: Thierry Reding <[e-mail]>
> ---
> 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.

>  drivers/pci/setup-irq.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/pci/setup-irq.c b/drivers/pci/setup-irq.c
> index eb219a1..f0bcd56 100644
> --- a/drivers/pci/setup-irq.c
> +++ b/drivers/pci/setup-irq.c
> @@ -18,7 +18,7 @@
>  #include <linux/cache.h>
>
>
> -static void __init
> +static void __devinit
>  pdev_fixup_irq(struct pci_dev *dev,
>                u8 (*swizzle)(struct pci_dev *, u8 *),
>                int (*map_irq)(const struct pci_dev *, u8, u8))
> @@ -54,7 +54,7 @@ pdev_fixup_irq(struct pci_dev *dev,
>         pcibios_update_irq(dev, irq);
>  }
>
> -void __init
> +void __devinit
>  pci_fixup_irqs(u8 (*swizzle)(struct pci_dev *, u8 *),
>                int (*map_irq)(const struct pci_dev *, u8, u8))
>  {
> --
> 1.7.11.2
>
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Thierry Reding Aug. 15, 2012, 7:28 p.m. UTC | #4

On Wed, Aug 15, 2012 at 10:06:27AM -0700, Bjorn Helgaas wrote:
> On Thu, Jul 26, 2012 at 12:55 PM, Thierry Reding
> <[e-mail]> wrote:
> > When using deferred driver probing, PCI host controller drivers may
> > actually require this function after the init stage.
> >
> > Signed-off-by: Thierry Reding <[e-mail]>
> > ---
> > 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.

Thierry

Bjorn Helgaas Aug. 15, 2012, 7:42 p.m. UTC | #5

On Wed, Aug 15, 2012 at 1:28 PM, Thierry Reding
<[e-mail]> 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
>> <[e-mail]> wrote:
>> > When using deferred driver probing, PCI host controller drivers may
>> > actually require this function after the init stage.
>> >
>> > Signed-off-by: Thierry Reding <[e-mail]>
>> > ---
>> > 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
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Thierry Reding Aug. 15, 2012, 8:01 p.m. UTC | #6

On Wed, Aug 15, 2012 at 01:42:39PM -0600, Bjorn Helgaas wrote:
> On Wed, Aug 15, 2012 at 1:28 PM, Thierry Reding
> <[e-mail]> 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
> >> <[e-mail]> wrote:
> >> > When using deferred driver probing, PCI host controller drivers may
> >> > actually require this function after the init stage.
> >> >
> >> > Signed-off-by: Thierry Reding <[e-mail]>
> >> > ---
> >> > 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!

Great! Thanks. It's nice to see the patch series shrink. =)

Thierry

Stephen Warren Sept. 7, 2012, 4:19 p.m. UTC | #7

On 08/15/2012 01:42 PM, Bjorn Helgaas wrote:
> On Wed, Aug 15, 2012 at 1:28 PM, Thierry Reding
> <[e-mail]> 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
>>> <[e-mail]> wrote:
>>>> When using deferred driver probing, PCI host controller drivers may
>>>> actually require this function after the init stage.
>>>>
>>>> Signed-off-by: Thierry Reding <[e-mail]>
>>>> ---
>>>> 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?

For the full history, see: http://patchwork.ozlabs.org/patch/173495/.

Thanks.
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Thierry Reding Sept. 7, 2012, 5 p.m. UTC | #8

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
> > <[e-mail]> 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
> >>> <[e-mail]> wrote:
> >>>> When using deferred driver probing, PCI host controller drivers may
> >>>> actually require this function after the init stage.
> >>>>
> >>>> Signed-off-by: Thierry Reding <[e-mail]>
> >>>> ---
> >>>> 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.

Thierry

[0]: http://www.linux-mips.org/archives/linux-mips/2012-08/msg00250.html

Bjorn Helgaas Sept. 7, 2012, 5:22 p.m. UTC | #9

On Fri, Sep 7, 2012 at 10:00 AM, Thierry Reding
<[e-mail]> 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
>> > <[e-mail]> 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
>> >>> <[e-mail]> wrote:
>> >>>> When using deferred driver probing, PCI host controller drivers may
>> >>>> actually require this function after the init stage.
>> >>>>
>> >>>> Signed-off-by: Thierry Reding <[e-mail]>
>> >>>> ---
>> >>>> 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

Bjorn
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Thierry Reding Sept. 14, 2012, 6:55 p.m. UTC | #10

On Fri, Sep 07, 2012 at 10:22:28AM -0700, Bjorn Helgaas wrote:
> On Fri, Sep 7, 2012 at 10:00 AM, Thierry Reding
> <[e-mail]> 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
> >> > <[e-mail]> 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
> >> >>> <[e-mail]> wrote:
> >> >>>> When using deferred driver probing, PCI host controller drivers may
> >> >>>> actually require this function after the init stage.
> >> >>>>
> >> >>>> Signed-off-by: Thierry Reding <[e-mail]>
> >> >>>> ---
> >> >>>> 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

Bjorn Helgaas Sept. 14, 2012, 7:45 p.m. UTC | #11

On Fri, Sep 14, 2012 at 12:55 PM, Thierry Reding
<[e-mail]> wrote:
> On Fri, Sep 07, 2012 at 10:22:28AM -0700, Bjorn Helgaas wrote:
>> On Fri, Sep 7, 2012 at 10:00 AM, Thierry Reding
>> <[e-mail]> 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
>> >> > <[e-mail]> 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
>> >> >>> <[e-mail]> wrote:
>> >> >>>> When using deferred driver probing, PCI host controller drivers may
>> >> >>>> actually require this function after the init stage.
>> >> >>>>
>> >> >>>> Signed-off-by: Thierry Reding <[e-mail]>
>> >> >>>> ---
>> >> >>>> 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?

Sounds good to me.  DaveM is really responsive -- if you propose using
the generic implementation on SPARC, I bet he'd either ack it or tell
you why SPARC needs to be special.
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/drivers/pci/setup-irq.c b/drivers/pci/setup-irq.c
index eb219a1..f0bcd56 100644
--- a/drivers/pci/setup-irq.c
+++ b/drivers/pci/setup-irq.c
@@ -18,7 +18,7 @@ 
 #include <linux/cache.h>
 
 
-static void __init
+static void __devinit
 pdev_fixup_irq(struct pci_dev *dev,
 	       u8 (*swizzle)(struct pci_dev *, u8 *),
 	       int (*map_irq)(const struct pci_dev *, u8, u8))
@@ -54,7 +54,7 @@  pdev_fixup_irq(struct pci_dev *dev,
 	pcibios_update_irq(dev, irq);
 }
 
-void __init
+void __devinit
 pci_fixup_irqs(u8 (*swizzle)(struct pci_dev *, u8 *),
 	       int (*map_irq)(const struct pci_dev *, u8, u8))
 {