[PATCH v5 0/2] Make all it87 drivers SMP safe

[PATCH v5 0/2] Make all it87 drivers SMP safe

[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 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           |    6 +++++
 drivers/watchdog/it8712f_wdt.c |   11 +++++----
 drivers/watchdog/it87_wdt.c    |   41 +++++----------------------------------
 3 files changed, 18 insertions(+), 40 deletions(-)

Signed-off-by: Nat Gurumoorthy <natg@google.com>

Patch History:
v5:
- Remove unnecessary while from superio_enter.

v4:
- Remove extra braces in superio_enter routines.

v3:
- Totally abandon the spinlock based approach and use "request_muxed_region" to
  hold off requestors if the resource is busy.

v2:
- More verbose patch headers. Add In-Reply-To: field.

[PATCH v5 1/2] Use "request_muxed_region" in it87 watchdog drivers

[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 <natg@google.com>
---

diff --git a/drivers/watchdog/it8712f_wdt.c b/drivers/watchdog/it8712f_wdt.c
index 6143f52..51bfbc0 100644
--- a/drivers/watchdog/it8712f_wdt.c
+++ b/drivers/watchdog/it8712f_wdt.c
@@ -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)
 
 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);
+
 	outb(0x87, REG);
 	outb(0x01, REG);
 	outb(0x55, REG);
@@ -134,7 +137,7 @@ static inline void superio_exit(void)
 {
 	outb(0x02, REG);
 	outb(0x02, VAL);
-	spin_unlock(&io_lock);
+	release_region(REG, 2);
 }
 
 static inline void it8712f_wdt_ping(void)
@@ -382,8 +385,6 @@ static int __init it8712f_wdt_init(void)
 {
 	int err = 0;
 
-	spin_lock_init(&io_lock);
-
 	if (it8712f_wdt_find(&address))
 		return -ENODEV;
 
diff --git a/drivers/watchdog/it87_wdt.c b/drivers/watchdog/it87_wdt.c
index b1bc72f..ac7d26a 100644
--- a/drivers/watchdog/it87_wdt.c
+++ b/drivers/watchdog/it87_wdt.c
@@ -137,7 +137,6 @@
 
 static	unsigned int base, gpact, ciract, max_units, chip_type;
 static	unsigned long wdt_status;
-static	DEFINE_SPINLOCK(spinlock);
 
 static	int nogameport = DEFAULT_NOGAMEPORT;
 static	int exclusive  = DEFAULT_EXCLUSIVE;
@@ -165,6 +164,11 @@ MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started, default="
 
 static inline void superio_enter(void)
 {
+	/*
+	 * Reserve REG and REG + 1 for exclusive access.
+	 */
+	(void) request_muxed_region(REG, 2, WATCHDOG_NAME);
+
 	outb(0x87, REG);
 	outb(0x01, REG);
 	outb(0x55, REG);
@@ -175,6 +179,7 @@ static inline void superio_exit(void)
 {
 	outb(0x02, REG);
 	outb(0x02, VAL);
+	release_region(REG, 2);
 }
 
 static inline void superio_select(int ldn)
@@ -257,9 +262,6 @@ static void wdt_keepalive(void)
 
 static void wdt_start(void)
 {
-	unsigned long flags;
-
-	spin_lock_irqsave(&spinlock, flags);
 	superio_enter();
 
 	superio_select(GPIO);
@@ -270,14 +272,10 @@ static void wdt_start(void)
 	wdt_update_timeout();
 
 	superio_exit();
-	spin_unlock_irqrestore(&spinlock, flags);
 }
 
 static void wdt_stop(void)
 {
-	unsigned long flags;
-
-	spin_lock_irqsave(&spinlock, flags);
 	superio_enter();
 
 	superio_select(GPIO);
@@ -288,7 +286,6 @@ static void wdt_stop(void)
 		superio_outb(0x00, WDTVALMSB);
 
 	superio_exit();
-	spin_unlock_irqrestore(&spinlock, flags);
 }
 
 /**
@@ -303,8 +300,6 @@ static void wdt_stop(void)
 
 static int wdt_set_timeout(int t)
 {
-	unsigned long flags;
-
 	if (t < 1 || t > max_units * 60)
 		return -EINVAL;
 
@@ -313,14 +308,12 @@ static int wdt_set_timeout(int t)
 	else
 		timeout = t;
 
-	spin_lock_irqsave(&spinlock, flags);
 	if (test_bit(WDTS_TIMER_RUN, &wdt_status)) {
 		superio_enter();
 		superio_select(GPIO);
 		wdt_update_timeout();
 		superio_exit();
 	}
-	spin_unlock_irqrestore(&spinlock, flags);
 	return 0;
 }
 
@@ -339,11 +332,8 @@ static int wdt_set_timeout(int t)
 
 static int wdt_get_status(int *status)
 {
-	unsigned long flags;
-
 	*status = 0;
 	if (testmode) {
-		spin_lock_irqsave(&spinlock, flags);
 		superio_enter();
 		superio_select(GPIO);
 		if (superio_inb(WDTCTRL) & WDT_ZERO) {
@@ -353,7 +343,6 @@ static int wdt_get_status(int *status)
 		}
 
 		superio_exit();
-		spin_unlock_irqrestore(&spinlock, flags);
 	}
 	if (test_and_clear_bit(WDTS_KEEPALIVE, &wdt_status))
 		*status |= WDIOF_KEEPALIVEPING;
@@ -560,16 +549,13 @@ static int __init it87_wdt_init(void)
 	int rc = 0;
 	int try_gameport = !nogameport;
 	u8  chip_rev;
-	unsigned long flags;
 
 	wdt_status = 0;
 
-	spin_lock_irqsave(&spinlock, flags);
 	superio_enter();
 	chip_type = superio_inw(CHIPID);
 	chip_rev  = superio_inb(CHIPREV) & 0x0f;
 	superio_exit();
-	spin_unlock_irqrestore(&spinlock, flags);
 
 	switch (chip_type) {
 	case IT8702_ID:
@@ -603,7 +589,6 @@ static int __init it87_wdt_init(void)
 		return -ENODEV;
 	}
 
-	spin_lock_irqsave(&spinlock, flags);
 	superio_enter();
 
 	superio_select(GPIO);
@@ -621,14 +606,12 @@ static int __init it87_wdt_init(void)
 		gpact = superio_inb(ACTREG);
 		superio_outb(0x01, ACTREG);
 		superio_exit();
-		spin_unlock_irqrestore(&spinlock, flags);
 		if (request_region(base, 1, WATCHDOG_NAME))
 			set_bit(WDTS_USE_GP, &wdt_status);
 		else
 			rc = -EIO;
 	} else {
 		superio_exit();
-		spin_unlock_irqrestore(&spinlock, flags);
 	}
 
 	/* If we haven't Gameport support, try to get CIR support */
@@ -646,7 +629,6 @@ static int __init it87_wdt_init(void)
 			goto err_out;
 		}
 		base = CIR_BASE;
-		spin_lock_irqsave(&spinlock, flags);
 		superio_enter();
 
 		superio_select(CIR);
@@ -660,7 +642,6 @@ static int __init it87_wdt_init(void)
 		}
 
 		superio_exit();
-		spin_unlock_irqrestore(&spinlock, flags);
 	}
 
 	if (timeout < 1 || timeout > max_units * 60) {
@@ -711,21 +692,17 @@ err_out_reboot:
 err_out_region:
 	release_region(base, test_bit(WDTS_USE_GP, &wdt_status) ? 1 : 8);
 	if (!test_bit(WDTS_USE_GP, &wdt_status)) {
-		spin_lock_irqsave(&spinlock, flags);
 		superio_enter();
 		superio_select(CIR);
 		superio_outb(ciract, ACTREG);
 		superio_exit();
-		spin_unlock_irqrestore(&spinlock, flags);
 	}
 err_out:
 	if (try_gameport) {
-		spin_lock_irqsave(&spinlock, flags);
 		superio_enter();
 		superio_select(GAMEPORT);
 		superio_outb(gpact, ACTREG);
 		superio_exit();
-		spin_unlock_irqrestore(&spinlock, flags);
 	}
 
 	return rc;
@@ -733,10 +710,6 @@ err_out:
 
 static void __exit it87_wdt_exit(void)
 {
-	unsigned long flags;
-	int nolock;
-
-	nolock = !spin_trylock_irqsave(&spinlock, flags);
 	superio_enter();
 	superio_select(GPIO);
 	superio_outb(0x00, WDTCTRL);
@@ -752,8 +725,6 @@ static void __exit it87_wdt_exit(void)
 		superio_outb(ciract, ACTREG);
 	}
 	superio_exit();
-	if (!nolock)
-		spin_unlock_irqrestore(&spinlock, flags);
 
 	misc_deregister(&wdt_miscdev);
 	unregister_reboot_notifier(&wdt_notifier);
-- 
1.7.3.1

Re: [lm-sensors] [PATCH v5 1/2] Use "request_muxed_region" in it87 watchdog drivers

[Hans de Goede]

Hi,

On 04/12/2011 10:49 PM, Nat Gurumoorthy wrote:
> 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<natg@google.com>
> ---
>
> diff --git a/drivers/watchdog/it8712f_wdt.c b/drivers/watchdog/it8712f_wdt.c
> index 6143f52..51bfbc0 100644
> --- a/drivers/watchdog/it8712f_wdt.c
> +++ b/drivers/watchdog/it8712f_wdt.c
> @@ -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)
>
>   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.

Regards,

Hans

Re: [lm-sensors] [PATCH v5 1/2] Use "request_muxed_region" in it87 watchdog drivers

[Natarajan Gurumoorthy]

Hans,
      Comments below

On Tue, Apr 12, 2011 at 11:50 PM, Hans de Goede <hdegoede@redhat.com> wrote:
>
> 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.

Regards
Nat


>
> Regards,
>
> Hans
>



-- 
Regards
Nat Gurumoorthy AB6SJ

Re: [lm-sensors] [PATCH v5 1/2] Use "request_muxed_region" in it87 watchdog drivers

[Jean Delvare]

On Wed, 13 Apr 2011 01:05:33 -0700, Natarajan Gurumoorthy wrote:
> Hans,
>       Comments below
> 
> On Tue, Apr 12, 2011 at 11:50 PM, Hans de Goede <hdegoede@redhat.com> wrote:
> >
> > 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.

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

Re: [lm-sensors] [PATCH v5 1/2] Use "request_muxed_region" in it87 watchdog drivers

[Natarajan Gurumoorthy]

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.

Regards
Nat

On Wed, Apr 13, 2011 at 1:34 AM, Jean Delvare <khali@linux-fr.org> wrote:
> On Wed, 13 Apr 2011 01:05:33 -0700, Natarajan Gurumoorthy wrote:
>> Hans,
>>       Comments below
>>
>> On Tue, Apr 12, 2011 at 11:50 PM, Hans de Goede <hdegoede@redhat.com> wrote:
>> >
>> > 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.
>
> 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
>



-- 
Regards
Nat Gurumoorthy AB6SJ

Re: [lm-sensors] [PATCH v5 1/2] Use "request_muxed_region" in it87 watchdog drivers

[Jean Delvare]

Hi Nat,

Please don't top-post.

On Wed, 13 Apr 2011 10:59:45 -0700, Natarajan Gurumoorthy wrote:
> 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.

f71882fg and sch5627 already call request_muxed_region() so I think
these are safe. OTOH you missed pc87427 at least. And there are
probably a few non-watchdog, non-hwmon drivers which need the same fix
(for example drivers/parport/parport_pc.c and
drivers/net/irda/via-ircc.h are suspect.)

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

For one thing, you don't have to. Converting just a subset is already a
welcomed step in the right direction. But anyway the changes should be
fairly easy and not too risky so I wouldn't mind you doing them without
testing them all. And we can find testers for most hwmon drivers, I'm
sure (I can personally test f71805f and w83627ehf, maybe w83627hf).

-- 
Jean Delvare

Re: [lm-sensors] [PATCH v5 1/2] Use "request_muxed_region" in it87 watchdog drivers

[Alan Cox]

> 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.

I would just fix the it87 drivers. The rest is the problem of the
authors/users of the other drivers. Really only they can test it and
you've given them a worked reference !

Alan

Re: [lm-sensors] [PATCH v5 1/2] Use "request_muxed_region" in it87 watchdog drivers

[Guenter Roeck]

On Wed, 2011-04-13 at 02:50 -0400, Hans de Goede wrote:
> Hi,
> 
> On 04/12/2011 10:49 PM, Nat Gurumoorthy wrote:
> > 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<natg@google.com>
> > ---
> >
> > diff --git a/drivers/watchdog/it8712f_wdt.c b/drivers/watchdog/it8712f_wdt.c
> > index 6143f52..51bfbc0 100644
> > --- a/drivers/watchdog/it8712f_wdt.c
> > +++ b/drivers/watchdog/it8712f_wdt.c
> > @@ -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)
> >
> >   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.
> 
Absolutely agree - even if that means adding a return code to
superio_enter() and checking it from the calling code.

Guenter

Re: [PATCH v5 1/2] Use "request_muxed_region" in it87 watchdog drivers

[Wim Van Sebroeck]

Hi Nat,

> +	(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.

Re: [PATCH v5 1/2] Use "request_muxed_region" in it87 watchdog drivers

[Natarajan Gurumoorthy]

Wim,
      Old habits die hard. I will get rid of it and ship out a new patch.

Regards
Nat


On Wed, Apr 13, 2011 at 12:03 AM, Wim Van Sebroeck <wim@iguana.be> wrote:
> Hi Nat,
>
>> +     (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.
>
>
>



-- 
Regards
Nat Gurumoorthy AB6SJ

Re: [PATCH v5 1/2] Use "request_muxed_region" in it87 watchdog drivers

[Alan Cox]

On Wed, 13 Apr 2011 09:03:00 +0200
Wim Van Sebroeck <wim@iguana.be> wrote:

> Hi Nat,
> 
> > +	(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 ?

Re: [PATCH v5 1/2] Use "request_muxed_region" in it87 watchdog drivers

[Mike Waychison]

On Wed, Apr 13, 2011 at 4:29 AM, Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:
> On Wed, 13 Apr 2011 09:03:00 +0200
> Wim Van Sebroeck <wim@iguana.be> wrote:
>
>> Hi Nat,
>>
>> > +   (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.

Ie:

Driver init:

if (!request_muxed_region(base, size, DRV_NAME))
   goto cleanup_driver_init_failed;

Driver cleanup

release_muxed_region(base, size); /* returns void */

And then within drivers:

use_muxed_region(base, size); /* sleeps until region is usable --
returns void */
/* Do stuff */
unuse_muxed_region(base, size); /* returns void */



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.

Re: [PATCH v5 1/2] Use "request_muxed_region" in it87 watchdog drivers

[Alan Cox]

> 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.

Which is exactly how the hardware situation works. If some other driver
comes along and is rude then the only safe thing to do is to moan a bit.

> 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.

Send patches

> 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.

I guess you'd need some kind of

	r = add_muxed_resource()
	removed_muxed_resource(r)

that inserted/removed them from the resource tree ?

I think the basic resource tree code could be extended this way IFF
someone does the work, if not the current code actually should work just
fine.

Alan

[PATCH v5 2/2] Use "request_muxed_region" in it87 hwmon drivers

[Nat Gurumoorthy]

02 - Chages to hwmon it87 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.

Signed-off-by: Nat Gurumoorthy <natg@google.com>
---

diff --git a/drivers/hwmon/it87.c b/drivers/hwmon/it87.c
index 316b648..3945569 100644
--- a/drivers/hwmon/it87.c
+++ b/drivers/hwmon/it87.c
@@ -111,6 +111,11 @@ superio_select(int ldn)
 static inline void
 superio_enter(void)
 {
+	/*
+	 * Reserve REG and REG + 1 for exclusive access.
+	 */
+	(void) request_muxed_region(REG, 2, DRVNAME);
+
 	outb(0x87, REG);
 	outb(0x01, REG);
 	outb(0x55, REG);
@@ -122,6 +127,7 @@ superio_exit(void)
 {
 	outb(0x02, REG);
 	outb(0x02, VAL);
+	release_region(REG, 2);
 }
 
 /* Logical device 4 registers */
-- 
1.7.3.1