Detect hwmon and i2c bus drivers interfering with ACPI Operation Region resources

it seems Len's test tree and Linus tree diverged a bit, at least with this patch set things do not apply cleanly.

Therefore I post these for discussion whether and in which kernel tree they should end up before doing work for nothing.
If they are still a candidate for 2.6.24 (rather unintrusive), pls tell me whether and when I should base them against Len's test/release branch or whatever other tree.
If not, it would be great if they can be included into the -mm tree and I can rebase them against this one.

Quoted 10 lines · On Wed, 24 Oct 2007 16:31:59 +0200 Thomas Renninger <[email protected]> wrote:

it seems Len's test tree and Linus tree diverged a bit, at least with this patch set things do not apply cleanly.

Therefore I post these for discussion whether and in which kernel tree they should end up before doing work for nothing.
If they are still a candidate for 2.6.24 (rather unintrusive), pls tell me whether and when I should base them against Len's test/release branch or whatever other tree.
If not, it would be great if they can be included into the -mm tree and I can rebase them against this one.

I staged the three acpi patches against Len's tree and I staged the hwmon patch against Mark's tree and I staged the I2C patch against Jean's tree.

This means that if/when the ACPI patches have gone me->Len->Linus, I can send the I2C patch to Jean and the hwmon patch to Mark and we're all good.

Quoted 16 lines · Andrew Morton

Quoted 10 lines · On Wed, 24 Oct 2007 16:31:59 +0200 Thomas Renninger <[email protected]> wrote:

it seems Len's test tree and Linus tree diverged a bit, at least with this patch set things do not apply cleanly.

Therefore I post these for discussion whether and in which kernel tree they should end up before doing work for nothing.
If they are still a candidate for 2.6.24 (rather unintrusive), pls tell me whether and when I should base them against Len's test/release branch or whatever other tree.
If not, it would be great if they can be included into the -mm tree and I can rebase them against this one.

I staged the three acpi patches against Len's tree and I staged the hwmon patch against Mark's tree and I staged the I2C patch against Jean's tree.

Quoted 2 lines

This means that if/when the ACPI patches have gone me->Len->Linus, I can send the I2C patch to Jean and the hwmon patch to Mark and we're all good.

Thanks for picking these patches, having them in -mm for some time is exactly what we need. Let's see how many systems are affected by the resource conflicts and how we can fix them

Quoted 3 lines · Jean Delvare

Thanks for picking these patches, having them in -mm for some time is exactly what we need. Let's see how many systems are affected by the resource conflicts and how we can fix them

No probs. The main thing is to ensure that we have sufficient debug support in that patch so that if a tester does report a problem, we (ie:
you ;) can resolve it on the first pass. So feel free to make it really noisy - we can always drop the debug stuff later on.

Also, please try to avoid adding anythig which would disrupt that tester from going on and testing all the other new code. ie: try to fail gracefully and fall back to the old behaviour.

Hi,

it seems Len's test tree and Linus tree diverged a bit, at least with this patch set things do not apply cleanly.

Therefore I post these for discussion whether and in which kernel tree they should end up before doing work for nothing.
If they are still a candidate for 2.6.24 (rather unintrusive), pls tell me whether and when I should base them against Len's test/release branch or whatever other tree.
If not, it would be great if they can be included into the -mm tree and I can rebase them against this one.

(...)

A boot parameter acpi_enforce_resources=strict/lax/no is provided, which is default set to lax:
- strict: let conflicting drivers fail to load with an error message
- lax: let conflicting driver work normal with a warning message
- no: no functional change at all
Depending on the feedback and the kind of interferences we see, this should be set to strict at later time.

Hi Thomas:

I recently told someone in private that ACPI vs. hwmon conflicts are the biggest open problems for the hwmon subsystem. Thank you (and Jean) for doing this.

Quoted 5 lines · Thomas Renninger

Hi,

it seems Len's test tree and Linus tree diverged a bit, at least with this patch set things do not apply cleanly.

Quoted 7 lines

Therefore I post these for discussion whether and in which kernel tree they should end up before doing work for nothing.
If they are still a candidate for 2.6.24 (rather unintrusive), pls tell me whether and when I should base them against Len's test/release branch or whatever other tree.
If not, it would be great if they can be included into the -mm tree and I can rebase them against this one.

Andrew has already picked this series; I vote for extended time in -mm. On the hwmon side, there is almost guaranteed to be fallout from this that may take time to resolve.

Quoted 7 lines

A boot parameter acpi_enforce_resources=strict/lax/no is provided, which is default set to lax:
- strict: let conflicting drivers fail to load with an error message
- lax: let conflicting driver work normal with a warning message
- no: no functional change at all
Depending on the feedback and the kind of interferences we see, this should be set to strict at later time.

As long as it's in -mm, you may as well default to =strict right away. This will force people to report. Open the floodgates; I hope I don't drown.

In ACPI, AML can define accesses to IO ports and System Memory by Operation Regions. Those are not registered as done by PNPACPI using resource templates (and _CRS/_SRS methods).
The IO ports and System Memory regions may get accessed by arbitrary AML code. When native drivers are accessing the same resources bad things can happen (e.g. a critical shutdown temperature of 3000 C every 2 months or so).

It is not really possible to register the operation regions via request_resource, as they often overlap with pnp or other resources (e.g. statically setup IO resources below 0x100).

This approach stores all Operation Region declarations (IO and System Memory only) at ACPI table parse time. It offers a similar functionality like request_region and let drivers which are known to possibly use the same IO ports and Memory which are also often used by ACPI (hwmon and i2c) check for ACPI interference.

A boot parameter acpi_enforce_resources=strict/lax/no is provided, which is default set to lax:
- strict: let conflicting drivers fail to load with an error message
- lax: let conflicting driver work normal with a warning message
- no: no functional change at all
Depending on the feedback and the kind of interferences we see, this should be set to strict at later time.

Goal of this patch set is:
- Identify ACPI interferences in bug reports (very hard to reproduce and to identify)
- Find BIOSes for that an ACPI driver should exist for specific HW instead of a native one.
- stability in general

Quoted 11 lines · Thomas Renninger

In ACPI, AML can define accesses to IO ports and System Memory by Operation Regions. Those are not registered as done by PNPACPI using resource templates (and _CRS/_SRS methods).
The IO ports and System Memory regions may get accessed by arbitrary AML code. When native drivers are accessing the same resources bad things can happen (e.g. a critical shutdown temperature of 3000 C every 2 months or so).

It is not really possible to register the operation regions via request_resource, as they often overlap with pnp or other resources (e.g. statically setup IO resources below 0x100).

But we really *should* reserve things used by opregions, shouldn't we? After all, the whole point of resource reservation is to prevent conflicts.

If these opregion resources aren't reserved, but are only put in the ACPI resource_list, we will only find conflicts with drivers that use acpi_check_resource_conflict(). Any driver unmodified by your changeset could still have a conflict, and we'd never find it.

Isn't the real problem that we have a bunch of drivers that use some of the same resources, and if ACPI reserved all the right resources, all those drivers would break?

I think it would be better to:

- Make ACPI reserve resources used by opregions unless

    acpi_enforce_resources == no.

- Change the drivers so if their request_region() fails, they check acpi_enforce_resources and either fail (in strict mode) or print the warning and continue (in lax mode).

Admittedly, this is harder for the drivers that use the shiny new platform device stuff.

Quoted 20 lines

This approach stores all Operation Region declarations (IO and System Memory only) at ACPI table parse time. It offers a similar functionality like request_region and let drivers which are known to possibly use the same IO ports and Memory which are also often used by ACPI (hwmon and i2c) check for ACPI interference.

A boot parameter acpi_enforce_resources=strict/lax/no is provided, which is default set to lax:
- strict: let conflicting drivers fail to load with an error message
- lax: let conflicting driver work normal with a warning message
- no: no functional change at all
Depending on the feedback and the kind of interferences we see, this should be set to strict at later time.

Goal of this patch set is:
- Identify ACPI interferences in bug reports (very hard to reproduce and to identify)
- Find BIOSes for that an ACPI driver should exist for specific HW instead of a native one.
- stability in general

Quoted 3 lines · Bjorn Helgaas

Isn't the real problem that we have a bunch of drivers that use some of the same resources, and if ACPI reserved all the right resources, all those drivers would break?

Yes, but:
It is really impossible to register them just like that.
I only tried on one machine I implemented on, Jean tested with more machines. Even there the resources did not only interfere, but overlap. The Operation Region declarations in ACPI don't belong to a real driver, but may be used in all kind of functions.

E.g. the thermal driver can't know which operation regions it may use later, e.g. by invoking _THM which in turn invokes a couple of other functions where IO ports are addressed via operation regions.

Also the BIOS developers seem to choose the regions in a very dump way sometimes.
Just some imaginary values, but I saw similar (overlapping):
- For a PNP device IO ports from 0x400-0x410 are reserved
- A operation region is declared from 0x399-0x401

Also the current request_resource interface (not the interface but the callers), need to be polished up first.
1) E.g. PNPACPI needs to dynamically allocate its resources (as we already discussed, I hope to be able to send something soon. Already works, but this will also affect PNPBIOS and ISAPNP, a lot old code and this needs careful review/testing).

2) I have no idea why e.g. i386/x86_64 kernel/setup.c code statically requests some ports below 0x100 and whether it can be ripped out or gets requested via the operation regions then in a sane way. I know they interfere on some systems with operation regions.

Quoted 11 lines · Thomas Renninger

Quoted 3 lines · Bjorn Helgaas

Isn't the real problem that we have a bunch of drivers that use some of the same resources, and if ACPI reserved all the right resources, all those drivers would break?

Yes, but:
It is really impossible to register them just like that.
I only tried on one machine I implemented on, Jean tested with more machines. Even there the resources did not only interfere, but overlap. The Operation Region declarations in ACPI don't belong to a real driver, but may be used in all kind of functions.

Can we reserve the resources when we register the opregion? Or does the opregion declaration just not tell us what resources it might use?

Quoted 3 lines

E.g. the thermal driver can't know which operation regions it may use later, e.g. by invoking _THM which in turn invokes a couple of other functions where IO ports are addressed via operation regions.

I would think the opregion resources should be reserved in connection with the opregion, not with drivers that use the opregion.

Quoted 6 lines

Also the BIOS developers seem to choose the regions in a very dump way sometimes.
Just some imaginary values, but I saw similar (overlapping):
- For a PNP device IO ports from 0x400-0x410 are reserved
- A operation region is declared from 0x399-0x401

If we know the resources the opregion can use, we can at least reserve the union of those used by the opregion and by other PNP devices. A little messy to deal with overlapping areas, I agree, but it should still be possible by shrinking the region or allocating a port at a time or something.

Quoted 6 lines

Also the current request_resource interface (not the interface but the callers), need to be polished up first.
1) E.g. PNPACPI needs to dynamically allocate its resources (as we already discussed, I hope to be able to send something soon. Already works, but this will also affect PNPBIOS and ISAPNP, a lot old code and this needs careful review/testing).

I have a patch for this (see below). It is risky, and I'm nervous about it. But the alternative (leaving PNP devices active but not reserving their resources, as we do today) is also risky.

If you have a similar patch in the works, I'd like to compare with mine.

Quoted 4 lines

2) I have no idea why e.g. i386/x86_64 kernel/setup.c code statically requests some ports below 0x100 and whether it can be ripped out or gets requested via the operation regions then in a sane way. I know they interfere on some systems with operation regions.

You mean the stuff in request_standard_resources()? I think that's legacy from before we had PNPBIOS and ACPI. Theoretically, we should be able to remove it if we have PNPBIOS or ACPI. But I think that would be too dangerous.

It should be safe to leave it -- it's OK if we reserve a little bit too much. The problem is if we reserve too little.

Bjorn

NOT FOR APPLICATION -- NOT FOR APPLICATION -- NOT FOR APPLICATION

PNP: request ioport and iomem resources used by active devices

For platform-type devices, PNP tells us what devices are present, whether they're active, and what resources they consume. To prevent conflicts, the PNP core should request the resources used by active devices before we assign resources to any other devices.

This overlaps with request_standard_resources(), which requests resources for a built-in list of "standard PC devices" such as DMA controllers, PICs, timers, keyboard, etc. PNP tells us which devices are actually present on a specific machine.

Sometimes the built-in standard resources are larger than what PNP reports, or they combine things that PNP reports separately, which makes things look a little funny:

    0000-001f : dma1		<-- built-in resource includes 2 controllers
      0000-000f : 00:02		<-- PNP reports only one DMA controller
    0020-0021 : pic1
    002e-002f : 00:06
    0040-0043 : timer0
    0050-0053 : timer1
    0060-006f : keyboard	<-- built-in resource groups several things
      0060-0060 : 00:04		<-- PNP reports 8042 controller data register
      0061-0061 : 00:03		<-- PNP reports AT-style speaker
      0064-0064 : 00:04		<-- PNP reports 8042 controller status register
    0070-0073 : 00:06
      0070-0071 : rtc

This doesn't mark the resources busy; they should be marked busy when a driver claims them. But if driver attempts to claim a region larger than what PNP reported, it will fail.

Index: w/drivers/pnp/core.c
===================================================================

drivers/pnp/core.c +3 -0

@@ -124,6 +124,9 @@
 	list_add_tail(&dev->protocol_list, &dev->protocol->devices);
 	spin_unlock(&pnp_lock);
 
+	if (dev->active)
+		pnp_request_resources(dev);
+
 	ret = device_register(&dev->dev);
 	if (ret)
 		return ret;

Index: w/drivers/pnp/resource.c
===================================================================

drivers/pnp/resource.c +67 -0

@@ -459,6 +459,73 @@
 #endif
 }
 
+int pnp_request_resources(struct pnp_dev *dev)
+{
+	int i, ret;
+	struct resource *res;
+
+	/*
+	 * We use insert_resource() rather than request_resource() because
+	 * request_standard_resources() has already requested some standard
+	 * areas that are described by PNP.
+	 */
+	for (i = 0; i < PNP_MAX_PORT; i++) {
+		if (pnp_port_valid(dev, i)) {
+			res = &dev->res.port_resource[i];
+			res->name = dev->dev.bus_id;
+			ret = insert_resource(&ioport_resource, res);
+			if (ret)
+				dev_warn(&dev->dev,
+					"can't allocate I/O ports at 0x%llx\n",
+					(unsigned long long) res->start);
+		}
+	}
+
+	for (i = 0; i < PNP_MAX_MEM; i++) {
+		if (pnp_mem_valid(dev, i)) {
+			res = &dev->res.mem_resource[i];
+			res->name = dev->dev.bus_id;
+			ret = insert_resource(&iomem_resource, res);
+			if (ret)
+				dev_warn(&dev->dev,
+					"can't allocate MMIO space at 0x%llx\n",
+					(unsigned long long) res->start);
+		}
+	}
+
+	return 0;
+}
+
+int pnp_release_resources(struct pnp_dev *dev)
+{
+	int i, ret;
+	struct resource *res;
+
+	for (i = 0; i < PNP_MAX_PORT; i++) {
+		if (pnp_port_valid(dev, i)) {
+			res = &dev->res.port_resource[i];
+			ret = release_resource(res);
+			if (ret)
+				dev_warn(&dev->dev,
+					"can't release I/O ports at 0x%llx\n",
+					(unsigned long long) res->start);
+		}
+	}
+
+	for (i = 0; i < PNP_MAX_MEM; i++) {
+		if (pnp_mem_valid(dev, i)) {
+			res = &dev->res.mem_resource[i];
+			ret = release_resource(res);
+			if (ret)
+				dev_warn(&dev->dev,
+					"can't release MMIO space at 0x%llx\n",
+					(unsigned long long) res->start);
+		}
+	}
+
+	return 0;
+}
+
 /* format is: pnp_reserve_irq=irq1[,irq2] .... */
 static int __init pnp_setup_reserve_irq(char *str)
 {

Index: w/drivers/pnp/base.h
===================================================================

drivers/pnp/base.h +2 -0

@@ -5,6 +5,8 @@
 void pnp_free_option(struct pnp_option *option);
 int __pnp_add_device(struct pnp_dev *dev);
 void __pnp_remove_device(struct pnp_dev *dev);
+int pnp_request_resources(struct pnp_dev *dev);
+int pnp_release_resources(struct pnp_dev *dev);
 
 int pnp_check_port(struct pnp_dev * dev, int idx);
 int pnp_check_mem(struct pnp_dev * dev, int idx);

Index: w/drivers/pnp/manager.c
===================================================================

drivers/pnp/manager.c +7 -3

@@ -391,9 +391,11 @@
 
 	if (!pnp_can_configure(dev))
 		return -ENODEV;
+
 	bak = pnp_alloc(sizeof(struct pnp_resource_table));
 	if (!bak)
 		return -ENOMEM;
+
 	*bak = dev->res;
 
 	down(&pnp_res_mutex);

@@ -463,7 +465,7 @@
  * pnp_start_dev - low-level start of the PnP device
  * @dev: pointer to the desired device
  *
- * assumes that resources have already been allocated
+ * assumes that resources have already been assigned to the device
  */
 int pnp_start_dev(struct pnp_dev *dev)
 {

@@ -472,6 +474,9 @@
 		return -EINVAL;
 	}
 
+	if (pnp_request_resources(dev))
+		dev_err(&dev->dev, "could not allocate resources\n");
+
 	if (dev->protocol->set(dev, &dev->res) < 0) {
 		dev_err(&dev->dev, "activation failed\n");
 		return -EIO;

@@ -484,8 +489,6 @@
 /**
  * pnp_stop_dev - low-level disable of the PnP device
  * @dev: pointer to the desired device
- *
- * does not free resources
  */
 int pnp_stop_dev(struct pnp_dev *dev)
 {

@@ -493,11 +496,14 @@
 		dev_dbg(&dev->dev, "disabling not supported\n");
 		return -EINVAL;
 	}
+

Quoted 8 lines · Bjorn Helgaas

Quoted 5 lines · Thomas Renninger

Also the BIOS developers seem to choose the regions in a very dump way sometimes.
Just some imaginary values, but I saw similar (overlapping):
- For a PNP device IO ports from 0x400-0x410 are reserved
- A operation region is declared from 0x399-0x401

Quoted 5 lines

If we know the resources the opregion can use, we can at least reserve the union of those used by the opregion and by other PNP devices. A little messy to deal with overlapping areas, I agree, but it should still be possible by shrinking the region or allocating a port at a time or something.

You mean merging them together to something like:
0x400-0x410 pnp device
0x399 ACPI Op region's name
This needs touching of kernel/resource.c and modification in a very ugly way affecting all architectures...

Maybe the list set up by this patch set should be similarly exported like /proc/ioports /proc/iomem
to where ever appropriate (maybe /sys/devices/system/acpi/opreg_io and /sys/devices/system/acpi/opreg_mem?) to get a better overview what kind of devices are served by the Op regions on different machines.

I think I worked on something else.
Do I get this right:
This one is to request all active resources of all ACPI devices that export resources via _CRS/_SRS?
As this is currently done for the motherboard devices only by pnp/system.c?

If you are interested I can try to fast cleanup and send you a pre-version of all patches (but not before beginning of next week).

Quoted 5 lines · Thomas Renninger

Maybe the list set up by this patch set should be similarly exported like /proc/ioports /proc/iomem
to where ever appropriate (maybe /sys/devices/system/acpi/opreg_io and /sys/devices/system/acpi/opreg_mem?) to get a better overview what kind of devices are served by the Op regions on different machines.

I want to try hard to avoid making ACPI a special case in this way. The current /proc/io{ports,mem} contains reservations for everything else. I'd hate to have to look two places just because of ACPI.

But we really *should* reserve things used by opregions, shouldn't we? After all, the whole point of resource reservation is to prevent conflicts.

Quoted 3 lines · Bjorn Helgaas

But we really *should* reserve things used by opregions, shouldn't we? After all, the whole point of resource reservation is to prevent conflicts.

Only if you're happy to lose functionality like IDE, sadly.

Quoted 6 lines · Matthew Garrett

Quoted 3 lines · Bjorn Helgaas

But we really *should* reserve things used by opregions, shouldn't we? After all, the whole point of resource reservation is to prevent conflicts.

Only if you're happy to lose functionality like IDE, sadly.

That's a simplistic answer to a complex problem. I don't think we should just ignore the whole problem, cross our fingers, and hope that firmware stays out of our way.

Quoted 6 lines · Matthew Garrett

Quoted 3 lines · Bjorn Helgaas

But we really *should* reserve things used by opregions, shouldn't we? After all, the whole point of resource reservation is to prevent conflicts.

Only if you're happy to lose functionality like IDE, sadly.

That's a simplistic answer to a complex problem. I don't think we should just ignore the whole problem, cross our fingers, and hope that firmware stays out of our way.

Hi,

it seems Len's test tree and Linus tree diverged a bit, at least with this patch set things do not apply cleanly.

Therefore I post these for discussion whether and in which kernel tree they should end up before doing work for nothing.
If they are still a candidate for 2.6.24 (rather unintrusive), pls tell me whether and when I should base them against Len's test/release branch or whatever other tree.
If not, it would be great if they can be included into the -mm tree and I can rebase them against this one.

Be aware that there is a small change in ACPICA (first patch, that's also the reason why this one would not compile on its own).

Many thanks for detailed review, testing and a lot implementation help go to Jean Delvare, without his help I would not be able to post anything right now.

Thanks for any help/advise,

Thomas

--------------
Short general description:

In ACPI, AML can define accesses to IO ports and System Memory by Operation Regions. Those are not registered as done by PNPACPI using resource templates (and _CRS/_SRS methods).
The IO ports and System Memory regions may get accessed by arbitrary AML code. When native drivers are accessing the same resources bad things can happen (e.g. a critical shutdown temperature of 3000 C every 2 months or so).
It is not really possible to register the operation regions via request_resource, as they often overlap with pnp or other resources (e.g. statically setup IO resources below 0x100).
This approach stores all Operation Region declarations (IO and System Memory only) at ACPI table parse time. It offers a similar functionality like request_region and let drivers which are known to possibly use the same IO ports and Memory which are also often used by ACPI (hwmon and i2c) check for ACPI interference.

A boot parameter acpi_enforce_resources=strict/lax/no is provided, which is default set to lax:
- strict: let conflicting drivers fail to load with an error message
- lax: let conflicting driver work normal with a warning message
- no: no functional change at all
Depending on the feedback and the kind of interferences we see, this should be set to strict at later time.

Goal of this patch set is:
- Identify ACPI interferences in bug reports (very hard to reproduce and to identify)
- Find BIOSes for that an ACPI driver should exist for specific HW instead of a native one.
- stability in general

-
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html