public inbox for linux-acpi@vger.kernel.org
 help / color / mirror / Atom feed
* Re: [lm-sensors] 2.6.24-rc4 hwmon it87 probe fails
@ 2007-12-21 19:00 Bjorn Helgaas
  2007-12-22 11:21 ` Jean Delvare
  0 siblings, 1 reply; 6+ messages in thread
From: Bjorn Helgaas @ 2007-12-21 19:00 UTC (permalink / raw)
  To: Jean Delvare
  Cc: Shaohua Li, Mike Houston, Adrian Bunk, Elvis Pranskevichus,
	mhoffman, linux-kernel, lm-sensors, Adam Belay, Zhao Yakui,
	Thomas Renninger, lenb, linux-acpi

On Tuesday 18 December 2007 10:59:18 am Jean Delvare wrote:
> My initial idea was to identify the faulty motherboard using DMI and to
> force pnpacpi=off on the faulty motherboards. If this is considered too
> aggressive, maybe we can just reject resource declarations that
> intersect (but don't match) 0x290-0x297 for these motherboards. Either
> way, we have to do something, and we have to do it quickly. 2.6.24
> final isn't too far away, and more importantly, the patch that revealed
> the problem has been backported to 2.6.23.10 so people are experiencing
> regressions already.

What do you think of something like the following patch?  If we do
this, I don't think we'd need to force pnpacpi=off or change the
way PNP reserves resources.

I'll be on vacation until about January 2, so I won't be able to
do much with this until then.



[patch] it87: request only Environment Controller ports

The IT8705F and related parts are Super I/O controllers that contain
many separate devices.

Some BIOSes describe IT8705F I/O port usage under a motherboard device
(PNP0C02) with overlapping regions, e.g., 0x290-0x29f and 0x290-0x294.

The it87 driver supports only the Environment Controller, which requires
only two ISA ports, but it used to request an eight-port range.  If that
range exceeds a range reported by the BIOS, as 0x290-0x297 would, the
request fails, and the it87 driver cannot claim the device.

This patch makes the it87 driver request only the two ports used for the
Environment Controller device.

Systems where this problem has been reported:
    Gigabyte GA-K8N Ultra 9
    Gigabyte M56S-S3
    Gigabyte GA-965G-DS3

Kernel bug reports:
    http://bugzilla.kernel.org/show_bug.cgi?id=9514
    http://lkml.org/lkml/2007/12/4/466

Related change:
    http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=a7839e960675b549f06209d18283d5cee2ce9261

    The patch above increases the number of PNP port resources we support.
    Prior to this patch, we ignored some port resources, which masked the
    it87 problem.

Signed-off-by: Bjorn Helgaas <bjorn.helgaas@hp.com>

Index: work4/drivers/hwmon/it87.c
===================================================================
--- work4.orig/drivers/hwmon/it87.c	2007-12-21 10:38:46.000000000 -0700
+++ work4/drivers/hwmon/it87.c	2007-12-21 11:43:50.000000000 -0700
@@ -2,6 +2,14 @@
     it87.c - Part of lm_sensors, Linux kernel modules for hardware
              monitoring.
 
+    The IT8705F is an LPC-based Super I/O part that contains UARTs, a
+    parallel port, an IR port, a MIDI port, a floppy controller, etc., in
+    addition to an Environment Controller (Enhanced Hardware Monitor and
+    Fan Controller)
+
+    This driver supports only the Environment Controller in the IT8705F and
+    similar parts.  The other devices are supported by different drivers.
+
     Supports: IT8705F  Super I/O chip w/LPC interface
               IT8712F  Super I/O chip w/LPC interface
               IT8716F  Super I/O chip w/LPC interface
@@ -118,9 +126,15 @@
 /* Length of ISA address segment */
 #define IT87_EXTENT 8
 
-/* Where are the ISA address/data registers relative to the base address */
-#define IT87_ADDR_REG_OFFSET 5
-#define IT87_DATA_REG_OFFSET 6
+/* Length of ISA address segment for Environmental Controller */
+#define IT87_EC_EXTENT 2
+
+/* Offset of EC registers from ISA base address */
+#define IT87_EC_OFFSET 5
+
+/* Where are the ISA address/data registers relative to the EC base address */
+#define IT87_ADDR_REG_OFFSET 0
+#define IT87_DATA_REG_OFFSET 1
 
 /*----- The IT87 registers -----*/
 
@@ -968,10 +982,10 @@
 	};
 
 	res = platform_get_resource(pdev, IORESOURCE_IO, 0);
-	if (!request_region(res->start, IT87_EXTENT, DRVNAME)) {
+	if (!request_region(res->start, IT87_EC_EXTENT, DRVNAME)) {
 		dev_err(dev, "Failed to request region 0x%lx-0x%lx\n",
 			(unsigned long)res->start,
-			(unsigned long)(res->start + IT87_EXTENT - 1));
+			(unsigned long)(res->start + IT87_EC_EXTENT - 1));
 		err = -EBUSY;
 		goto ERROR0;
 	}
@@ -1124,7 +1138,7 @@
 	platform_set_drvdata(pdev, NULL);
 	kfree(data);
 ERROR1:
-	release_region(res->start, IT87_EXTENT);
+	release_region(res->start, IT87_EC_EXTENT);
 ERROR0:
 	return err;
 }
@@ -1137,7 +1151,7 @@
 	sysfs_remove_group(&pdev->dev.kobj, &it87_group);
 	sysfs_remove_group(&pdev->dev.kobj, &it87_group_opt);
 
-	release_region(data->addr, IT87_EXTENT);
+	release_region(data->addr, IT87_EC_EXTENT);
 	platform_set_drvdata(pdev, NULL);
 	kfree(data);
 
@@ -1402,8 +1416,8 @@
 				  const struct it87_sio_data *sio_data)
 {
 	struct resource res = {
-		.start	= address ,
-		.end	= address + IT87_EXTENT - 1,
+		.start	= address + IT87_EC_OFFSET,
+		.end	= address + IT87_EC_OFFSET + IT87_EC_EXTENT - 1,
 		.name	= DRVNAME,
 		.flags	= IORESOURCE_IO,
 	};

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [lm-sensors] 2.6.24-rc4 hwmon it87 probe fails
@ 2007-12-23 23:14 Bjorn Helgaas
  2007-12-25 21:31 ` Jean Delvare
  0 siblings, 1 reply; 6+ messages in thread
From: Bjorn Helgaas @ 2007-12-23 23:14 UTC (permalink / raw)
  To: Jean Delvare
  Cc: Shaohua Li, Mike Houston, Adrian Bunk, Elvis Pranskevichus,
	Mark M. Hoffman, linux-kernel, lm-sensors, Adam Belay, Zhao Yakui,
	Thomas Renninger, lenb, linux-acpi

On Sunday 23 December 2007 2:28:05 am Jean Delvare wrote:
> Le 23/12/2007, Bjorn Helgaas a écrit:
> >On Saturday 22 December 2007 4:21:41 am Jean Delvare wrote:
> >> >This patch makes the it87 driver request only the two ports used for
> >> > the Environment Controller device.
> >>
> >> The problem is that the IT87xxF chips do decode 4 ports (recent chips,
> >> 0x294-0x297) or 8 ports (older chips, 0x290-0x297), not 2 as the
> >> datasheets say. The ITE Super-I/O chips differ from the Winbond
> >> Super-I/O chips in this respect. The it87 driver only needs to access
> >> ports 0x295 and 0x296, true, but the device itself decodes more
> >> addresses than that. So, with this proposed patch, ports which are busy
> >> will be shown as being free. This pretty much voids the point of
> >> resource declarations, doesn't it? This might not cause too much
> >> trouble in practice, but to me this still looks like the wrong way to
> >> go.
> >
> >Yes, all the ports decoded by the chip should certainly be reserved,
> >but I think the entire range should be reserved at a higher level,
> >like the PNP core, and the driver should reserve only the ports it
> >uses.  Then the entire range is reserved even if there is no driver
> >or the driver is not loaded.
>
> The problem is that the it87 driver is used on a variety of motherboards,
> some where the hardware monitoring device I/O ports are reserved by the
> BIOS, some where they aren't. How am I supposed to deal with this?

I assume you mean some boards describe those ports in ACPI, and some
don't?  I think the ideal solution would be to figure out how the
BIOS writers intended the device to be used, and do that.  But the
documentation may be lacking, and it degenerates into an OEM-specific
mess.  Second-best seems like a PNP quirk that pokes enough to determine
that a Super I/O device is present, and then reserves resources for
it.  Then we avoid the collision even if it87 isn't present.

> >That's the approach we use for PCI, e.g.,
> >
> >  e8100000-e81fffff : 0000:00:08.0          <-- reserved by PCI core
> >    e8100000-e8102fff : CS46xx_BA1_data0    <-- reserved by driver
> >    e8110000-e81137ff : CS46xx_BA1_data1
> >    e8120000-e8126fff : CS46xx_BA1_pmem
> >    e8130000-e81300ff : CS46xx_BA1_reg
>
> PCI is an entirely different beast. With PCI you know the PCI device ID
> that is associated with the resources, and for a given device, the
> resources are always declared (if standard BARs are used) or never
> declared; there's no "may be". So it's much easier to handle the
> resources properly.

That's the way PNP is supposed to work, too (more about this below).

> >> The resource declarations made by these motherboard BIOSes are totally
> >> bogus. 0x290-0x29f is much larger than what the chip decodes.
> >> 0x290-0x294 is a subset that doesn't make any sense. The GA-K8N Ultra 9
> >> is even funnier with 0x295-0x314, which again doesn't correspond to
> >> anything real.
> >
> >I agree those declarations are probably wrong.  But at least they're
> >larger than required, so they should be safe.
>
> That's not really safe, no. They may overlap with other device resources
> and prevent those drivers from loading.

True.  If that happens, I think we definitely need some kind of DMI-
based quirk to fix the resources.  My points are (a) the quirk needs
to be at the PNP level; it can't be in a driver, and (b) I'm lazy and
would wait until a problem happens before implementing it, because I
know so little about PNP and the specific board, and by waiting, there's
a chance I'll learn enough to avoid a mistake :-)

> >> All these happen to not intersect with 0x295-0x296 but I
> >> wouldn't count on it. If Gigabyte (and possibly others) care that
> >> little about these declarations, pretty much anything could be seen. So
> >> while your proposed workaround happens to fix the problem at hand, it is
> >> not really correct technically, and could break again soon.
> >>
> >> I'd rather fix the problem at its source, or, as fixing it as the source
> >> isn't very realistic in this case, as near of the source as possible.
> >> That would mean DMI-based quirks for the affected motherboards, that
> >> would discard or adjust the bogus resource declarations.
> >
> >Well, I think the driver change *is* correct, assuming that the
> >entire range can be reserved at a higher level.  In this case,
> >it is, via a PNP0C02 device.
>
> As I wrote above, the problem is that you can't assume that. Some
> motherboards do declare the range at PNP level but some don't. That's
> the reason why the it87 driver (and most other hwmon drivers for
> Super-I/O devices) do declare the I/O resource again.

The overlapping device problem is a subsystem problem, not a
driver problem.  We can't solve it in the driver because we can't
depend on the it87 driver being loaded.

> Another problem is how do I connect this specific I/O port range of the
> PNP0C02 device with the it87 driver? I am by far no PNP expert but it
> looks to me like this PNP device covers more than one actual device, and
> I/O port ranges do not have labels nor identifiers that would let me
> find the one that corresponds to the IT87xxF device (if it exists at
> all.)

The general rule is that you have a PNP device with an identifier
like PNP0500 that means "16550 UART" and some resources underneath
it.  The PNP ID ("PNP0500") tells the OS which driver to bind to the
device, and the resources tell the driver where the device is.  The
serial driver in drivers/serial/8250_pnp.c is a good example of the
"normal" way PNP drivers work.

it87 doesn't fit the model very well because usually the BIOS
doesn't describe the device explicitly.  I think the expectation
is that the OS will get sensor readings some other way, such as
by evaluating ACPI "_TMP" methods, or by using some OEM-specific
ACPI device.

It's very irritating when ACPI is used to take some device that
would otherwise be nicely generic and machine-independent, and
wrap it up into some abstract device that requires an OEM-specific
driver, but I think that's what's happening here.  I suspect it
has to do with the fact that the BIOS wants to retain some
control over the device so it can deal with things like thermal
emergencies, even if the OS driver is missing or broken.

> I'm all for integrating the it87 driver into the PNP subsystem if it is
> going to solve problems, but I just don't know how it works. I you do
> some work in this direction, I'll be happy to help with reviews and
> testing.

You don't see how it works because the BIOS writers have deliberately
obscured things (though no doubt they had good reasons).

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2008-01-12  9:57 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
     [not found] <fa.QPUBl9Xd2PDsImgWn6hbR+ShV1U@ifi.uio.no>
     [not found] ` <fa.ri5Klmu4+MwjYC8x5nSms+xKXfI@ifi.uio.no>
     [not found]   ` <fa.B/J+j9kGGZ1+gW9FrfHky+cj0Eo@ifi.uio.no>
     [not found]     ` <fa.Xv3BmMxnCGLOeEijySte5mKDO5k@ifi.uio.no>
2007-12-20  1:09       ` [lm-sensors] 2.6.24-rc4 hwmon it87 probe fails Robert Hancock
2008-01-12  9:56         ` Jean Delvare
2007-12-21 19:00 [lm-sensors] " Bjorn Helgaas
2007-12-22 11:21 ` Jean Delvare
  -- strict thread matches above, loose matches on Subject: below --
2007-12-23 23:14 [lm-sensors] " Bjorn Helgaas
2007-12-25 21:31 ` Jean Delvare
2008-01-02 18:30   ` Bjorn Helgaas
2008-01-12  9:49     ` Jean Delvare

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox