[PATCH]The resource allocator does not like zero-sized fixed resources

[Patrick Georgi]

Hi,

attached patch fixes a small issue with the resource allocator. In a 
loop in which only fixed resources are processed, zero-sized resources 
were able to modify the limits used for resource allocation in a later 
phase. This leads to overlapping resources on kontron.
I had a patch for that a while ago already, but this one is much cleaner 
and much more obvious in its intent.
If you think there shouldn't be zero-sized fixed resources, please take 
a look at src/devices/pci_device.c, line 331. There, a fixed resource 
with no size is created, and there is no obvious value to set for size.

Even if there shouldn't be such resources, I think it's still good to 
prevent them from causing trouble if they appear.

Signed-off-by: Patrick Georgi

[PATCH]The resource allocator does not like zero-sized fixed resources

[Myles Watson]

On Wed, Aug 19, 2009 at 10:14 AM, Patrick Georgi wrote:
> Hi,
>
> attached patch fixes a small issue with the resource allocator. In a loop in
> which only fixed resources are processed, zero-sized resources were able to
> modify the limits used for resource allocation in a later phase. This leads
> to overlapping resources on kontron.
> I had a patch for that a while ago already, but this one is much cleaner and
> much more obvious in its intent.
> If you think there shouldn't be zero-sized fixed resources, please take a
> look at src/devices/pci_device.c, line 331. There, a fixed resource with no
> size is created, and there is no obvious value to set for size.

The problem is that they have bogus limits, not that they have no
size.  Even if you commit this fix, you should fix the incorrect
limits.  They are two different problems.
> Even if there shouldn't be such resources, I think it's still good to
> prevent them from causing trouble if they appear.

My only objection is that it masks the real problem.

Thanks,
Myles

[PATCH]The resource allocator does not like zero-sized fixed resources

[Patrick Georgi]

Am 19.08.2009 18:18, schrieb Myles Watson:
> The problem is that they have bogus limits, not that they have no
> size.

A zero-sized resource with proper limits still fails. See:

/* Is it already outside the limits? */
if (res->size && (((res->base + res->size -1) < lim->base) || (res->base
> lim->limit)))

continue;

A resource with size == 0 is never "outside the limits". The code after 
these lines assumes that any resource that passes this test is inside 
the limits.
> Even if you commit this fix, you should fix the incorrect
> limits.  They are two different problems.

A fixed resource is supposed to have base+size=limit, with base and size 
of the right alignment and granularity, right?
How about we warn about that explicitely? I can prepare a patch, the 
location where I put the current patch would be a good place for that 
warning, right?

Patrick

[PATCH]The resource allocator does not like zero-sized fixed resources

[Myles Watson]

> Am 19.08.2009 18:18, schrieb Myles Watson:
> > The problem is that they have bogus limits, not that they have no
> > size.
>
> A zero-sized resource with proper limits still fails. See:
>
> /* Is it already outside the limits? */
> if (res->size && (((res->base + res->size -1) < lim->base) || (res->base
> > lim->limit)))
>  continue;

You're right.  It's obvious that I haven't been thinking about this much
lately.  So we can commit your patch that ignores fixed resources of size 0.

The example you gave in src/devices/pci_device.c was for the old way of
specifying PCI ROM locations.  Now that CBFS is going mainstream, that
should disappear.  If it doesn't, I think it should have a size.  Even 1
would work, since no Option ROM can be smaller than that.

What other fixed resources have 0 size, especially on Kontron?  I'd like to
fix the root cause.
> A resource with size == 0 is never "outside the limits". The code after
> these lines assumes that any resource that passes this test is inside
> the limits.
> > Even if you commit this fix, you should fix the incorrect
> > limits.  They are two different problems.
>
> A fixed resource is supposed to have base+size=limit, with base and size
> of the right alignment and granularity, right?

No.  limit = architectural limit. 0xffff or 0xffffffff usually.

Thanks,
Myles

[PATCH]The resource allocator does not like zero-sized fixed resources

[Patrick Georgi]

Am 19.08.2009 19:08, schrieb Myles Watson:
> The example you gave in src/devices/pci_device.c was for the old way of
> specifying PCI ROM locations.  Now that CBFS is going mainstream, that
> should disappear.  If it doesn't, I think it should have a size.  Even 1
> would work, since no Option ROM can be smaller than that.

I'd love to get rid of the rom_address thing once we drop the pre-cbfs 
way of doing things (which will take quite some time, still), but I'm 
not sure if there isn't some need for it left.
> What other fixed resources have 0 size, especially on Kontron?  I'd like to
> fix the root cause.

That's the only one on Kontron. I just wanted to have proper behaviour 
even with such weird entries.

Patrick

[PATCH]The resource allocator does not like zero-sized fixed resources

[Myles Watson]

On Wed, Aug 19, 2009 at 11:32 AM, Patrick Georgi wrote:
> Am 19.08.2009 19:08, schrieb Myles Watson:
> > The example you gave in src/devices/pci_device.c was for the old way of
> > specifying PCI ROM locations.  Now that CBFS is going mainstream, that
> > should disappear.  If it doesn't, I think it should have a size.  Even 1
> > would work, since no Option ROM can be smaller than that.
>
> I'd love to get rid of the rom_address thing once we drop the pre-cbfs way
> of doing things (which will take quite some time, still), but I'm not sure
> if there isn't some need for it left.

OK.
> > What other fixed resources have 0 size, especially on Kontron?  I'd like
> > to
> > fix the root cause.
>
> That's the only one on Kontron. I just wanted to have proper behaviour even
> with such weird entries.

Let's add something like this patch.  I'm happy to have the wording
changed, but I think it's important that we don't silently fail to
avoid fixed resources.

Signed-off-by: Myles Watson 
Thanks,
Myles

[PATCH]The resource allocator does not like zero-sized fixed resources

[Patrick Georgi]

Am 19.08.2009 19:59, schrieb Myles Watson:
> Let's add something like this patch.  I'm happy to have the wording
> changed, but I think it's important that we don't silently fail to
> avoid fixed resources.

I'm fine with the wording
> Signed-off-by: Myles Watson

Acked-by: Patrick Georgi

[PATCH]The resource allocator does not like zero-sized fixed resources

[Myles Watson]

On Wed, Aug 19, 2009 at 1:05 PM, Patrick Georgi wrote:
> Am 19.08.2009 19:59, schrieb Myles Watson:
> > Let's add something like this patch.  I'm happy to have the wording
> > changed, but I think it's important that we don't silently fail to
> > avoid fixed resources.
>
> I'm fine with the wording
> > Signed-off-by: Myles Watson
>
> Acked-by: Patrick Georgi

Rev 4557.

Thanks,
Myles

[PATCH]The resource allocator does not like zero-sized fixed resources

[Myles Watson]

> Signed-off-by: Patrick Georgi

Acked-by: Myles Watson

Thanks,
Myles