Message-ID: <20100325180347.GA2761@salidar.me.mortis.eu>
Date:	Thu, 25 Mar 2010 19:03:47 +0100
From:	Giel van Schijndel <me@...tis.eu>
To:	Alan Cox <alan@...rguk.ukuu.org.uk>
Cc:	Hans de Goede <hdegoede@...hat.com>,
	Jean Delvare <khali@...ux-fr.org>,
	Jonathan Cameron <jic23@....ac.uk>,
	Laurens Leemans <laurens@...nips.com>,
	lm-sensors@...sensors.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/3] resource: shared I/O region support

On Thu, Mar 25, 2010 at 03:57:05PM +0000, Alan Cox wrote:
> On Thu, 25 Mar 2010 14:17:41 +0100 Giel van Schijndel wrote:
>> From: Alan Cox <alan@...rguk.ukuu.org.uk>
>> 
>> This patch was originally written by Alan Cox [1]. I only updated it to
>> apply correctly to Linus' current tree and changed IORESOURCE_MUXED's
>> value from 0x00200000 to 0x00400000 because the former has already been
>> taken in use since.
>> 
>> [1] https://patchwork.kernel.org/patch/32397/
>> 
>> Patch after this line:
>> ========================================================================
>> ... snip ...
>> @@ -687,7 +690,14 @@ struct resource * __request_region(struct resource *parent,
>>  			if (!(conflict->flags & IORESOURCE_BUSY))
>>  				continue;
>>  		}
>> -
>> +		if (conflict->flags & flags & IORESOURCE_MUXED) {
>> +			add_wait_queue(&muxed_resource_wait, &wait);
>> +			write_unlock(&resource_lock);
> 
> needs a set_current_state(TASK_UNINTERRUPTIBLE); here
> 
> (error from my original test patch that I noticed re-reviewing it)

Always fun reviewing your own code ;-)

New patch attached.

One question though. I did some reading in
Documentation/memory-barriers.txt and think it is correct *not* to call
set_current_state(TASK_INTERRUPTIBLE) after schedule() finishes. Could
you confirm or deny that?

Patch after this line:
resource: shared I/O region support

SuperIO devices share regions and use lock/unlock operations to chip
select.  We therefore need to be able to request a resource and wait for
it to be freed by whichever other SuperIO device currently hogs it.
Right now you have to poll which is horrible.

Add a MUXED field to IO port resources. If the MUXED field is set on the
resource and on the request (via request_muxed_region) then we block
until the previous owner of the muxed resource releases their region.

This allows us to implement proper resource sharing and locking for
superio chips using code of the form

enable_my_superio_dev() {
	request_muxed_region(0x44, 0x02, "superio:watchdog");
	outb() ..sequence to enable chip
}

disable_my_superio_dev() {
	outb() .. sequence of disable chip
	release_region(0x44, 0x02);
}

Signed-off-by: Giel van Schijndel <me@...tis.eu>
---
 include/linux/ioport.h |    4 +++-
 kernel/resource.c      |   16 +++++++++++++++-
 2 files changed, 18 insertions(+), 2 deletions(-)

diff --git a/include/linux/ioport.h b/include/linux/ioport.h
index 71ab79d..604fd29 100644
--- a/include/linux/ioport.h
+++ b/include/linux/ioport.h
@@ -52,6 +52,7 @@ struct resource_list {
 
 #define IORESOURCE_MEM_64	0x00100000
 #define IORESOURCE_WINDOW	0x00200000	/* forwarded by bridge */
+#define IORESOURCE_MUXED	0x00400000	/* Resource is software muxed */
 
 #define IORESOURCE_EXCLUSIVE	0x08000000	/* Userland may not map this resource */
 #define IORESOURCE_DISABLED	0x10000000
@@ -141,7 +142,8 @@ static inline unsigned long resource_type(const struct resource *res)
 }
 
 /* Convenience shorthand with allocation */
-#define request_region(start,n,name)	__request_region(&ioport_resource, (start), (n), (name), 0)
+#define request_region(start,n,name)		__request_region(&ioport_resource, (start), (n), (name), 0)
+#define request_muxed_region(start,n,name)	__request_region(&ioport_resource, (start), (n), (name), IORESOURCE_MUXED)
 #define __request_mem_region(start,n,name, excl) __request_region(&iomem_resource, (start), (n), (name), excl)
 #define request_mem_region(start,n,name) __request_region(&iomem_resource, (start), (n), (name), 0)
 #define request_mem_region_exclusive(start,n,name) \
diff --git a/kernel/resource.c b/kernel/resource.c
index 2d5be5d..ab44c7f 100644
--- a/kernel/resource.c
+++ b/kernel/resource.c
@@ -15,6 +15,7 @@
 #include <linux/spinlock.h>
 #include <linux/fs.h>
 #include <linux/proc_fs.h>
+#include <linux/sched.h>
 #include <linux/seq_file.h>
 #include <linux/device.h>
 #include <linux/pfn.h>
@@ -651,6 +652,8 @@ resource_size_t resource_alignment(struct resource *res)
  * release_region releases a matching busy region.
  */
 
+static DECLARE_WAIT_QUEUE_HEAD(muxed_resource_wait);
+
 /**
  * __request_region - create a new busy resource region
  * @parent: parent resource descriptor
@@ -663,6 +666,7 @@ struct resource * __request_region(struct resource *parent,
 				   resource_size_t start, resource_size_t n,
 				   const char *name, int flags)
 {
+	DECLARE_WAITQUEUE(wait, current);
 	struct resource *res = kzalloc(sizeof(*res), GFP_KERNEL);
 
 	if (!res)
@@ -687,7 +691,15 @@ struct resource * __request_region(struct resource *parent,
 			if (!(conflict->flags & IORESOURCE_BUSY))
 				continue;
 		}
-
+		if (conflict->flags & flags & IORESOURCE_MUXED) {
+			add_wait_queue(&muxed_resource_wait, &wait);
+			write_unlock(&resource_lock);
+			set_current_state(TASK_UNINTERRUPTIBLE);
+			schedule();
+			remove_wait_queue(&muxed_resource_wait, &wait);
+			write_lock(&resource_lock);
+			continue;
+		}
 		/* Uhhuh, that didn't work out.. */
 		kfree(res);
 		res = NULL;
@@ -761,6 +773,8 @@ void __release_region(struct resource *parent, resource_size_t start,
 				break;
 			*p = res->sibling;
 			write_unlock(&resource_lock);
+			if (res->flags & IORESOURCE_MUXED)
+				wake_up(&muxed_resource_wait);
 			kfree(res);
 			return;
 		}
-- 
1.6.4.4

-- 
Met vriendelijke groet,
With kind regards,
Giel van Schijndel

Download attachment "signature.asc" of type "application/pgp-signature" (199 bytes)

Powered by blists - more mailing lists