hwspinlock: a bug fix, trivial changes

Hi Ohad,

Recently I wrote a hwspinlock driver for our devices, and during the review and debug process, we found a bug in the core code. Could you take a look please?

The first patch is a bug fix. The latter two patches are what I think nice-to-have. If you find useful, just take it, thanks!

Shinya Kuribayashi (3):
hwspinlock/core: use global ID to register hwspinlocks on multiple devices hwspinlock/core: add notes on lock element in 'struct hwspinlock' hwspinlock/core: allow hwspinlock_device to have bank-specific private data

 drivers/hwspinlock/hwspinlock_core.c     | 2 +-
 drivers/hwspinlock/hwspinlock_internal.h | 7 ++++++-
 2 files changed, 7 insertions(+), 2 deletions(-)

Shinya Kuribayashi
Renesas Electronics

Commit 300bab9770 (hwspinlock/core: register a bank of hwspinlocks in a single API call, 2011-09-06) introduced 'hwspin_lock_register_single()' to register numerous (a bank of) hwspinlock instances in a single API, 'hwspin_lock_register()'.

At which time, 'hwspin_lock_register()' accidentally passes 'local IDs' to 'hwspin_lock_register_single()', despite that ..._single() requires 'global IDs' to register hwspinlocks.

We have to convert into global IDs by supplying the missing 'base_id'.

---

Note that we used to have a similar bug in omap_hwspinlock.c driver, and fixed it in this commit:

| commit c3c1250e93a7ab1327a9fc49d2a22405672f4204
| Author: Ohad Ben-Cohen <[email protected]>
| Date:   Mon Sep 5 23:15:06 2011 +0300
|
|     hwspinlock/core/omap: fix id issues on multiple hwspinlock devices

Commit 300bab9770 made the same mistake when sorting our the core code.

 drivers/hwspinlock/hwspinlock_core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index ed4e000..ba45f96 100644
@@ -345,7 +345,7 @@ int hwspin_lock_register(struct hwspinlock_device *bank, struct device *dev,
 		spin_lock_init(&hwlock->lock);
 		hwlock->bank = bank;
 
-		ret = hwspin_lock_register_single(hwlock, i);
+		ret = hwspin_lock_register_single(hwlock, base_id + i);
 		if (ret)
 			goto reg_failed;
 	}

1.7.11.1

Applied, thanks!

While at it, I also fixed the error path of hwspin_lock_register, which had the same bug.

'lock' must be placed at the end of struct hwspinlock_device because we're doing 'hwlock = &bank->lock[0]' to get a pointer to the first 'struct hwspinlock' instance.

---
 drivers/hwspinlock/hwspinlock_internal.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

index d26f78b..c60318c 100644
@@ -57,7 +57,8 @@ struct hwspinlock {
  * @ops: platform-specific hwspinlock handlers
  * @base_id: id index of the first lock in this device
  * @num_locks: number of locks in this device
- * @lock: dynamically allocated array of 'struct hwspinlock'
+ * @lock: dynamically allocated array of 'struct hwspinlock' (must be placed
+ *	  at the end of the hwspinlock_device)
  */
 struct hwspinlock_device {
 	struct device *dev;

1.7.11.1

Hi Shinya,

I don't mind taking this one, but it will be nicer if the commit log would describe the patch itself and its motivation (even if it's trivial like this one).

Thanks,
Ohad.

It would be useful, for example, to save ioremap()ed 'io_base' address, when a platform-specific driver uses 'priv' in 'struct hwspinlock' for something different.

---
 drivers/hwspinlock/hwspinlock_internal.h | 4 ++++
 1 file changed, 4 insertions(+)

index c60318c..dc7e522 100644
@@ -57,6 +57,9 @@ struct hwspinlock {
  * @ops: platform-specific hwspinlock handlers
  * @base_id: id index of the first lock in this device
  * @num_locks: number of locks in this device
+ * @bank_data: private data which can be shared across 'struct hwspinlock'
+ *	       instances in this device, owned by the underlying platform-
+ *	       specific hwspinlock driver
  * @lock: dynamically allocated array of 'struct hwspinlock' (must be placed
  *	  at the end of the hwspinlock_device)
  */

@@ -65,6 +68,7 @@ struct hwspinlock_device {
 	const struct hwspinlock_ops *ops;
 	int base_id;
 	int num_locks;
+	void *bank_data;
 	struct hwspinlock lock[0];
 };
 

1.7.11.1

Hi Shinya,

I don't mind taking this one as long as we have a (justifiable) user for it.

I assume you're using this in your driver, so let's with this patch until you post your driver?

Thanks!
Ohad.

Hi Shinya,

Great! looking forward to see it :)

I looked and it's great. Will send to Linus soon.

I commented on the latter two (generally both look ok sans a small outstanding comment).

Thanks,
Ohad.

Hi,

Unfortunately I'm not planning to submit that driver at the moment, as I'm 120% sure that it won't be utilized by any module in our BSPs in the mainline kernel.

Thanks for fixing up the error path I msised.

Thanks, but I don't mind you dropping the latter two, either. If I have a chance to revisit hwspinlock drivers, I'll go through patches again with your comments in mind.

Shinya Kuribayashi
Renesas Electronics