Skip to contentSkip to conversation

Make all it87 drivers SMP safe.

PATCH In review Nat Gurumoorthy 5 participants 14 messages Apr 5, 2011 – Apr 8, 2011

Subsystems (3)
Hardware monitoring Watchdog device drivers IT87 hardware monitoring driver
lore
Share & tools
lore

Nat Gurumoorthy

author
#0 lore raw

PATCH 1/1 Make all it87 drivers SMP safe.

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 defining a global spinlock it87_io_lock in a file it87_lock.c. This lock has to be acquired by each of the it87 drivers before it can access the hardware. We have defined a new Kconfig option IT87_LOCK. When it is selected it87_lock.c is compiled into the kernel thereby making the lock global and accessable to the it87 drivers which are typically built as loadable modules. All the it87 drivers select IT87_LOCK to compile the lock into the kernel.
The routines accessing the hardware are being called from module init, open, ioctl and module exit routines and hence it is sufficient to use calls to spin_lock and spin_unlock to acquire and release the locks. For the same reasons it87_wdt.c has extensive changes to remove calls to spin_lock_irqsave and spin_unlock_irqrestore. The lock is now acquired in superio_enter and released in superio_exit. This is now identical to the code in drivers/hwmon/it87.c and drivers/watchdog/it8712f_wdt.c. Added __acquire and __release annotations wherever needed.

---
 drivers/hwmon/Kconfig          |    1 +
 drivers/hwmon/it87.c           |   14 ++++++++++++-
 drivers/watchdog/Kconfig       |   12 +++++++++++
 drivers/watchdog/Makefile      |    1 +
 drivers/watchdog/it8712f_wdt.c |   10 ++++----
 drivers/watchdog/it87_lock.c   |   27 +++++++++++++++++++++++++
 drivers/watchdog/it87_wdt.c    |   42 ++++++---------------------------------
 include/linux/it87_lock.h      |   28 ++++++++++++++++++++++++++
 8 files changed, 94 insertions(+), 41 deletions(-)
 create mode 100644 drivers/watchdog/it87_lock.c
 create mode 100644 include/linux/it87_lock.h
Signed-off-by Nat Gurumoorthy <[email protected]>
drivers/hwmon/Kconfig +1 -0
index 297bc9a..afe671a 100644
@@ -436,6 +436,7 @@ config SENSORS_IBMPEX
 config SENSORS_IT87
 	tristate "ITE IT87xx and compatibles"
 	select HWMON_VID
+	select IT87_LOCK
 	help
 	  If you say yes here you get support for ITE IT8705F, IT8712F,
 	  IT8716F, IT8718F, IT8720F, IT8721F, IT8726F and IT8758E sensor
drivers/hwmon/it87.c +13 -1
index 316b648..bc32535 100644
@@ -55,6 +55,8 @@
 #include <linux/dmi.h>
 #include <linux/acpi.h>
 #include <linux/io.h>
+#include <linux/spinlock.h>
+#include <linux/it87_lock.h>
 
 #define DRVNAME "it87"
 

@@ -110,7 +112,9 @@ superio_select(int ldn)
 
 static inline void
 superio_enter(void)
+__acquires(&it87_io_lock)
 {
+	spin_lock(&it87_io_lock);
 	outb(0x87, REG);
 	outb(0x01, REG);
 	outb(0x55, REG);

@@ -119,9 +123,11 @@ superio_enter(void)
 
 static inline void
 superio_exit(void)
+__releases(&it87_io_lock)
 {
 	outb(0x02, REG);
 	outb(0x02, VAL);
+	spin_unlock(&it87_io_lock);
 }
 
 /* Logical device 4 registers */

@@ -1899,8 +1905,12 @@ static int __devexit it87_remove(struct platform_device *pdev)
    would slow down the IT87 access and should not be necessary. */
 static int it87_read_value(struct it87_data *data, u8 reg)
 {
+	int ret;
+	spin_lock(&it87_io_lock);
 	outb_p(reg, data->addr + IT87_ADDR_REG_OFFSET);
-	return inb_p(data->addr + IT87_DATA_REG_OFFSET);
+	ret = inb_p(data->addr + IT87_DATA_REG_OFFSET);
+	spin_unlock(&it87_io_lock);
+	return ret;
 }
 
 /* Must be called with data->update_lock held, except during initialization.

@@ -1908,8 +1918,10 @@ static int it87_read_value(struct it87_data *data, u8 reg)
    would slow down the IT87 access and should not be necessary. */
 static void it87_write_value(struct it87_data *data, u8 reg, u8 value)
 {
+	spin_lock(&it87_io_lock);
 	outb_p(reg, data->addr + IT87_ADDR_REG_OFFSET);
 	outb_p(value, data->addr + IT87_DATA_REG_OFFSET);
+	spin_unlock(&it87_io_lock);
 }
 
 /* Return 1 if and only if the PWM interface is safe to use */
drivers/watchdog/Kconfig +12 -0
index 31649b7..6ffc682 100644
@@ -562,9 +562,20 @@ config ITCO_VENDOR_SUPPORT
 	  devices. At this moment we only have additional support for some
 	  SuperMicro Inc. motherboards.
 
+config IT87_LOCK
+	bool
+	default n
+	---help---
+	  This defines the global lock needed by all IT87 drivers that
+	  touch the Super I/0 chipset used on many motherboards. This
+	  is needed to serialize access to the registers in SMP
+	  systems. All it87 drivers will select this Kconfig option to compile
+	  the lock into the kernel.
+
 config IT8712F_WDT
 	tristate "IT8712F (Smart Guardian) Watchdog Timer"
 	depends on X86
+	select IT87_LOCK
 	---help---
 	  This is the driver for the built-in watchdog timer on the IT8712F
 	  Super I/0 chipset used on many motherboards.

@@ -578,6 +589,7 @@ config IT8712F_WDT
 config IT87_WDT
 	tristate "IT87 Watchdog Timer"
 	depends on X86 && EXPERIMENTAL
+	select IT87_LOCK
 	---help---
 	  This is the driver for the hardware watchdog on the ITE IT8702,
 	  IT8712, IT8716, IT8718, IT8720, IT8726, IT8712 Super I/O chips.
drivers/watchdog/Makefile +1 -0
index 20e44c4..6895445 100644
@@ -81,6 +81,7 @@ obj-$(CONFIG_ITCO_WDT) += iTCO_wdt.o
 ifeq ($(CONFIG_ITCO_VENDOR_SUPPORT),y)
 obj-$(CONFIG_ITCO_WDT) += iTCO_vendor_support.o
 endif
+obj-$(CONFIG_IT87_LOCK) += it87_lock.o
 obj-$(CONFIG_IT8712F_WDT) += it8712f_wdt.o
 obj-$(CONFIG_IT87_WDT) += it87_wdt.o
 obj-$(CONFIG_HP_WATCHDOG) += hpwdt.o
drivers/watchdog/it8712f_wdt.c +5 -5
index b32c6c0..03a3e89 100644
@@ -32,6 +32,7 @@
 #include <linux/spinlock.h>
 #include <linux/uaccess.h>
 #include <linux/io.h>
+#include <linux/it87_lock.h>
 
 #define NAME "it8712f_wdt"
 

@@ -51,7 +52,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 */

@@ -122,8 +122,9 @@ static inline void superio_select(int ldn)
 }
 
 static inline void superio_enter(void)
+__acquires(&it87_io_lock)
 {
-	spin_lock(&io_lock);
+	spin_lock(&it87_io_lock);
 	outb(0x87, REG);
 	outb(0x01, REG);
 	outb(0x55, REG);

@@ -131,10 +132,11 @@ static inline void superio_enter(void)
 }
 
 static inline void superio_exit(void)
+__releases(&it87_io_lock)
 {
 	outb(0x02, REG);
 	outb(0x02, VAL);
-	spin_unlock(&io_lock);
+	spin_unlock(&it87_io_lock);
 }
 
 static inline void it8712f_wdt_ping(void)

@@ -382,8 +384,6 @@ static int __init it8712f_wdt_init(void)
 {
 	int err = 0;
 
-	spin_lock_init(&io_lock);
-
 	if (it8712f_wdt_find(&address))
 		return -ENODEV;
drivers/watchdog/it87_lock.c new file +27 -0
new file mode 100644
index 0000000..06c0b49
@@ -0,0 +1,27 @@
+/*
+ *      IT87xxx super IO chipset access lock
+ *
+ *      Copyright (c) 2011 Nat Gurumoorthy <[email protected]>
+ *
+ *      Based on info and code taken from:
+ *
+ *      drivers/char/watchdog/scx200_wdt.c
+ *      drivers/hwmon/it87.c
+ *      IT8712F EC-LPC I/O Preliminary Specification 0.8.2
+ *      IT8712F EC-LPC I/O Preliminary Specification 0.9.3
+ *
+ *      This program is free software; you can redistribute it and/or
+ *      modify it under the terms of the GNU General Public License as
+ *      published by the Free Software Foundation; either version 2 of the
+ *      License, or (at your option) any later version.
+ *
+ *      The author(s) of this software shall not be held liable for damages
+ *      of any nature resulting due to the use of this software. This
+ *      software is provided AS-IS with no warranties.
+ */
+
+#include <linux/module.h>
+#include <linux/spinlock.h>
+
+DEFINE_SPINLOCK(it87_io_lock);
+EXPORT_SYMBOL(it87_io_lock);
drivers/watchdog/it87_wdt.c +7 -35

208 lines not on this page · raw message

include/linux/it87_lock.h new file +28 -0
new file mode 100644
index 0000000..ae91ce5
@@ -0,0 +1,28 @@
+/*
+ *      IT87xxx super IO chipset access lock
+ *
+ *      Copyright (c) 2011 Nat Gurumoorthy <[email protected]>
+ *
+ *      Based on info and code taken from:
+ *
+ *      drivers/char/watchdog/scx200_wdt.c
+ *      drivers/hwmon/it87.c
+ *      IT8712F EC-LPC I/O Preliminary Specification 0.8.2
+ *      IT8712F EC-LPC I/O Preliminary Specification 0.9.3
+ *
+ *      This program is free software; you can redistribute it and/or
+ *      modify it under the terms of the GNU General Public License as
+ *      published by the Free Software Foundation; either version 2 of the
+ *      License, or (at your option) any later version.
+ *
+ *      The author(s) of this software shall not be held liable for damages
+ *      of any nature resulting due to the use of this software. This
+ *      software is provided AS-IS with no warranties.
+ */
+
+#include <linux/spinlock.h>
+
+/*
+ * Global lock used by all it87 drivers to serialize access to hardware.
+ */
+extern spinlock_t it87_io_lock;
Signature · 7 lines
1.7.3.1


_______________________________________________
lm-sensors mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
#1 lore raw
Quoted 31 lines · Nat Gurumoorthy

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 defining a global spinlock it87_io_lock in a file it87_lock.c. This lock has to be acquired by each of the it87 drivers before it can access the hardware. We have defined a new Kconfig option IT87_LOCK. When it is selected it87_lock.c is compiled into the kernel thereby making the lock global and accessable to the it87 drivers which are typically built as loadable modules. All the it87 drivers select IT87_LOCK to compile the lock into the kernel.
The routines accessing the hardware are being called from module init, open, ioctl and module exit routines and hence it is sufficient to use calls to spin_lock and spin_unlock to acquire and release the locks. For the same reasons it87_wdt.c has extensive changes to remove calls to spin_lock_irqsave and spin_unlock_irqrestore. The lock is now acquired in superio_enter and released in superio_exit. This is now identical to the code in drivers/hwmon/it87.c and drivers/watchdog/it8712f_wdt.c. Added __acquire and __release annotations wherever needed.

---
 drivers/hwmon/Kconfig          |    1 +
 drivers/hwmon/it87.c           |   14 ++++++++++++-
 drivers/watchdog/Kconfig       |   12 +++++++++++
 drivers/watchdog/Makefile      |    1 +
 drivers/watchdog/it8712f_wdt.c |   10 ++++----
 drivers/watchdog/it87_lock.c   |   27 +++++++++++++++++++++++++
 drivers/watchdog/it87_wdt.c    |   42 ++++++---------------------------------
 include/linux/it87_lock.h      |   28 ++++++++++++++++++++++++++
 8 files changed, 94 insertions(+), 41 deletions(-)
 create mode 100644 drivers/watchdog/it87_lock.c
 create mode 100644 include/linux/it87_lock.h
Quoted 2 lines
Signed-off-by Nat Gurumoorthy <[email protected]>

Seems to me those should be separate patches, one per affected subsystem.

Guenter

_______________________________________________
lm-sensors mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

author
#2 lore raw

Guenter,
This patch patch applies a common fix to all IT87 driver.
Separating it into 2 patches (one for drivers/hwmon and one for drivers/watchdog) does not seem to be the right thing to do. I think having all the patches in one file makes it easier to understand the rationale behind the patch. The it87_io_lock.c and it87_io_lock.h files are in drivers/watchdog directory and the lock defined in it is needed by the changes in drivers/hwmon/it87.c

Regards
Nat

Quoted 33 lines · Guenter Roeck
Quoted 31 lines · Nat Gurumoorthy

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 defining a global spinlock it87_io_lock in a file it87_lock.c. This lock has to be acquired by each of the it87 drivers before it can access the hardware. We have defined a new Kconfig option IT87_LOCK. When it is selected it87_lock.c is compiled into the kernel thereby making the lock global and accessable to the it87 drivers which are typically built as loadable modules. All the it87 drivers select IT87_LOCK to compile the lock into the kernel.
The routines accessing the hardware are being called from module init, open, ioctl and module exit routines and hence it is sufficient to use calls to spin_lock and spin_unlock to acquire and release the locks. For the same reasons it87_wdt.c has extensive changes to remove calls to spin_lock_irqsave and spin_unlock_irqrestore. The lock is now acquired in superio_enter and released in superio_exit. This is now identical to the code in drivers/hwmon/it87.c and drivers/watchdog/it8712f_wdt.c. Added __acquire and __release annotations wherever needed.
---
 drivers/hwmon/Kconfig          |    1 +
 drivers/hwmon/it87.c           |   14 ++++++++++++- drivers/watchdog/Kconfig       |   12 +++++++++++ drivers/watchdog/Makefile      |    1 + drivers/watchdog/it8712f_wdt.c |   10 ++++---- drivers/watchdog/it87_lock.c   |   27 +++++++++++++++++++++++++ drivers/watchdog/it87_wdt.c    |   42 ++++++--------------------------------- include/linux/it87_lock.h      |   28 ++++++++++++++++++++++++++

 8 files changed, 94 insertions(+), 41 deletions(-)

 create mode 100644 drivers/watchdog/it87_lock.c
 create mode 100644 include/linux/it87_lock.h

Quoted 5 lines
Quoted 2 lines
Signed-off-by Nat Gurumoorthy <[email protected]>

Seems to me those should be separate patches, one per affected subsystem.

Guenter

--
Regards
Nat Gurumoorthy AB6SJ

_______________________________________________
lm-sensors mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

author
#3 lore raw
In reply to Nat Gurumoorthy

Guenter,
This patch patch applies a common fix to all IT87 driver. Separating
it into 2 patches (one for drivers/hwmon and one for drivers/watchdog) does not seem to be the right thing to do. I think having all the patches in one file makes it easier to understand the rationale behind the patch. The it87_io_lock.c and it87_io_lock.h files are in drivers/watchdog directory and the lock defined in it is needed by the changes in drivers/hwmon/it87.c

Regards
Nat

Quoted 43 lines · Guenter Roeck
Quoted 2 lines · Nat Gurumoorthy

There are 3 different drivers that touch the it87 hardware registers. The 3 drivers have been written independently and access the it87

hardware

Quoted 2 lines

registers assuming they are the only driver accessing it. This change attempts to serialize access to the hardware by defining a global

spinlock

Quoted 3 lines

it87_io_lock in a file it87_lock.c. This lock has to be acquired by each of the it87 drivers before it can access the hardware. We have defined a new Kconfig option IT87_LOCK. When it is selected it87_lock.c is

compiled

Quoted 2 lines

into the kernel thereby making the lock global and accessable to the it87 drivers which are typically built as loadable modules. All the it87

drivers

Quoted 9 lines

select IT87_LOCK to compile the lock into the kernel.
The routines accessing the hardware are being called from module init, open, ioctl and module exit routines and hence it is sufficient to use calls to spin_lock and spin_unlock to acquire and release the locks. For the same reasons it87_wdt.c has extensive changes to remove calls to spin_lock_irqsave and spin_unlock_irqrestore. The lock is now acquired in superio_enter and released in superio_exit. This is now identical to the code in drivers/hwmon/it87.c and drivers/watchdog/it8712f_wdt.c. Added __acquire and __release annotations wherever needed.

Quoted 8 lines
---
 drivers/hwmon/Kconfig          |    1 +
 drivers/hwmon/it87.c           |   14 ++++++++++++-
 drivers/watchdog/Kconfig       |   12 +++++++++++
 drivers/watchdog/Makefile      |    1 +
 drivers/watchdog/it8712f_wdt.c |   10 ++++----
 drivers/watchdog/it87_lock.c   |   27 +++++++++++++++++++++++++
 drivers/watchdog/it87_wdt.c    |   42

++++++---------------------------------

Quoted 7 lines
 include/linux/it87_lock.h      |   28 ++++++++++++++++++++++++++
 8 files changed, 94 insertions(+), 41 deletions(-)
 create mode 100644 drivers/watchdog/it87_lock.c
 create mode 100644 include/linux/it87_lock.h
Signed-off-by Nat Gurumoorthy <[email protected]>

Seems to me those should be separate patches, one per affected subsystem.

Guenter

Signature · 2 lines
Regards
Nat Gurumoorthy AB6SJ

_______________________________________________
lm-sensors mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

#4 lore raw
Quoted 8 lines · Natarajan Gurumoorthy

Guenter,
This patch patch applies a common fix to all IT87 driver. Separating it
into 2 patches (one for drivers/hwmon and one for drivers/watchdog) does not seem to be the right thing to do. I think having all the patches in one file makes it easier to understand the rationale behind the patch. The it87_io_lock.c and it87_io_lock.h files are in drivers/watchdog directory and the lock defined in it is needed by the changes in drivers/hwmon/it87.c

I dislike the idea of a single patch crossing multiple subsystems. Maybe you'll find another maintainer accepting your change as a single patch, but I won't.

That doesn't mean that the patches would have to be committed into multiple trees. That is for the subsystem maintainers to sort out, and a different issue.

Thanks,
Guenter

_______________________________________________
lm-sensors mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

author
#5 lore raw

Guenter,
      How would you partition it out? Are you suggesting that we do the following:

Patch1:
   drivers/hwmon/Kconfig          |    1 + drivers/hwmon/it87.c           |   14 ++++++++++++-

Patch2:
   drivers/watchdog/Kconfig       |   12 +++++++++++ drivers/watchdog/Makefile      |    1 + drivers/watchdog/it8712f_wdt.c |   10 ++++---- drivers/watchdog/it87_lock.c   |   27 +++++++++++++++++++++++++ drivers/watchdog/it87_wdt.c    |   42 ++++++---------------------------------

Patch3:
   include/linux/it87_lock.h      |   28 ++++++++++++++++++++++++++

Nat

Quoted 10 lines · Guenter Roeck
Quoted 8 lines · Natarajan Gurumoorthy

Guenter,
       This patch patch applies a common fix to all IT87 driver. Separating it into 2 patches (one for drivers/hwmon and one for drivers/watchdog) does not seem to be the right thing to do. I think having all the patches in one file makes it easier to understand the rationale behind the patch. The it87_io_lock.c and it87_io_lock.h files are in drivers/watchdog directory and the lock defined in it is needed by the changes in drivers/hwmon/it87.c

Quoted 9 lines

I dislike the idea of a single patch crossing multiple subsystems. Maybe you'll find another maintainer accepting your change as a single patch, but I won't.

That doesn't mean that the patches would have to be committed into multiple trees. That is for the subsystem maintainers to sort out, and a different issue.

Thanks,
Guenter

--
Regards
Nat Gurumoorthy AB6SJ

_______________________________________________
lm-sensors mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

#6 lore raw
Quoted 15 lines · Natarajan Gurumoorthy

Guenter,
      How would you partition it out? Are you suggesting that we do the following:

Patch1:
   drivers/hwmon/Kconfig          |    1 + drivers/hwmon/it87.c           |   14 ++++++++++++-

Patch2:
   drivers/watchdog/Kconfig       |   12 +++++++++++ drivers/watchdog/Makefile      |    1 + drivers/watchdog/it8712f_wdt.c |   10 ++++---- drivers/watchdog/it87_lock.c   |   27 +++++++++++++++++++++++++ drivers/watchdog/it87_wdt.c    |   42 ++++++---------------------------------

Quoted 3 lines

Patch3:
   include/linux/it87_lock.h      |   28 ++++++++++++++++++++++++++

No, not really. The include file is part of the locking code, and the sequence is wrong.

I personally would introduce the lock in the 1st patch. This would affect
drivers/watchdog/it87_lock.c include/linux/it87_lock.h drivers/watchdog/Makefile drivers/watchdog/Kconfig

The second patch would update the watchdog driver, affecting

	drivers/watchdog/Kconfig
	drivers/watchdog/it8712f_wdt.c

and the last patch would update the hwmon driver.
drivers/hwmon/Kconfig
drivers/hwmon/it87.c

Others may argue that patch 1 and 2 (introducing the lock and updating the watchdog driver) should be in a single patch, since the lock alone does not do anything without being used. This is a matter of opinion and really depends on the maintainer of the watchdog subsystem.

Note that your patch has practical problems. If I disable WATCHDOG but enable the IT87 hwmon driver, I get:

warning: (SENSORS_IT87) selects IT87_LOCK which has unmet direct dependencies (WATCHDOG)

during configuration, and undefined references to it87_io_lock when linking. So it looks like you might want to consider moving the locking code to a location outside the watchdog code.

Thanks,
Guenter

_______________________________________________
lm-sensors mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

author
#7 lore raw

Guenter,
Thank you for spotting the fact the everything goes south if you
disable "watchdog". I am working on a solution. Looks like the ideal place to store it87_io_lock.c will be drivers/misc and the IT87_LOCK config will be placed before the MISC_DEVICES entry in drivers/misc/Kconfig file. This will be similar to the SENSORS_LIS3LV02D entry in that Kconfig file.

Now going back to the partitioning do I send this change out as
a multi patch set consisting of 4 parts something as below:

patch 0 has a description

patch 1 has only the lock and related files
drivers/misc/Kconfig
drivers/misc/Makefile
include/linux/it87_lock.h
drivers/misc/it87_lock.c

patch 2 has drivers/watchdog changes

        drivers/watchdog/Kconfig
        drivers/watchdog/it8712f_wdt.c
        drivers/watchdog/it87_wdt.c

patch 3 has drives/hwmon changes
drivers/hwmon/Kconfig
drivers/hwmon/it87.c

Regards
Nat

Quoted 45 lines · Guenter Roeck
Quoted 15 lines · Natarajan Gurumoorthy

Guenter,
      How would you partition it out? Are you suggesting that we do the following:

Patch1:
   drivers/hwmon/Kconfig          |    1 + drivers/hwmon/it87.c           |   14 ++++++++++++-

Patch2:
   drivers/watchdog/Kconfig       |   12 +++++++++++ drivers/watchdog/Makefile      |    1 + drivers/watchdog/it8712f_wdt.c |   10 ++++---- drivers/watchdog/it87_lock.c   |   27 +++++++++++++++++++++++++ drivers/watchdog/it87_wdt.c    |   42 ++++++---------------------------------

Quoted 3 lines

Patch3:
   include/linux/it87_lock.h      |   28 ++++++++++++++++++++++++++

No, not really. The include file is part of the locking code, and the sequence is wrong.

I personally would introduce the lock in the 1st patch. This would affect
       drivers/watchdog/it87_lock.c include/linux/it87_lock.h drivers/watchdog/Makefile drivers/watchdog/Kconfig

The second patch would update the watchdog driver, affecting drivers/watchdog/Kconfig drivers/watchdog/it8712f_wdt.c

and the last patch would update the hwmon driver.
       drivers/hwmon/Kconfig
       drivers/hwmon/it87.c

Others may argue that patch 1 and 2 (introducing the lock and updating the watchdog driver) should be in a single patch, since the lock alone does not do anything without being used. This is a matter of opinion and really depends on the maintainer of the watchdog subsystem.

Note that your patch has practical problems. If I disable WATCHDOG but enable the IT87 hwmon driver, I get:

Quoted 8 lines

warning: (SENSORS_IT87) selects IT87_LOCK which has unmet direct dependencies (WATCHDOG)

during configuration, and undefined references to it87_io_lock when linking. So it looks like you might want to consider moving the locking code to a location outside the watchdog code.

Thanks,
Guenter

--
Regards
Nat Gurumoorthy AB6SJ

_______________________________________________
lm-sensors mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

#8 lore raw
Quoted 8 lines · Natarajan Gurumoorthy

Guenter,
Thank you for spotting the fact the everything goes south if you
disable "watchdog". I am working on a solution. Looks like the ideal place to store it87_io_lock.c will be drivers/misc and the IT87_LOCK config will be placed before the MISC_DEVICES entry in drivers/misc/Kconfig file. This will be similar to the SENSORS_LIS3LV02D entry in that Kconfig file.

Almost, only afaik that is only used inside the misc directory, or at least has some other components there. I don't really know how to handle this situation correctly, except you could of course write a mfd driver to handle the generic parts.

Quoted 11 lines

Now going back to the partitioning do I send this change out as
a multi patch set consisting of 4 parts something as below:

patch 0 has a description

patch 1 has only the lock and related files
drivers/misc/Kconfig
drivers/misc/Makefile
include/linux/it87_lock.h
drivers/misc/it87_lock.c

Quoted 9 lines

patch 2 has drivers/watchdog changes

        drivers/watchdog/Kconfig
        drivers/watchdog/it8712f_wdt.c
        drivers/watchdog/it87_wdt.c

patch 3 has drives/hwmon changes
drivers/hwmon/Kconfig
drivers/hwmon/it87.c

Something like that. I have some doubts about using drivers/misc, but I guess you'll get feedback on that after you submit the patch set.

Thanks,
Guenter

_______________________________________________
lm-sensors mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

author
#9 lore raw

Guenter,
Thank you for your feedback. I found an easier way to deal with
the it87_lock.c file. I do not need to move it to the driver/misc directory. The easier thing to do was to move the IT87_LOCK entry to the beginning of the drivers/watchdog/Kconfig file. This moves the entry before the "menuconfig WATCHDOG" entry. They use the exact same trick in drivers/misc/Kconfig file. The very first lines of drivers/watchdog/Kconfig file will look something like this:

#
# Watchdog device configuration
#

# This one has to live outside of the WATCHDOG conditional,

# because it may be selected by drivers/hwmon/it87.c.
config IT87_LOCK
	bool
	default n
	---help---
	  This defines the global lock needed by all IT87 drivers that
	  touch the Super I/0 chipset used on many motherboards. This
	  is needed to serialize access to the registers in SMP
	  systems. All it87 drivers will select this Kconfig option to compile
	  the lock into the kernel.

menuconfig WATCHDOG
bool "Watchdog Timer Support"
---help---
If you say Y here (and to one of the following options) and create a

Regards
Nat

Quoted 40 lines · Guenter Roeck
Quoted 8 lines · Natarajan Gurumoorthy

Guenter,
      Thank you for spotting the fact the everything goes south if you disable "watchdog". I am working on a solution. Looks like the ideal place to store it87_io_lock.c will be drivers/misc and the IT87_LOCK config will be placed before the MISC_DEVICES entry in drivers/misc/Kconfig file. This will be similar to the SENSORS_LIS3LV02D entry in that Kconfig file.

Almost, only afaik that is only used inside the misc directory, or at least has some other components there. I don't really know how to handle this situation correctly, except you could of course write a mfd driver to handle the generic parts.

Quoted 11 lines

      Now going back to the partitioning do I send this change out as a multi patch set consisting of 4 parts something as below:

patch 0 has a description

patch 1 has only the lock and related files
        drivers/misc/Kconfig
        drivers/misc/Makefile
        include/linux/it87_lock.h
        drivers/misc/it87_lock.c

Quoted 9 lines

patch 2 has drivers/watchdog changes
        drivers/watchdog/Kconfig
        drivers/watchdog/it8712f_wdt.c
        drivers/watchdog/it87_wdt.c

patch 3 has drives/hwmon changes
        drivers/hwmon/Kconfig
        drivers/hwmon/it87.c

Something like that. I have some doubts about using drivers/misc, but I guess you'll get feedback on that after you submit the patch set.

Thanks,
Guenter

Signature · 7 lines
Regards
Nat Gurumoorthy AB6SJ

_______________________________________________
lm-sensors mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
#10 lore raw
Quoted 8 lines · Natarajan Gurumoorthy

Guenter,
     Thank you for your feedback. I found an easier way to deal with the it87_lock.c file. I do not need to move it to the driver/misc directory. The easier thing to do was to move the IT87_LOCK entry to the beginning of the drivers/watchdog/Kconfig file. This moves the entry before the "menuconfig WATCHDOG" entry. They use the exact same trick in drivers/misc/Kconfig file. The very first lines of drivers/watchdog/Kconfig file will look something like this:

Is the Kconfig even warranted? I mean, if there is a shared resource
and it needs locking, then just apply the locking. Where is the value
in adding another Kconfig?

Also the it87_lock.h file -- maybe you want to just call it it87.h -- then if someone in the future cleans up the drivers and makes some macros that can be shared across all the drivers, there is already a proper home for such macros. No point painting yourself in a corner and making the file lock specific right from the beginning.

Paul.

Quoted 70 lines

#
# Watchdog device configuration
#

# This one has to live outside of the WATCHDOG conditional,

# because it may be selected by drivers/hwmon/it87.c.
config IT87_LOCK
       bool
       default n
       ---help---
         This defines the global lock needed by all IT87 drivers that
         touch the Super I/0 chipset used on many motherboards. This
         is needed to serialize access to the registers in SMP
         systems. All it87 drivers will select this Kconfig option to compile
         the lock into the kernel.

menuconfig WATCHDOG
       bool "Watchdog Timer Support"
       ---help---
         If you say Y here (and to one of the following options) and create a

Regards
Nat

Quoted 34 lines · Guenter Roeck
Quoted 8 lines · Natarajan Gurumoorthy

Guenter,
      Thank you for spotting the fact the everything goes south if you disable "watchdog". I am working on a solution. Looks like the ideal place to store it87_io_lock.c will be drivers/misc and the IT87_LOCK config will be placed before the MISC_DEVICES entry in drivers/misc/Kconfig file. This will be similar to the SENSORS_LIS3LV02D entry in that Kconfig file.

Almost, only afaik that is only used inside the misc directory, or at least has some other components there. I don't really know how to handle this situation correctly, except you could of course write a mfd driver to handle the generic parts.

Quoted 11 lines

      Now going back to the partitioning do I send this change out as a multi patch set consisting of 4 parts something as below:

patch 0 has a description

patch 1 has only the lock and related files
        drivers/misc/Kconfig
        drivers/misc/Makefile
        include/linux/it87_lock.h
        drivers/misc/it87_lock.c

Quoted 9 lines

patch 2 has drivers/watchdog changes
        drivers/watchdog/Kconfig
        drivers/watchdog/it8712f_wdt.c
        drivers/watchdog/it87_wdt.c

patch 3 has drives/hwmon changes
        drivers/hwmon/Kconfig
        drivers/hwmon/it87.c

Quoted 6 lines

Something like that. I have some doubts about using drivers/misc, but I guess you'll get feedback on that after you submit the patch set.

Thanks,
Guenter

Quoted 9 lines

--
Regards
Nat Gurumoorthy AB6SJ
--
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/

_______________________________________________
lm-sensors mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

author
#11 lore raw

Paul,
Comments are below.

Quoted 10 lines · Paul Gortmaker
Quoted 8 lines · Natarajan Gurumoorthy

Guenter,
     Thank you for your feedback. I found an easier way to deal with the it87_lock.c file. I do not need to move it to the driver/misc directory. The easier thing to do was to move the IT87_LOCK entry to the beginning of the drivers/watchdog/Kconfig file. This moves the entry before the "menuconfig WATCHDOG" entry. They use the exact same trick in drivers/misc/Kconfig file. The very first lines of drivers/watchdog/Kconfig file will look something like this:

Quoted 4 lines

Is the Kconfig even warranted?  I mean, if there is a shared resource and it needs locking, then just apply the locking.   Where is the value in adding another Kconfig?

The reason we are forced to do this is that all the it87 drivers are usually modules. We need a global lock that is compiled into the kernel if one or more of these drivers are selected. If none of them are specified then the lock is not built into the kernel. The Kconfig entry that I have put is not human selectable. When any IT87 driver is selected it will select IT87_LOCK there by compiling the lock into the kernel.

Signature · 7 lines
Regards
Nat Gurumoorthy AB6SJ

_______________________________________________
lm-sensors mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
#12 lore raw
In reply to Guenter Roeck
Quoted 30 lines · Guenter Roeck
Quoted 8 lines · Natarajan Gurumoorthy

Guenter,
Thank you for spotting the fact the everything goes south if you
disable "watchdog". I am working on a solution. Looks like the ideal place to store it87_io_lock.c will be drivers/misc and the IT87_LOCK config will be placed before the MISC_DEVICES entry in drivers/misc/Kconfig file. This will be similar to the SENSORS_LIS3LV02D entry in that Kconfig file.

Almost, only afaik that is only used inside the misc directory, or at least has some other components there. I don't really know how to handle this situation correctly, except you could of course write a mfd driver to handle the generic parts.

Quoted 16 lines

Now going back to the partitioning do I send this change out as
a multi patch set consisting of 4 parts something as below:

patch 0 has a description

patch 1 has only the lock and related files
drivers/misc/Kconfig
drivers/misc/Makefile
include/linux/it87_lock.h
drivers/misc/it87_lock.c

patch 2 has drivers/watchdog changes

        drivers/watchdog/Kconfig
        drivers/watchdog/it8712f_wdt.c
        drivers/watchdog/it87_wdt.c
Quoted 6 lines
Quoted 4 lines

patch 3 has drives/hwmon changes
drivers/hwmon/Kconfig
drivers/hwmon/it87.c

Something like that. I have some doubts about using drivers/misc, but I guess you'll get feedback on that after you submit the patch set.

I would definitely prefer drivers/mfd over drivers/misc. The problematic we are trying to solve here is typically a multi-function device one.

This also raises concerns about the implementation. The shared spinlock looks like a band-aid solution to me. The initial problem is that all these drivers access I/O ports they did NOT reserve as they were supposed to do. If they did, the conflict would have been spotted much earlier.

I seem to recall that there has been work in the past on a new "superio" subsystem which would help centralize detection of and I/O access to all Super-I/O chips (bringing driver autoloading in almost all cases as a nice side benefit.) I never had the time to review it, but as I recall others had reviewed it so it may be in a suitable shape for upstream inclusion (after forward-porting - the code is getting old.)

http://www.google.com/search?q=site%3Alists.lm-sensors.org+superio+lock

Don't get me wrong, the current situation is bad enough that a band-aid solution is welcome. But it doesn't seem good enough for the long run.

Signature · 6 lines
Jean Delvare

_______________________________________________
lm-sensors mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
#13 lore raw
In reply to Nat Gurumoorthy
Quoted 3 lines

attempts to serialize access to the hardware by defining a global spinlock it87_io_lock in a file it87_lock.c. This lock has to be acquired by each of the it87 drivers before it can access the hardware. We have defined

I think this needs a NAK

The resource handler can do this for you and would ensure proper behaviour.

See
IORESOURCE_MUXED

Providing the it87 drivers request the resources (which they need to do anyway!) and use IORESOURCE_MUXED, it should all work ?

Alan

_______________________________________________
lm-sensors mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors