Re: [PATCH v4 1/4] drivers: hwspinlock: add framework
* Russell King - ARM Linux li...@arm.linux.org.uk [110202 04:10]: On Tue, Feb 01, 2011 at 10:12:38AM +0200, Ohad Ben-Cohen wrote: On Tue, Feb 1, 2011 at 9:40 AM, Andrew Morton a...@linux-foundation.org wrote: On Tue, 1 Feb 2011 09:36:22 +0200 Ohad Ben-Cohen o...@wizery.com wrote: On Tue, Feb 1, 2011 at 8:38 AM, Andrew Morton a...@linux-foundation.org wrote: I'll do a quick respin of the patches with that and the hwspinlock_internal.h comment above. OK.. The patch series looks OK to me. Can I add your Acked-by on the non-omap parts when I respin the series ? spose so. I don't normally do acked-by's. I think it's my way of avoiding getting blamed when it all blows up. I don't want to be breaking old habits then :) Maybe Tony or Russell or Greg can grab them if they like the look of it all? Let's just wait a bit for Tony's or Russell's or Greg's answer. As it touches OMAP code, it makes sense to merge it through Tony's tree as that should reduce the number of possible conflicts. OK I'll take them then. 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 v4 1/4] drivers: hwspinlock: add framework
On Tue, Feb 01, 2011 at 10:12:38AM +0200, Ohad Ben-Cohen wrote: On Tue, Feb 1, 2011 at 9:40 AM, Andrew Morton a...@linux-foundation.org wrote: On Tue, 1 Feb 2011 09:36:22 +0200 Ohad Ben-Cohen o...@wizery.com wrote: On Tue, Feb 1, 2011 at 8:38 AM, Andrew Morton a...@linux-foundation.org wrote: I'll do a quick respin of the patches with that and the hwspinlock_internal.h comment above. OK.. The patch series looks OK to me. Can I add your Acked-by on the non-omap parts when I respin the series ? spose so. I don't normally do acked-by's. I think it's my way of avoiding getting blamed when it all blows up. I don't want to be breaking old habits then :) Maybe Tony or Russell or Greg can grab them if they like the look of it all? Let's just wait a bit for Tony's or Russell's or Greg's answer. As it touches OMAP code, it makes sense to merge it through Tony's tree as that should reduce the number of possible conflicts. -- 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 v4 1/4] drivers: hwspinlock: add framework
On Tue, Feb 1, 2011 at 9:40 AM, Andrew Morton a...@linux-foundation.org wrote: On Tue, 1 Feb 2011 09:36:22 +0200 Ohad Ben-Cohen o...@wizery.com wrote: On Tue, Feb 1, 2011 at 8:38 AM, Andrew Morton a...@linux-foundation.org wrote: I'll do a quick respin of the patches with that and the hwspinlock_internal.h comment above. OK.. The patch series looks OK to me. Can I add your Acked-by on the non-omap parts when I respin the series ? spose so. I don't normally do acked-by's. I think it's my way of avoiding getting blamed when it all blows up. I don't want to be breaking old habits then :) Maybe Tony or Russell or Greg can grab them if they like the look of it all? Let's just wait a bit for Tony's or Russell's or Greg's answer. 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 v4 1/4] drivers: hwspinlock: add framework
On Mon, Jan 31, 2011 at 10:38:59PM -0800, Andrew Morton wrote: On Tue, 1 Feb 2011 08:20:13 +0200 Ohad Ben-Cohen o...@wizery.com wrote: On Tue, Feb 1, 2011 at 1:38 AM, Andrew Morton a...@linux-foundation.org wrote: It's a little irritating having two hwspinlock.h's. hwspinlock_internal.h wold be a conventional approach. __But it's not a big deal. ... +/** + * __hwspin_lock_timeout() - lock an hwspinlock with timeout limit + * @hwlock: the hwspinlock to be locked + * @timeout: timeout value in jiffies hm, why in jiffies? The problem here is that lazy programmers will use __ __ __ __hwspin_lock_timeout(lock, 10, ...) and their code will work happily with HZ=100 but will explode with HZ=1000. IOW, this interface *requires* that all callers perform a seconds-to-jiffies conversion before calling hwspin_lock_timeout(). __So why not reduce their effort and their ability to make mistakes by defining the API to take seconds? I considered that, but then decided to use jiffies in order to be consistent with wait_event_timeout/schedule_timeout (although I don't return the remaining jiffies in case the lock is taken before the timeout elapses), and also to allow user-selected granularity. But I do kind of like the idea of not using jiffies. We can probably even move to msecs, since anyway this is an error condition, and people who needs a quick check should just use the trylock() version. I'll do a quick respin of the patches with that and the hwspinlock_internal.h comment above. OK.. The patch series looks OK to me. But there isn't a lot of point in me putting them into my tree. Maybe Tony or Russell or Greg can grab them if they like the look of it all? As it's an arm-specific thing, it should probably go through Russell's tree. 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 v4 1/4] drivers: hwspinlock: add framework
On Monday 31 January 2011, Ohad Ben-Cohen wrote: Add a platform-independent hwspinlock framework. Hardware spinlock devices are needed, e.g., in order to access data that is shared between remote processors, that otherwise have no alternative mechanism to accomplish synchronization and mutual exclusion operations. Signed-off-by: Ohad Ben-Cohen o...@wizery.com Cc: Hari Kanigeri h-kanige...@ti.com Cc: Benoit Cousson b-cous...@ti.com Cc: Kevin Hilman khil...@ti.com Cc: Grant Likely grant.lik...@secretlab.ca Cc: Arnd Bergmann a...@arndb.de Cc: Paul Walmsley p...@pwsan.com Acked-by: Tony Lindgren t...@atomide.com Acked-by: Arnd Bergmann a...@arndb.de -- 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 v4 1/4] drivers: hwspinlock: add framework
On Mon, 31 Jan 2011 12:33:41 +0200 Ohad Ben-Cohen o...@wizery.com wrote: Add a platform-independent hwspinlock framework. Hardware spinlock devices are needed, e.g., in order to access data that is shared between remote processors, that otherwise have no alternative mechanism to accomplish synchronization and mutual exclusion operations. Signed-off-by: Ohad Ben-Cohen o...@wizery.com Cc: Hari Kanigeri h-kanige...@ti.com Cc: Benoit Cousson b-cous...@ti.com Cc: Kevin Hilman khil...@ti.com Cc: Grant Likely grant.lik...@secretlab.ca Cc: Arnd Bergmann a...@arndb.de Cc: Paul Walmsley p...@pwsan.com Acked-by: Tony Lindgren t...@atomide.com --- Documentation/hwspinlock.txt | 299 ++ drivers/Kconfig |2 + drivers/Makefile |2 + drivers/hwspinlock/Kconfig | 13 + drivers/hwspinlock/Makefile |5 + drivers/hwspinlock/hwspinlock.h | 61 drivers/hwspinlock/hwspinlock_core.c | 557 ++ include/linux/hwspinlock.h | 298 ++ It's a little irritating having two hwspinlock.h's. hwspinlock_internal.h wold be a conventional approach. But it's not a big deal. ... +/* + * A radix tree is used to maintain the available hwspinlock instances. + * The tree associates hwspinlock pointers with their integer key id, + * and provides easy-to-use API which makes the hwspinlock core code simple + * and easy to read. + * + * Radix trees are quick on lookups, and reasonably efficient in terms of + * storage, especially with high density usages such as this framework + * requires (a continuous range of integer keys, beginning with zero, is + * used as the ID's of the hwspinlock instances). + * + * The radix tree API supports tagging items in the tree, which this + * framework uses to mark unused hwspinlock instances (see the + * HWSPINLOCK_UNUSED tag above). As a result, the process of querying the + * tree, looking for an unused hwspinlock instance, is now reduced to a + * single radix tree API call. + */ Nice comment! +static RADIX_TREE(hwspinlock_tree, GFP_KERNEL); + ... +/** + * __hwspin_lock_timeout() - lock an hwspinlock with timeout limit + * @hwlock: the hwspinlock to be locked + * @timeout: timeout value in jiffies hm, why in jiffies? The problem here is that lazy programmers will use hwspin_lock_timeout(lock, 10, ...) and their code will work happily with HZ=100 but will explode with HZ=1000. IOW, this interface *requires* that all callers perform a seconds-to-jiffies conversion before calling hwspin_lock_timeout(). So why not reduce their effort and their ability to make mistakes by defining the API to take seconds? + * @mode: mode which controls whether local interrupts are disabled or not + * @flags: a pointer to where the caller's interrupt state will be saved at (if + * requested) + * + * This function locks the given @hwlock. If the @hwlock + * is already taken, the function will busy loop waiting for it to + * be released, but give up when @timeout jiffies have elapsed. If @timeout + * is %MAX_SCHEDULE_TIMEOUT, the function will never give up (therefore if a + * faulty remote core never releases the @hwlock, it will deadlock). + * + * Upon a successful return from this function, preemption is disabled + * (and possibly local interrupts, too), 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. + * + * The user decides whether local interrupts are disabled or not, and if yes, + * whether he wants their previous state to be saved. It is up to the user + * to choose the appropriate @mode of operation, exactly the same way users + * should decide between spin_lock, spin_lock_irq and spin_lock_irqsave. + * + * Returns 0 when the @hwlock was successfully taken, and an appropriate + * error code otherwise (most notably -ETIMEDOUT if the @hwlock is still + * busy after @timeout meets jiffies). The function will never sleep. + */ ... -- 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 v4 1/4] drivers: hwspinlock: add framework
On Tue, Feb 1, 2011 at 1:38 AM, Andrew Morton a...@linux-foundation.org wrote: It's a little irritating having two hwspinlock.h's. hwspinlock_internal.h wold be a conventional approach. But it's not a big deal. ... +/** + * __hwspin_lock_timeout() - lock an hwspinlock with timeout limit + * @hwlock: the hwspinlock to be locked + * @timeout: timeout value in jiffies hm, why in jiffies? The problem here is that lazy programmers will use hwspin_lock_timeout(lock, 10, ...) and their code will work happily with HZ=100 but will explode with HZ=1000. IOW, this interface *requires* that all callers perform a seconds-to-jiffies conversion before calling hwspin_lock_timeout(). So why not reduce their effort and their ability to make mistakes by defining the API to take seconds? I considered that, but then decided to use jiffies in order to be consistent with wait_event_timeout/schedule_timeout (although I don't return the remaining jiffies in case the lock is taken before the timeout elapses), and also to allow user-selected granularity. But I do kind of like the idea of not using jiffies. We can probably even move to msecs, since anyway this is an error condition, and people who needs a quick check should just use the trylock() version. I'll do a quick respin of the patches with that and the hwspinlock_internal.h comment above. 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 v4 1/4] drivers: hwspinlock: add framework
On Tue, 1 Feb 2011 08:20:13 +0200 Ohad Ben-Cohen o...@wizery.com wrote: On Tue, Feb 1, 2011 at 1:38 AM, Andrew Morton a...@linux-foundation.org wrote: It's a little irritating having two hwspinlock.h's. hwspinlock_internal.h wold be a conventional approach. __But it's not a big deal. ... +/** + * __hwspin_lock_timeout() - lock an hwspinlock with timeout limit + * @hwlock: the hwspinlock to be locked + * @timeout: timeout value in jiffies hm, why in jiffies? The problem here is that lazy programmers will use __ __ __ __hwspin_lock_timeout(lock, 10, ...) and their code will work happily with HZ=100 but will explode with HZ=1000. IOW, this interface *requires* that all callers perform a seconds-to-jiffies conversion before calling hwspin_lock_timeout(). __So why not reduce their effort and their ability to make mistakes by defining the API to take seconds? I considered that, but then decided to use jiffies in order to be consistent with wait_event_timeout/schedule_timeout (although I don't return the remaining jiffies in case the lock is taken before the timeout elapses), and also to allow user-selected granularity. But I do kind of like the idea of not using jiffies. We can probably even move to msecs, since anyway this is an error condition, and people who needs a quick check should just use the trylock() version. I'll do a quick respin of the patches with that and the hwspinlock_internal.h comment above. OK.. The patch series looks OK to me. But there isn't a lot of point in me putting them into my tree. Maybe Tony or Russell or Greg can grab them if they like the look of it all? -- 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 v4 1/4] drivers: hwspinlock: add framework
On Tue, Feb 1, 2011 at 8:38 AM, Andrew Morton a...@linux-foundation.org wrote: I'll do a quick respin of the patches with that and the hwspinlock_internal.h comment above. OK.. The patch series looks OK to me. Can I add your Acked-by on the non-omap parts when I respin the series ? 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 v4 1/4] drivers: hwspinlock: add framework
On Tue, 1 Feb 2011 09:36:22 +0200 Ohad Ben-Cohen o...@wizery.com wrote: On Tue, Feb 1, 2011 at 8:38 AM, Andrew Morton a...@linux-foundation.org wrote: I'll do a quick respin of the patches with that and the hwspinlock_internal.h comment above. OK.. The patch series looks OK to me. Can I add your Acked-by on the non-omap parts when I respin the series ? spose so. I don't normally do acked-by's. I think it's my way of avoiding getting blamed when it all blows up. -- 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
