[PATCH] PCI, EDAC: fix ordering assign resource and bus_add

[PATCH] PCI, EDAC: fix ordering assign resource and bus_add

[Yinghai Lu]

We should assign unassigned resource before pci_bus_add_device.

as late one will enable driver and create sysfs file that will need
pci io resources from assign unassigned code.

Signed-off-by: Yinghai Lu <yinghai@kernel.org>

---
 drivers/edac/i82875p_edac.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Index: linux-2.6/drivers/edac/i82875p_edac.c
===================================================================
--- linux-2.6.orig/drivers/edac/i82875p_edac.c
+++ linux-2.6/drivers/edac/i82875p_edac.c
@@ -293,13 +293,14 @@ static int i82875p_setup_overfl_dev(stru
 		if (dev == NULL)
 			return 1;
 
+		pci_bus_assign_resources(dev->bus);
+
 		err = pci_bus_add_device(dev);
 		if (err) {
 			i82875p_printk(KERN_ERR,
 				"%s(): pci_bus_add_device() Failed\n",
 				__func__);
 		}
-		pci_bus_assign_resources(dev->bus);
 	}
 
 	*ovrfl_pdev = dev;

Re: [PATCH] PCI, EDAC: fix ordering assign resource and bus_add

[Bjorn Helgaas]

On Tue, May 07, 2013 at 04:29:31PM -0700, Yinghai Lu wrote:
> We should assign unassigned resource before pci_bus_add_device.
> 
> as late one will enable driver and create sysfs file that will need
> pci io resources from assign unassigned code.
> 
> Signed-off-by: Yinghai Lu <yinghai@kernel.org>

Applied to pci/resource for v3.16, thanks!

> ---
>  drivers/edac/i82875p_edac.c |    3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> Index: linux-2.6/drivers/edac/i82875p_edac.c
> ===================================================================
> --- linux-2.6.orig/drivers/edac/i82875p_edac.c
> +++ linux-2.6/drivers/edac/i82875p_edac.c
> @@ -293,13 +293,14 @@ static int i82875p_setup_overfl_dev(stru
>  		if (dev == NULL)
>  			return 1;
>  
> +		pci_bus_assign_resources(dev->bus);
> +
>  		err = pci_bus_add_device(dev);
>  		if (err) {
>  			i82875p_printk(KERN_ERR,
>  				"%s(): pci_bus_add_device() Failed\n",
>  				__func__);
>  		}
> -		pci_bus_assign_resources(dev->bus);
>  	}
>  
>  	*ovrfl_pdev = dev;

Re: [PATCH] PCI, EDAC: fix ordering assign resource and bus_add

[Borislav Petkov]

On Fri, May 30, 2014 at 11:01:03AM -0600, Bjorn Helgaas wrote:
> On Tue, May 07, 2013 at 04:29:31PM -0700, Yinghai Lu wrote:
> > We should assign unassigned resource before pci_bus_add_device.
> > 
> > as late one will enable driver and create sysfs file that will need
> > pci io resources from assign unassigned code.
> > 
> > Signed-off-by: Yinghai Lu <yinghai@kernel.org>
> 
> Applied to pci/resource for v3.16, thanks!

This is touching drivers/edac/ and I guess I'm fine with it,

Acked-by: Borislav Petkov <bp@suse.de>

> > ---
> >  drivers/edac/i82875p_edac.c |    3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > Index: linux-2.6/drivers/edac/i82875p_edac.c
> > ===================================================================
> > --- linux-2.6.orig/drivers/edac/i82875p_edac.c
> > +++ linux-2.6/drivers/edac/i82875p_edac.c
> > @@ -293,13 +293,14 @@ static int i82875p_setup_overfl_dev(stru
> >  		if (dev == NULL)
> >  			return 1;
> >  
> > +		pci_bus_assign_resources(dev->bus);
> > +
> >  		err = pci_bus_add_device(dev);
> >  		if (err) {
> >  			i82875p_printk(KERN_ERR,
> >  				"%s(): pci_bus_add_device() Failed\n",
> >  				__func__);
> >  		}
> > -		pci_bus_assign_resources(dev->bus);a


... one question though: how are people using those pci_bus*
functions supposed to know that pci_bus_assign_resources needs to
go before pci_bus_add_device? The comment in pci_bus_add_device()
mentions something about it but wouldn't it be possible to call
pci_bus_assign_resources() in pci_bus_add_device() when resources are
not assigned?

Sorry if my question is completely dumb - I have no presumption of
knowing pci and haven't looked either. Just asking with my user hat on.

Thanks.

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: [PATCH] PCI, EDAC: fix ordering assign resource and bus_add

[Bjorn Helgaas]

On Fri, May 30, 2014 at 3:46 PM, Borislav Petkov <bp@alien8.de> wrote:
> On Fri, May 30, 2014 at 11:01:03AM -0600, Bjorn Helgaas wrote:
>> On Tue, May 07, 2013 at 04:29:31PM -0700, Yinghai Lu wrote:
>> > We should assign unassigned resource before pci_bus_add_device.
>> >
>> > as late one will enable driver and create sysfs file that will need
>> > pci io resources from assign unassigned code.
>> >
>> > Signed-off-by: Yinghai Lu <yinghai@kernel.org>
>>
>> Applied to pci/resource for v3.16, thanks!
>
> This is touching drivers/edac/ and I guess I'm fine with it,
>
> Acked-by: Borislav Petkov <bp@suse.de>

Thanks, I should have waited for you.  I was assuming the merge window
would open on Monday, and I hoped for a day or two in -next, but now
it sounds like we might have an -rc8, so I needn't have been in such a
hurry.

>> > ---
>> >  drivers/edac/i82875p_edac.c |    3 ++-
>> >  1 file changed, 2 insertions(+), 1 deletion(-)
>> >
>> > Index: linux-2.6/drivers/edac/i82875p_edac.c
>> > ===================================================================
>> > --- linux-2.6.orig/drivers/edac/i82875p_edac.c
>> > +++ linux-2.6/drivers/edac/i82875p_edac.c
>> > @@ -293,13 +293,14 @@ static int i82875p_setup_overfl_dev(stru
>> >             if (dev == NULL)
>> >                     return 1;
>> >
>> > +           pci_bus_assign_resources(dev->bus);
>> > +
>> >             err = pci_bus_add_device(dev);
>> >             if (err) {
>> >                     i82875p_printk(KERN_ERR,
>> >                             "%s(): pci_bus_add_device() Failed\n",
>> >                             __func__);
>> >             }
>> > -           pci_bus_assign_resources(dev->bus);a
>
>
> ... one question though: how are people using those pci_bus*
> functions supposed to know that pci_bus_assign_resources needs to
> go before pci_bus_add_device? The comment in pci_bus_add_device()
> mentions something about it but wouldn't it be possible to call
> pci_bus_assign_resources() in pci_bus_add_device() when resources are
> not assigned?

Yeah, this is really a mess.  The pci_bus_*() functions (at least
pci_bus_add_device() and pci_bus_assign_resources()) are not really
intended to be called by drivers at all -- they should only be used by
the arch code that drives PCI enumeration.  Eventually I'd like to
even move them out of the arch code and into the PCI core.

In this i82875p case, it looks like this is basically a quirk that
unhides a device.  I think it's sort of dubious to poke at things the
BIOS has explicitly hidden from the OS, but apart from that, if we
turned this into an actual DECLARE_PCI_FIXUP_CLASS_EARLY() quirk on
the 82875_0 device, I think the normal PCI enumeration process would
find the 82875_6 device, and we could get rid of the whole "if (dev ==
NULL)" chunk.

Bjorn

Re: [PATCH] PCI, EDAC: fix ordering assign resource and bus_add

[Borislav Petkov]

On Fri, May 30, 2014 at 04:00:17PM -0600, Bjorn Helgaas wrote:
> Thanks, I should have waited for you. I was assuming the merge window
> would open on Monday, and I hoped for a day or two in -next, but now
> it sounds like we might have an -rc8, so I needn't have been in such a
> hurry.

Bah, no worries. :-) The patch is simple enough and is requiring your
expertise so I wouldnt've said anything, except I wanted to ask about
that pci_bus* functions ordering situation, so... :-)

> Yeah, this is really a mess.  The pci_bus_*() functions (at least
> pci_bus_add_device() and pci_bus_assign_resources()) are not really
> intended to be called by drivers at all -- they should only be used by
> the arch code that drives PCI enumeration.  Eventually I'd like to
> even move them out of the arch code and into the PCI core.

Ah, ok. pci_bus_add_device is called also in some x86 platform drivers,
except the pci core itself, so yeah, it looks like you're trying to hide
it. :)

> In this i82875p case, it looks like this is basically a quirk that
> unhides a device.  I think it's sort of dubious to poke at things the
> BIOS has explicitly hidden from the OS, but apart from that, if we
> turned this into an actual DECLARE_PCI_FIXUP_CLASS_EARLY() quirk on
> the 82875_0 device, I think the normal PCI enumeration process would
> find the 82875_6 device, and we could get rid of the whole "if (dev ==
> NULL)" chunk.

Yeah, I don't think that's worth it - this driver is probably ancient
now, if I trust this:

http://ark.intel.com/products/27711/Intel-82875P-Memory-Controller

Launch Date: Q1'02 - that's a century ago in HW years. :-P

I guess we can leave it at that - even if we did a quirk, I'm at a loss
as to where or who could test it. :-)

Thanks.

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--