Re: [PATCH 1/3] drivers: misc: add omap_hwspinlock driver

[Grant Likely]

On Mon, Oct 18, 2010 at 1:44 AM, Ohad Ben-Cohen <ohad@xxxxxxxxxx> wrote:
> From: Simon Que <sque@xxxxxx>
>
> Add driver for OMAP's Hardware Spinlock module.
>
> The OMAP Hardware Spinlock module, initially introduced in OMAP4,
> provides hardware assistance for synchronization between the
> multiple processors in the device (Cortex-A9, Cortex-M3 and
> C64x+ DSP).

Hi Ohad, A couple of comments below.

>
> Signed-off-by: Simon Que <sque@xxxxxx>
> Signed-off-by: Hari Kanigeri <h-kanigeri2@xxxxxx>
> Signed-off-by: Krishnamoorthy, Balaji T <balajitk@xxxxxx>
> [ohad@xxxxxxxxxx: disable interrupts/preemption to prevent hw abuse]
> [ohad@xxxxxxxxxx: add memory barriers to prevent memory reordering issues]
> [ohad@xxxxxxxxxx: relax omap interconnect between subsequent lock attempts]
> [ohad@xxxxxxxxxx: timeout param to use jiffies instead of number of attempts]
> [ohad@xxxxxxxxxx: remove code duplication in lock, trylock, lock_timeout]
> [ohad@xxxxxxxxxx: runtime pm usage count to reflect num of requested locks]
> [ohad@xxxxxxxxxx: move to drivers/misc, general cleanups, document]
> Signed-off-by: Ohad Ben-Cohen <ohad@xxxxxxxxxx>
> Cc: Benoit Cousson <b-cousson@xxxxxx>
> Cc: Tony Lindgren <tony@xxxxxxxxxxx>
> ---
>  Documentation/misc-devices/omap_hwspinlock.txt |  199 +++++++++
>  drivers/misc/Kconfig                           |   10 +
>  drivers/misc/Makefile                          |    1 +
>  drivers/misc/omap_hwspinlock.c                 |  555 ++++++++++++++++++++++++
>  include/linux/omap_hwspinlock.h                |  108 +++++
>  5 files changed, 873 insertions(+), 0 deletions(-)
>  create mode 100644 Documentation/misc-devices/omap_hwspinlock.txt
>  create mode 100644 drivers/misc/omap_hwspinlock.c
>  create mode 100644 include/linux/omap_hwspinlock.h
>
> diff --git a/Documentation/misc-devices/omap_hwspinlock.txt b/Documentation/misc-devices/omap_hwspinlock.txt
> new file mode 100644
> index 0000000..b093347
> --- /dev/null
> +++ b/Documentation/misc-devices/omap_hwspinlock.txt
> @@ -0,0 +1,199 @@
> +OMAP Hardware Spinlocks
> +
> +1. Introduction
> +
> +Hardware spinlock modules provide hardware assistance for synchronization
> +and mutual exclusion between heterogeneous processors and those not operating
> +under a single, shared operating system.
> +
> +For example, OMAP4 has dual Cortex-A9, dual Cortex-M3 and a C64x+ DSP,
> +each of which is running a different Operating System (the master, A9,
> +is usually running Linux and the slave processors, the M3 and the DSP,
> +are running some flavor of RTOS).
> +
> +A hwspinlock driver allows kernel code to access data structures (or hardware
> +resources) that are shared with any of the existing remote processors, with
> +which there is no alternative mechanism to accomplish synchronization and
> +mutual exclusion operations.
> +
> +This is necessary, for example, for Inter-processor communications:
> +on OMAP4, cpu-intensive multimedia tasks are offloaded by the host to the
> +remote M3 and/or C64x+ slave processors (by an IPC subsystem called Syslink).
> +
> +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 simple data structures that are shared between
> +the remote processors, and access to them is synchronized using the hwspinlock
> +module (remote processor directly places new messages in this shared data
> +structure).
> +
> +2. User API
> +
> +  struct omap_hwspinlock *omap_hwspinlock_request(void);
> +   - dynamically assign an hwspinlock and return its address, or
> +     ERR_PTR(-EBUSY) if an unused hwspinlock isn't available. Users of this
> +     API will usually want to communicate the lock's id to the remote core
> +     before it can be used to achieve synchronization (to get the id of the
> +     lock, use omap_hwspinlock_get_id()).
> +     Can be called from an atomic context (this function will not sleep) but
> +     not from within interrupt context.

I strongly recommend reconsidering the ERR_PTR() pattern in new driver
code.  It is impossible to tell from looking at the prototype of a
function if it returns an ERR_PTR() value, or a NULL on failure.  I
pretty much guarantee that new users of this code will miss the
subtlety and introduce new bugs by assuming that the return value can
be tested with "if (!hwlock)".

ERR_PTR() is only appropriate when the caller actually cares about the
failure code and has different behaviour depending on the result.  For
example, filesystem code that needs to return to userspace a specific
error code.  Very seldom does driver code like users of this API
actually care.  Using it is just asking for bugs with no benefit.

> +  struct omap_hwspinlock *omap_hwspinlock_request_specific(unsigned int id);
> +   - assign a specific hwspinlock id and return its address, or
> +     ERR_PTR(-EBUSY) if that hwspinlock is already in use. Usually board code
> +     will be calling this function in order to reserve specific hwspinlock
> +     ids for predefined purposes.
> +     Can be called from an atomic context (this function will not sleep) but
> +     not from within interrupt context.
> +
> +  int omap_hwspinlock_free(struct omap_hwspinlock *hwlock);
> +   - free a previously-assigned hwspinlock; returns 0 on success, or an
> +     appropriate error code on failure (e.g. -EINVAL if the hwspinlock
> +     was not assigned).
> +     Can be called from an atomic context (this function will not sleep) but
> +     not from within interrupt context.
> +
> +  int omap_hwspin_lock(struct omap_hwspinlock *hwlock, unsigned long *flags);
> +   - 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 if hwlock is invalid, but otherwise it will
> +     always succeed (or deadlock; see above) and will never sleep. It is safe
> +     to call it from any context.
> +     Upon a successful return from this function, interrupts and preemption
> +     are 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.
> +     The flags parameter is a pointer to where the interrupts state of the
> +     caller will be saved at.
> +
> +  int omap_hwspin_lock_timeout(struct omap_hwspinlock *hwlock,
> +                               unsigned long timeout, unsigned long *flags);
> +   - 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, interrupts and preemption
> +     are 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

Disabling irqs *might* be a concern as a source of RT latency.  It
might be better to make the caller responsible for managing local spin
locks and irq disable/enable.

OTOH, I also see that a spin lock is still needed internally to
protect the hwspinlock data structure.

g.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/