Submitter | Stefan Reinauer |
---|---|
Date | 2010-06-04 16:29:59 |
Message ID | <4C092A07.3070408@coresystems.de> |
Download | mbox | patch |
Permalink | /patch/1465/ |
State | Rejected |
Headers | show |
Comments
if (!(res->flags & IORESOURCE_FIXED)) continue; ... /* Is it already outside the limits? */ - if (((res->base + res->size -1) < lim->base) || (res->base > lim->limit)) + if (((res->base + res->size -1) < lim->base) || (res->base > lim->limit)) { + printk(BIOS_ERR, "%s: Resources not available.\n", dev_path(dev)); continue; + } This code only is executed for fixed resources. If they are outside of the limits it's a good thing. I don't think there should be an error message. Thanks, Myles
On 6/4/10 6:36 PM, Myles Watson wrote: > if (!(res->flags & IORESOURCE_FIXED)) > continue; > ... > > /* Is it already outside the limits? */ > - if (((res->base + res->size -1) < lim->base) || (res->base > lim->limit)) > + if (((res->base + res->size -1) < lim->base) || (res->base > lim->limit)) { > + printk(BIOS_ERR, "%s: Resources not available.\n", dev_path(dev)); > continue; > + } > > This code only is executed for fixed resources. If they are outside > of the limits it's a good thing. I don't think there should be an > error message. > Why is that good? (can we add that explanation to the code as a comment?)
On Fri, Jun 4, 2010 at 10:41 AM, Stefan Reinauer <stepan@coresystems.de> wrote: > On 6/4/10 6:36 PM, Myles Watson wrote: >> if (!(res->flags & IORESOURCE_FIXED)) >> continue; >> ... >> >> /* Is it already outside the limits? */ >> - if (((res->base + res->size -1) < lim->base) || (res->base > lim->limit)) >> + if (((res->base + res->size -1) < lim->base) || (res->base > lim->limit)) { >> + printk(BIOS_ERR, "%s: Resources not available.\n", dev_path(dev)); >> continue; >> + } >> >> This code only is executed for fixed resources. If they are outside >> of the limits it's a good thing. I don't think there should be an >> error message. >> > Why is that good? (can we add that explanation to the code as a comment?) Sure. I guess since I wrote it I'm not a good judge of clarifying comments. Here's the big picture: You start with a full address space You constrain what the allocator can use by subtracting fixed resources And a completely contrived example: - take I/O 0x0000-0xffff - let's imagine that 0x400-420, 0x40-0x80, and 0xf000-0xf800 are fixed resources (the allocator can't use that space) First pass, the allocator chooses to use 0x420-0xffff (larger than 0x0-0x400) Second pass, the allocator does nothing because 0x40-0x80 is already outside the limits Third pass, the allocator chooses to use 0x420-0xf800 (larger than 0xf800-0xffff. I'd love it if you'd add some clarifying comments. Thanks, Myles
Patch
Index: src/devices/device.c =================================================================== --- src/devices/device.c (revision 5568) +++ src/devices/device.c (working copy) @@ -578,8 +578,10 @@ continue; /* Is it already outside the limits? */ - if (((res->base + res->size -1) < lim->base) || (res->base > lim->limit)) + if (((res->base + res->size -1) < lim->base) || (res->base > lim->limit)) { + printk(BIOS_ERR, "%s: Resources not available.\n", dev_path(dev)); continue; + } /* Choose to be above or below fixed resources. This * check is signed so that "negative" amounts of space
See patch Warn if resources could not be assigned. Signed-off-by: Stefan Reinauer <stepan@coresystems.de>