[PATCH] hwspinlock/core: Add testing capabilities

[PATCH] hwspinlock/core: Add testing capabilities

[Ido Yariv]

Add testing capabilities for verifying correctness of the underlying
hwspinlock layers. This can be handy especially during development.
These tests are performed only once as part of the hwspinlock
registration.

Signed-off-by: Ido Yariv <ido@wizery.com>
---
 drivers/hwspinlock/Kconfig           |    9 +++++
 drivers/hwspinlock/hwspinlock_core.c |   54 ++++++++++++++++++++++++++++++++++
 2 files changed, 63 insertions(+), 0 deletions(-)

diff --git a/drivers/hwspinlock/Kconfig b/drivers/hwspinlock/Kconfig
index c7c3128..ad632cd 100644
--- a/drivers/hwspinlock/Kconfig
+++ b/drivers/hwspinlock/Kconfig
@@ -8,6 +8,15 @@ config HWSPINLOCK
 
 menu "Hardware Spinlock drivers"
 
+config HWSPINLOCK_TEST
+	bool "Verify underlying hwspinlock implementation"
+	depends on HWSPINLOCK
+	help
+	  Say Y here to perform tests on the underlying hwspinlock
+	  implementation. The tests are only performed once per implementation.
+
+	  Say N, unless you absolutely know what you are doing.
+
 config HWSPINLOCK_OMAP
 	tristate "OMAP Hardware Spinlock device"
 	depends on ARCH_OMAP4
diff --git a/drivers/hwspinlock/hwspinlock_core.c b/drivers/hwspinlock/hwspinlock_core.c
index 085e28e..1874e85 100644
--- a/drivers/hwspinlock/hwspinlock_core.c
+++ b/drivers/hwspinlock/hwspinlock_core.c
@@ -307,6 +307,53 @@ out:
 	return hwlock;
 }
 
+#ifdef CONFIG_HWSPINLOCK_TEST
+#define NUM_OF_TEST_ITERATIONS 100
+static int hwspin_lock_tests(const struct hwspinlock_ops *ops,
+			     struct hwspinlock *hwlock)
+{
+	int i;
+	int ret;
+
+	for (i = 0; i < NUM_OF_TEST_ITERATIONS; i++) {
+		ret = ops->trylock(hwlock);
+		if (!ret) {
+			pr_err("%s: Initial lock failed\n", __func__);
+			return -EFAULT;
+		}
+
+		/* Verify lock actually works - re-acquiring it should fail */
+		ret = ops->trylock(hwlock);
+		if (ret) {
+			/* Keep locks balanced even in failure cases */
+			ops->unlock(hwlock);
+			ops->unlock(hwlock);
+			pr_err("%s: Recursive lock succeeded unexpectedly\n",
+			       __func__);
+			return -EFAULT;
+		}
+
+		/* Verify unlock by re-acquiring the lock after releasing it */
+		ops->unlock(hwlock);
+		ret = ops->trylock(hwlock);
+		if (!ret) {
+			pr_err("%s: Unlock failed\n", __func__);
+			return -EINVAL;
+		}
+
+		ops->unlock(hwlock);
+	}
+
+	return 0;
+}
+#else /* CONFIG_HWSPINLOCK_TEST*/
+static int hwspin_lock_tests(const struct hwspinlock_ops *ops,
+			     struct hwspinlock *hwlock)
+{
+	return 0;
+}
+#endif
+
 /**
  * hwspin_lock_register() - register a new hw spinlock device
  * @bank: the hwspinlock device, which usually provides numerous hw locks
@@ -345,6 +392,13 @@ int hwspin_lock_register(struct hwspinlock_device *bank, struct device *dev,
 		spin_lock_init(&hwlock->lock);
 		hwlock->bank = bank;
 
+		ret = hwspin_lock_tests(ops, hwlock);
+		if (ret) {
+			pr_err("hwspinlock tests failed on lock %d\n",
+			       base_id + i);
+			goto reg_failed;
+		}
+
 		ret = hwspin_lock_register_single(hwlock, base_id + i);
 		if (ret)
 			goto reg_failed;
-- 
1.7.7.6

Re: [PATCH] hwspinlock/core: Add testing capabilities

[Ido Yariv]

Hi Steve,

On Sat, Dec 29, 2012 at 05:19:08PM +0800, steve.zhan wrote:
> Hi,
> 
>     It is good idea add this feature.
> 
> 1: Can we let the "ret = hwspin_lock_tests(ops, hwlock);" add after
> hwspin_lock_register_single have return
> succeed, that can avoid test duplicated Or error lockid. Of course, If
> this interface is intend to test soc hardware capability only, we can
> put it in the arch module not this core framework. For driver hardware
> sanity check, i would add it after software have register it.

I'd rather not test the spinlocks after they are registering as they
might already be in use by then.

While this feature only verifies the underlying platform implementation,
I think it's best to avoid code duplication and keep it in one place
that will always get called.

> 2:Is it possible that interface add configs that choose which locks
> will be test? Because the hwspinlock module is init late in
> postcore_initcall phase, Maybe MACH/ARCH code(for example: code in
> early_initcall) need use private other interfaces to lock some
> hwspinlocks and then register hw locks to hwspinlock framework, Maybe
> some hw locks is in lock status but which test failed.

It was assumed that up to the point where the hw spinlocks are
registered they will not be used, regardless of when this module is
initialized.
If this assumption does not hold for your platform, the simplest
solution would be to set this config option to 'N', as there is no safe
way of verifying spinlocks that are actively being used.

Thanks for reviewing this,
Ido.

Re: [PATCH] hwspinlock/core: Add testing capabilities

[steve.zhan]

[-- Attachment #1.1: Type: text/plain, Size: 1935 bytes --]

Hi,
   Acked-by: Steve zhan zhanzhenbo@gmail.com

   "I'd rather not test the spinlocks after they are registering as they
might already be in use by then."

    Why? I think user must use it after hwspin_lock_register have retured
sucess.


Steve


2012/12/30 Ido Yariv <ido@wizery.com>

> Hi Steve,
>
> On Sat, Dec 29, 2012 at 05:19:08PM +0800, steve.zhan wrote:
> > Hi,
> >
> >     It is good idea add this feature.
> >
> > 1: Can we let the "ret = hwspin_lock_tests(ops, hwlock);" add after
> > hwspin_lock_register_single have return
> > succeed, that can avoid test duplicated Or error lockid. Of course, If
> > this interface is intend to test soc hardware capability only, we can
> > put it in the arch module not this core framework. For driver hardware
> > sanity check, i would add it after software have register it.
>
> I'd rather not test the spinlocks after they are registering as they
> might already be in use by then.
>
> While this feature only verifies the underlying platform implementation,
> I think it's best to avoid code duplication and keep it in one place
> that will always get called.
>
> > 2:Is it possible that interface add configs that choose which locks
> > will be test? Because the hwspinlock module is init late in
> > postcore_initcall phase, Maybe MACH/ARCH code(for example: code in
> > early_initcall) need use private other interfaces to lock some
> > hwspinlocks and then register hw locks to hwspinlock framework, Maybe
> > some hw locks is in lock status but which test failed.
>
> It was assumed that up to the point where the hw spinlocks are
> registered they will not be used, regardless of when this module is
> initialized.
> If this assumption does not hold for your platform, the simplest
> solution would be to set this config option to 'N', as there is no safe
> way of verifying spinlocks that are actively being used.
>
> Thanks for reviewing this,
> Ido.
>



-- 
Steve Zhan

[-- Attachment #1.2: Type: text/html, Size: 2616 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] hwspinlock/core: Add testing capabilities

[Ido Yariv]

Hi Steve,

On Sun, Dec 30, 2012 at 10:13:08PM +0800, steve.zhan wrote:
> Hi,
>    Acked-by: Steve zhan zhanzhenbo@gmail.com
>  
>    "I'd rather not test the spinlocks after they are registering as they
> might already be in use by then."
> 
>     Why? I think user must use it after hwspin_lock_register have retured
> sucess.

Once the spinlocks are registered anyone can request and acquire these.
The tests could then interfere with any users of the hwspinlock layer,
or simply fail (because the spinlocks might already be acquired).

Thanks,
Ido.