Prev: module: fix __module_ref_addr()
Next: oprofile, perf, x86: introduce new functions to reserve perfctrs by index
From: Giel van Schijndel on 29 Mar 2010 13:40
On Mon, Mar 29, 2010 at 09:07:13AM -0700, Jesse Barnes wrote:
> On Mon, 29 Mar 2010 10:18:34 +0200> Giel van Schijndel wrote:
>> On Thu, Mar 25, 2010 at 06:16:33PM +0000, Alan Cox wrote:
>>>> + if (res->flags & IORESOURCE_MUXED)
>>>> + wake_up(&muxed_resource_wait);
>>>> kfree(res);
>>>> return;
>>>
>>> And you'll want a
>>>
>>> Signed-off-by: Alan Cox <alan(a)linux.intel.com>
>>
>> Yes, thanks.
>>
>> Anyone specific I should mail to get this patch applied? I don't see any
>> entry in MAINTAINERS for kernel/resource.c or include/linux/resource.h.
>
> I can take it; sometimes Linus applies resource stuff directly though.
>
> Can you resend with the appropriate s-o-bs etc?

Sure.

(I copied Alan's signed-off-by from his reply to the patch)

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(a)mortis.eu>
Signed-off-by: Alan Cox <alan(a)linux.intel.com>
---
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
From: Giel van Schijndel on 29 Mar 2010 13:50
On Mon, Mar 29, 2010 at 07:38:00PM +0200, Giel van Schijndel wrote:
> On Mon, Mar 29, 2010 at 09:07:13AM -0700, Jesse Barnes wrote:
>> I can take it; sometimes Linus applies resource stuff directly though.
>>
>> Can you resend with the appropriate s-o-bs etc?
>
> Sure.
>
> (I copied Alan's signed-off-by from his reply to the patch)
>
> Patch after this line:
> ... snip ...

PS (to be sure authorship is maintained):
> 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/

--
Met vriendelijke groet,
With kind regards,
Giel van Schijndel
From: H. Peter Anvin on 29 Mar 2010 14:00
On 03/29/2010 10:38 AM, Giel van Schijndel wrote:
>
> 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(a)mortis.eu>
> Signed-off-by: Alan Cox <alan(a)linux.intel.com>

I have to question this approach a bit.

I would much rather see this as a two-step process, where multiple
devices request the same region with a "sharable" flag, and then have a
mutex associated with the struct resource (perhaps we need an outer
container called "struct muxed_resource" or some such.)

What I *really* object to with this patch is that it inherently assumes
that there is only one multiplexed resource in the entire system... but
of course nowhere enforces that.

-hpa
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Jesse Barnes on 29 Mar 2010 14:10
On Mon, 29 Mar 2010 19:38:00 +0200
Giel van Schijndel <me(a)mortis.eu> wrote:

> On Mon, Mar 29, 2010 at 09:07:13AM -0700, Jesse Barnes wrote:
> > On Mon, 29 Mar 2010 10:18:34 +0200> Giel van Schijndel wrote:
> >> On Thu, Mar 25, 2010 at 06:16:33PM +0000, Alan Cox wrote:
> >>>> + if (res->flags & IORESOURCE_MUXED)
> >>>> + wake_up(&muxed_resource_wait);
> >>>> kfree(res);
> >>>> return;
> >>>
> >>> And you'll want a
> >>>
> >>> Signed-off-by: Alan Cox <alan(a)linux.intel.com>
> >>
> >> Yes, thanks.
> >>
> >> Anyone specific I should mail to get this patch applied? I don't see any
> >> entry in MAINTAINERS for kernel/resource.c or include/linux/resource.h.
> >
> > I can take it; sometimes Linus applies resource stuff directly though.
> >
> > Can you resend with the appropriate s-o-bs etc?
>
> Sure.
>
> (I copied Alan's signed-off-by from his reply to the patch)

....looking again I see it should be in my linux-next branch instead,
I'll move it over.

Thanks,
--
Jesse Barnes, Intel Open Source Technology Center
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: H. Peter Anvin on 29 Mar 2010 14:30
On 03/29/2010 11:06 AM, Jesse Barnes wrote:
>>
>> I have to question this approach a bit.
>>
>> I would much rather see this as a two-step process, where multiple
>> devices request the same region with a "sharable" flag, and then have a
>> mutex associated with the struct resource (perhaps we need an outer
>> container called "struct muxed_resource" or some such.)
>>
>> What I *really* object to with this patch is that it inherently assumes
>> that there is only one multiplexed resource in the entire system... but
>> of course nowhere enforces that.
>
> Well that does keep it simple, and with just one user that's probably
> best.
>
> But why not use the common bus driver method? Muxing at the resource
> level only seems to solve part of the problem... It doesn't guarantee
> for example that driver A does something to a shared region that breaks
> driver B; it just makes sure they don't access the same region at the
> same time.
>

The common bus driver method is the obvious thing to do, but it would
presumably be greatly helped by librarization.

-hpa
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
First  |  Prev  |  Next  |  Last
Pages: 1 2 3
Prev: module: fix __module_ref_addr()
Next: oprofile, perf, x86: introduce new functions to reserve perfctrs by index