IORESOURCE_PCI_FIXED usage in pcibios_fixup_device_resources()

IORESOURCE_PCI_FIXED usage in pcibios_fixup_device_resources()

[Bjorn Helgaas]

The SH implementation of pcibios_fixup_device_resources() does the
equivalent of pcibios_bus_to_resource() for all the resources of a
device, except that it skips resources with the IORESOURCE_PCI_FIXED
flag set.

This looks like a bug to me.  IORESOURCE_PCI_FIXED means the resource
cannot be moved, not that we should skip pcibios_bus_to_resource().

Does anybody know why pcibios_fixup_device_resources() does this check?

The only other IORESOURCE_PCI_FIXED reference I see in arch/sh is in
gapspci_fixup_resources(), so maybe it has something to do with that?

Bjorn

Re: IORESOURCE_PCI_FIXED usage in pcibios_fixup_device_resources()

[Paul Mundt]

On Thu, Jun 03, 2010 at 04:38:32PM -0600, Bjorn Helgaas wrote:
> The SH implementation of pcibios_fixup_device_resources() does the
> equivalent of pcibios_bus_to_resource() for all the resources of a
> device, except that it skips resources with the IORESOURCE_PCI_FIXED
> flag set.
> 
> This looks like a bug to me.  IORESOURCE_PCI_FIXED means the resource
> cannot be moved, not that we should skip pcibios_bus_to_resource().
> 
> Does anybody know why pcibios_fixup_device_resources() does this check?
> 
> The only other IORESOURCE_PCI_FIXED reference I see in arch/sh is in
> gapspci_fixup_resources(), so maybe it has something to do with that?
> 
gapspci is a very special case, in that it's not really PCI but a bit of
mangling logic in a custom ASIC that interfaces an 8139 ethernet device
directly on to an ISA-style bus. If you take a look at
arch/sh/drivers/pci/fixups-dreamcast.c you can see where the flag is set
and the rationale behind it.

Looking at the code again though it seems like pcibios_bus_to_resource()
will not cause any problems, especially given that the io and mem offsets
are both cleared to 0 for that controller. I'll kill off the test from
pcibios_fixup_device_resources(), I assume this was a remnant from the
old pci-auto code which we moved off of some time ago.

We still need to retain the flag though, since nothing can be permitted
to touch the resources for any devices on that particular controller.

Re: IORESOURCE_PCI_FIXED usage in pcibios_fixup_device_resources()

[Bjorn Helgaas]

On Thursday, June 03, 2010 06:42:53 pm Paul Mundt wrote:
> On Thu, Jun 03, 2010 at 04:38:32PM -0600, Bjorn Helgaas wrote:
> > The SH implementation of pcibios_fixup_device_resources() does the
> > equivalent of pcibios_bus_to_resource() for all the resources of a
> > device, except that it skips resources with the IORESOURCE_PCI_FIXED
> > flag set.
> > 
> > This looks like a bug to me.  IORESOURCE_PCI_FIXED means the resource
> > cannot be moved, not that we should skip pcibios_bus_to_resource().
> > 
> > Does anybody know why pcibios_fixup_device_resources() does this check?
> > 
> > The only other IORESOURCE_PCI_FIXED reference I see in arch/sh is in
> > gapspci_fixup_resources(), so maybe it has something to do with that?
> > 
> gapspci is a very special case, in that it's not really PCI but a bit of
> mangling logic in a custom ASIC that interfaces an 8139 ethernet device
> directly on to an ISA-style bus. If you take a look at
> arch/sh/drivers/pci/fixups-dreamcast.c you can see where the flag is set
> and the rationale behind it.

Yes, using IORESOURCE_PCI_FIXED in gapspci_fixup_resources() sounds
reasonable; it's just that it doesn't make sense to test for it in
pcibios_fixup_device_resources() or pcibios_bus_to_resource().

> Looking at the code again though it seems like pcibios_bus_to_resource()
> will not cause any problems, especially given that the io and mem offsets
> are both cleared to 0 for that controller. I'll kill off the test from
> pcibios_fixup_device_resources(), I assume this was a remnant from the
> old pci-auto code which we moved off of some time ago.

Thanks.  I'll watch for your checkin.  I'm working on some patches
to move the generic parts of pcibios_fixup_bus() out of the arches
and into drivers/pci, and that depends on this change.

Bjorn

Re: IORESOURCE_PCI_FIXED usage in pcibios_fixup_device_resources()

[Paul Mundt]

On Fri, Jun 04, 2010 at 10:04:30AM -0600, Bjorn Helgaas wrote:
> On Thursday, June 03, 2010 06:42:53 pm Paul Mundt wrote:
> > On Thu, Jun 03, 2010 at 04:38:32PM -0600, Bjorn Helgaas wrote:
> > > The SH implementation of pcibios_fixup_device_resources() does the
> > > equivalent of pcibios_bus_to_resource() for all the resources of a
> > > device, except that it skips resources with the IORESOURCE_PCI_FIXED
> > > flag set.
> > > 
> > > This looks like a bug to me.  IORESOURCE_PCI_FIXED means the resource
> > > cannot be moved, not that we should skip pcibios_bus_to_resource().
> > > 
> > > Does anybody know why pcibios_fixup_device_resources() does this check?
> > > 
> > > The only other IORESOURCE_PCI_FIXED reference I see in arch/sh is in
> > > gapspci_fixup_resources(), so maybe it has something to do with that?
> > > 
> > gapspci is a very special case, in that it's not really PCI but a bit of
> > mangling logic in a custom ASIC that interfaces an 8139 ethernet device
> > directly on to an ISA-style bus. If you take a look at
> > arch/sh/drivers/pci/fixups-dreamcast.c you can see where the flag is set
> > and the rationale behind it.
> 
> Yes, using IORESOURCE_PCI_FIXED in gapspci_fixup_resources() sounds
> reasonable; it's just that it doesn't make sense to test for it in
> pcibios_fixup_device_resources() or pcibios_bus_to_resource().
> 
> > Looking at the code again though it seems like pcibios_bus_to_resource()
> > will not cause any problems, especially given that the io and mem offsets
> > are both cleared to 0 for that controller. I'll kill off the test from
> > pcibios_fixup_device_resources(), I assume this was a remnant from the
> > old pci-auto code which we moved off of some time ago.
> 
> Thanks.  I'll watch for your checkin.  I'm working on some patches
> to move the generic parts of pcibios_fixup_bus() out of the arches
> and into drivers/pci, and that depends on this change.
> 
I've checked in a fix for this now, which I'll send to Linus for -rc3.