[PATCH 0/3] hwspinlock: a bug fix, trivial changes

[PATCH 0/3] hwspinlock: a bug fix, trivial changes

[Shinya Kuribayashi]

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

[PATCH 1/3] hwspinlock/core: use global ID to register hwspinlocks on multiple devices

[Shinya Kuribayashi]

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'.

Signed-off-by: Shinya Kuribayashi <shinya.kuribayashi.px@renesas.com>
---

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 <ohad@wizery.com>
| 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(-)

diff --git a/drivers/hwspinlock/hwspinlock_core.c b/drivers/hwspinlock/hwspinlock_core.c
index ed4e000..ba45f96 100644
--- a/drivers/hwspinlock/hwspinlock_core.c
+++ b/drivers/hwspinlock/hwspinlock_core.c
@@ -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

[PATCH 1/3] hwspinlock/core: use global ID to register hwspinlocks on multiple devices

[Ohad Ben-Cohen]

On Fri, Jul 6, 2012 at 7:56 AM, Shinya Kuribayashi
<shinya.kuribayashi.px@renesas.com> wrote:
> 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'.
>
> Signed-off-by: Shinya Kuribayashi <shinya.kuribayashi.px@renesas.com>

Applied, thanks!

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

[PATCH 2/3] hwspinlock/core: add notes on lock element in 'struct hwspinlock'

[Shinya Kuribayashi]

'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.

Signed-off-by: Shinya Kuribayashi <shinya.kuribayashi.px@renesas.com>
---
 drivers/hwspinlock/hwspinlock_internal.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/hwspinlock/hwspinlock_internal.h b/drivers/hwspinlock/hwspinlock_internal.h
index d26f78b..c60318c 100644
--- a/drivers/hwspinlock/hwspinlock_internal.h
+++ b/drivers/hwspinlock/hwspinlock_internal.h
@@ -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

[PATCH 2/3] hwspinlock/core: add notes on lock element in 'struct hwspinlock'

[Ohad Ben-Cohen]

Hi Shinya,

On Fri, Jul 6, 2012 at 7:56 AM, Shinya Kuribayashi
<shinya.kuribayashi.px@renesas.com> wrote:
> '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.

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.

>
> Signed-off-by: Shinya Kuribayashi <shinya.kuribayashi.px@renesas.com>
> ---
>  drivers/hwspinlock/hwspinlock_internal.h | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/hwspinlock/hwspinlock_internal.h b/drivers/hwspinlock/hwspinlock_internal.h
> index d26f78b..c60318c 100644
> --- a/drivers/hwspinlock/hwspinlock_internal.h
> +++ b/drivers/hwspinlock/hwspinlock_internal.h
> @@ -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
>

[PATCH 3/3] hwspinlock/core: allow hwspinlock_device to have bank-specific private data

[Shinya Kuribayashi]

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.

Signed-off-by: Shinya Kuribayashi <shinya.kuribayashi.px@renesas.com>
---
 drivers/hwspinlock/hwspinlock_internal.h | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/hwspinlock/hwspinlock_internal.h b/drivers/hwspinlock/hwspinlock_internal.h
index c60318c..dc7e522 100644
--- a/drivers/hwspinlock/hwspinlock_internal.h
+++ b/drivers/hwspinlock/hwspinlock_internal.h
@@ -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

[PATCH 3/3] hwspinlock/core: allow hwspinlock_device to have bank-specific private data

[Ohad Ben-Cohen]

Hi Shinya,

On Fri, Jul 6, 2012 at 7:56 AM, Shinya Kuribayashi
<shinya.kuribayashi.px@renesas.com> wrote:
> 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.

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.

>
> Signed-off-by: Shinya Kuribayashi <shinya.kuribayashi.px@renesas.com>
> ---
>  drivers/hwspinlock/hwspinlock_internal.h | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/drivers/hwspinlock/hwspinlock_internal.h b/drivers/hwspinlock/hwspinlock_internal.h
> index c60318c..dc7e522 100644
> --- a/drivers/hwspinlock/hwspinlock_internal.h
> +++ b/drivers/hwspinlock/hwspinlock_internal.h
> @@ -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
>

[PATCH 0/3] hwspinlock: a bug fix, trivial changes

[Ohad Ben-Cohen]

Hi Shinya,

On Fri, Jul 6, 2012 at 7:55 AM, Shinya Kuribayashi
<shinya.kuribayashi.px@renesas.com> wrote:
> Recently I wrote a hwspinlock driver for our devices

Great! looking forward to see it :)

> and during
> the review and debug process, we found a bug in the core code.
> Could you take a look please?

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

> 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!

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

Thanks,
Ohad.

[PATCH 0/3] hwspinlock: a bug fix, trivial changes

[Shinya Kuribayashi]

Hi,

On 7/7/2012 7:59 PM, Ohad Ben-Cohen wrote:
>> Recently I wrote a hwspinlock driver for our devices
> 
> Great! looking forward to see it :)

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.

>> and during
>> the review and debug process, we found a bug in the core code.
>> Could you take a look please?
> 
> I looked and it's great. Will send to Linus soon.

Thanks for fixing up the error path I msised.

>> 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!
> 
> I commented on the latter two (generally both look ok sans a small
> outstanding comment).

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