pci_assign_resource() inconsistency

pci_assign_resource() inconsistency

[Benjamin Herrenschmidt]

Hi !

pci_assign_resource() was recently marked as much check the result code
(which I find annoying since it alread contains a printk if it fails but
anyway ... ).

So we do have a call to in in arch/powerpc/kernel/pci_32.c and I went to
fix the warning and ... wondered what should we do there ? So I looked
at other users and found it a bit strange...

So at first, an unassigned resource has the IORESOURCE_UNSET flag set
(or is supposed to). pci_assign_resource() itself will clear that flag
if it succeeds.

However, pretty much nothing else checks that flag, so it's mostly
useful.

Now, we have drivers/pci/setup-bus.c doing:

                if (pci_assign_resource(list->dev, idx)) {
                        res->start = 0;
                        res->end = 0;
                        res->flags = 0;
                }

So it basically destroys the resource content utterly when
pci_assign_resource() fails...

There are questions raised here:

 - Shouldn't we instead fix things so that instead, we properly
test for IORESOURCE_UNSET in pci_request_* & friends and just have
pci_assign_resource() continue as it's doing now, that is not clear that
flag if the assignment fails ?

 - setup-bus.c is a bit violent: As soon as it hits a p2p bridge, it
will bluntly re-assign everybody, not trying to check wether a resource
was already correctly assigned by the firmware or not. However, it never
sets IORESOURCE_UNSET. Thus if we do the above, we should probably have
it always set that bit before calling pci_assign_resource()...

Now the question is, what should I do in pci_32.c ... right now, we
unconditionally clear IORESOURCE_UNSET, which isn't very correct, then
call pci_assign_resource().

Should I do like the setup-bus.c and just completely wipe the resource
if pci_assign_resource() fail ? Or should I just stop clearing
IORESOURCE_UNSET (and thus rely on pci_assign_resource() to clear it
only if it succeeds, which seems to work) in which case I see no point
in making that function much check since there is nothing useful to do
when it fails and it does printk already.

Ben.
 

Re: pci_assign_resource() inconsistency

[Ivan Kokshaysky]

On Mon, Dec 11, 2006 at 02:47:55PM +1100, Benjamin Herrenschmidt wrote:
> So at first, an unassigned resource has the IORESOURCE_UNSET flag set
> (or is supposed to). pci_assign_resource() itself will clear that flag
> if it succeeds.
> 
> However, pretty much nothing else checks that flag, so it's mostly
> useful.

I doubt of the generic usefulness of that flag, as it would be mere
equivalent of resource->parent == NULL.
I think the IORESOURCE_UNSET flag came from PnP and PCI subsystem has
never used it.

> Now, we have drivers/pci/setup-bus.c doing:
> 
>                 if (pci_assign_resource(list->dev, idx)) {
>                         res->start = 0;
>                         res->end = 0;
>                         res->flags = 0;
>                 }
> 
> So it basically destroys the resource content utterly when
> pci_assign_resource() fails...

Yes, it's ugly. But, IIRC, this was the only way to prevent some drivers
and arch code from using an unassigned resource...

> There are questions raised here:
> 
>  - Shouldn't we instead fix things so that instead, we properly
> test for IORESOURCE_UNSET in pci_request_* & friends and just have
> pci_assign_resource() continue as it's doing now, that is not clear that
> flag if the assignment fails ?
> 
>  - setup-bus.c is a bit violent: As soon as it hits a p2p bridge, it
> will bluntly re-assign everybody, not trying to check wether a resource
> was already correctly assigned by the firmware or not. However, it never

No - it checks the resource->parent and doesn't touch the resources which
are already assigned.

> sets IORESOURCE_UNSET. Thus if we do the above, we should probably have
> it always set that bit before calling pci_assign_resource()...
> 
> Now the question is, what should I do in pci_32.c ... right now, we
> unconditionally clear IORESOURCE_UNSET, which isn't very correct, then
> call pci_assign_resource().
> 
> Should I do like the setup-bus.c and just completely wipe the resource
> if pci_assign_resource() fail ? Or should I just stop clearing
> IORESOURCE_UNSET (and thus rely on pci_assign_resource() to clear it
> only if it succeeds, which seems to work) in which case I see no point
> in making that function much check since there is nothing useful to do
> when it fails and it does printk already.

Well, as IORESOURCE_UNSET usage seems to be an exclusive PPC32 thing,
there is no much difference. ;-)
Though I agree that ignoring the return value of pci_assign_resource()
is entirely valid in most cases.

Ivan.