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 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 +0 -1

index 6143f52..51bfbc0 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 */

You shouldn't (void) this, there is a reason you get a warning otherwise! request_muxed_region can still fail if some other driver has done a none muxed request_region on the same region, and in that case you should not continue with accessing the io-ports.

Regards,

Hans

Hans,
Comments below

Quoted 5 lines · Hans de Goede

You shouldn't (void) this, there is a reason you get a warning otherwise! request_muxed_region can still fail if some other driver has done a none muxed request_region on the same region, and in that case you should not continue with accessing the io-ports.

There are 3 it87 drivers and they all have to do the exact same sequence to put the chip into a mode where they can modify its state. The sequences involve non atomic sequences that write locations 0x2e and 0x2f. When they are done they write a different sequence to these 2 locations. The entry routine is superio_enter and exit is superio_exit. All the it87 drivers reserve these 2 locations before they start manipulating the chip. This macro will hold off requestors if the resource is busy because one of the other drivers is manipulating the chip. Once the an it87 driver is done it calls superio_exit which will release the reservation on those 2 locations letting any other driver on the wait queue to now gain access two locations.

Please read code in kernel/resource.c function "__request_region". "request_muxed_region" turns on IORESOURCE_MUXED bit and that means that only way an it87 driver will get back from a call to "request_muxed_region" is when it gets hold of the region.

The scenario you mention above can never happen.

Quoted 22 lines · Natarajan Gurumoorthy

Hans,
      Comments below

Quoted 5 lines · Hans de Goede

You shouldn't (void) this, there is a reason you get a warning otherwise! request_muxed_region can still fail if some other driver has done a none muxed request_region on the same region, and in that case you should not continue with accessing the io-ports.

There are 3 it87 drivers and they all have to do the exact same sequence to put the chip into a mode where they can modify its state. The sequences involve non atomic sequences that write locations 0x2e and 0x2f. When they are done they write a different sequence to these 2 locations. The entry routine is superio_enter and exit is superio_exit. All the it87 drivers reserve these 2 locations before they start manipulating the chip. This macro will hold off requestors if the resource is busy because one of the other drivers is manipulating the chip. Once the  an it87 driver is done it calls superio_exit which will release the reservation on those 2 locations letting any other driver on the wait queue to now gain access two locations.

Quoted 6 lines

Please read code in kernel/resource.c function "__request_region". "request_muxed_region" turns on IORESOURCE_MUXED bit and that means that only  way an it87 driver will get back from a call to "request_muxed_region" is when it gets hold of the region.

The scenario you mention above can never happen.

Let me be straight clear, as apparently you have difficulties understanding Hans's simple request:

You do not get to (void) the return of request_muxed_region(), period. This is _not_ negotiable.

What other it87 drivers currently in the tree do or don't do is totally irrelevant. There can be new it87 drivers added later, there can be out-of-tree it87 drivers (including old copies of in-tree ones), and there can be non-it87 drivers accessing the I/O ports (or at least attempting to.) So the scenario mentioned by Hans can very well happen, and you have to deal with it.

Thanks,
--
Jean Delvare

John,
Unfortunately you are right. After grepping though drivers/hwmon
and drivers/watchdog I realize the ugliness is fairly deep rooted. All of the following drivers need to be cleaned up as far as the "superio_enter" "superio_exit" operation is concerned if we want this stuff to be SMP clean and allow multiple drivers to coexist.

drivers/watchdog
f71808e_wdt.c
it8712f_wdt.c
it87_wdt.c
sch311x_wdt.c
iTCO_vendor_support.c
ibmasr.c
w83697hf_wdt.c
w83697ug_wdt.c:

drivers/hwmon
dme1737.c:1935:static inline void dme1737_sio_enter(int sio_cip) f71805f.c:100:superio_enter(int base) f71882fg.c:196:static inline int superio_enter(int base); it87.c:112:superio_enter(void)
pc87360.c:1009: /* No superio_enter */
sch5627.c:118:static inline int superio_enter(int base) smsc47b397.c:75:static inline void superio_enter(void) smsc47m1.c:77:superio_enter(void)
vt1211.c:223:static inline void superio_enter(int sio_cip) w83627ehf.c:135:superio_enter(int ioreg) w83627hf.c:134:superio_enter(struct w83627hf_sio_data *sio)

Have I missed any other drivers that also need to be cleaned.

Even if
I got my boss to sign up to clean this I can only test the it8712f drivers and thats it.

Have I missed any other drivers that also need to be cleaned. Even if I got my boss to sign up to clean this I can only test the it8712f drivers and thats it.

Hi,

Quoted 26 lines · Nat Gurumoorthy

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 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 +0 -1

index 6143f52..51bfbc0 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 */

@@ -123,7 +122,11 @@ static inline void superio_select(int ldn)
 

drivers/watchdog/it8712f_wdt.c static inline void superio_select(int ldn) · line 129

Quoted 8 lines

drivers/watchdog/it8712f_wdt.c +5 -1

  static inline void superio_enter(void)
  {
-	spin_lock(&io_lock);
+	/*
+	 * Reserve REG and REG + 1 for exclusive access.
+	 */
+	(void) request_muxed_region(REG, 2, NAME);
+

You shouldn't (void) this, there is a reason you get a warning otherwise! request_muxed_region can still fail if some other driver has done a none muxed request_region on the same region, and in that case you should not continue with accessing the io-ports.

Hi Nat,

Quoted 1 line

+     (void) request_muxed_region(REG, 2, NAME);

Why do we need to typecast this? Can't we just use
+       request_muxed_region(REG, 2, NAME);

Kind regards,
Wim.

Quoted 6 lines · On Wed, 13 Apr 2011 09:03:00 +0200 Wim Van Sebroeck <[email protected]> wrote:

Hi Nat,

Quoted 1 line

+   (void) request_muxed_region(REG, 2, NAME);

Why do we need to typecast this? Can't we just use
+     request_muxed_region(REG, 2, NAME);

We really ought to use

       if ()

in theory the request can fail if someone has hogged the resource and not muxed it. I'm not clear what the right thing to do in that case is - given it should never happen I guess log an error and bail out but that's a rather bigger change and perhaps should be a follow up patch ?

request_muxed_region() is sorta gross: it's essentially acting like a lock, meant to be used for short periods of time, but it can fail if someone else decides it should.

Would it make more sense to have drivers that need to use the current request_muxed_region() be able to force a region into a mux-only state at driver init? That would lead to much less contorted code to handle the off-chance that the request_muxed_region() fails.

I realize the above example re-uses the 'request_muxed_region()' name, but at least this would be much more consistent with how request_region is used in other drivers.