Make all it87 drivers SMP safe

01 - Changes to it87 watchdog driver to use "request_muxed_region" Serialize access to the hardware by using "request_muxed_region" macro defined by Alan Cox. Call to this macro will hold off the requestor if the resource is currently busy. The first call to request_muxed_region will attempt 10 times to reserve the region before it gives up. This will typically get called from the driver init routines. If this succeeds then subsequent calls wait forever for the resource to be available.

The use of the above macro makes it possible to get rid of spinlocks in it8712f_wdt.c and it87_wdt.c watchdog drivers. This also greatly simplifies the implementation of it87_wdt.c driver.

Signed-off-by Nat Gurumoorthy <[email protected]>

---

drivers/watchdog/it8712f_wdt.c +12 -3

index 6143f52..8bf2524 100644
@@ -51,7 +51,6 @@ MODULE_PARM_DESC(nowayout, "Disable watchdog shutdown on close");
 
 static unsigned long wdt_open;
 static unsigned expect_close;
-static spinlock_t io_lock;
 static unsigned char revision;
 
 /* Dog Food address - We use the game port address */

@@ -121,9 +120,46 @@ static inline void superio_select(int ldn)
 	outb(ldn, VAL);
 }
 
-static inline void superio_enter(void)
+static inline int
+try_superio_enter(void)
 {
-	spin_lock(&io_lock);
+	int num_tries = 10;
+	/*
+	 * Try to reserve REG and REG + 1 for exclusive access.
+	 * Give up after 10 attempts.
+	 */
+	while (num_tries--) {
+		if (!request_muxed_region(REG, 2, NAME)) {
+			if (num_tries)
+				continue;
+

This is way too complicated. Just return an error if request_muxed_region fails, like all other callers of request_muxed_region do.

Guenter

Quoted 4 lines · Guenter Roeck

This is way too complicated. Just return an error if request_muxed_region fails, like all other callers of request_muxed_region do.

Guenter,
This maybe a little complicated but it is a solution that will
correctly deal with the following scenario:
Hardware: it8712f
cpu: Multicore
kernel: smp
Modules being loaded during initialization: it8712f_wtd and w83697hf

The problem is one of the cores is halfway through initializing it8712f_wdt and another core is just starting to run the "w83697hf_check_wdt" which is going to call "request_region" to reserve 0x2e 0x2f region. Just after that call on the other core it8712f_wdt_init calls it8712f_wdt_disable which calls superio_enter which will call "request_muxed_region" for the exact same region. This is guaranteed to fail because as of now w83697hf is one of the many non compliant drivers who are calling "request_region". The w83697hf will do the probe and conclude there is no w83697hf chip in the system and abort rest of the driver initialization. Meanwhile the it8712f_wtd driver initialization will fail. This will be one of those intermittent failures that make most kernel device driver writers prematurely bald :-).

The way I have reworked the driver will survive the above scenario.

Unless I am missing something, the problem is that there is another driver calling request_region() instead of request_muxed_region().

w83697hf_check_wdt() (and other drivers doing the same) should call request_muxed_region(). Otherwise, the same problem could happen with the caller(s) of request_region() - those calls could fail as well. Plus, other callers of request_muxed_reason() not implementing your retry code could end up with the same problem, ie with spurious failures.

I think we will have to find a solution which does not require retries when calling request_muxed_region(). On the other side, others know this code much better than I do, so maybe someone has a better solution.

Thanks,
Guenter

Quoted 9 lines · Guenter Roeck

Unless I am missing something, the problem is that there is another driver calling request_region() instead of request_muxed_region().

w83697hf_check_wdt() (and other drivers doing the same) should call request_muxed_region(). Otherwise, the same problem could happen with the caller(s) of request_region() - those calls could fail as well. Plus, other callers of request_muxed_reason() not implementing your retry code could end up with the same problem, ie with spurious failures.

Quoted 4 lines

I think we will have to find a solution which does not require retries when calling request_muxed_region(). On the other side, others know this code much better than I do, so maybe someone has a better solution.

Guenter,
Your conclusion is on the money. request_muxed_region will fail if
someone else called request_region for the same region. The knife cuts both ways and reguest_region will fail when a request_muxed_region calll has been made for the same zone. I am not sure what happens when the second driver calls request_muxed_region for only part of the region that has been reserved by another driver. This is of course one of the problems. The other problem is that there are drivers which don't call either of those 2 interfaces and just go ahead and clobber the 0x2e 0x2f zone because they think they own all of it. I created a list drivers in hwmon and watchdog directories that need to be fixed in one of my earlier emails. John Delvare replied that are other drivers in parport that all need fixing. I got started on this because we use 2 different it87 drivers (hwmon/it87.c and watchdog/it8712f_wdt.c) concurrently and had to solve this problem.

I have seen the list. I don't think you should fix everything in one go. First step might be to get the w83697hf and it87 to work together, then go from there.

Is there a reason for loading (or trying to load) both the it87 and the w83697hf driver at the same time ? Those drivers are usually only loaded if the respective chip is known to exist. If there is no reason to try loading both drivers, a simple workaround would be to not do it.

Thanks,
Guenter

Quoted 9 lines · Guenter Roeck

I have seen the list. I don't think you should fix everything in one go. First step might be to get the w83697hf and it87 to work together, then go from there.

Is there a reason for loading (or trying to load) both the it87 and the w83697hf driver at the same time ? Those drivers are usually only loaded if the respective chip is known to exist. If there is no reason to try loading both drivers, a simple workaround would be to not do it.

Guenter,
I agree the above should never happen. The only way the 2
drivers will be loaded at the same time is a misconfigured kernel where these 2 drivers get built and the rc scripts end up loading them too. If we are agreed that I suggest we make the superio_enter routine be the following:

static inline void
superio_enter(void)

{
	/*
	 * Reserve REG and REG + 1 for exclusive access.
	 */
	while (!request_muxed_region(REG, 2, WATCHDOG_NAME))
		continue;

	outb(0x87, REG);
	outb(0x01, REG);
	outb(0x55, REG);
	outb(0x55, REG);
}

What I am suggesting is not returning an error and instead keep calling "request_muxed_region" till it succeeds. If superio_enter returns an error then we will have to rewrite a large chunk of it8712f_wdt to deal with it. There are 8-9 calls to superio_enter. We will have supreio_enter returning errors at awkward places in the driver where the current logic has no code to deal with errors.

In case of properly written drivers the while loop will eventually exit. I agree this is ugly as sin but it limits the perturbation of the driver. Feedback please.

Regards
Nat

Quoted 4 lines

Thanks,
Guenter

Signature · 2 lines

Regards
Nat Gurumoorthy AB6SJ

Quoted 9 lines · Guenter Roeck

I have seen the list. I don't think you should fix everything in one go. First step might be to get the w83697hf and it87 to work together, then go from there.

Is there a reason for loading (or trying to load) both the it87 and the w83697hf driver at the same time ? Those drivers are usually only loaded if the respective chip is known to exist. If there is no reason to try loading both drivers, a simple workaround would be to not do it.

Guenter,
I agree the above should never happen. The only way the 2
drivers will be loaded at the same time is a misconfigured kernel where these 2 drivers get built and the rc scripts end up loading them too. If we are agreed that I suggest we make the superio_enter routine be the following:

static inline void
superio_enter(void)

{
	/*
	 * Reserve REG and REG + 1 for exclusive access.
	 */
	while (!request_muxed_region(REG, 2, WATCHDOG_NAME))
		continue;

There are 3 different drivers that touch the it87 hardware registers. The 3 drivers have been written independently and access the it87 hardware registers assuming they are the only driver accessing it. This change attempts to serialize access to the hardware by using "request_muxed_region" macro defined by Alan Cox. Call to this macro will hold off the requestor if the resource is currently busy. The use of the above macro makes it possible to get rid of spinlocks in it8712f_wdt.c and it87_wdt.c watchdog drivers. This also greatly simplifies the implementation of it87_wdt.c driver.

01 - Changes to it87 watchdog driver to use "request_muxed_region" drivers/watchdog/it8712f_wdt.c drivers/watchdog/it87_wdt.c

02 - Chages to hwmon it87 driver to use "request_muxed_region" drivers/hwmon/it87.c

drivers/hwmon/it87.c           |   30 +++++++++++++-
drivers/watchdog/it8712f_wdt.c |   50 +++++++++++++++++++++---
drivers/watchdog/it87_wdt.c    |   80 +++++++++++++++++++++------------------
3 files changed, 113 insertions(+), 47 deletions(-)