[PATCH] Request only really used I/O ports in w83627hf driver

[PATCH] Request only really used I/O ports in w83627hf driver

[Petr Vandrovec]

Hello,
  my motherboard (Tyan S2885) reports range 295-296 in its PNP hardware
descriptors, and due to this w83627hf driver fails to load, as it requests
290-297 range, which is not subrange of this PNP resource.  As hardware 
monitor chip responds to 295/296 addresses only, there is no reason to 
request full 8 byte I/O.

  While I was doing that, I also changed W83781D_*_REG_OFFSET definitions
from 5/6 to 0/1.  Code is a bit smaller after doing that, and it looks
better now since we do not allocate full 8 byte range.

  cat /proc/ioports is now much happier and monitor finally works.
...
0295-0296 : pnp 00:09
  0295-0296 : w83627hf
...

						Thanks,
							Petr Vandrovec

Signed-off-by:  Petr Vandrovec <vandrove@vc.cvut.cz>


diff -urN linux-2.6.13-5bca.dist/drivers/hwmon/w83627hf.c linux-2.6.13-5bca/drivers/hwmon/w83627hf.c
--- linux-2.6.13-5bca.dist/drivers/hwmon/w83627hf.c     2005-09-06 13:50:03.000000000 +0200
+++ linux-2.6.13-5bca/drivers/hwmon/w83627hf.c  2005-09-07 19:54:08.000000000 +0200
@@ -138,12 +138,16 @@
 #define WINB_BASE_REG 0x60
 /* Constants specified below */
 
-/* Length of ISA address segment */
-#define WINB_EXTENT 8
+/* Alignment of ISA address */
+#define WINB_ALIGNMENT		~7
 
-/* Where are the ISA address/data registers relative to the base address */
-#define W83781D_ADDR_REG_OFFSET 5
-#define W83781D_DATA_REG_OFFSET 6
+/* Offset & size of I/O region we are interested in */
+#define WINB_REGION_OFFSET	5
+#define WINB_REGION_SIZE	2
+
+/* Where are the ISA address/data registers relative to the region start */
+#define W83781D_ADDR_REG_OFFSET 0
+#define W83781D_DATA_REG_OFFSET 1
 
 /* The W83781D registers */
 /* The W83782D registers for nr=7,8 are in bank 5 */
@@ -977,7 +981,7 @@
 	superio_select(W83627HF_LD_HWM);
 	val = (superio_inb(WINB_BASE_REG) << 8) |
 	       superio_inb(WINB_BASE_REG + 1);
-	*addr = val & ~(WINB_EXTENT - 1);
+	*addr = val & WINB_ALIGNMENT;
 	if (*addr == 0 && force_addr == 0) {
 		superio_exit();
 		return -ENODEV;
@@ -994,11 +998,13 @@
 	struct w83627hf_data *data;
 	int err = 0;
 	const char *client_name = "";
+	unsigned short addr;
 
 	if(force_addr)
-		address = force_addr & ~(WINB_EXTENT - 1);
+		address = force_addr & WINB_ALIGNMENT;
+	addr = address + WINB_REGION_OFFSET;
 
-	if (!request_region(address, WINB_EXTENT, w83627hf_driver.name)) {
+	if (!request_region(addr, WINB_REGION_SIZE, w83627hf_driver.name)) {
 		err = -EBUSY;
 		goto ERROR0;
 	}
@@ -1045,7 +1051,7 @@
 
 	new_client = &data->client;
 	i2c_set_clientdata(new_client, data);
-	new_client->addr = address;
+	new_client->addr = addr;
 	init_MUTEX(&data->lock);
 	new_client->adapter = adapter;
 	new_client->driver = &w83627hf_driver;
@@ -1144,7 +1150,7 @@
       ERROR2:
 	kfree(data);
       ERROR1:
-	release_region(address, WINB_EXTENT);
+	release_region(addr, WINB_REGION_SIZE);
       ERROR0:
 	return err;
 }
@@ -1159,7 +1165,7 @@
 	if ((err = i2c_detach_client(client)))
 		return err;
 
-	release_region(client->addr, WINB_EXTENT);
+	release_region(client->addr, WINB_REGION_SIZE);
 	kfree(data);
 
 	return 0;

[lm-sensors] [PATCH] Request only really used I/O ports in w83627hf

[Petr Vandrovec]

Hello,
  my motherboard (Tyan S2885) reports range 295-296 in its PNP hardware
descriptors, and due to this w83627hf driver fails to load, as it requests
290-297 range, which is not subrange of this PNP resource.  As hardware 
monitor chip responds to 295/296 addresses only, there is no reason to 
request full 8 byte I/O.

  While I was doing that, I also changed W83781D_*_REG_OFFSET definitions
from 5/6 to 0/1.  Code is a bit smaller after doing that, and it looks
better now since we do not allocate full 8 byte range.

  cat /proc/ioports is now much happier and monitor finally works.
...
0295-0296 : pnp 00:09
  0295-0296 : w83627hf
...

						Thanks,
							Petr Vandrovec

Signed-off-by:  Petr Vandrovec <vandrove@vc.cvut.cz>


diff -urN linux-2.6.13-5bca.dist/drivers/hwmon/w83627hf.c linux-2.6.13-5bca/drivers/hwmon/w83627hf.c
--- linux-2.6.13-5bca.dist/drivers/hwmon/w83627hf.c     2005-09-06 13:50:03.000000000 +0200
+++ linux-2.6.13-5bca/drivers/hwmon/w83627hf.c  2005-09-07 19:54:08.000000000 +0200
@@ -138,12 +138,16 @@
 #define WINB_BASE_REG 0x60
 /* Constants specified below */
 
-/* Length of ISA address segment */
-#define WINB_EXTENT 8
+/* Alignment of ISA address */
+#define WINB_ALIGNMENT		~7
 
-/* Where are the ISA address/data registers relative to the base address */
-#define W83781D_ADDR_REG_OFFSET 5
-#define W83781D_DATA_REG_OFFSET 6
+/* Offset & size of I/O region we are interested in */
+#define WINB_REGION_OFFSET	5
+#define WINB_REGION_SIZE	2
+
+/* Where are the ISA address/data registers relative to the region start */
+#define W83781D_ADDR_REG_OFFSET 0
+#define W83781D_DATA_REG_OFFSET 1
 
 /* The W83781D registers */
 /* The W83782D registers for nr=7,8 are in bank 5 */
@@ -977,7 +981,7 @@
 	superio_select(W83627HF_LD_HWM);
 	val = (superio_inb(WINB_BASE_REG) << 8) |
 	       superio_inb(WINB_BASE_REG + 1);
-	*addr = val & ~(WINB_EXTENT - 1);
+	*addr = val & WINB_ALIGNMENT;
 	if (*addr = 0 && force_addr = 0) {
 		superio_exit();
 		return -ENODEV;
@@ -994,11 +998,13 @@
 	struct w83627hf_data *data;
 	int err = 0;
 	const char *client_name = "";
+	unsigned short addr;
 
 	if(force_addr)
-		address = force_addr & ~(WINB_EXTENT - 1);
+		address = force_addr & WINB_ALIGNMENT;
+	addr = address + WINB_REGION_OFFSET;
 
-	if (!request_region(address, WINB_EXTENT, w83627hf_driver.name)) {
+	if (!request_region(addr, WINB_REGION_SIZE, w83627hf_driver.name)) {
 		err = -EBUSY;
 		goto ERROR0;
 	}
@@ -1045,7 +1051,7 @@
 
 	new_client = &data->client;
 	i2c_set_clientdata(new_client, data);
-	new_client->addr = address;
+	new_client->addr = addr;
 	init_MUTEX(&data->lock);
 	new_client->adapter = adapter;
 	new_client->driver = &w83627hf_driver;
@@ -1144,7 +1150,7 @@
       ERROR2:
 	kfree(data);
       ERROR1:
-	release_region(address, WINB_EXTENT);
+	release_region(addr, WINB_REGION_SIZE);
       ERROR0:
 	return err;
 }
@@ -1159,7 +1165,7 @@
 	if ((err = i2c_detach_client(client)))
 		return err;
 
-	release_region(client->addr, WINB_EXTENT);
+	release_region(client->addr, WINB_REGION_SIZE);
 	kfree(data);
 
 	return 0;

Re: [PATCH] Request only really used I/O ports in w83627hf driver

[Jean Delvare]

Hi Petr,

> my motherboard (Tyan S2885) reports range 295-296 in its PNP hardware
> descriptors, and due to this w83627hf driver fails to load, as it
> requests 290-297 range, which is not subrange of this PNP resource. 
> As hardware monitor chip responds to 295/296 addresses only, there is
> no reason to request full 8 byte I/O.

Another point of view would be: as the physical address of the chip is
0x290-0x297, there is no reason to even think of requesting a different
range. And there is a very valid reason to request the full 8 byte I/O
range: to let the user know that this range is not available for other
devices. Mapping another device to the unused I/O ports of the W83627HF
would not work, right? Also consider that future chips of this family
could use additional ports.

The cause of your trouble is, IMVHO, a buggy BIOS. Ask Tyan to fix their
BIOS to allocate the real I/O range to the W83627HF chip, and you're
done.

http://bugzilla.kernel.org/show_bug.cgi?id=4014

Your fix might come in handy for your own situation, but it looks wrong
in the long run. We are not going to shrink the requested I/O range of
every random driver each time a motherboard manufacturer releases a
buggy BIOS.

If getting the manufacturers to provide fixed BIOSes doesn't seem to be
feasable, then the PNPACPI code certainly needs to be extended to handle
this case transparently. This could be achived using quirks similar to
what we have for PCI, or PNPACPI could simply accept requests of I/O
ranges which include a single PNP range as defined by the BIOS. Note
that everything was working just fine for everyone before PNPACPI was
added to the kernel.

> While I was doing that, I also changed W83781D_*_REG_OFFSET definitions
> from 5/6 to 0/1.  Code is a bit smaller after doing that, and it looks
> better now since we do not allocate full 8 byte range.

I find this change rather confusing myself, and it makes the code
bigger, not smaller.

-- 
Jean Delvare

[lm-sensors] Re: [PATCH] Request only really used I/O ports in

[Jean Delvare]

Hi Petr,

> my motherboard (Tyan S2885) reports range 295-296 in its PNP hardware
> descriptors, and due to this w83627hf driver fails to load, as it
> requests 290-297 range, which is not subrange of this PNP resource. 
> As hardware monitor chip responds to 295/296 addresses only, there is
> no reason to request full 8 byte I/O.

Another point of view would be: as the physical address of the chip is
0x290-0x297, there is no reason to even think of requesting a different
range. And there is a very valid reason to request the full 8 byte I/O
range: to let the user know that this range is not available for other
devices. Mapping another device to the unused I/O ports of the W83627HF
would not work, right? Also consider that future chips of this family
could use additional ports.

The cause of your trouble is, IMVHO, a buggy BIOS. Ask Tyan to fix their
BIOS to allocate the real I/O range to the W83627HF chip, and you're
done.

http://bugzilla.kernel.org/show_bug.cgi?id@14

Your fix might come in handy for your own situation, but it looks wrong
in the long run. We are not going to shrink the requested I/O range of
every random driver each time a motherboard manufacturer releases a
buggy BIOS.

If getting the manufacturers to provide fixed BIOSes doesn't seem to be
feasable, then the PNPACPI code certainly needs to be extended to handle
this case transparently. This could be achived using quirks similar to
what we have for PCI, or PNPACPI could simply accept requests of I/O
ranges which include a single PNP range as defined by the BIOS. Note
that everything was working just fine for everyone before PNPACPI was
added to the kernel.

> While I was doing that, I also changed W83781D_*_REG_OFFSET definitions
> from 5/6 to 0/1.  Code is a bit smaller after doing that, and it looks
> better now since we do not allocate full 8 byte range.

I find this change rather confusing myself, and it makes the code
bigger, not smaller.

-- 
Jean Delvare

Re: [PATCH] Request only really used I/O ports in w83627hf driver

[Petr Vandrovec]

Jean Delvare wrote:
> Hi Petr,
> 
> 
>>my motherboard (Tyan S2885) reports range 295-296 in its PNP hardware
>>descriptors, and due to this w83627hf driver fails to load, as it
>>requests 290-297 range, which is not subrange of this PNP resource. 
>>As hardware monitor chip responds to 295/296 addresses only, there is
>>no reason to request full 8 byte I/O.
> 
> 
> Another point of view would be: as the physical address of the chip is
> 0x290-0x297, there is no reason to even think of requesting a different
> range. And there is a very valid reason to request the full 8 byte I/O
> range: to let the user know that this range is not available for other
> devices. Mapping another device to the unused I/O ports of the W83627HF
> would not work, right? Also consider that future chips of this family
> could use additional ports.

No, it would perfectly work.  W83627HF (and all other hardware monitoring
chips from Winbond) respond only to xxx5 and xxx6 address, not to the
other addresses in the range xxx0-xxx7.  So requesting full 8 byte range
is not only unnecessary, but also incorrect.

For example I can place 4-byte printer port at 0xC00-0xC03, while sensors
live at 0xC05-0xC06.  If I place sensors and serial port at same address,
I get bytes (from C00-C07) "04 00 01 00 13 40 01 00".  When I move senors
away, I get "04 00 01 00 13 06 00 00", so you can see that only address + 5
and + 6 are affected.  And when I move serial port away, leaving sensors
in place, I see "FF FF FF FF FF 40 01 FF" - again chip respons to +5 & +6
addresses only, not to the full range.

And even if it would answer to the other addresses, you should not request
them, as driver apparently does not need them, while some other driver might.
Take a look at i2c-amd756 driver for example.  It uses bytes 0xE0-0xEF
from large 256 byte I/O window on amd7x6/amd8111 chips.  And it correctly
requests only region 0xE0-0xEF, so other drivers can drive other parts
of the chip.

> The cause of your trouble is, IMVHO, a buggy BIOS. Ask Tyan to fix their
> BIOS to allocate the real I/O range to the W83627HF chip, and you're
> done.
> 
> http://bugzilla.kernel.org/show_bug.cgi?id=4014

I do not agree with your analysis here.  Chip just wants 8 byte aligned
address, nowhere in the documentation is stated that full 8 bytes are
used, actually doc is quite explicit that accesses are decoded only if
low nibble is 5 or 6 (for example w83627hf/f/hg/g datasheet revision A1,
page 28).

> Your fix might come in handy for your own situation, but it looks wrong
> in the long run. We are not going to shrink the requested I/O range of
> every random driver each time a motherboard manufacturer releases a
> buggy BIOS.

It is not buggy BIOS.  It is incorrect assumption of driver writter about
hardware, and about request_region API.  If you want to put potential
resources to the tree, you should not tag them BUSY, as they are not
busy, they are free for use by the driver.

> If getting the manufacturers to provide fixed BIOSes doesn't seem to be
> feasable, then the PNPACPI code certainly needs to be extended to handle
> this case transparently. This could be achived using quirks similar to
> what we have for PCI, or PNPACPI could simply accept requests of I/O
> ranges which include a single PNP range as defined by the BIOS. Note
> that everything was working just fine for everyone before PNPACPI was
> added to the kernel.

No.  Unfortunately BIOS knows how hardware works while w83627hf driver
apparently does not, so I do not think manufacturers are going to break
BIOS just to match Linux driver expectations.
						Petr Vandrovec

[lm-sensors] Re: [PATCH] Request only really used I/O ports in

[Petr Vandrovec]

Jean Delvare wrote:
> Hi Petr,
> 
> 
>>my motherboard (Tyan S2885) reports range 295-296 in its PNP hardware
>>descriptors, and due to this w83627hf driver fails to load, as it
>>requests 290-297 range, which is not subrange of this PNP resource. 
>>As hardware monitor chip responds to 295/296 addresses only, there is
>>no reason to request full 8 byte I/O.
> 
> 
> Another point of view would be: as the physical address of the chip is
> 0x290-0x297, there is no reason to even think of requesting a different
> range. And there is a very valid reason to request the full 8 byte I/O
> range: to let the user know that this range is not available for other
> devices. Mapping another device to the unused I/O ports of the W83627HF
> would not work, right? Also consider that future chips of this family
> could use additional ports.

No, it would perfectly work.  W83627HF (and all other hardware monitoring
chips from Winbond) respond only to xxx5 and xxx6 address, not to the
other addresses in the range xxx0-xxx7.  So requesting full 8 byte range
is not only unnecessary, but also incorrect.

For example I can place 4-byte printer port at 0xC00-0xC03, while sensors
live at 0xC05-0xC06.  If I place sensors and serial port at same address,
I get bytes (from C00-C07) "04 00 01 00 13 40 01 00".  When I move senors
away, I get "04 00 01 00 13 06 00 00", so you can see that only address + 5
and + 6 are affected.  And when I move serial port away, leaving sensors
in place, I see "FF FF FF FF FF 40 01 FF" - again chip respons to +5 & +6
addresses only, not to the full range.

And even if it would answer to the other addresses, you should not request
them, as driver apparently does not need them, while some other driver might.
Take a look at i2c-amd756 driver for example.  It uses bytes 0xE0-0xEF
from large 256 byte I/O window on amd7x6/amd8111 chips.  And it correctly
requests only region 0xE0-0xEF, so other drivers can drive other parts
of the chip.

> The cause of your trouble is, IMVHO, a buggy BIOS. Ask Tyan to fix their
> BIOS to allocate the real I/O range to the W83627HF chip, and you're
> done.
> 
> http://bugzilla.kernel.org/show_bug.cgi?id@14

I do not agree with your analysis here.  Chip just wants 8 byte aligned
address, nowhere in the documentation is stated that full 8 bytes are
used, actually doc is quite explicit that accesses are decoded only if
low nibble is 5 or 6 (for example w83627hf/f/hg/g datasheet revision A1,
page 28).

> Your fix might come in handy for your own situation, but it looks wrong
> in the long run. We are not going to shrink the requested I/O range of
> every random driver each time a motherboard manufacturer releases a
> buggy BIOS.

It is not buggy BIOS.  It is incorrect assumption of driver writter about
hardware, and about request_region API.  If you want to put potential
resources to the tree, you should not tag them BUSY, as they are not
busy, they are free for use by the driver.

> If getting the manufacturers to provide fixed BIOSes doesn't seem to be
> feasable, then the PNPACPI code certainly needs to be extended to handle
> this case transparently. This could be achived using quirks similar to
> what we have for PCI, or PNPACPI could simply accept requests of I/O
> ranges which include a single PNP range as defined by the BIOS. Note
> that everything was working just fine for everyone before PNPACPI was
> added to the kernel.

No.  Unfortunately BIOS knows how hardware works while w83627hf driver
apparently does not, so I do not think manufacturers are going to break
BIOS just to match Linux driver expectations.
						Petr Vandrovec

Re: [PATCH] Request only really used I/O ports in w83627hf driver

[Jean Delvare]

Hi Petr,

Sorry for the delay.

> No, it would perfectly work.  W83627HF (and all other hardware monitoring
> chips from Winbond) respond only to xxx5 and xxx6 address, not to the
> other addresses in the range xxx0-xxx7. So requesting full 8 byte range
> is not only unnecessary, but also incorrect.

Incorrect WRT to which specification or coding rules guide, please?

> For example I can place 4-byte printer port at 0xC00-0xC03, while sensors
> live at 0xC05-0xC06.  If I place sensors and serial port at same address,
> I get bytes (from C00-C07) "04 00 01 00 13 40 01 00".  When I move senors
> away, I get "04 00 01 00 13 06 00 00", so you can see that only address + 5
> and + 6 are affected.  And when I move serial port away, leaving sensors
> in place, I see "FF FF FF FF FF 40 01 FF" - again chip respons to +5 & +6
> addresses only, not to the full range.

I see a problem with your demonstration. Your 4-byte printer port was
seemingly affecting not only 0xC00-0xC03, but also 0xC04 and 0xC07. Had
you tried without sensors in the way, I bet we would have seen ports
0xC05 and 0xC06 change values as well. This means that your printer port
does answer to reads on I/O ports it has no use for. This is exactly
what I (erroneously) thought would happen with the W83627HF chip.

I'm no electronics expert (far from that actually) but isn't it a
problem, from an electronic point of view, to have two devices
competing for I/O ports? What if your printer port were "stronger" than
your W83627HF? My guess is that the latter wouldn't work anymore.

I just checked on an IT8705F Super-I/O with hardware monitoring. It has
an 8-byte I/O region with +5 and +6 only used, just like the W836x7HF
family. The device presence changes the I/O region from:
  ff ff ff ff ff ff ff ff
to:
  04 04 04 04 04 51 04 04
So, the IT8705F definitely answers on the full I/O range, even ports it
isn't supposed to care about, just like the printer port in your
example. Same is true of the IT8712F, BTW.

> And even if it would answer to the other addresses, you should not request
> them, as driver apparently does not need them, while some other driver might.

This is a non-issue until this hypothetical other driver exists.

> Take a look at i2c-amd756 driver for example.  It uses bytes 0xE0-0xEF
> from large 256 byte I/O window on amd7x6/amd8111 chips.  And it correctly
> requests only region 0xE0-0xEF, so other drivers can drive other parts
> of the chip.

In this case, the rest of the region has other uses, and other drivers
actually need it, so this is a different situation. As far as I can see,
no driver is going to need the leftover I/O ports from hardware
monitoring chips. If these ports are ever used at some point in time,
we can reasonably assume that the new feature needing them will belong
to the same hardware monitoring driver.

> I do not agree with your analysis here.  Chip just wants 8 byte aligned
> address, nowhere in the documentation is stated that full 8 bytes are
> used, actually doc is quite explicit that accesses are decoded only if
> low nibble is 5 or 6 (for example w83627hf/f/hg/g datasheet revision A1,
> page 28).

This is a valuable technical information, and it conforms to your
experimental results above. This seems to differ from the behavior of
the IT87xxF chips and your printer port.

> It is not buggy BIOS.  It is incorrect assumption of driver writter about
> hardware, and about request_region API.  If you want to put potential
> resources to the tree, you should not tag them BUSY, as they are not
> busy, they are free for use by the driver.

There are plenty of drivers out there which request more I/O ports than
they strictly need, just because the author knew that no other driver
would ever need these. Reasons to do this are multiple: sparse used I/O
ports (you're not going to call request_region 3 times if you need 3 I/O
ports spread over a large I/O region), plans to implement more features
in that driver in the future, anticipation of hardware evolution. You
really can't blame the driver writer.

> No.  Unfortunately BIOS knows how hardware works while w83627hf driver
> apparently does not, so I do not think manufacturers are going to break
> BIOS just to match Linux driver expectations.

Sure, BIOSes are never broken :)

My summarized impression is that what matters here is not the fact that
a given driver does or does not need a given I/O port, but the fact
that a given chip will or will not decode the I/O requests for all
ports within its base I/O range. I agree it does make sense, and should
be safe, not to request I/O ports that are not *decoded* by a chip.
OTOH, I would expect problems to arise if two devices have intersecting
base I/O ranges and at least one I/O port is decoded by both devices. I
would expect even more problems if one device does use that one I/O
port.

This leads me to the following conclusions:
1* Reserving only 0x295-0x296 for the W836x7HF family of chips should
be safe, if and only if all other drivers do reserve all I/O ports they
decode, as opposed to only I/O ports they have a use for.
2* Your printer port above should really reserve the 8 I/O ports it
decodes, rather than just the 4 it needs. Which driver is this?

The bottom line is that I am now inclined to accept your patch. I would
only ask you for one minor change: please drop the local "addr"
variable you have been introducing, you can do without it very easily
and it should make the code more readable.

I would also want you to check that all of the W83627HF, W83627THF,
W83697HF and W83637HF chips do not decode ports other than +5 and +6. I
hope and guess so, but if not we will need slightly more complex code.

Could you please additionally check whether this applies to the
W83627EHF/EHG chips as well? Maybe we need to modify the w83627ehf
driver in a similar way.

Thanks,
-- 
Jean Delvare

[lm-sensors] Re: [PATCH] Request only really used I/O ports in

[Jean Delvare]

Hi Petr,

Sorry for the delay.

> No, it would perfectly work.  W83627HF (and all other hardware monitoring
> chips from Winbond) respond only to xxx5 and xxx6 address, not to the
> other addresses in the range xxx0-xxx7. So requesting full 8 byte range
> is not only unnecessary, but also incorrect.

Incorrect WRT to which specification or coding rules guide, please?

> For example I can place 4-byte printer port at 0xC00-0xC03, while sensors
> live at 0xC05-0xC06.  If I place sensors and serial port at same address,
> I get bytes (from C00-C07) "04 00 01 00 13 40 01 00".  When I move senors
> away, I get "04 00 01 00 13 06 00 00", so you can see that only address + 5
> and + 6 are affected.  And when I move serial port away, leaving sensors
> in place, I see "FF FF FF FF FF 40 01 FF" - again chip respons to +5 & +6
> addresses only, not to the full range.

I see a problem with your demonstration. Your 4-byte printer port was
seemingly affecting not only 0xC00-0xC03, but also 0xC04 and 0xC07. Had
you tried without sensors in the way, I bet we would have seen ports
0xC05 and 0xC06 change values as well. This means that your printer port
does answer to reads on I/O ports it has no use for. This is exactly
what I (erroneously) thought would happen with the W83627HF chip.

I'm no electronics expert (far from that actually) but isn't it a
problem, from an electronic point of view, to have two devices
competing for I/O ports? What if your printer port were "stronger" than
your W83627HF? My guess is that the latter wouldn't work anymore.

I just checked on an IT8705F Super-I/O with hardware monitoring. It has
an 8-byte I/O region with +5 and +6 only used, just like the W836x7HF
family. The device presence changes the I/O region from:
  ff ff ff ff ff ff ff ff
to:
  04 04 04 04 04 51 04 04
So, the IT8705F definitely answers on the full I/O range, even ports it
isn't supposed to care about, just like the printer port in your
example. Same is true of the IT8712F, BTW.

> And even if it would answer to the other addresses, you should not request
> them, as driver apparently does not need them, while some other driver might.

This is a non-issue until this hypothetical other driver exists.

> Take a look at i2c-amd756 driver for example.  It uses bytes 0xE0-0xEF
> from large 256 byte I/O window on amd7x6/amd8111 chips.  And it correctly
> requests only region 0xE0-0xEF, so other drivers can drive other parts
> of the chip.

In this case, the rest of the region has other uses, and other drivers
actually need it, so this is a different situation. As far as I can see,
no driver is going to need the leftover I/O ports from hardware
monitoring chips. If these ports are ever used at some point in time,
we can reasonably assume that the new feature needing them will belong
to the same hardware monitoring driver.

> I do not agree with your analysis here.  Chip just wants 8 byte aligned
> address, nowhere in the documentation is stated that full 8 bytes are
> used, actually doc is quite explicit that accesses are decoded only if
> low nibble is 5 or 6 (for example w83627hf/f/hg/g datasheet revision A1,
> page 28).

This is a valuable technical information, and it conforms to your
experimental results above. This seems to differ from the behavior of
the IT87xxF chips and your printer port.

> It is not buggy BIOS.  It is incorrect assumption of driver writter about
> hardware, and about request_region API.  If you want to put potential
> resources to the tree, you should not tag them BUSY, as they are not
> busy, they are free for use by the driver.

There are plenty of drivers out there which request more I/O ports than
they strictly need, just because the author knew that no other driver
would ever need these. Reasons to do this are multiple: sparse used I/O
ports (you're not going to call request_region 3 times if you need 3 I/O
ports spread over a large I/O region), plans to implement more features
in that driver in the future, anticipation of hardware evolution. You
really can't blame the driver writer.

> No.  Unfortunately BIOS knows how hardware works while w83627hf driver
> apparently does not, so I do not think manufacturers are going to break
> BIOS just to match Linux driver expectations.

Sure, BIOSes are never broken :)

My summarized impression is that what matters here is not the fact that
a given driver does or does not need a given I/O port, but the fact
that a given chip will or will not decode the I/O requests for all
ports within its base I/O range. I agree it does make sense, and should
be safe, not to request I/O ports that are not *decoded* by a chip.
OTOH, I would expect problems to arise if two devices have intersecting
base I/O ranges and at least one I/O port is decoded by both devices. I
would expect even more problems if one device does use that one I/O
port.

This leads me to the following conclusions:
1* Reserving only 0x295-0x296 for the W836x7HF family of chips should
be safe, if and only if all other drivers do reserve all I/O ports they
decode, as opposed to only I/O ports they have a use for.
2* Your printer port above should really reserve the 8 I/O ports it
decodes, rather than just the 4 it needs. Which driver is this?

The bottom line is that I am now inclined to accept your patch. I would
only ask you for one minor change: please drop the local "addr"
variable you have been introducing, you can do without it very easily
and it should make the code more readable.

I would also want you to check that all of the W83627HF, W83627THF,
W83697HF and W83637HF chips do not decode ports other than +5 and +6. I
hope and guess so, but if not we will need slightly more complex code.

Could you please additionally check whether this applies to the
W83627EHF/EHG chips as well? Maybe we need to modify the w83627ehf
driver in a similar way.

Thanks,
-- 
Jean Delvare

Re: [PATCH] Request only really used I/O ports in w83627hf driver

[Petr Vandrovec]

Jean Delvare wrote:
> Hi Petr,
>>No, it would perfectly work.  W83627HF (and all other hardware monitoring
>>chips from Winbond) respond only to xxx5 and xxx6 address, not to the
>>other addresses in the range xxx0-xxx7. So requesting full 8 byte range
>>is not only unnecessary, but also incorrect.
> 
> 
> Incorrect WRT to which specification or coding rules guide, please?

WRT specification.  W836[239]7HF datasheet, W83627THF datasheet and W83627EHF 
datasheet.  Chip responds to the XXX5 and XXX6 address only, where XXX is 
defined by registers 0x60/0x61 in the hardware monitoring function in the 
SuperIO.  And you should not request I/O addresses into which chip does not respond.

>>For example I can place 4-byte printer port at 0xC00-0xC03, while sensors
>>live at 0xC05-0xC06.  If I place sensors and serial port at same address,
                                                ^^^^^^^^^^^
>>I get bytes (from C00-C07) "04 00 01 00 13 40 01 00".  When I move senors
>>away, I get "04 00 01 00 13 06 00 00", so you can see that only address + 5
>>and + 6 are affected.  And when I move serial port away, leaving sensors
>>in place, I see "FF FF FF FF FF 40 01 FF" - again chip respons to +5 & +6
>>addresses only, not to the full range.
> 
> I see a problem with your demonstration. Your 4-byte printer port was
> seemingly affecting not only 0xC00-0xC03, but also 0xC04 and 0xC07. Had
> you tried without sensors in the way, I bet we would have seen ports
> 0xC05 and 0xC06 change values as well. This means that your printer port
> does answer to reads on I/O ports it has no use for. This is exactly
> what I (erroneously) thought would happen with the W83627HF chip.

It was not parallel port, but serial one.  I do not understand your paragraph - 
my demonstration clearly shows that sensors chip answers to C05 and C06 only, 
leaving C00-C04 and C07 with values they have when sensors are not mapped here.

> I'm no electronics expert (far from that actually) but isn't it a
> problem, from an electronic point of view, to have two devices
> competing for I/O ports? What if your printer port were "stronger" than
> your W83627HF? My guess is that the latter wouldn't work anymore.

Yes, that is (actually would be...) problem when 8 byte device is there, like in 
  the test I performed above.  But as you see, contents of C00-C04 & C07 did not 
change, although it "won" on C05/C06.  So you can see chip does not respond to 
C00-C04 & C07.

So I've rerun tests again, this time with real printer port instead of using 
8-byte serial port.

SIO LPT and SENSORS disabled
0C00: FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF
SIO LPT at 0C00
0C00: 04 87 CC FF FF FF FF FF FF FF FF FF FF FF FF FF
SIO LPT at 0C00, SENSORS at 0C00
0C00: 04 87 CC FF FF 4F A3 FF FF FF FF FF FF FF FF FF
SIO LPT disabled, SENSORS at 0C00
0C00: FF FF FF FF FF 4F A3 FF FF FF FF FF FF FF FF FF

You can see parallel port affecting C00-C02, and sensors C05-C06.  No conflicts.

> I just checked on an IT8705F Super-I/O with hardware monitoring. It has
> an 8-byte I/O region with +5 and +6 only used, just like the W836x7HF
> family. The device presence changes the I/O region from:
>   ff ff ff ff ff ff ff ff
> to:
>   04 04 04 04 04 51 04 04
> So, the IT8705F definitely answers on the full I/O range, even ports it
> isn't supposed to care about, just like the printer port in your
> example. Same is true of the IT8712F, BTW.

Maybe.  I do not have IT* datasheets.  Also I'm talking about w83627hf.  I just 
added notes about other chips to the bugzilla without verifying their datasheets.

>>It is not buggy BIOS.  It is incorrect assumption of driver writter about
>>hardware, and about request_region API.  If you want to put potential
>>resources to the tree, you should not tag them BUSY, as they are not
>>busy, they are free for use by the driver.
> 
> There are plenty of drivers out there which request more I/O ports than
> they strictly need, just because the author knew that no other driver
> would ever need these. Reasons to do this are multiple: sparse used I/O
> ports (you're not going to call request_region 3 times if you need 3 I/O
> ports spread over a large I/O region), plans to implement more features
> in that driver in the future, anticipation of hardware evolution. You
> really can't blame the driver writer.

See for example parport...  It requests 0x378-0x37A and 0x37B-0x37F separately. 
  And processor_core seems to also request only small part of some larger region 
('ACPI CPU throttle' region).  But ACPI may be different, it just reserves 6 
bytes at some address it somehow computed, so maybe somebody could want other 
bytes of that region - but it does not on my board.

On my boxes rtc & keyboard are drivers which are overallocating their regions. 
But they are always here, and so other drivers know that they should not call 
request region at all (particullary serio knows it should not do request_region, 
as well as kernel & nvram knows to not register 'rtc' (although they use it), as 
rtc driver is one who does request_region).  And fortunately for them 
keyboard/dma... are registered before pnpacpi regions are processed - otherwise 
they would all fail...

> This leads me to the following conclusions:
> 1* Reserving only 0x295-0x296 for the W836x7HF family of chips should
> be safe, if and only if all other drivers do reserve all I/O ports they
> decode, as opposed to only I/O ports they have a use for.

Yes.

> 2* Your printer port above should really reserve the 8 I/O ports it
> decodes, rather than just the 4 it needs. Which driver is this?

Yes.  I did not do tests with parallel port, but with serial port to show that 
even on conflicting devices sensors chip does not affect other addresses (i.e. 
it does not drive other addresses to 0xFF, it just leaves bus floating).

> The bottom line is that I am now inclined to accept your patch. I would
> only ask you for one minor change: please drop the local "addr"
> variable you have been introducing, you can do without it very easily
> and it should make the code more readable.

I'll revert that part of patch completely, just changing request_region.

> I would also want you to check that all of the W83627HF, W83627THF,
> W83697HF and W83637HF chips do not decode ports other than +5 and +6. I
> hope and guess so, but if not we will need slightly more complex code.

I've tested multiple revisions of W83627HF and W83627THF in various Tyan and 
ASUS boards.  I'll perform some search accross my other computers, but I'm not 
aware about any using W83697HF or W83637HF.

Their datasheet has paragraph about LPC interface identical to W83627{HF,THF}, 
but it may be insufficient for you (spec says 'The first interface uses LPC Bus 
to access which ports of low byte (bit2-bit0) are defined in the port 5h and 6h. 
  The other higher bits of these ports is set by W83697HF itself.  The general 
decoded address is set to port 295h and port 296h.'  It is identical for 627HF, 
637HF, 697HF and 627EHF.  For 627THF 'The first interface' is just reworded as 
THF does not have i2c interface.)

> Could you please additionally check whether this applies to the
> W83627EHF/EHG chips as well? Maybe we need to modify the w83627ehf
> driver in a similar way.

I'll try them tomorrow, I know we have boards with this chip, unfortunately they 
run Windows by default, so it will need some preparation.
								Petr

[lm-sensors] Re: [PATCH] Request only really used I/O ports in

[Petr Vandrovec]

Jean Delvare wrote:
> Hi Petr,
>>No, it would perfectly work.  W83627HF (and all other hardware monitoring
>>chips from Winbond) respond only to xxx5 and xxx6 address, not to the
>>other addresses in the range xxx0-xxx7. So requesting full 8 byte range
>>is not only unnecessary, but also incorrect.
> 
> 
> Incorrect WRT to which specification or coding rules guide, please?

WRT specification.  W836[239]7HF datasheet, W83627THF datasheet and W83627EHF 
datasheet.  Chip responds to the XXX5 and XXX6 address only, where XXX is 
defined by registers 0x60/0x61 in the hardware monitoring function in the 
SuperIO.  And you should not request I/O addresses into which chip does not respond.

>>For example I can place 4-byte printer port at 0xC00-0xC03, while sensors
>>live at 0xC05-0xC06.  If I place sensors and serial port at same address,
                                                ^^^^^^^^^^^
>>I get bytes (from C00-C07) "04 00 01 00 13 40 01 00".  When I move senors
>>away, I get "04 00 01 00 13 06 00 00", so you can see that only address + 5
>>and + 6 are affected.  And when I move serial port away, leaving sensors
>>in place, I see "FF FF FF FF FF 40 01 FF" - again chip respons to +5 & +6
>>addresses only, not to the full range.
> 
> I see a problem with your demonstration. Your 4-byte printer port was
> seemingly affecting not only 0xC00-0xC03, but also 0xC04 and 0xC07. Had
> you tried without sensors in the way, I bet we would have seen ports
> 0xC05 and 0xC06 change values as well. This means that your printer port
> does answer to reads on I/O ports it has no use for. This is exactly
> what I (erroneously) thought would happen with the W83627HF chip.

It was not parallel port, but serial one.  I do not understand your paragraph - 
my demonstration clearly shows that sensors chip answers to C05 and C06 only, 
leaving C00-C04 and C07 with values they have when sensors are not mapped here.

> I'm no electronics expert (far from that actually) but isn't it a
> problem, from an electronic point of view, to have two devices
> competing for I/O ports? What if your printer port were "stronger" than
> your W83627HF? My guess is that the latter wouldn't work anymore.

Yes, that is (actually would be...) problem when 8 byte device is there, like in 
  the test I performed above.  But as you see, contents of C00-C04 & C07 did not 
change, although it "won" on C05/C06.  So you can see chip does not respond to 
C00-C04 & C07.

So I've rerun tests again, this time with real printer port instead of using 
8-byte serial port.

SIO LPT and SENSORS disabled
0C00: FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF
SIO LPT at 0C00
0C00: 04 87 CC FF FF FF FF FF FF FF FF FF FF FF FF FF
SIO LPT at 0C00, SENSORS at 0C00
0C00: 04 87 CC FF FF 4F A3 FF FF FF FF FF FF FF FF FF
SIO LPT disabled, SENSORS at 0C00
0C00: FF FF FF FF FF 4F A3 FF FF FF FF FF FF FF FF FF

You can see parallel port affecting C00-C02, and sensors C05-C06.  No conflicts.

> I just checked on an IT8705F Super-I/O with hardware monitoring. It has
> an 8-byte I/O region with +5 and +6 only used, just like the W836x7HF
> family. The device presence changes the I/O region from:
>   ff ff ff ff ff ff ff ff
> to:
>   04 04 04 04 04 51 04 04
> So, the IT8705F definitely answers on the full I/O range, even ports it
> isn't supposed to care about, just like the printer port in your
> example. Same is true of the IT8712F, BTW.

Maybe.  I do not have IT* datasheets.  Also I'm talking about w83627hf.  I just 
added notes about other chips to the bugzilla without verifying their datasheets.

>>It is not buggy BIOS.  It is incorrect assumption of driver writter about
>>hardware, and about request_region API.  If you want to put potential
>>resources to the tree, you should not tag them BUSY, as they are not
>>busy, they are free for use by the driver.
> 
> There are plenty of drivers out there which request more I/O ports than
> they strictly need, just because the author knew that no other driver
> would ever need these. Reasons to do this are multiple: sparse used I/O
> ports (you're not going to call request_region 3 times if you need 3 I/O
> ports spread over a large I/O region), plans to implement more features
> in that driver in the future, anticipation of hardware evolution. You
> really can't blame the driver writer.

See for example parport...  It requests 0x378-0x37A and 0x37B-0x37F separately. 
  And processor_core seems to also request only small part of some larger region 
('ACPI CPU throttle' region).  But ACPI may be different, it just reserves 6 
bytes at some address it somehow computed, so maybe somebody could want other 
bytes of that region - but it does not on my board.

On my boxes rtc & keyboard are drivers which are overallocating their regions. 
But they are always here, and so other drivers know that they should not call 
request region at all (particullary serio knows it should not do request_region, 
as well as kernel & nvram knows to not register 'rtc' (although they use it), as 
rtc driver is one who does request_region).  And fortunately for them 
keyboard/dma... are registered before pnpacpi regions are processed - otherwise 
they would all fail...

> This leads me to the following conclusions:
> 1* Reserving only 0x295-0x296 for the W836x7HF family of chips should
> be safe, if and only if all other drivers do reserve all I/O ports they
> decode, as opposed to only I/O ports they have a use for.

Yes.

> 2* Your printer port above should really reserve the 8 I/O ports it
> decodes, rather than just the 4 it needs. Which driver is this?

Yes.  I did not do tests with parallel port, but with serial port to show that 
even on conflicting devices sensors chip does not affect other addresses (i.e. 
it does not drive other addresses to 0xFF, it just leaves bus floating).

> The bottom line is that I am now inclined to accept your patch. I would
> only ask you for one minor change: please drop the local "addr"
> variable you have been introducing, you can do without it very easily
> and it should make the code more readable.

I'll revert that part of patch completely, just changing request_region.

> I would also want you to check that all of the W83627HF, W83627THF,
> W83697HF and W83637HF chips do not decode ports other than +5 and +6. I
> hope and guess so, but if not we will need slightly more complex code.

I've tested multiple revisions of W83627HF and W83627THF in various Tyan and 
ASUS boards.  I'll perform some search accross my other computers, but I'm not 
aware about any using W83697HF or W83637HF.

Their datasheet has paragraph about LPC interface identical to W83627{HF,THF}, 
but it may be insufficient for you (spec says 'The first interface uses LPC Bus 
to access which ports of low byte (bit2-bit0) are defined in the port 5h and 6h. 
  The other higher bits of these ports is set by W83697HF itself.  The general 
decoded address is set to port 295h and port 296h.'  It is identical for 627HF, 
637HF, 697HF and 627EHF.  For 627THF 'The first interface' is just reworded as 
THF does not have i2c interface.)

> Could you please additionally check whether this applies to the
> W83627EHF/EHG chips as well? Maybe we need to modify the w83627ehf
> driver in a similar way.

I'll try them tomorrow, I know we have boards with this chip, unfortunately they 
run Windows by default, so it will need some preparation.
								Petr

Re: [PATCH] Request only really used I/O ports in w83627hf driver

[Petr Vandrovec]

On Mon, Sep 26, 2005 at 12:07:05AM +0200, Petr Vandrovec wrote:
> Jean Delvare wrote:
> 
> >I would also want you to check that all of the W83627HF, W83627THF,
> >W83697HF and W83637HF chips do not decode ports other than +5 and +6. I
> >hope and guess so, but if not we will need slightly more complex code.
> 
> I've tested multiple revisions of W83627HF and W83627THF in various Tyan 
> and ASUS boards.  I'll perform some search accross my other computers, but 
> I'm not aware about any using W83697HF or W83637HF.
> 
> Their datasheet has paragraph about LPC interface identical to 
> W83627{HF,THF}, but it may be insufficient for you (spec says 'The first 
> interface uses LPC Bus to access which ports of low byte (bit2-bit0) are 
> defined in the port 5h and 6h. The other higher bits of these ports is set 
>  by W83697HF itself.  The general decoded address is set to port 295h and 
> port 296h.'  It is identical for 627HF, 637HF, 697HF and 627EHF.  For 
> 627THF 'The first interface' is just reworded as THF does not have i2c 
> interface.)
> 
> >Could you please additionally check whether this applies to the
> >W83627EHF/EHG chips as well? Maybe we need to modify the w83627ehf
> >driver in a similar way.
> 
> I'll try them tomorrow, I know we have boards with this chip, unfortunately 
> they run Windows by default, so it will need some preparation.

Hello,
  W83627EHF/EHG behave like other Winbond chips - 290-294 & 297 do nothing.
I've not found any box with 637HF or 697HF, but I just prefer to use no
additional if-s around request/release region.

I've left *_REG_OFFSET at 5/6 to make it simillar to the other drivers as 
much as possible.
						Thanks,
							Petr Vandrovec

---

This patch changes w83627hf and w83627ehf drivers to reserve only ports
0x295-0x296, instead of full 0x290-0x297 range.  While some other sensors
chips respond to all addresses in 0x290-0x297 range, Winbond chips respond
to 0x295-0x296 only (this behavior is implied by documentation, and matches
behavior observed on real systems).  This is not problem alone, as no
BIOS was found to put something at these unused addresses, and sensors
chip itself provides nothing there as well.

But in addition to only respond to these two addresses, also BIOS vendors 
report in their ACPI-PnP structures that there is some resource at I/O 
address 0x295 of length 2.  And when later this hwmon driver attempts to
request region with base 0x290/length 8, it fails as one request_region
cannot span more than one device.

Due to this we have to ask only for region this hardware really occupies,
otherwise driver cannot be loaded on systems with ACPI-PnP enabled.


Signed-off-by:  Petr Vandrovec <vandrove@vc.cvut.cz>


diff -urdN linux/drivers/hwmon/w83627ehf.c linux/drivers/hwmon/w83627ehf.c
--- linux/drivers/hwmon/w83627ehf.c	2005-09-26 21:00:36.000000000 +0200
+++ linux/drivers/hwmon/w83627ehf.c	2005-09-26 23:40:37.000000000 +0200
@@ -105,7 +105,9 @@
  * ISA constants
  */
 
-#define REGION_LENGTH		8
+#define REGION_ALIGNMENT	~7
+#define REGION_OFFSET		5
+#define REGION_LENGTH		2
 #define ADDR_REG_OFFSET		5
 #define DATA_REG_OFFSET		6
 
@@ -673,7 +675,8 @@
 	struct w83627ehf_data *data;
 	int i, err = 0;
 
-	if (!request_region(address, REGION_LENGTH, w83627ehf_driver.name)) {
+	if (!request_region(address + REGION_OFFSET, REGION_LENGTH,
+	                    w83627ehf_driver.name)) {
 		err = -EBUSY;
 		goto exit;
 	}
@@ -762,7 +765,7 @@
 exit_free:
 	kfree(data);
 exit_release:
-	release_region(address, REGION_LENGTH);
+	release_region(address + REGION_OFFSET, REGION_LENGTH);
 exit:
 	return err;
 }
@@ -776,7 +779,7 @@
 
 	if ((err = i2c_detach_client(client)))
 		return err;
-	release_region(client->addr, REGION_LENGTH);
+	release_region(client->addr + REGION_OFFSET, REGION_LENGTH);
 	kfree(data);
 
 	return 0;
@@ -807,7 +810,7 @@
 	superio_select(W83627EHF_LD_HWM);
 	val = (superio_inb(SIO_REG_ADDR) << 8)
 	    | superio_inb(SIO_REG_ADDR + 1);
-	*addr = val & ~(REGION_LENGTH - 1);
+	*addr = val & REGION_ALIGNMENT;
 	if (*addr == 0) {
 		superio_exit();
 		return -ENODEV;
diff -urdN linux/drivers/hwmon/w83627hf.c linux/drivers/hwmon/w83627hf.c
--- linux/drivers/hwmon/w83627hf.c	2005-09-26 21:00:36.000000000 +0200
+++ linux/drivers/hwmon/w83627hf.c	2005-09-26 23:30:18.000000000 +0200
@@ -142,10 +142,14 @@
 #define WINB_BASE_REG 0x60
 /* Constants specified below */
 
-/* Length of ISA address segment */
-#define WINB_EXTENT 8
+/* Alignment of the base address */
+#define WINB_ALIGNMENT		~7
 
-/* Where are the ISA address/data registers relative to the base address */
+/* Offset & size of I/O region we are interested in */
+#define WINB_REGION_OFFSET	5
+#define WINB_REGION_SIZE	2
+
+/* Where are the sensors address/data registers relative to the base address */
 #define W83781D_ADDR_REG_OFFSET 5
 #define W83781D_DATA_REG_OFFSET 6
 
@@ -981,7 +985,7 @@
 	superio_select(W83627HF_LD_HWM);
 	val = (superio_inb(WINB_BASE_REG) << 8) |
 	       superio_inb(WINB_BASE_REG + 1);
-	*addr = val & ~(WINB_EXTENT - 1);
+	*addr = val & WINB_ALIGNMENT;
 	if (*addr == 0 && force_addr == 0) {
 		superio_exit();
 		return -ENODEV;
@@ -1000,9 +1004,10 @@
 	const char *client_name = "";
 
 	if(force_addr)
-		address = force_addr & ~(WINB_EXTENT - 1);
+		address = force_addr & WINB_ALIGNMENT;
 
-	if (!request_region(address, WINB_EXTENT, w83627hf_driver.name)) {
+	if (!request_region(address + WINB_REGION_OFFSET, WINB_REGION_SIZE,
+	                    w83627hf_driver.name)) {
 		err = -EBUSY;
 		goto ERROR0;
 	}
@@ -1148,7 +1153,7 @@
       ERROR2:
 	kfree(data);
       ERROR1:
-	release_region(address, WINB_EXTENT);
+	release_region(address + WINB_REGION_OFFSET, WINB_REGION_SIZE);
       ERROR0:
 	return err;
 }
@@ -1163,7 +1168,7 @@
 	if ((err = i2c_detach_client(client)))
 		return err;
 
-	release_region(client->addr, WINB_EXTENT);
+	release_region(client->addr + WINB_REGION_OFFSET, WINB_REGION_SIZE);
 	kfree(data);
 
 	return 0;

[lm-sensors] Re: [PATCH] Request only really used I/O ports in

[Petr Vandrovec]

On Mon, Sep 26, 2005 at 12:07:05AM +0200, Petr Vandrovec wrote:
> Jean Delvare wrote:
> 
> >I would also want you to check that all of the W83627HF, W83627THF,
> >W83697HF and W83637HF chips do not decode ports other than +5 and +6. I
> >hope and guess so, but if not we will need slightly more complex code.
> 
> I've tested multiple revisions of W83627HF and W83627THF in various Tyan 
> and ASUS boards.  I'll perform some search accross my other computers, but 
> I'm not aware about any using W83697HF or W83637HF.
> 
> Their datasheet has paragraph about LPC interface identical to 
> W83627{HF,THF}, but it may be insufficient for you (spec says 'The first 
> interface uses LPC Bus to access which ports of low byte (bit2-bit0) are 
> defined in the port 5h and 6h. The other higher bits of these ports is set 
>  by W83697HF itself.  The general decoded address is set to port 295h and 
> port 296h.'  It is identical for 627HF, 637HF, 697HF and 627EHF.  For 
> 627THF 'The first interface' is just reworded as THF does not have i2c 
> interface.)
> 
> >Could you please additionally check whether this applies to the
> >W83627EHF/EHG chips as well? Maybe we need to modify the w83627ehf
> >driver in a similar way.
> 
> I'll try them tomorrow, I know we have boards with this chip, unfortunately 
> they run Windows by default, so it will need some preparation.

Hello,
  W83627EHF/EHG behave like other Winbond chips - 290-294 & 297 do nothing.
I've not found any box with 637HF or 697HF, but I just prefer to use no
additional if-s around request/release region.

I've left *_REG_OFFSET at 5/6 to make it simillar to the other drivers as 
much as possible.
						Thanks,
							Petr Vandrovec

---

This patch changes w83627hf and w83627ehf drivers to reserve only ports
0x295-0x296, instead of full 0x290-0x297 range.  While some other sensors
chips respond to all addresses in 0x290-0x297 range, Winbond chips respond
to 0x295-0x296 only (this behavior is implied by documentation, and matches
behavior observed on real systems).  This is not problem alone, as no
BIOS was found to put something at these unused addresses, and sensors
chip itself provides nothing there as well.

But in addition to only respond to these two addresses, also BIOS vendors 
report in their ACPI-PnP structures that there is some resource at I/O 
address 0x295 of length 2.  And when later this hwmon driver attempts to
request region with base 0x290/length 8, it fails as one request_region
cannot span more than one device.

Due to this we have to ask only for region this hardware really occupies,
otherwise driver cannot be loaded on systems with ACPI-PnP enabled.


Signed-off-by:  Petr Vandrovec <vandrove@vc.cvut.cz>


diff -urdN linux/drivers/hwmon/w83627ehf.c linux/drivers/hwmon/w83627ehf.c
--- linux/drivers/hwmon/w83627ehf.c	2005-09-26 21:00:36.000000000 +0200
+++ linux/drivers/hwmon/w83627ehf.c	2005-09-26 23:40:37.000000000 +0200
@@ -105,7 +105,9 @@
  * ISA constants
  */
 
-#define REGION_LENGTH		8
+#define REGION_ALIGNMENT	~7
+#define REGION_OFFSET		5
+#define REGION_LENGTH		2
 #define ADDR_REG_OFFSET		5
 #define DATA_REG_OFFSET		6
 
@@ -673,7 +675,8 @@
 	struct w83627ehf_data *data;
 	int i, err = 0;
 
-	if (!request_region(address, REGION_LENGTH, w83627ehf_driver.name)) {
+	if (!request_region(address + REGION_OFFSET, REGION_LENGTH,
+	                    w83627ehf_driver.name)) {
 		err = -EBUSY;
 		goto exit;
 	}
@@ -762,7 +765,7 @@
 exit_free:
 	kfree(data);
 exit_release:
-	release_region(address, REGION_LENGTH);
+	release_region(address + REGION_OFFSET, REGION_LENGTH);
 exit:
 	return err;
 }
@@ -776,7 +779,7 @@
 
 	if ((err = i2c_detach_client(client)))
 		return err;
-	release_region(client->addr, REGION_LENGTH);
+	release_region(client->addr + REGION_OFFSET, REGION_LENGTH);
 	kfree(data);
 
 	return 0;
@@ -807,7 +810,7 @@
 	superio_select(W83627EHF_LD_HWM);
 	val = (superio_inb(SIO_REG_ADDR) << 8)
 	    | superio_inb(SIO_REG_ADDR + 1);
-	*addr = val & ~(REGION_LENGTH - 1);
+	*addr = val & REGION_ALIGNMENT;
 	if (*addr = 0) {
 		superio_exit();
 		return -ENODEV;
diff -urdN linux/drivers/hwmon/w83627hf.c linux/drivers/hwmon/w83627hf.c
--- linux/drivers/hwmon/w83627hf.c	2005-09-26 21:00:36.000000000 +0200
+++ linux/drivers/hwmon/w83627hf.c	2005-09-26 23:30:18.000000000 +0200
@@ -142,10 +142,14 @@
 #define WINB_BASE_REG 0x60
 /* Constants specified below */
 
-/* Length of ISA address segment */
-#define WINB_EXTENT 8
+/* Alignment of the base address */
+#define WINB_ALIGNMENT		~7
 
-/* Where are the ISA address/data registers relative to the base address */
+/* Offset & size of I/O region we are interested in */
+#define WINB_REGION_OFFSET	5
+#define WINB_REGION_SIZE	2
+
+/* Where are the sensors address/data registers relative to the base address */
 #define W83781D_ADDR_REG_OFFSET 5
 #define W83781D_DATA_REG_OFFSET 6
 
@@ -981,7 +985,7 @@
 	superio_select(W83627HF_LD_HWM);
 	val = (superio_inb(WINB_BASE_REG) << 8) |
 	       superio_inb(WINB_BASE_REG + 1);
-	*addr = val & ~(WINB_EXTENT - 1);
+	*addr = val & WINB_ALIGNMENT;
 	if (*addr = 0 && force_addr = 0) {
 		superio_exit();
 		return -ENODEV;
@@ -1000,9 +1004,10 @@
 	const char *client_name = "";
 
 	if(force_addr)
-		address = force_addr & ~(WINB_EXTENT - 1);
+		address = force_addr & WINB_ALIGNMENT;
 
-	if (!request_region(address, WINB_EXTENT, w83627hf_driver.name)) {
+	if (!request_region(address + WINB_REGION_OFFSET, WINB_REGION_SIZE,
+	                    w83627hf_driver.name)) {
 		err = -EBUSY;
 		goto ERROR0;
 	}
@@ -1148,7 +1153,7 @@
       ERROR2:
 	kfree(data);
       ERROR1:
-	release_region(address, WINB_EXTENT);
+	release_region(address + WINB_REGION_OFFSET, WINB_REGION_SIZE);
       ERROR0:
 	return err;
 }
@@ -1163,7 +1168,7 @@
 	if ((err = i2c_detach_client(client)))
 		return err;
 
-	release_region(client->addr, WINB_EXTENT);
+	release_region(client->addr + WINB_REGION_OFFSET, WINB_REGION_SIZE);
 	kfree(data);
 
 	return 0;

Re: [PATCH] Request only really used I/O ports in w83627hf driver

[Grant Coady]

On Wed, 28 Sep 2005 04:49:56 +0200, Petr Vandrovec <vandrove@vc.cvut.cz> wrote:

>On Mon, Sep 26, 2005 at 12:07:05AM +0200, Petr Vandrovec wrote:
>> Jean Delvare wrote:
>> 
>> >I would also want you to check that all of the W83627HF, W83627THF,
>> >W83697HF and W83637HF chips do not decode ports other than +5 and +6. I
>> >hope and guess so, but if not we will need slightly more complex code.
>> 
>> I've tested multiple revisions of W83627HF and W83627THF in various Tyan 
>> and ASUS boards.  I'll perform some search accross my other computers, but 
>> I'm not aware about any using W83697HF or W83637HF.
                                 ^^^^^^^^
root@sempro:~# sensors
w83697hf-isa-0290
Adapter: ISA adapter
  VCore:   +1.55 V  (min =  +1.52 V, max =  +1.68 V)
   3.3V:   +3.28 V  (min =  +3.14 V, max =  +3.47 V)
     5V:   +5.08 V  (min =  +4.76 V, max =  +5.24 V)
    12V:  +11.98 V  (min = +11.37 V, max = +12.59 V)
   -12V:  -11.86 V  (min = -12.60 V, max = -11.36 V)
    -5V:   -5.10 V  (min =  -5.25 V, max =  -4.75 V)
   V5SB:   +4.95 V  (min =  +4.75 V, max =  +5.26 V)
  VBatt:   +3.38 V  (min =  +2.90 V, max =  +3.41 V)
CPU Fan:  3183 RPM  (min = 1500 RPM, div = 4)
CaseFan:  1371 RPM  (min = 1196 RPM, div = 8)
Ambient:     +32°C  (high =   +50°C, hyst =   +45°C)   sensor = thermistor
    CPU:   +56.5°C  (high =   +66°C, hyst =   +61°C)   sensor = diode           (beep)
alarms:
beep_enable:
          Sound alarm enabled

root@sempro:~# lsmod
Module                  Size  Used by
w83627hf               24656  0
hwmon_vid               2048  1 w83627hf
i2c_isa                 3392  1 w83627hf
e100                   33860  0
root@sempro:~# uname -r
2.6.14-rc2-git6b

Your patch didn't break things here :o)

Grant.

[lm-sensors] Re: [PATCH] Request only really used I/O ports in

[Grant Coady]

On Wed, 28 Sep 2005 04:49:56 +0200, Petr Vandrovec <vandrove@vc.cvut.cz> wrote:

>On Mon, Sep 26, 2005 at 12:07:05AM +0200, Petr Vandrovec wrote:
>> Jean Delvare wrote:
>> 
>> >I would also want you to check that all of the W83627HF, W83627THF,
>> >W83697HF and W83637HF chips do not decode ports other than +5 and +6. I
>> >hope and guess so, but if not we will need slightly more complex code.
>> 
>> I've tested multiple revisions of W83627HF and W83627THF in various Tyan 
>> and ASUS boards.  I'll perform some search accross my other computers, but 
>> I'm not aware about any using W83697HF or W83637HF.
                                 ^^^^^^^^
root@sempro:~# sensors
w83697hf-isa-0290
Adapter: ISA adapter
  VCore:   +1.55 V  (min =  +1.52 V, max =  +1.68 V)
   3.3V:   +3.28 V  (min =  +3.14 V, max =  +3.47 V)
     5V:   +5.08 V  (min =  +4.76 V, max =  +5.24 V)
    12V:  +11.98 V  (min = +11.37 V, max = +12.59 V)
   -12V:  -11.86 V  (min = -12.60 V, max = -11.36 V)
    -5V:   -5.10 V  (min =  -5.25 V, max =  -4.75 V)
   V5SB:   +4.95 V  (min =  +4.75 V, max =  +5.26 V)
  VBatt:   +3.38 V  (min =  +2.90 V, max =  +3.41 V)
CPU Fan:  3183 RPM  (min = 1500 RPM, div = 4)
CaseFan:  1371 RPM  (min = 1196 RPM, div = 8)
Ambient:     +32?C  (high =   +50?C, hyst =   +45?C)   sensor = thermistor
    CPU:   +56.5?C  (high =   +66?C, hyst =   +61?C)   sensor = diode           (beep)
alarms:
beep_enable:
          Sound alarm enabled

root@sempro:~# lsmod
Module                  Size  Used by
w83627hf               24656  0
hwmon_vid               2048  1 w83627hf
i2c_isa                 3392  1 w83627hf
e100                   33860  0
root@sempro:~# uname -r
2.6.14-rc2-git6b

Your patch didn't break things here :o)

Grant.

Re: [PATCH] Request only really used I/O ports in w83627hf driver

[Jean Delvare]

Hi Petr,

> This patch changes w83627hf and w83627ehf drivers to reserve only ports
> 0x295-0x296, instead of full 0x290-0x297 range.  While some other sensors
> chips respond to all addresses in 0x290-0x297 range, Winbond chips respond
> to 0x295-0x296 only (this behavior is implied by documentation, and matches
> behavior observed on real systems).  This is not problem alone, as no
> BIOS was found to put something at these unused addresses, and sensors
> chip itself provides nothing there as well.
> 
> But in addition to only respond to these two addresses, also BIOS vendors 
> report in their ACPI-PnP structures that there is some resource at I/O 
> address 0x295 of length 2.  And when later this hwmon driver attempts to
> request region with base 0x290/length 8, it fails as one request_region
> cannot span more than one device.
> 
> Due to this we have to ask only for region this hardware really occupies,
> otherwise driver cannot be loaded on systems with ACPI-PnP enabled.
> 
> Signed-off-by:  Petr Vandrovec <vandrove@vc.cvut.cz>

OK, looks good, applied to my local tree. I'll push it to Greg KH in a
week or so. Thanks.

-- 
Jean Delvare

[lm-sensors] Re: [PATCH] Request only really used I/O ports in

[Jean Delvare]

Hi Petr,

> This patch changes w83627hf and w83627ehf drivers to reserve only ports
> 0x295-0x296, instead of full 0x290-0x297 range.  While some other sensors
> chips respond to all addresses in 0x290-0x297 range, Winbond chips respond
> to 0x295-0x296 only (this behavior is implied by documentation, and matches
> behavior observed on real systems).  This is not problem alone, as no
> BIOS was found to put something at these unused addresses, and sensors
> chip itself provides nothing there as well.
> 
> But in addition to only respond to these two addresses, also BIOS vendors 
> report in their ACPI-PnP structures that there is some resource at I/O 
> address 0x295 of length 2.  And when later this hwmon driver attempts to
> request region with base 0x290/length 8, it fails as one request_region
> cannot span more than one device.
> 
> Due to this we have to ask only for region this hardware really occupies,
> otherwise driver cannot be loaded on systems with ACPI-PnP enabled.
> 
> Signed-off-by:  Petr Vandrovec <vandrove@vc.cvut.cz>

OK, looks good, applied to my local tree. I'll push it to Greg KH in a
week or so. Thanks.

-- 
Jean Delvare