Linux kernel development

Ohad Ben-Cohen | 23 Nov 2010 16:38

[PATCH v2 0/4] Introduce common hardware spinlock interface

OMAP4 introduces a Hardware Spinlock device, which provides hardware
assistance for synchronization and mutual exclusion between heterogeneous
processors and those not operating under a single, shared operating system
(e.g. OMAP4 has dual Cortex-A9, dual Cortex-M3 and a C64x+ DSP).

The intention of this hardware device is to allow remote processors,
that have no alternative mechanism to accomplish synchronization and mutual
exclusion operations, to share resources (such as memory and/or any other
hardware resource).

This patchset adds a generic hwspinlock framework that makes it possible
for drivers to use those hwspinlock devices and stay platform-independent.

Currently there are two users for this hwspinlock interface:

1. Inter-processor communications: on OMAP4, cpu-intensive multimedia
tasks are offloaded by the host to the remote M3 and/or C64x+ slave
processors.

To achieve fast message-based communications, a minimal kernel support
is needed to deliver messages arriving from a remote processor to the
appropriate user process.

This communication is based on a simple data structure that is shared between
the remote processors, and access to it is synchronized using the hwspinlock
module (remote processor directly places new messages in this shared data
structure).

2. On some OMAP4 boards, the I2C bus is shared between the A9 and the M3,
and the hwspinlock is used to synchronize access to it.

(Continue reading)

headers

Ohad Ben-Cohen | 23 Nov 2010 16:38

[PATCH v2 1/4] drivers: hwspinlock: add generic framework

Add a common, platform-independent, hwspinlock framework.

Hardware spinlock devices are needed, e.g., in order to access data
that is shared between remote processors, that otherwise have no
alternative mechanism to accomplish synchronization and mutual exclusion
operations.

Signed-off-by: Ohad Ben-Cohen <ohad <at> wizery.com>
Cc: Hari Kanigeri <h-kanigeri2 <at> ti.com>
Cc: Benoit Cousson <b-cousson <at> ti.com>
Cc: Kevin Hilman <khilman <at> deeprootsystems.com>
Cc: Grant Likely <grant.likely <at> secretlab.ca>
Cc: Tony Lindgren <tony <at> atomide.com>
---
 Documentation/hwspinlock.txt         |  339 ++++++++++++++++++++
 drivers/Kconfig                      |    2 +
 drivers/Makefile                     |    1 +
 drivers/hwspinlock/Kconfig           |   13 +
 drivers/hwspinlock/Makefile          |    5 +
 drivers/hwspinlock/hwspinlock.h      |   61 ++++
 drivers/hwspinlock/hwspinlock_core.c |  561 ++++++++++++++++++++++++++++++++++
 include/linux/hwspinlock.h           |  376 +++++++++++++++++++++++
 8 files changed, 1358 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/hwspinlock.txt
 create mode 100644 drivers/hwspinlock/Kconfig
 create mode 100644 drivers/hwspinlock/Makefile
 create mode 100644 drivers/hwspinlock/hwspinlock.h
 create mode 100644 drivers/hwspinlock/hwspinlock_core.c
 create mode 100644 include/linux/hwspinlock.h

(Continue reading)

headers

Kamoolkar, Mugdha | 24 Nov 2010 08:44

RE: [PATCH v2 1/4] drivers: hwspinlock: add generic framework

Ohad,

> -----Original Message-----
> From: linux-omap-owner <at> vger.kernel.org [mailto:linux-omap-
> owner <at> vger.kernel.org] On Behalf Of Ohad Ben-Cohen
> Sent: Tuesday, November 23, 2010 9:09 PM
> To: linux-omap <at> vger.kernel.org; linux-kernel <at> vger.kernel.org; linux-arm-
> kernel <at> lists.infradead.org
> Cc: akpm <at> linux-foundation.org; Greg KH; Tony Lindgren; Cousson, Benoit;
> Grant Likely; Kanigeri, Hari; Anna, Suman; Kevin Hilman; Arnd Bergmann;
> Ohad Ben-Cohen
> Subject: [PATCH v2 1/4] drivers: hwspinlock: add generic framework
>
> Add a common, platform-independent, hwspinlock framework.
>
> Hardware spinlock devices are needed, e.g., in order to access data
> that is shared between remote processors, that otherwise have no
> alternative mechanism to accomplish synchronization and mutual exclusion
> operations.
>
> Signed-off-by: Ohad Ben-Cohen <ohad <at> wizery.com>
> Cc: Hari Kanigeri <h-kanigeri2 <at> ti.com>
> Cc: Benoit Cousson <b-cousson <at> ti.com>
> Cc: Kevin Hilman <khilman <at> deeprootsystems.com>
> Cc: Grant Likely <grant.likely <at> secretlab.ca>
> Cc: Tony Lindgren <tony <at> atomide.com>
> ---
>  Documentation/hwspinlock.txt         |  339 ++++++++++++++++++++
>  drivers/Kconfig                      |    2 +
>  drivers/Makefile                     |    1 +

(Continue reading)

headers

Ohad Ben-Cohen | 24 Nov 2010 20:59

Re: [PATCH v2 1/4] drivers: hwspinlock: add generic framework

Hi Mugdha,

On Wed, Nov 24, 2010 at 9:44 AM, Kamoolkar, Mugdha <mugdha <at> ti.com> wrote:
> How do multiple clients get a handle that they can use? Are they expected to
> share the handle they get from the call above?

Currently, yes.

> What if they are independent
> clients with no means of communication between them? There may be a need of
> an API that returns the handle for a specific ID. For example, a module over
> the hwspinlock driver that does some level of management of IDs (e.g. name
> to ID mapping) and enables users to get multi-core as well as multi-client
> protection on Linux.

I'm not sure I understand the use case. Can you please elaborate ?

> For example:
> struct hwspinlock *hwspinlock_get_handle(unsigned int id);

I'm afraid such an API will be easily abused, e.g., drivers that will
try to use a predefined hwspinlock id without taking care of reserving
it early enough will probably use it to overcome an "oops this
hwspinlock has already been assigned to someone else".

So let me suggest we should first understand the use case for the API
you propose, and then see how we solve it ?

> Why are some of the APIs hwspinlock_ and others hwspin_?

(Continue reading)

headers

David Brownell | 25 Nov 2010 04:59

Re: [PATCH v2 1/4] drivers: hwspinlock: add generic framework

My rule of thumb is that nothing is "generic"
until at least three whatever-it-is instances
plug in to it.  Sometimes this is called
the "Rule of Three".

Other than OMAP, what's providing hardware
spinlocks that plug into this framework?

- Dave

headers

Ohad Ben-Cohen | 25 Nov 2010 07:40

Re: [PATCH v2 1/4] drivers: hwspinlock: add generic framework

On Thu, Nov 25, 2010 at 5:59 AM, David Brownell <david-b <at> pacbell.net> wrote:
> My rule of thumb is that nothing is "generic"
> until at least three whatever-it-is instances
> plug in to it.  Sometimes this is called
> the "Rule of Three".
>
> Other than OMAP, what's providing hardware
> spinlocks that plug into this framework?

We are not aware of any.

That's why the first iteration was just an omap-specific misc driver.

But we were asked not to pollute device drivers with omap-specific
interfaces (see the discussion on [1]). I think it's a good goal (it
will keep the IPC drivers that will come from TI platform-agnostic),
so we split the driver into a generic interface plus small
omap-specific implementation.

This way platforms [2] can easily plug into the framework anything
they need to achieve multi-core synchronization. E.g., even in case a
platform doesn't have dedicated silicon, but still need this
functionality, it can still plug in an implementation which is based
on Peterson's shared memory mutual exclusion algorithm (see
http://en.wikipedia.org/wiki/Peterson's_algorithm).

The third alternative is to have this driver completely hidden inside
the omap folders and deliver pdata func pointer to drivers that use
it. I am not fond of this, since the driver really only have a tiny
omap-specific part, and most of it should really sit in drivers/. In

(Continue reading)

headers

David Brownell | 25 Nov 2010 21:22

Re: [PATCH v2 1/4] drivers: hwspinlock: add generic framework

On Thu, 2010-11-25 at 08:40 +0200, Ohad Ben-Cohen wrote:
> On Thu, Nov 25, 2010 at 5:59 AM, David Brownell <david-b <at> pacbell.net> wrote:
> > My rule of thumb is that nothing is "generic"
> > until at least three whatever-it-is instances
> > plug in to it.  Sometimes this is called
> > the "Rule of Three".
> >
> > Other than OMAP, what's providing hardware
> > spinlocks that plug into this framework?
> 
> We are not aware of any.

So there's no strong reason to think this is
actually "ggeneric".  Function names no longer
specify OMAP, but without other hardware under
the interface, calling it "generic" reflects
more optimism than reality.  (That was the
implication of my observations...)

To find other hardware spinlocks, you might be
able to look at fault tolerant multiprocessors.
Ages ago I worked with one of those, where any
spinlock failures integrated with CPU/OS fault
detection; HW cwould yank (checkpointed) CPU boards
off the bus so they could be recovered (some
might continue later from checkpoints, etc.)...

> This way platforms [2] can easily plug into the framework anything
> they need to achieve multi-core synchronization. E.g., even in case a
> platform doesn't have dedicated silicon, but still need this

(Continue reading)

headers

Ohad Ben-Cohen | 26 Nov 2010 08:34

Re: [PATCH v2 1/4] drivers: hwspinlock: add generic framework

On Thu, Nov 25, 2010 at 10:22 PM, David Brownell <david-b <at> pacbell.net> wrote:
> So there's no strong reason to think this is
> actually "ggeneric".  Function names no longer
> specify OMAP, but without other hardware under
> the interface, calling it "generic" reflects
> more optimism than reality.  (That was the
> implication of my observations...)

Well, it's not omap-specific anymore. Drivers can stay
platform-agnostic, and other vendors can plug in anything they need in
order to use the same drivers (including software implementations as
mentioned below).

As I mentioned, the other two options are going back to an omap
specific driver (which will omapify drivers who use it), or putting
the driver in the omap arch folders (which some people doesn't seem to
like, e.g. http://www.spinics.net/lists/linux-arm-msm/msg00324.html).

Which one of those would you prefer to see ?

I guess that by now we have all three implementations so it's pretty
easy to switch :)

> To find other hardware spinlocks, you might be
> able to look at fault tolerant multiprocessors.
> Ages ago I worked with one of those, where any
> spinlock failures integrated with CPU/OS fault
> detection; HW cwould yank (checkpointed) CPU boards
> off the bus so they could be recovered (some
> might continue later from checkpoints, etc.)...

(Continue reading)

headers

David Brownell | 27 Nov 2010 02:24

Re: [PATCH v2 1/4] drivers: hwspinlock: add generic framework

On Fri, 2010-11-26 at 09:34 +0200, Ohad Ben-Cohen wrote:
> On Thu, Nov 25, 2010 at 10:22 PM, David Brownell <david-b <at> pacbell.net> wrote:
> > So there's no strong reason to think this is
> > actually "ggeneric".  Function names no longer
> > specify OMAP, but without other hardware under
> > the interface, calling it "generic" reflects
> > more optimism than reality.  (That was the
> > implication of my observations...)
> 
> Well, it's not omap-specific anymore.

You haven't (and evidently can't) show non-OMAP hardware under your
calls, though ... so in a practical sense, it's still OMAP-specific code
(since nothing else is working).  (And for that matter, what non-OMAP
code should try using these locks??)

Your intent "generic" is fine, but you've not achieved it and thus I
think you shouldn't imply that you have.   Dropping the word "generic"
should  suffice; it _is_ a framework, and maybe the next person working
with hardware spinlocks can finish generalizing (and add use cases).

> > To find other hardware spinlocks, you might be
> > able to look at fault tolerant multiprocessors.

(For much the same reasons as the various ASMP chips care
about HW spinlocks:...  SMP can use pure software spinlocks, but when
there are special hardware (or system) circumstances, they may not
be sufficiently robust/ or reliable.  (Consider just the impact of
differeent memory and caching models, ARM vs DSP in the OMAP case.

(Continue reading)

headers

Ohad Ben-Cohen | 29 Nov 2010 10:57

Re: [PATCH v2 1/4] drivers: hwspinlock: add generic framework

On Sat, Nov 27, 2010 at 3:24 AM, David Brownell <david-b <at> pacbell.net> wrote:
> Your intent "generic" is fine, but you've not achieved it and thus I
> think you shouldn't imply that you have.   Dropping the word "generic"
> should  suffice; it _is_ a framework, and maybe the next person working
> with hardware spinlocks can finish generalizing (and add use cases).

Sure, I can drop the word "generic".

> Haven't looked at RT in a long time.  Just look at the current RT
> patchset to see if it still has several spinlock variants.  ISTR the
> "raw" spinlock stuff came from there not long ago.  Much compile-time
> magic was involved in switching between variants.

I'll take a look there and see if I catch anything relevant.

Thanks,
Ohad.

>
> - Dave
>
>
>

headers

Kamoolkar, Mugdha | 25 Nov 2010 07:05

RE: [PATCH v2 1/4] drivers: hwspinlock: add generic framework

Ohad,

> -----Original Message-----
> From: Ohad Ben-Cohen [mailto:ohad <at> wizery.com]
> Sent: Thursday, November 25, 2010 1:29 AM
> To: Kamoolkar, Mugdha
> Cc: linux-omap <at> vger.kernel.org; linux-kernel <at> vger.kernel.org; linux-arm-
> kernel <at> lists.infradead.org; akpm <at> linux-foundation.org; Greg KH; Tony
> Lindgren; Cousson, Benoit; Grant Likely; Kanigeri, Hari; Anna, Suman;
> Kevin Hilman; Arnd Bergmann
> Subject: Re: [PATCH v2 1/4] drivers: hwspinlock: add generic framework
> 
> Hi Mugdha,
> 
> On Wed, Nov 24, 2010 at 9:44 AM, Kamoolkar, Mugdha <mugdha <at> ti.com> wrote:
> > How do multiple clients get a handle that they can use? Are they
> expected to
> > share the handle they get from the call above?
> 
> Currently, yes.
> 
> > What if they are independent
> > clients with no means of communication between them? There may be a need
> of
> > an API that returns the handle for a specific ID. For example, a module
> over
> > the hwspinlock driver that does some level of management of IDs (e.g.
> name
> > to ID mapping) and enables users to get multi-core as well as multi-
> client

(Continue reading)

headers

Ohad Ben-Cohen | 25 Nov 2010 15:29

Re: [PATCH v2 1/4] drivers: hwspinlock: add generic framework

On Thu, Nov 25, 2010 at 8:05 AM, Kamoolkar, Mugdha <mugdha <at> ti.com> wrote:
> Consider a software module on top of the hwspinlock, which provides a
> generic way for multi-core critical section, say GateMP. This module enables
> users to create critical section objects by name. Any other client can open
> the critical section by name and get a handle to it. I would expect this
> module to make a call to request a resource when creating the GateMP object.
> Suppose that the object is actually created by the remote core, and the call
> comes to Linux on the host processor to allocate the system resource (as the
> owner of the system resources). It will call hwspinlock_request, get a
> handle, get the ID from it, and return the ID to the remote processor. There
> is no point in the remote processor holding the handle that's not valid in
> its virtual space. The ID, in this case, is the single portable value that
> every processor understands in a different way. When this object were being
> deleted, the ID would be passed to Linux, and a corresponding Linux entity
> would then have to get the handle from the ID and call _free.

How is it different from any other hardware resource that the host
will allocate on behalf of the slaves ? Do we have such _open API for,
e.g., dmtimer ?

It looks like this is a broader problem that needs to be solved by the
kernel module that will manage the resources of the remote processors.

>
> Similarly, suppose the creation is happening from user-space, the user-space
> object should store the ID in the user object, and get the handle from the
> ID when performing any actions on the lock from kernel-side.

The user space interface is not yet defined, but -

(Continue reading)

headers

Kamoolkar, Mugdha | 24 Nov 2010 08:44

RE: [PATCH v2 1/4] drivers: hwspinlock: add generic framework

Ohad,

> -----Original Message-----
> From: linux-omap-owner <at> vger.kernel.org [mailto:linux-omap-
> owner <at> vger.kernel.org] On Behalf Of Ohad Ben-Cohen
> Sent: Tuesday, November 23, 2010 9:09 PM
> To: linux-omap <at> vger.kernel.org; linux-kernel <at> vger.kernel.org; linux-arm-
> kernel <at> lists.infradead.org
> Cc: akpm <at> linux-foundation.org; Greg KH; Tony Lindgren; Cousson, Benoit;
> Grant Likely; Kanigeri, Hari; Anna, Suman; Kevin Hilman; Arnd Bergmann;
> Ohad Ben-Cohen
> Subject: [PATCH v2 1/4] drivers: hwspinlock: add generic framework
>
> Add a common, platform-independent, hwspinlock framework.
>
> Hardware spinlock devices are needed, e.g., in order to access data
> that is shared between remote processors, that otherwise have no
> alternative mechanism to accomplish synchronization and mutual exclusion
> operations.
>
> Signed-off-by: Ohad Ben-Cohen <ohad <at> wizery.com>
> Cc: Hari Kanigeri <h-kanigeri2 <at> ti.com>
> Cc: Benoit Cousson <b-cousson <at> ti.com>
> Cc: Kevin Hilman <khilman <at> deeprootsystems.com>
> Cc: Grant Likely <grant.likely <at> secretlab.ca>
> Cc: Tony Lindgren <tony <at> atomide.com>
> ---
>  Documentation/hwspinlock.txt         |  339 ++++++++++++++++++++
>  drivers/Kconfig                      |    2 +
>  drivers/Makefile                     |    1 +

(Continue reading)

headers

Olof Johansson | 26 Nov 2010 05:59

Re: [PATCH v2 1/4] drivers: hwspinlock: add generic framework

Hi,

In addition to other comments from others, here are a few on the
implementation.

There's a fair amount of potentially spammy and redundant debug code
left in the generic code. I've commented on some of them below, but the
same comments would apply to other locations as well.

On Tue, Nov 23, 2010 at 05:38:57PM +0200, Ohad Ben-Cohen wrote:
> diff --git a/drivers/hwspinlock/hwspinlock_core.c b/drivers/hwspinlock/hwspinlock_core.c
> new file mode 100644
> index 0000000..cd230bc
> --- /dev/null
> +++ b/drivers/hwspinlock/hwspinlock_core.c
>  <at>  <at>  -0,0 +1,561  <at>  <at> 
> +/*
> + * Generic hardware spinlock framework
> + *
> + * Copyright (C) 2010 Texas Instruments Incorporated - http://www.ti.com
> + *
> + * Contact: Ohad Ben-Cohen <ohad <at> wizery.com>
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License version 2 as published
> + * by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the

(Continue reading)

headers

Grant Likely | 26 Nov 2010 08:18

Re: [PATCH v2 1/4] drivers: hwspinlock: add generic framework

On Thu, Nov 25, 2010 at 9:59 PM, Olof Johansson <olof <at> lixom.net> wrote:
> On Tue, Nov 23, 2010 at 05:38:57PM +0200, Ohad Ben-Cohen wrote:
>> +#define pr_fmt(fmt)    "%s: " fmt, __func__
>
> Not used.

pr_fmt() is a magic #define that changes the behaviour of the pr_*()
macros.  See include/linux/kernel.h

g.
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo <at> vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

headers

Olof Johansson | 26 Nov 2010 22:00

Re: [PATCH v2 1/4] drivers: hwspinlock: add generic framework

On Fri, Nov 26, 2010 at 12:18:49AM -0700, Grant Likely wrote:
> On Thu, Nov 25, 2010 at 9:59 PM, Olof Johansson <olof <at> lixom.net> wrote:
> > On Tue, Nov 23, 2010 at 05:38:57PM +0200, Ohad Ben-Cohen wrote:
> >> +#define pr_fmt(fmt)    "%s: " fmt, __func__
> >
> > Not used.
> 
> pr_fmt() is a magic #define that changes the behaviour of the pr_*()
> macros.  See include/linux/kernel.h

D'oh, thanks.

-Olof
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo <at> vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

headers

Ohad Ben-Cohen | 26 Nov 2010 09:53

Re: [PATCH v2 1/4] drivers: hwspinlock: add generic framework

On Fri, Nov 26, 2010 at 6:59 AM, Olof Johansson <olof <at> lixom.net> wrote:
>> +#define pr_fmt(fmt)    "%s: " fmt, __func__
>
> Not used.

Yes, it is, check out how the pr_* macros are implemented:

#define pr_info(fmt, ...) \
        printk(KERN_INFO pr_fmt(fmt), ##__VA_ARGS__)

I use it to ensure that the function name is printed with any pr_*
macro, without having to explicitly specify the __func__ param every
time.

>> +int __hwspin_trylock(struct hwspinlock *hwlock, int mode, unsigned long *flags)
>> +{
>> +     int ret;
>> +
>> +     if (unlikely(!hwlock)) {
>> +             pr_err("invalid hwlock\n");
>
> These kind of errors can get very spammy for buggy drivers.

Yeah, but that's the purpose - I want to catch such egregious drivers
who try to crash the kernel.

> It's likely
> more useful to either do a WARN_ON(), and/or move them under a debug
> config option.

(Continue reading)

headers

Russell King - ARM Linux | 26 Nov 2010 10:18

Re: [PATCH v2 1/4] drivers: hwspinlock: add generic framework

On Fri, Nov 26, 2010 at 10:53:10AM +0200, Ohad Ben-Cohen wrote:
> >> +int __hwspin_trylock(struct hwspinlock *hwlock, int mode, unsigned long *flags)
> >> +{
> >> +     int ret;
> >> +
> >> +     if (unlikely(!hwlock)) {
> >> +             pr_err("invalid hwlock\n");
> >
> > These kind of errors can get very spammy for buggy drivers.
> 
> Yeah, but that's the purpose - I want to catch such egregious drivers
> who try to crash the kernel.

That can be better - because you get a backtrace, and it causes people
to report the problem rather than just ignore it.  It may also prevent
the driver author releasing his code (as it won't work on their
initial testing.)

> > It's likely
> > more useful to either do a WARN_ON(), and/or move them under a debug
> > config option.
> 
> Why would you prefer to compile out reporting of such extremely buggy
> behavior ?

If it's "extremely buggy behaviour" then the drivers deserve to crash.
Such stuff should cause them not to get out the door.  A simple printk
with an error return can just be ignored.

headers

Ohad Ben-Cohen | 26 Nov 2010 11:16

Re: [PATCH v2 1/4] drivers: hwspinlock: add generic framework

On Fri, Nov 26, 2010 at 11:18 AM, Russell King - ARM Linux
<linux <at> arm.linux.org.uk> wrote:
> On Fri, Nov 26, 2010 at 10:53:10AM +0200, Ohad Ben-Cohen wrote:
>> >> +int __hwspin_trylock(struct hwspinlock *hwlock, int mode, unsigned long *flags)
>> >> +{
>> >> +     int ret;
>> >> +
>> >> +     if (unlikely(!hwlock)) {
>> >> +             pr_err("invalid hwlock\n");
>> >
>> > These kind of errors can get very spammy for buggy drivers.
>>
>> Yeah, but that's the purpose - I want to catch such egregious drivers
>> who try to crash the kernel.
>
> That can be better - because you get a backtrace, and it causes people
> to report the problem rather than just ignore it.  It may also prevent
> the driver author releasing his code (as it won't work on their
> initial testing.)
>
...
>
> If it's "extremely buggy behaviour" then the drivers deserve to crash.
> Such stuff should cause them not to get out the door.  A simple printk
> with an error return can just be ignored.

I like this approach too, but recently we had a few privilege
escalation exploits which involved NULL dereference kernel bugs
(process context mapped address 0 despite a positive mmap_min_addr).

(Continue reading)

headers

Russell King - ARM Linux | 26 Nov 2010 11:45

Re: [PATCH v2 1/4] drivers: hwspinlock: add generic framework

On Fri, Nov 26, 2010 at 12:16:39PM +0200, Ohad Ben-Cohen wrote:
> On Fri, Nov 26, 2010 at 11:18 AM, Russell King - ARM Linux
> <linux <at> arm.linux.org.uk> wrote:
> > On Fri, Nov 26, 2010 at 10:53:10AM +0200, Ohad Ben-Cohen wrote:
> >> >> +int __hwspin_trylock(struct hwspinlock *hwlock, int mode, unsigned long *flags)
> >> >> +{
> >> >> +     int ret;
> >> >> +
> >> >> +     if (unlikely(!hwlock)) {
> >> >> +             pr_err("invalid hwlock\n");
> >> >
> >> > These kind of errors can get very spammy for buggy drivers.
> >>
> >> Yeah, but that's the purpose - I want to catch such egregious drivers
> >> who try to crash the kernel.
> >
> > That can be better - because you get a backtrace, and it causes people
> > to report the problem rather than just ignore it.  It may also prevent
> > the driver author releasing his code (as it won't work on their
> > initial testing.)
> >
> ...
> >
> > If it's "extremely buggy behaviour" then the drivers deserve to crash.
> > Such stuff should cause them not to get out the door.  A simple printk
> > with an error return can just be ignored.
> 
> I like this approach too, but recently we had a few privilege
> escalation exploits which involved NULL dereference kernel bugs
> (process context mapped address 0 despite a positive mmap_min_addr).

(Continue reading)

headers

Ohad Ben-Cohen | 26 Nov 2010 23:18

Re: [PATCH v2 1/4] drivers: hwspinlock: add generic framework

On Fri, Nov 26, 2010 at 12:45 PM, Russell King - ARM Linux
<linux <at> arm.linux.org.uk> wrote:
> On Fri, Nov 26, 2010 at 12:16:39PM +0200, Ohad Ben-Cohen wrote:
>> On Fri, Nov 26, 2010 at 11:18 AM, Russell King - ARM Linux
>> <linux <at> arm.linux.org.uk> wrote:
>> > On Fri, Nov 26, 2010 at 10:53:10AM +0200, Ohad Ben-Cohen wrote:
>> >> >> +int __hwspin_trylock(struct hwspinlock *hwlock, int mode, unsigned long *flags)
>> >> >> +{
>> >> >> +     int ret;
>> >> >> +
>> >> >> +     if (unlikely(!hwlock)) {
>> >> >> +             pr_err("invalid hwlock\n");
>> >> >
>> >> > These kind of errors can get very spammy for buggy drivers.
>> >>
>> >> Yeah, but that's the purpose - I want to catch such egregious drivers
>> >> who try to crash the kernel.
>> >
>> > That can be better - because you get a backtrace, and it causes people
>> > to report the problem rather than just ignore it.  It may also prevent
>> > the driver author releasing his code (as it won't work on their
>> > initial testing.)
>> >
>> ...
>> >
>> > If it's "extremely buggy behaviour" then the drivers deserve to crash.
>> > Such stuff should cause them not to get out the door.  A simple printk
>> > with an error return can just be ignored.
>>
>> I like this approach too, but recently we had a few privilege

(Continue reading)

headers

Russell King - ARM Linux | 26 Nov 2010 23:53

Re: [PATCH v2 1/4] drivers: hwspinlock: add generic framework

On Sat, Nov 27, 2010 at 12:18:55AM +0200, Ohad Ben-Cohen wrote:
> But then there's the other (quite reasonable) claim that says we
> shouldn't crash the machine because of a non fatal bug: if a crappy
> driver messes up, the user (not the developer) will most probably
> prefer the machine to keep running with degraded functionality rather
> than boot.

There's also the quite reasonable expectation that we shouldn't corrupt
user data.  With locking interfaces, if someone abuses them and they
fail to work, then the risk is data corruption due to races.  The safe
thing in that case is to panic - terminate that thread before it does
anything unsafe, thereby preventing data corruption.

Yes, it may mean that something becomes unavailable, but that's better
than corrupting data.

Take a look at the kernel's own spinlock implementation.  Do we do lots
of checks in there for things like someone passing a NULL pointer to
the spinlock, or do we get an oops instead?

Also look at the list implementation.  Do we check for NULL pointers
there, or do we get an oops instead?

Same for mutex.  The same goes for lots of other infrastructure interfaces.

headers

Ohad Ben-Cohen | 29 Nov 2010 10:46

Re: [PATCH v2 1/4] drivers: hwspinlock: add generic framework

On Sat, Nov 27, 2010 at 12:53 AM, Russell King - ARM Linux
<linux <at> arm.linux.org.uk> wrote:
> On Sat, Nov 27, 2010 at 12:18:55AM +0200, Ohad Ben-Cohen wrote:
>> But then there's the other (quite reasonable) claim that says we
>> shouldn't crash the machine because of a non fatal bug: if a crappy
>> driver messes up, the user (not the developer) will most probably
>> prefer the machine to keep running with degraded functionality rather
>> than boot.
>
> There's also the quite reasonable expectation that we shouldn't corrupt
> user data.  With locking interfaces, if someone abuses them and they
> fail to work, then the risk is data corruption due to races.  The safe
> thing in that case is to panic - terminate that thread before it does
> anything unsafe, thereby preventing data corruption.

Makes sense.

I considered hwspinlock as a peripheral which doesn't qualify to take
down the system on failure, but in general I agree that there indeed
might be critical user data involved. Especially if this is a
framework and not just another driver.

But as a framework that can be used on non ARM architectures as well,
I do prefer to check for NULL pointers and not rely on the Oops.

If we had a macro that would be compiled out on architectures that
reliably produces an Oops on NULL dereference, but otherwise, would
BUG_ON on them, that should satisfy everyone.

The BUG_ON_MAPPABLE_NULL() macro below should achieve exactly that,

(Continue reading)

headers

Olof Johansson | 26 Nov 2010 23:51

Re: [PATCH v2 1/4] drivers: hwspinlock: add generic framework

Hi,

On Fri, Nov 26, 2010 at 10:53:10AM +0200, Ohad Ben-Cohen wrote:
> On Fri, Nov 26, 2010 at 6:59 AM, Olof Johansson <olof <at> lixom.net> wrote:
> >> +#define pr_fmt(fmt)    "%s: " fmt, __func__
> >
> > Not used.
> 
> Yes, it is, check out how the pr_* macros are implemented:

Yep, my bad.

> >> +int __hwspin_trylock(struct hwspinlock *hwlock, int mode, unsigned long *flags)
> >> +{
> >> +     int ret;
> >> +
> >> +     if (unlikely(!hwlock)) {
> >> +             pr_err("invalid hwlock\n");
> >
> > These kind of errors can get very spammy for buggy drivers.
> 
> Yeah, but that's the purpose - I want to catch such egregious drivers
> who try to crash the kernel.

You're not really catching the driver, since nothing points at what
made this be printed in syslog. That's why WARN_ON/__WARN has a benefit.

> > It's likely
> > more useful to either do a WARN_ON(), and/or move them under a debug
> > config option.

(Continue reading)

headers

Ohad Ben-Cohen | 29 Nov 2010 22:31

Re: [PATCH v2 1/4] drivers: hwspinlock: add generic framework

Hi Olof,

On Sat, Nov 27, 2010 at 12:51 AM, Olof Johansson <olof <at> lixom.net> wrote:
>> >> +int __hwspin_trylock(struct hwspinlock *hwlock, int mode, unsigned long *flags)
>> >> +{
>> >> +     int ret;
>> >> +
>> >> +     if (unlikely(!hwlock)) {
>> >> +             pr_err("invalid hwlock\n");
>> >
>> > These kind of errors can get very spammy for buggy drivers.
>>
>> Yeah, but that's the purpose - I want to catch such egregious drivers
>> who try to crash the kernel.
...
> Anyway, above plus a __WARN() would be OK with me.

Np, but let's wait a bit to see if something like the
BUG_ON_MAPPABLE_NULL macro materializes or not. That should satisfy
everyone in a generic way.

>> >> +     /*
>> >> +      * We can be sure the other core's memory operations
>> >> +      * are observable to us only _after_ we successfully take
>> >> +      * the hwspinlock, so we must make sure that subsequent memory
>> >> +      * operations will not be reordered before we actually took the
>> >> +      * hwspinlock.
>> >> +      *
>> >> +      * Note: the implicit memory barrier of the spinlock above is too
>> >> +      * early, so we need this additional explicit memory barrier.

(Continue reading)

headers

Tony Lindgren | 30 Nov 2010 20:00

Re: [PATCH v2 1/4] drivers: hwspinlock: add generic framework

* Ohad Ben-Cohen <ohad <at> wizery.com> [101123 07:27]:
> Add a common, platform-independent, hwspinlock framework.
> 
> Hardware spinlock devices are needed, e.g., in order to access data
> that is shared between remote processors, that otherwise have no
> alternative mechanism to accomplish synchronization and mutual exclusion
> operations.

<snip>

> +  int hwspin_lock(struct hwspinlock *hwlock);
> +   - lock a previously assigned hwspinlock. If the hwspinlock is already
> +     taken, the function will busy loop waiting for it to be released.
> +     Note: if a faulty remote core never releases this lock, this function
> +     will deadlock.
> +     This function will fail only if hwlock is invalid. Otherwise, it will
> +     always succeed (or deadlock; see above) and it will never sleep.
> +     Upon a successful return from this function, preemption is disabled so
> +     the caller must not sleep, and is advised to release the hwspinlock as
> +     soon as possible, in order to minimize remote cores polling on the
> +     hardware interconnect.
...

> +  int hwspin_lock_timeout(struct hwspinlock *hwlock, unsigned long timeout);
> +   - lock a previously-assigned hwspinlock with a timeout limit (specified in
> +     jiffies). If the hwspinlock is already taken, the function will busy loop
> +     waiting for it to be released, but give up when the timeout meets jiffies.
> +     If timeout is 0, the function will never give up (therefore if a faulty
> +     remote core never releases the hwspinlock, it will deadlock).
> +     Upon a successful return from this function, preemption is disabled so

(Continue reading)

headers

Ohad Ben-Cohen | 30 Nov 2010 23:20

Re: [PATCH v2 1/4] drivers: hwspinlock: add generic framework

On Tue, Nov 30, 2010 at 11:00 AM, Tony Lindgren <tony <at> atomide.com> wrote:
> Do we even need the hwspin_lock variants,

I personally don't have any specific use case in mind.

It's just a simple wrapper over the _timeout variants, provided for
API completeness, but -

> why can't we always use the hwspin_lock_timeout variants?

We can. I can just remove the _lock variants.

headers

Tony Lindgren | 30 Nov 2010 23:23

Re: [PATCH v2 1/4] drivers: hwspinlock: add generic framework

* Ohad Ben-Cohen <ohad <at> wizery.com> [101130 14:10]:
> On Tue, Nov 30, 2010 at 11:00 AM, Tony Lindgren <tony <at> atomide.com> wrote:
> > Do we even need the hwspin_lock variants,
> 
> I personally don't have any specific use case in mind.
> 
> It's just a simple wrapper over the _timeout variants, provided for
> API completeness, but -
> 
> > why can't we always use the hwspin_lock_timeout variants?
> 
> We can. I can just remove the _lock variants.

OK sounds good to me.

Tony

headers

Ohad Ben-Cohen | 23 Nov 2010 16:38

[PATCH v2 2/4] drivers: hwspinlock: add OMAP implementation

From: Simon Que <sque <at> ti.com>

Add hwspinlock support for the OMAP4 Hardware Spinlock device.

The Hardware Spinlock device on OMAP4 provides hardware assistance
for synchronization between the multiple processors in the system
(dual Cortex-A9, dual Cortex-M3 and a C64x+ DSP).

[ohad <at> wizery.com: adapt to generic hwspinlock api, tidy up]
Signed-off-by: Simon Que <sque <at> ti.com>
Signed-off-by: Hari Kanigeri <h-kanigeri2 <at> ti.com>
Signed-off-by: Krishnamoorthy, Balaji T <balajitk <at> ti.com>
Signed-off-by: Ohad Ben-Cohen <ohad <at> wizery.com>
Cc: Benoit Cousson <b-cousson <at> ti.com>
Cc: Kevin Hilman <khilman <at> deeprootsystems.com>
Cc: Grant Likely <grant.likely <at> secretlab.ca>
Cc: Tony Lindgren <tony <at> atomide.com>
---
 drivers/hwspinlock/Kconfig           |    9 ++
 drivers/hwspinlock/Makefile          |    1 +
 drivers/hwspinlock/omap_hwspinlock.c |  231 ++++++++++++++++++++++++++++++++++
 3 files changed, 241 insertions(+), 0 deletions(-)
 create mode 100644 drivers/hwspinlock/omap_hwspinlock.c

diff --git a/drivers/hwspinlock/Kconfig b/drivers/hwspinlock/Kconfig
index 9dd8db4..eb4af28 100644
--- a/drivers/hwspinlock/Kconfig
+++ b/drivers/hwspinlock/Kconfig
 <at>  <at>  -11,3 +11,12  <at>  <at>  config HWSPINLOCK
 	  coprocessors).

(Continue reading)

headers

Ionut Nicu | 24 Nov 2010 00:23

Re: [PATCH v2 2/4] drivers: hwspinlock: add OMAP implementation

Hi Ohad,

On Tue, 2010-11-23 at 17:38 +0200, Ohad Ben-Cohen wrote:
> From: Simon Que <sque <at> ti.com>
> 
> Add hwspinlock support for the OMAP4 Hardware Spinlock device.
> 

<snip>

> +
> +	io_base = ioremap(res->start, resource_size(res));
> +	if (!io_base) {
> +		ret = -ENOMEM;
> +		goto free_state;
> +	}
> +
> +	/* Determine number of locks */
> +	i = readl(io_base + SYSSTATUS_OFFSET);
> +	i >>= SPINLOCK_NUMLOCKS_BIT_OFFSET;
> +
> +	/* exactly one of the four least significant bits must be 1 */
> +	if (!i || !is_power_of_2(i) || i > 8) {
> +		ret = -EINVAL;
> +		goto iounmap_base;
> +	}
> +

At iounmap_base you unmap state->io_base, but that's set only after this
check. You should probably iounmap(io_base).

(Continue reading)

headers

Ohad Ben-Cohen | 24 Nov 2010 11:33

Re: [PATCH v2 2/4] drivers: hwspinlock: add OMAP implementation

On Wed, Nov 24, 2010 at 1:23 AM, Ionut Nicu <ionut.nicu <at> mindbit.ro> wrote:
> At iounmap_base you unmap state->io_base, but that's set only after this
> check. You should probably iounmap(io_base).

Of course. Thanks.

>
> Regards,
> Ionut.
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majordomo <at> vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

headers

Ohad Ben-Cohen | 23 Nov 2010 16:39

[PATCH v2 4/4] omap: add hwspinlock device

From: Simon Que <sque <at> ti.com>

Build and register an hwspinlock platform device.

Although only OMAP4 supports the hardware spinlock module (for now),
it is still safe to run this initcall on all omaps, because hwmod lookup
will simply fail on hwspinlock-less platforms.

Signed-off-by: Simon Que <sque <at> ti.com>
Signed-off-by: Hari Kanigeri <h-kanigeri2 <at> ti.com>
Signed-off-by: Ohad Ben-Cohen <ohad <at> wizery.com>
Cc: Benoit Cousson <b-cousson <at> ti.com>
Cc: Kevin Hilman <khilman <at> deeprootsystems.com>
Cc: Paul Walmsley <paul <at> pwsan.com>
---
 arch/arm/mach-omap2/Makefile     |    1 +
 arch/arm/mach-omap2/hwspinlock.c |   63 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 64 insertions(+), 0 deletions(-)
 create mode 100644 arch/arm/mach-omap2/hwspinlock.c

diff --git a/arch/arm/mach-omap2/Makefile b/arch/arm/mach-omap2/Makefile
index ce7b1f0..5162665 100644
--- a/arch/arm/mach-omap2/Makefile
+++ b/arch/arm/mach-omap2/Makefile
 <at>  <at>  -197,3 +197,4  <at>  <at>  obj-y					+= $(smc91x-m) $(smc91x-y)

 smsc911x-$(CONFIG_SMSC911X)		:= gpmc-smsc911x.o
 obj-y					+= $(smsc911x-m) $(smsc911x-y)
+obj-$(CONFIG_ARCH_OMAP4)		+= hwspinlock.o
diff --git a/arch/arm/mach-omap2/hwspinlock.c b/arch/arm/mach-omap2/hwspinlock.c

(Continue reading)

headers

Ohad Ben-Cohen | 23 Nov 2010 16:38

[PATCH v2 3/4] OMAP4: hwmod data: Add hwspinlock

From: Benoit Cousson <b-cousson <at> ti.com>

Add hwspinlock hwmod data for OMAP4 chip

Signed-off-by: Cousson, Benoit <b-cousson <at> ti.com>
Signed-off-by: Hari Kanigeri <h-kanigeri2 <at> ti.com>
Signed-off-by: Ohad Ben-Cohen <ohad <at> wizery.com>
Cc: Paul Walmsley <paul <at> pwsan.com>
---
 arch/arm/mach-omap2/omap_hwmod_44xx_data.c |   63 ++++++++++++++++++++++++++++
 1 files changed, 63 insertions(+), 0 deletions(-)

diff --git a/arch/arm/mach-omap2/omap_hwmod_44xx_data.c b/arch/arm/mach-omap2/omap_hwmod_44xx_data.c
index 0d5c6eb..07c3654 100644
--- a/arch/arm/mach-omap2/omap_hwmod_44xx_data.c
+++ b/arch/arm/mach-omap2/omap_hwmod_44xx_data.c
 <at>  <at>  -1043,6 +1043,66  <at>  <at>  static struct omap_hwmod omap44xx_uart4_hwmod = {
 	.omap_chip	= OMAP_CHIP_INIT(CHIP_IS_OMAP4430),
 };

+/*
+ * 'spinlock' class
+ * spinlock provides hardware assistance for synchronizing the processes
+ * running on multiple processors
+ */
+
+static struct omap_hwmod_class_sysconfig omap44xx_spinlock_sysc = {
+	.rev_offs	= 0x0000,
+	.sysc_offs	= 0x0010,
+	.syss_offs	= 0x0014,

(Continue reading)