On Fri, 2007-07-20 at 15:54 +0200, Thomas Renninger wrote: > On Wed, 2007-07-18 at 16:10 -0600, Bjorn Helgaas wrote: > > On Wednesday 18 July 2007 11:50:38 am Thomas Renninger wrote: > > > On Fri, 2007-07-13 at 10:56 -0600, Bjorn Helgaas wrote: > > > > The PNP core does not request resources for devices that are active > > > > at boot. Those resources currently don't get requested until a driver > > > > claims the device. I think this is a bug that we should fix. > > > Yep, this is also a problem for the ACPI variables (in fact not really, > > > as long as not overlapping like the rtc resource) and probably the > > > reason why pnpacpi ignores addresses below 0x100? > > > > By "ACPI variables," do you mean PM1a_EVT_BLK and the like? Those > > should be subsets of the _CRS resources for some device. > No, I mean SystemIO OperationRegion declarations, like: > OperationRegion (IOID, SystemIO, 0x2E, 0x02) > Field (IOID, ByteAcc, NoLock, Preserve) > { > INDX, 8, > DATA, 8 > } > It may happen that you see e.g.: > 0080-008f : dma page reg # statically requested early in setup.c > 0080-0080 : ACPI DEB0 # ACPI SystemIO OperationRegion > # requested at ACPI table parse time > > It's not nice..., but also does not hurt, as long as those do not > overlap. Even if these overlap the ACPI region/variable just won't be > able to reserve the region, not perfect, but better than before when > ACPI regions/variables were simply ignored. > > > > IMO: > > > - Making sure ACPI claimed regions do not interfere with native > > > drivers is very important and will get much more important in near > > > future > > > > I agree with this. I know some of these native drivers (lm-sensors, etc) > > are pretty essential right now. But they just aren't safe because they > > use resources that ACPI thinks it owns. I think the drivers should be > > changed to explicitly override (with appropriate KERN_WARN messages) > > any prior ACPI resource reservations. > > > > > > > The rtc driver seems to request the whole rtc space (0x70-0x77) > > > > > which fails because 0x72-0x73 has already been requested SHARED by > > > > > an ACPI SystemIO variable. At least parts of the rtc space got > > > > > requested (rtc0), I haven't checked whether the device was working > > > > > correctly... > > > > > > > > I tripped over a couple of these when I changed the PNP core to request > > > > resources for active devices. The RTC is one; often BIOS says the RTC > > > > is at 0x70-0x71 and 0x72-0x73, but the rtc drivers like to claim 0x8 > > > > (RTC_IO_EXTENT) ports. However, I think the driver only *uses* two > > > > ports, so we should change the driver to only request what it is > > > > using. > > > > > But there could be more? > > > > I think the PNP core should request all the ports the device responds > > to (as reported by the BIOS). The driver should request only the ports > > it uses. If the rtc driver only uses two ports, there's no reason for > > it to request all eight. > > > > > Argggggh, normally it should be: > > > 5000-50fe : ACPI PMIO > > > ... > > > 50c0-50df : pnp 00:07 > > > 50e0-50ff : pnp 00:07 # this one is missing because > > > # it's bigger than the parent > > > 50e0-50ef : amd756_smbus > > > > IMO, this is backwards. "pnp 00:07" should be the parent and should > > be reserved by the PNP core. A side-effect of this is that > > drivers/pnp/system.c would not be needed at all. > > This patch is about to reserve the ACPI regions/variables (see the AML > descriptions at the beginning). > If I interpreted the code and your comment right, only PNP0c02 and > PNP0c01 is served (reserved) by pnpacpi layer currently (didn't see this > first). And you want to let pnpacpi reserve all ACPI device specific > resources (_CRS)? This should be done by removing system.c and the > interface to identify specific PNP devices (probe, device_id,..), but > just check for a _CRS function for all ACPI devices in > drivers/pnp/pnpacpi/core.c and reserve the resources. > IMO yes, this should be done and would be the first step. > Also reserve ACPI variables/OperationRegions, this is one is for, would > be the second step (and needs some more rework anyway). > > I am off for a week, but will read and try out a bit more. > The next iteration will take some days (please stop me if you already > see some undoable hurdles that I may have overseen)... > > Thanks a lot Bjorn, for the detailed review and all your input. > > Thomas > > > > > > Summary: > > > > > ... > > > > > - Not fixable (maybe someone has an idea): If ACPI IO region declares > > > > > a smaller IO part than the later native driver (e.g. example above > > > > > with rtc driver), the partially overlapping resource cannot be > > > > > registered and the native driver fails to load with strict and lax > > > > > option. > > > > > > > > Assuming the BIOS describes the region correctly, I'd say a driver > > > > that claims a larger region is buggy and should be fixed. > > > > > > Yep, but a temporary solution where everything just works fine and a > > > message: "This driver needs fixing" is needed IMO (if the code gets > > > accepted... It's possible, but ...). > > > > How about something like this: We fix all the native drivers we know > > about. We make the PNP and ACPI cores request resources for all active > > devices by default, but add a flag like "pnp=no_reservations" that turns > > this off. Then native drivers that request conflicting resources > > will fail, but the user can limp along by using the boot-time option. > > Yes sounds good. > What I wanted to do is to also reserve the OperationRegions. I didn't > realize that pnp layer only reserves resources for specific devices and > this should be done first (reserve all). > I am pretty sure arbitrary AML code using variables from > OperationRegions can still be outside device specific resources and we > need to reserve those too (or at least have a boot option to identify > possible interference) to make sure ACPI interpreted code does not clash > with native drivers or at least identify which drivers potentially > interfere. > However, this needs an ugly implementation (extension of > kernel/resources.c) because those could partly overlap with device > specific ACPI resources (the pnp ones): > > 5000-50fe : ACPI PMIO # ACPI variable/OperationRegion -> for ASL > # example definition, see beginning of the mail > ... > 50c0-50df : pnp 00:07 # These two are device specific ACPI resources > 50e0-50ff : pnp 00:07 # This one's end address overlaps by one byte > # with its parent -> must get accepted > 50e0-50ef : amd756_smbus > > Then you'd have the pnp (pnpacpi only) and ACPI > variables/OperationRegions that should always be the parents (where the > variables are the parents of the pnpacpi resources) and those are > allowed to overlap and represent/reserve all ACPI resources. Another idea just came to my mind: Not reserving the ACPI variables/OperationRegions at all in kernel/resources.c. Instead store these in a list in drivers/acpi/osl.c as I've already done and add something at the beginning of: kernel/resources.c:request_region() like (pseudo-code): #ifdef ACPI err = check_for_acpi_operation_region_clashes(struct *resource) if (err) ... #endif this should be the most unintrusive way, (nearly) not polluting kernel/resources.c and in drivers/acpi/osl.c: check_for_acpi_operation_region_clashes(struct *resource){ ... if (clash) if (acpi_enforce_resources == LAX) pr_info ("Resource %s might interfere with ACPI" " Operation region %s\n", ...); return no_error; else { printk (KERN_ERR "Resource %s interferes with ACPI" " Operation region %s\n", ...); return error; } ... } Will try a bit more..., I will still read mails next week... Thanks, Thomas - To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html