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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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

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

[Index of Archives]     [Kernel Newbies]     [Netfilter]     [Bugtraq]     [Photo]     [Gimp]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Video 4 Linux]     [Linux for the blind]
  Powered by Linux