Date:	Wed, 24 Nov 2010 21:59:15 +0200
From:	Ohad Ben-Cohen <ohad@...ery.com>
To:	"Kamoolkar, Mugdha" <mugdha@...com>
Cc:	"linux-omap@...r.kernel.org" <linux-omap@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"linux-arm-kernel@...ts.infradead.org" 
	<linux-arm-kernel@...ts.infradead.org>,
	"akpm@...ux-foundation.org" <akpm@...ux-foundation.org>,
	Greg KH <greg@...ah.com>, Tony Lindgren <tony@...mide.com>,
	"Cousson, Benoit" <b-cousson@...com>,
	Grant Likely <grant.likely@...retlab.ca>,
	"Kanigeri, Hari" <h-kanigeri2@...com>,
	"Anna, Suman" <s-anna@...com>,
	Kevin Hilman <khilman@...prootsystems.com>,
	Arnd Bergmann <arnd@...db.de>
Subject: Re: [PATCH v2 1/4] drivers: hwspinlock: add generic framework

Hi Mugdha,

On Wed, Nov 24, 2010 at 9:44 AM, Kamoolkar, Mugdha <mugdha@...com> wrote:
> How do multiple clients get a handle that they can use? Are they expected to
> share the handle they get from the call above?

Currently, yes.

> What if they are independent
> clients with no means of communication between them? There may be a need of
> an API that returns the handle for a specific ID. For example, a module over
> the hwspinlock driver that does some level of management of IDs (e.g. name
> to ID mapping) and enables users to get multi-core as well as multi-client
> protection on Linux.

I'm not sure I understand the use case. Can you please elaborate ?

> For example:
> struct hwspinlock *hwspinlock_get_handle(unsigned int id);

I'm afraid such an API will be easily abused, e.g., drivers that will
try to use a predefined hwspinlock id without taking care of reserving
it early enough will probably use it to overcome an "oops this
hwspinlock has already been assigned to someone else".

So let me suggest we should first understand the use case for the API
you propose, and then see how we solve it ?

> Why are some of the APIs hwspinlock_ and others hwspin_?

I'm following the regular spinlock naming, which nicely avoids having
the word 'lock' twice in the API. So you get APIs like hwspin_lock,
hwspin_unlock, hwspin_trylock. In our case we still have stuff like
hwspinlock_request and hwspinlock_free. I can probably make it
hwspin_lock_request, does that look nicer ?

>> +  int hwspin_lock_timeout(struct hwspinlock *hwlock, unsigned long timeout);
>> +   - 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).
> If timeout is 0, shouldn't it rather behave as a trylock? If timeout is
> MAX_SCHEDULE_TIMEOUT, then it should behave as a wait-forever.

Good point, thanks!

>> +  int hwspin_trylock(struct hwspinlock *hwlock);
>> +   - attempt to lock a previously-assigned hwspinlock, but immediately fail
>> if
>> +     it is already taken.
>> +     Upon a successful return from this function, preemption is disabled so
>> +     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.
>> +     Returns 0 on success and an appropriate error code otherwise (most
>> +     notably -EBUSY if the hwspinlock was already taken).
>> +     The function will never sleep.
> Is this function needed at all if timeout 0 behaves similar to trylock?

Yeah. Drivers should use the _trylock version when applicable because
it'd make the code more readable, and it's more intuitive (just like
the spin_trylock API).

>> +  struct hwspinlock *hwspinlock_unregister(unsigned int id);
>> +   - to be called from the underlying vendor-specific implementation, in
>> order
>> +     to unregister an existing (and unused) hwspinlock instance.
>> +     Can be called from an atomic context (will not sleep) but not from
>> +     within interrupt context.
>> +     Returns the address of hwspinlock on success, or NULL on error (e.g.
>> +     if the hwspinlock is sill in use).
> Does this need to be called for all hwspinlocks?

Currently, yes.

I actually am planning an improvement that would allow registering a
block of hwspinlocks; I don't think that the multiple calls of
register/unregister is that bad (it happens only once at boot), but I
want to allow sharing of the owner, ops and dev members of the
hwspinlock instances (to save some memory).

Anyway, it's not a big improvement, so I planned first to get things
merged and then look into stuff like that.

>> +int __hwspin_lock_timeout(struct hwspinlock *hwlock, unsigned long to,
>> +                                     int mode, unsigned long *flags)
>> +{
>> +     int ret;
>> +
>> +     for (;;) {
>> +             /* Try to take the hwspinlock */
>> +             ret = __hwspin_trylock(hwlock, mode, flags);
>> +             if (ret != -EBUSY)
>> +                     break;
>> +
>> +             /*
>> +              * The lock is already taken, let's check if the user wants
>> +              * us to try again
>> +              */
>> +             if (to && time_is_before_eq_jiffies(to))
>> +                     return -ETIMEDOUT;
>> +
>> +             /*
>> +              * Allow platform-specific relax handlers to prevent
>> +              * hogging the interconnect (no sleeping, though)
>> +              */
>> +             if (hwlock->ops->relax)
>> +                     hwlock->ops->relax(hwlock);
> There should be a way to have an escape mechanism for the case where a deadlock
> has occurred due to remote side taking the spinlock and crashing.

This is exactly what the timeout parameter is for..

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

Powered by blists - more mailing lists