Re: [PATCH 1/3] drivers: misc: add omap_hwspinlock driver
* Ohad Ben-Cohen o...@wizery.com [101018 00:41]: From: Simon Que s...@ti.com 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). ... +EXPORT_SYMBOL_GPL(omap_hwspin_trylock); +EXPORT_SYMBOL_GPL(omap_hwspin_lock_timeout); +EXPORT_SYMBOL_GPL(omap_hwspin_unlock); +EXPORT_SYMBOL_GPL(omap_hwspinlock_request); +EXPORT_SYMBOL_GPL(omap_hwspinlock_request_specific); +EXPORT_SYMBOL_GPL(omap_hwspinlock_free); +EXPORT_SYMBOL_GPL(omap_hwspinlock_get_id); Please let's not add yet another omap specific layer that will make it incrementally harder to have generic drivers. Instead, we can do the omap specific locking in the platform code and then the drivers can use the functions passed in the platform_data if they're implemented. Regards, Tony -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] drivers: misc: add omap_hwspinlock driver
On Thursday 21 October 2010, Ohad Ben-Cohen wrote:
This sounds like adding a set of API that resembles spin_{unlock,lock}_irq.
My gut feeling here is that while this may be useful and simple at
certain places, it is somewhat error prone; a driver which would
erroneously use this at the wrong place will end up enabling
interrupts where it really shouldn't.
Don't you feel that proving a simple spin_lock_irqsave-like API is
actually safer and less error prone ?
I guess that is one of the reasons why spin_lock_irqsave is much more
popular than spin_lock_irq - people just know it will never screw up.
People can screw that up in different ways, e.g.
spin_lock_irqsave(lock_a, flags);
spin_lock_irqsave(lock_b, flags); /* overwrites flags */
spin_lock_irqrestore(lock_b, flags);
spin_lock_irqrestore(lock_a, flags); /* still disabled! */
I use the presence of spin_lock_irqsave in driver code as an indication
of whether the author had any clue about locking. Most experienced
coders use the right version intuitively, while beginners often just
use _irqsave because they didn't understand the API and they were told
that using this is safe.
Arnd
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] drivers: misc: add omap_hwspinlock driver
On Thu, Oct 21, 2010 at 11:04 AM, Arnd Bergmann a...@arndb.de wrote:
On Thursday 21 October 2010, Ohad Ben-Cohen wrote:
This sounds like adding a set of API that resembles spin_{unlock,lock}_irq.
My gut feeling here is that while this may be useful and simple at
certain places, it is somewhat error prone; a driver which would
erroneously use this at the wrong place will end up enabling
interrupts where it really shouldn't.
Don't you feel that proving a simple spin_lock_irqsave-like API is
actually safer and less error prone ?
I guess that is one of the reasons why spin_lock_irqsave is much more
popular than spin_lock_irq - people just know it will never screw up.
People can screw that up in different ways, e.g.
Sure...
I use the presence of spin_lock_irqsave in driver code as an indication
of whether the author had any clue about locking. Most experienced
coders use the right version intuitively, while beginners often just
use _irqsave because they didn't understand the API and they were told
that using this is safe.
Agree.
I'll add the _irq pair of APIs, this will make the users' code
cleaner. This is valuable especially since all of our current users
have their interrupts enabled anyway.
The change is also pretty trivial:
* move the internal locking implementation to raw_ methods
* the raw_ methods would save the current interrupt state only if
given a placeholder
* wrap those raw_ methods with the desired API (but only support the
_irq and _irqsave flavors)
Thanks,
Ohad.
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] drivers: misc: add omap_hwspinlock driver
On Thursday 21 October 2010, Ohad Ben-Cohen wrote: The change is also pretty trivial: * move the internal locking implementation to raw_ methods * the raw_ methods would save the current interrupt state only if given a placeholder * wrap those raw_ methods with the desired API (but only support the _irq and _irqsave flavors) Sounds good! Arnd -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] drivers: misc: add omap_hwspinlock driver
On Tue, Oct 19, 2010 at 7:16 PM, Kevin Hilman khil...@deeprootsystems.com wrote: Ohad Ben-Cohen o...@wizery.com writes: From: Simon Que s...@ti.com 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). ... +config OMAP_HWSPINLOCK + bool OMAP Hardware Spinlock module Should be tristate Agree, so it can also be built as a module by default. e.g., when building multi-OMAP kernels, no reason or this to be built-in. It can then be loaded as a module on OMAP4 only. But considering the current built-in use cases (i2c) we would then need to have the relevant MACH_OMAP4_* select OMAP_HWSPINLOCK (only on the OMAP4 machines on which the I2C bus is shared). Kevin -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] drivers: misc: add omap_hwspinlock driver
Ohad Ben-Cohen o...@wizery.com writes: On Tue, Oct 19, 2010 at 7:16 PM, Kevin Hilman khil...@deeprootsystems.com wrote: Ohad Ben-Cohen o...@wizery.com writes: From: Simon Que s...@ti.com 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). ... +config OMAP_HWSPINLOCK + bool OMAP Hardware Spinlock module Should be tristate Agree, so it can also be built as a module by default. e.g., when building multi-OMAP kernels, no reason or this to be built-in. It can then be loaded as a module on OMAP4 only. But considering the current built-in use cases (i2c) we would then need to have the relevant MACH_OMAP4_* select OMAP_HWSPINLOCK (only on the OMAP4 machines on which the I2C bus is shared). Yes, that is ok. At least then it can be tested by default as a module. Kevin -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] drivers: misc: add omap_hwspinlock driver
On Tue, Oct 19, 2010 at 11:08 PM, Arnd Bergmann a...@arndb.de wrote:
Right. There are two more things to consider though:
If you know that interrupts are disabled, you may still want to avoid
having to save the interrupt flag to the stack, to save some cycles
saving and restoring it. I don't know how expensive that is on ARM,
some other architectures take an microseconds to restore the interrupt
enabled flag from a register.
To do this we need to introduce a new set of API which will not
disable interrupts, and which should only be used when the caller
knows that interrupts are already disabled.
This will save some cycles, but my concern is that this API will be
abused by drivers, and will eventually be used to take the hwspinlocks
for lengthy period of times (which might even involve sleeping).
Given that the access to the L4 interconnect is anyway slow I also
feel that the gain is negligible.
Nevertheless, it's a viable way to squeeze some cycles.
We don't have a use case in which we know the interrupts are already
disabled, but when we will, we will consider adding such API.
Even in the case where you know that interrupts are enabled, you may
want to avoid saving the interrupt flag to the stack, the simpler
API (one argument instead of two) gives less room for screwing up.
This sounds like adding a set of API that resembles spin_{unlock,lock}_irq.
My gut feeling here is that while this may be useful and simple at
certain places, it is somewhat error prone; a driver which would
erroneously use this at the wrong place will end up enabling
interrupts where it really shouldn't.
Don't you feel that proving a simple spin_lock_irqsave-like API is
actually safer and less error prone ?
I guess that is one of the reasons why spin_lock_irqsave is much more
popular than spin_lock_irq - people just know it will never screw up.
Thanks,
Ohad.
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] drivers: misc: add omap_hwspinlock driver
On Mon, Oct 18, 2010 at 09:44:33AM +0200, Ohad Ben-Cohen wrote:
+#else /* !CONFIG_OMAP_HWSPINLOCK */
+
+static inline struct omap_hwspinlock *omap_hwspinlock_request(void)
+{
+ return ERR_PTR(-ENOSYS);
+}
One note, do you really want to fail if this option isn't built into the
kernel, yet you have a driver that is asking for it? Shouldn't you
instead just silently succeed, and let the code path get compiled away?
We did that for debugfs, after learning the pain that procfs had with
its api for is not built. Doing it the way you are requires the user
to always test for -ENOSYS, when in reality, if that is returned,
there's nothing the driver can do about it, so it should just not worry
about it.
Just something to think about.
thanks,
greg k-h
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] drivers: misc: add omap_hwspinlock driver
Ohad Ben-Cohen o...@wizery.com writes:
From: Simon Que s...@ti.com
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).
[...]
+/**
+ * omap_hwspin_trylock() - attempt to lock a specific hwspinlock
+ * @hwlock: a hwspinlock which we want to trylock
+ * @flags: a pointer to where the caller's interrupt state will be saved at
+ *
+ * This function attempt to lock the underlying hwspinlock. Unlike
+ * hwspinlock_lock, this function will immediately fail if the hwspinlock
+ * is already taken.
+ *
+ * Upon a successful return from this function, preemption and interrupts
+ * are disabled, so the caller must not sleep, and is advised to release
+ * the hwspinlock as soon as possible. This is required in order to minimize
+ * remote cores polling on the hardware interconnect.
+ *
+ * This function can be called from any context.
+ *
+ * Returns 0 if we successfully locked the hwspinlock, -EBUSY if
+ * the hwspinlock was already taken, and -EINVAL if @hwlock is invalid.
+ */
+int omap_hwspin_trylock(struct omap_hwspinlock *hwlock, unsigned long *flags)
+{
+ u32 ret;
+
+ if (IS_ERR_OR_NULL(hwlock)) {
+ pr_err(invalid hwlock\n);
+ return -EINVAL;
+ }
+
+ /*
+ * This spin_trylock_irqsave serves two purposes:
+
+ * 1. Disable local interrupts and preemption, in order to
+ *minimize the period of time in which the hwspinlock
+ *is taken (so caller will not preempted). This is
+ *important in order to minimize the possible polling on
+ *the hardware interconnect by a remote user of this lock.
+ *
+ * 2. Make this hwspinlock primitive SMP-safe (so we can try to
+ *take it from additional contexts on the local cpu)
+ */
3. Ensures that in_atomic/might_sleep checks catch potential problems
with hwspinlock usage (e.g. scheduler checks like 'scheduling while
atomic' etc.)
+ if (!spin_trylock_irqsave(hwlock-lock, *flags))
+ return -EBUSY;
+
+ /* attempt to acquire the lock by reading its value */
+ ret = readl(hwlock-addr);
+
+ /* lock is already taken */
+ if (ret == SPINLOCK_TAKEN) {
+ spin_unlock_irqrestore(hwlock-lock, *flags);
+ return -EBUSY;
+ }
+
+ /*
+ * 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.
+ */
+ mb();
+
+ return 0;
+}
+EXPORT_SYMBOL_GPL(omap_hwspin_trylock);
[...]
+/**
+ * omap_hwspinlock_unlock() - unlock a specific hwspinlock
minor nit: s/lock_unlock/_unlock/ to match name below
+ * @hwlock: a previously-acquired hwspinlock which we want to unlock
+ * @flags: a pointer to the caller's saved interrupts state
+ *
+ * This function will unlock a specific hwspinlock, enable preemption and
+ * restore the interrupts state. @hwlock must be taken (by us!) before
+ * calling this function: it is a bug to call unlock on a @hwlock that was
+ * not taken by us, i.e. using one of omap_hwspin_{lock trylock,
lock_timeout}.
+ *
+ * This function can be called from any context.
+ *
+ * Returns 0 when the @hwlock on success, or -EINVAL if @hwlock is invalid.
+ */
+int omap_hwspin_unlock(struct omap_hwspinlock *hwlock, unsigned long *flags)
+{
+ if (IS_ERR_OR_NULL(hwlock)) {
+ pr_err(invalid hwlock\n);
+ return -EINVAL;
+ }
+
+ /*
+ * We must make sure that memory operations, done before unlocking
+ * the hwspinlock, will not be reordered after the lock is released.
+ * The memory barrier induced by the spin_unlock below is too late:
+ * the other core is going to access memory soon after it will take
+ * the hwspinlock, and by then we want to be sure our memory operations
+ * were already observable.
+ */
+ mb();
+
+ /* release the lock by writing 0 to it (NOTTAKEN) */
+ writel(SPINLOCK_NOTTAKEN, hwlock-addr);
+
+ /* undo the spin_trylock_irqsave called in the locking function */
+ spin_unlock_irqrestore(hwlock-lock, *flags);
+
+ return 0;
+}
+EXPORT_SYMBOL_GPL(omap_hwspin_unlock);
[...]
Kevin
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] drivers: misc: add omap_hwspinlock driver
On Mon, Oct 18, 2010 at 1:44 AM, Ohad Ben-Cohen o...@wizery.com wrote: From: Simon Que s...@ti.com 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 s...@ti.com Signed-off-by: Hari Kanigeri h-kanige...@ti.com Signed-off-by: Krishnamoorthy, Balaji T balaj...@ti.com [o...@wizery.com: disable interrupts/preemption to prevent hw abuse] [o...@wizery.com: add memory barriers to prevent memory reordering issues] [o...@wizery.com: relax omap interconnect between subsequent lock attempts] [o...@wizery.com: timeout param to use jiffies instead of number of attempts] [o...@wizery.com: remove code duplication in lock, trylock, lock_timeout] [o...@wizery.com: runtime pm usage count to reflect num of requested locks] [o...@wizery.com: move to drivers/misc, general cleanups, document] Signed-off-by: Ohad Ben-Cohen o...@wizery.com Cc: Benoit Cousson b-cous...@ti.com Cc: Tony Lindgren t...@atomide.com --- 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 000..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
Re: [PATCH 1/3] drivers: misc: add omap_hwspinlock driver
Ohad Ben-Cohen o...@wizery.com writes: From: Simon Que s...@ti.com 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). [...] diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig index b743312..ce4b7a6 100644 --- a/drivers/misc/Kconfig +++ b/drivers/misc/Kconfig @@ -390,6 +390,16 @@ config BMP085 To compile this driver as a module, choose M here: the module will be called bmp085. +config OMAP_HWSPINLOCK + bool OMAP Hardware Spinlock module Should be tristate so it can also be built as a module by default. e.g., when building multi-OMAP kernels, no reason or this to be built-in. It can then be loaded as a module on OMAP4 only. Kevin -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] drivers: misc: add omap_hwspinlock driver
On Monday 18 October 2010 09:44:33 Ohad Ben-Cohen wrote: + int omap_hwspin_lock(struct omap_hwspinlock *hwlock, unsigned long *flags); ... + The flags parameter is a pointer to where the interrupts state of the + caller will be saved at. This seems to encourage sloppy coding: The only variant you allow is the one that corresponds to Linux's spin_lock_irqsave(), which is generally discouraged in all places where you know if you need to disable interrupts or not. IMHO the default should be a version that only allows locks that don't get taken at IRQ time and consequently don't require saving the interrupt flags. Arnd -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] drivers: misc: add omap_hwspinlock driver
On Tue, Oct 19, 2010 at 5:46 PM, Greg KH g...@kroah.com wrote:
On Mon, Oct 18, 2010 at 09:44:33AM +0200, Ohad Ben-Cohen wrote:
+#else /* !CONFIG_OMAP_HWSPINLOCK */
+
+static inline struct omap_hwspinlock *omap_hwspinlock_request(void)
+{
+ return ERR_PTR(-ENOSYS);
+}
One note, do you really want to fail if this option isn't built into the
kernel, yet you have a driver that is asking for it? Shouldn't you
instead just silently succeed, and let the code path get compiled away?
We did that for debugfs, after learning the pain that procfs had with
its api for is not built. Doing it the way you are requires the user
to always test for -ENOSYS, when in reality, if that is returned,
there's nothing the driver can do about it, so it should just not worry
about it.
Just something to think about.
Completely agree; if hwspinlock support is not needed, we better let
its users run uninterruptedly. I'll change it.
Thanks,
Ohad.
thanks,
greg k-h
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] drivers: misc: add omap_hwspinlock driver
On Tue, Oct 19, 2010 at 6:58 PM, Kevin Hilman khil...@deeprootsystems.com wrote: + * This spin_trylock_irqsave serves two purposes: + + * 1. Disable local interrupts and preemption, in order to + * minimize the period of time in which the hwspinlock + * is taken (so caller will not preempted). This is + * important in order to minimize the possible polling on + * the hardware interconnect by a remote user of this lock. + * + * 2. Make this hwspinlock primitive SMP-safe (so we can try to + * take it from additional contexts on the local cpu) + */ 3. Ensures that in_atomic/might_sleep checks catch potential problems with hwspinlock usage (e.g. scheduler checks like 'scheduling while atomic' etc.) Nice one. Added. +/** + * omap_hwspinlock_unlock() - unlock a specific hwspinlock minor nit: s/lock_unlock/_unlock/ to match name below Thanks. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] drivers: misc: add omap_hwspinlock driver
On Tue, Oct 19, 2010 at 7:01 PM, Grant Likely grant.lik...@secretlab.ca wrote: + struct omap_hwspinlock *omap_hwspinlock_request(void); ... ERR_PTR() is only appropriate when the caller actually cares about the failure code and has different behaviour depending on the result. Agree; the hwspinlock users can surely live without an explicit error code from the _request APIs, and some extra robustness can only do good. I'll remove it. 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. This a coming from an hardware requirement, rather than a choice the user should take. If a hwspinlock is taken over a long period of time, its other user (with which we try to achieve synchronization) might be polling the OMAP interconnect for too long (trying to take the hwspinlock) and thus preventing it to be used for other transactions. To prevent such lengthy polling on the interconnect, the hwspinlock should only be used for very short period of times, with preemption and interrupts disabled. That's why we don't give users the choice whether to disable interrupts or not - it's simply not a decision they should take. Thanks, Ohad. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] drivers: misc: add omap_hwspinlock driver
On Tue, Oct 19, 2010 at 7:21 PM, Arnd Bergmann a...@arndb.de wrote:
On Monday 18 October 2010 09:44:33 Ohad Ben-Cohen wrote:
+ int omap_hwspin_lock(struct omap_hwspinlock *hwlock, unsigned long
*flags);
...
+ The flags parameter is a pointer to where the interrupts state of the
+ caller will be saved at.
This seems to encourage sloppy coding: The only variant you allow is the one
that corresponds to Linux's spin_lock_irqsave()
Yes, this is a hardware requirement to minimize the period of time in
which the hwspinlock is taken, in order to prevent lengthy polling on
the interconnect (preventing it from serving other transactions).
, which is generally discouraged
in all places where you know if you need to disable interrupts or not.
IMHO the default should be a version that only allows locks that don't get
taken at IRQ time and consequently don't require saving the interrupt flags.
Please note that the hwspinlocks should never be used to achieve
synchronization with Linux contexts running on the same cpu - it's
always about achieving mutual exclusion with a remote processor.
So whether the lock is taken at IRQ time or not does not affect the
requirement to disable interrupts while it is taken (very differently
from local spin_lock{_irqsave} synchronizations).
Thanks,
Ohad.
Arnd
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] drivers: misc: add omap_hwspinlock driver
On Tuesday 19 October 2010 22:43:34 Ohad Ben-Cohen wrote: 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. This a coming from an hardware requirement, rather than a choice the user should take. If a hwspinlock is taken over a long period of time, its other user (with which we try to achieve synchronization) might be polling the OMAP interconnect for too long (trying to take the hwspinlock) and thus preventing it to be used for other transactions. This sounds exactly like any other spinlock. To prevent such lengthy polling on the interconnect, the hwspinlock should only be used for very short period of times, with preemption and interrupts disabled. Interrupts disabled in general might go a bit too far -- they are also short and infrequent events unless you have seriously broken drivers. When running with CONFIG_PREEMPT, I would guess you actually want to turn the omap_hwspinlock into a sleeping operation, though that would require much extra work to implement. Disabling preemption while the hwspinlock is held is of course a correct implementation technically, but it might not be what someone enabling CONFIG_PREEMPT expects. That's why we don't give users the choice whether to disable interrupts or not - it's simply not a decision they should take. What about those cases where you already know that interrupts are disabled, e.g. while holding a regular spin_lock_irq or inside of an interrupt handler? Arnd -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] drivers: misc: add omap_hwspinlock driver
On Tuesday 19 October 2010 22:51:22 Ohad Ben-Cohen wrote:
, which is generally discouraged
in all places where you know if you need to disable interrupts or not.
IMHO the default should be a version that only allows locks that don't get
taken at IRQ time and consequently don't require saving the interrupt flags.
Please note that the hwspinlocks should never be used to achieve
synchronization with Linux contexts running on the same cpu - it's
always about achieving mutual exclusion with a remote processor.
Ok, I see.
So whether the lock is taken at IRQ time or not does not affect the
requirement to disable interrupts while it is taken (very differently
from local spin_lock{_irqsave} synchronizations).
Right. There are two more things to consider though:
If you know that interrupts are disabled, you may still want to avoid
having to save the interrupt flag to the stack, to save some cycles
saving and restoring it. I don't know how expensive that is on ARM,
some other architectures take an microseconds to restore the interrupt
enabled flag from a register.
Even in the case where you know that interrupts are enabled, you may
want to avoid saving the interrupt flag to the stack, the simpler
API (one argument instead of two) gives less room for screwing up.
Arnd
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] drivers: misc: add omap_hwspinlock driver
On Tue, Oct 19, 2010 at 10:58 PM, Arnd Bergmann a...@arndb.de wrote: If a hwspinlock is taken over a long period of time, its other user (with which we try to achieve synchronization) might be polling the OMAP interconnect for too long (trying to take the hwspinlock) and thus preventing it to be used for other transactions. This sounds exactly like any other spinlock. The difference is hardware-specific: the hwspinlock device is located on the OMAP's L4 interconnect where access is slow, wasteful of power, and spinning on it may prevent other peripherals from interconnecting. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/3] drivers: misc: add omap_hwspinlock driver
From: Simon Que s...@ti.com 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). Signed-off-by: Simon Que s...@ti.com Signed-off-by: Hari Kanigeri h-kanige...@ti.com Signed-off-by: Krishnamoorthy, Balaji T balaj...@ti.com [o...@wizery.com: disable interrupts/preemption to prevent hw abuse] [o...@wizery.com: add memory barriers to prevent memory reordering issues] [o...@wizery.com: relax omap interconnect between subsequent lock attempts] [o...@wizery.com: timeout param to use jiffies instead of number of attempts] [o...@wizery.com: remove code duplication in lock, trylock, lock_timeout] [o...@wizery.com: runtime pm usage count to reflect num of requested locks] [o...@wizery.com: move to drivers/misc, general cleanups, document] Signed-off-by: Ohad Ben-Cohen o...@wizery.com Cc: Benoit Cousson b-cous...@ti.com Cc: Tony Lindgren t...@atomide.com --- 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 000..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. + + 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
