* [PATCH] ARM: SAMSUNG: Bug fix spin_lock recursion in clk_enable() and clk_disable() @ 2010-10-12 0:39 Jaecheol Lee 2010-10-18 23:09 ` Ben Dooks 0 siblings, 1 reply; 6+ messages in thread From: Jaecheol Lee @ 2010-10-12 0:39 UTC (permalink / raw) To: linux-arm-kernel From: Minho Ban <mhban@samsung.com> The clk_enable() and clk_disable() can be used process and ISR either. So spin_lock_irqsave should be used instead. Signed-off-by: Minho Ban <mhban@samsung.com> Signed-off-by: Jaecheol Lee <jc.lee@samsung.com> --- arch/arm/plat-samsung/clock.c | 12 ++++++++---- 1 files changed, 8 insertions(+), 4 deletions(-) diff --git a/arch/arm/plat-samsung/clock.c b/arch/arm/plat-samsung/clock.c index e8d20b0..2a991a5 100644 --- a/arch/arm/plat-samsung/clock.c +++ b/arch/arm/plat-samsung/clock.c @@ -138,31 +138,35 @@ void clk_put(struct clk *clk) int clk_enable(struct clk *clk) { + unsigned long flags; + if (IS_ERR(clk) || clk == NULL) return -EINVAL; clk_enable(clk->parent); - spin_lock(&clocks_lock); + spin_lock_irqsave(&clocks_lock, flags); if ((clk->usage++) == 0) (clk->enable)(clk, 1); - spin_unlock(&clocks_lock); + spin_unlock_irqrestore(&clocks_lock, flags); return 0; } void clk_disable(struct clk *clk) { + unsigned long flags; + if (IS_ERR(clk) || clk == NULL) return; - spin_lock(&clocks_lock); + spin_lock_irqsave(&clocks_lock, flags); if ((--clk->usage) == 0) (clk->enable)(clk, 0); - spin_unlock(&clocks_lock); + spin_unlock_irqrestore(&clocks_lock, flags); clk_disable(clk->parent); } -- 1.6.2.5 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH] ARM: SAMSUNG: Bug fix spin_lock recursion in clk_enable() and clk_disable() 2010-10-12 0:39 [PATCH] ARM: SAMSUNG: Bug fix spin_lock recursion in clk_enable() and clk_disable() Jaecheol Lee @ 2010-10-18 23:09 ` Ben Dooks 2010-10-19 1:12 ` Jassi Brar 2010-10-19 18:57 ` Russell King - ARM Linux 0 siblings, 2 replies; 6+ messages in thread From: Ben Dooks @ 2010-10-18 23:09 UTC (permalink / raw) To: linux-arm-kernel On 12/10/10 01:39, Jaecheol Lee wrote: > From: Minho Ban <mhban@samsung.com> > > The clk_enable() and clk_disable() can be used process and ISR either. > So spin_lock_irqsave should be used instead. > > Signed-off-by: Minho Ban <mhban@samsung.com> > Signed-off-by: Jaecheol Lee <jc.lee@samsung.com> > --- > arch/arm/plat-samsung/clock.c | 12 ++++++++---- > 1 files changed, 8 insertions(+), 4 deletions(-) > > diff --git a/arch/arm/plat-samsung/clock.c b/arch/arm/plat-samsung/clock.c > index e8d20b0..2a991a5 100644 > --- a/arch/arm/plat-samsung/clock.c > +++ b/arch/arm/plat-samsung/clock.c > @@ -138,31 +138,35 @@ void clk_put(struct clk *clk) > > int clk_enable(struct clk *clk) > { > + unsigned long flags; > + > if (IS_ERR(clk) || clk == NULL) > return -EINVAL; > > clk_enable(clk->parent); > > - spin_lock(&clocks_lock); > + spin_lock_irqsave(&clocks_lock, flags); > > if ((clk->usage++) == 0) > (clk->enable)(clk, 1); > > - spin_unlock(&clocks_lock); > + spin_unlock_irqrestore(&clocks_lock, flags); > return 0; > } > > void clk_disable(struct clk *clk) > { > + unsigned long flags; > + > if (IS_ERR(clk) || clk == NULL) > return; > > - spin_lock(&clocks_lock); > + spin_lock_irqsave(&clocks_lock, flags); > > if ((--clk->usage) == 0) > (clk->enable)(clk, 0); > > - spin_unlock(&clocks_lock); > + spin_unlock_irqrestore(&clocks_lock, flags); > clk_disable(clk->parent); I'm not sure, but I don't belive that the clk_ api has ever been callable from non-sleepable contexts such as interrupt handlers. I would welcome RMK's response (or any other response) about this issue? Personally, given that some clk_ calls may sleep esp. for pll enables, I would prefer to see this patch dropped in favour of fixing the users. -- Ben ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] ARM: SAMSUNG: Bug fix spin_lock recursion in clk_enable() and clk_disable() 2010-10-18 23:09 ` Ben Dooks @ 2010-10-19 1:12 ` Jassi Brar 2010-10-19 2:55 ` mhban 2010-10-19 18:57 ` Russell King - ARM Linux 1 sibling, 1 reply; 6+ messages in thread From: Jassi Brar @ 2010-10-19 1:12 UTC (permalink / raw) To: linux-arm-kernel On Tue, Oct 19, 2010 at 8:09 AM, Ben Dooks <ben-linux@fluff.org> wrote: > On 12/10/10 01:39, Jaecheol Lee wrote: >> From: Minho Ban <mhban@samsung.com> >> >> The clk_enable() and clk_disable() can be used process and ISR either. >> So spin_lock_irqsave should be used instead. >> >> Signed-off-by: Minho Ban <mhban@samsung.com> >> Signed-off-by: Jaecheol Lee <jc.lee@samsung.com> >> --- >> ?arch/arm/plat-samsung/clock.c | ? 12 ++++++++---- >> ?1 files changed, 8 insertions(+), 4 deletions(-) >> >> diff --git a/arch/arm/plat-samsung/clock.c b/arch/arm/plat-samsung/clock.c >> index e8d20b0..2a991a5 100644 >> --- a/arch/arm/plat-samsung/clock.c >> +++ b/arch/arm/plat-samsung/clock.c >> @@ -138,31 +138,35 @@ void clk_put(struct clk *clk) >> >> ?int clk_enable(struct clk *clk) >> ?{ >> + ? ? unsigned long flags; >> + >> ? ? ? if (IS_ERR(clk) || clk == NULL) >> ? ? ? ? ? ? ? return -EINVAL; >> >> ? ? ? clk_enable(clk->parent); >> >> - ? ? spin_lock(&clocks_lock); >> + ? ? spin_lock_irqsave(&clocks_lock, flags); >> >> ? ? ? if ((clk->usage++) == 0) >> ? ? ? ? ? ? ? (clk->enable)(clk, 1); >> >> - ? ? spin_unlock(&clocks_lock); >> + ? ? spin_unlock_irqrestore(&clocks_lock, flags); >> ? ? ? return 0; >> ?} >> >> ?void clk_disable(struct clk *clk) >> ?{ >> + ? ? unsigned long flags; >> + >> ? ? ? if (IS_ERR(clk) || clk == NULL) >> ? ? ? ? ? ? ? return; >> >> - ? ? spin_lock(&clocks_lock); >> + ? ? spin_lock_irqsave(&clocks_lock, flags); >> >> ? ? ? if ((--clk->usage) == 0) >> ? ? ? ? ? ? ? (clk->enable)(clk, 0); >> >> - ? ? spin_unlock(&clocks_lock); >> + ? ? spin_unlock_irqrestore(&clocks_lock, flags); >> ? ? ? clk_disable(clk->parent); > > I'm not sure, but I don't belive that the clk_ api has > ever been callable from non-sleepable contexts such as > interrupt handlers. > > I would welcome RMK's response (or any other response) > about this issue? > > Personally, given that some clk_ calls may sleep esp. > for pll enables, I would prefer to see this patch > dropped in favour of fixing the users. I tend to agree. I would like to know any such case where enabling/disabling of some clock is so urgent as to require doing from ISR and not from a tasklet scheduled from ISR. Also let us not make it impossible in future to switch to common clock api by Jeremy Kerr, that doesn't allow calls from IRQ context. ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] ARM: SAMSUNG: Bug fix spin_lock recursion in clk_enable() and clk_disable() 2010-10-19 1:12 ` Jassi Brar @ 2010-10-19 2:55 ` mhban 0 siblings, 0 replies; 6+ messages in thread From: mhban @ 2010-10-19 2:55 UTC (permalink / raw) To: linux-arm-kernel On Tue, 2010-10-19 at 10:12 +0900, Jassi Brar wrote: > On Tue, Oct 19, 2010 at 8:09 AM, Ben Dooks <ben-linux@fluff.org> wrote: > > On 12/10/10 01:39, Jaecheol Lee wrote: > >> From: Minho Ban <mhban@samsung.com> > >> > >> The clk_enable() and clk_disable() can be used process and ISR either. > >> So spin_lock_irqsave should be used instead. > >> > >> Signed-off-by: Minho Ban <mhban@samsung.com> > >> Signed-off-by: Jaecheol Lee <jc.lee@samsung.com> > >> --- > >> arch/arm/plat-samsung/clock.c | 12 ++++++++---- > >> 1 files changed, 8 insertions(+), 4 deletions(-) > >> > >> diff --git a/arch/arm/plat-samsung/clock.c b/arch/arm/plat-samsung/clock.c > >> index e8d20b0..2a991a5 100644 > >> --- a/arch/arm/plat-samsung/clock.c > >> +++ b/arch/arm/plat-samsung/clock.c > >> @@ -138,31 +138,35 @@ void clk_put(struct clk *clk) > >> > >> int clk_enable(struct clk *clk) > >> { > >> + unsigned long flags; > >> + > >> if (IS_ERR(clk) || clk == NULL) > >> return -EINVAL; > >> > >> clk_enable(clk->parent); > >> > >> - spin_lock(&clocks_lock); > >> + spin_lock_irqsave(&clocks_lock, flags); > >> > >> if ((clk->usage++) == 0) > >> (clk->enable)(clk, 1); > >> > >> - spin_unlock(&clocks_lock); > >> + spin_unlock_irqrestore(&clocks_lock, flags); > >> return 0; > >> } > >> > >> void clk_disable(struct clk *clk) > >> { > >> + unsigned long flags; > >> + > >> if (IS_ERR(clk) || clk == NULL) > >> return; > >> > >> - spin_lock(&clocks_lock); > >> + spin_lock_irqsave(&clocks_lock, flags); > >> > >> if ((--clk->usage) == 0) > >> (clk->enable)(clk, 0); > >> > >> - spin_unlock(&clocks_lock); > >> + spin_unlock_irqrestore(&clocks_lock, flags); > >> clk_disable(clk->parent); > > > > I'm not sure, but I don't belive that the clk_ api has > > ever been callable from non-sleepable contexts such as > > interrupt handlers. > > > > I would welcome RMK's response (or any other response) > > about this issue? > > > > Personally, given that some clk_ calls may sleep esp. > > for pll enables, I would prefer to see this patch > > dropped in favour of fixing the users. > > I tend to agree. > I would like to know any such case where enabling/disabling of some > clock is so urgent as to require doing from ISR and not from a tasklet > scheduled from ISR. > Also let us not make it impossible in future to switch to common clock api > by Jeremy Kerr, that doesn't allow calls from IRQ context. > -- > To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in > the body of a message to majordomo at vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html Hello, Mostly you are right but it would require some explicit comment in clk.h (eg, clk_get/put) not to be confusing to the driver developer. Thanks. Regards, Minho Ban -- Minho Ban (mhban at samsung.com) Senior Engineer Platform R&D Team, Mobile Comm. SAMSUNG ELECTRONICS Co.Ltd. ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] ARM: SAMSUNG: Bug fix spin_lock recursion in clk_enable() and clk_disable() 2010-10-18 23:09 ` Ben Dooks 2010-10-19 1:12 ` Jassi Brar @ 2010-10-19 18:57 ` Russell King - ARM Linux 2010-10-21 22:39 ` Ben Dooks 1 sibling, 1 reply; 6+ messages in thread From: Russell King - ARM Linux @ 2010-10-19 18:57 UTC (permalink / raw) To: linux-arm-kernel On Tue, Oct 19, 2010 at 12:09:25AM +0100, Ben Dooks wrote: > I'm not sure, but I don't belive that the clk_ api has > ever been callable from non-sleepable contexts such as > interrupt handlers. > > I would welcome RMK's response (or any other response) > about this issue? We have traditionally allowed it from non-sleepable contexts, mainly because the platforms I've been using it with have had such simple clock implementations that it's not worth artifically restricting it, or the implementation only requires a few us delay after enabling a clock. ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] ARM: SAMSUNG: Bug fix spin_lock recursion in clk_enable() and clk_disable() 2010-10-19 18:57 ` Russell King - ARM Linux @ 2010-10-21 22:39 ` Ben Dooks 0 siblings, 0 replies; 6+ messages in thread From: Ben Dooks @ 2010-10-21 22:39 UTC (permalink / raw) To: linux-arm-kernel On 19/10/10 19:57, Russell King - ARM Linux wrote: > On Tue, Oct 19, 2010 at 12:09:25AM +0100, Ben Dooks wrote: >> I'm not sure, but I don't belive that the clk_ api has >> ever been callable from non-sleepable contexts such as >> interrupt handlers. >> >> I would welcome RMK's response (or any other response) >> about this issue? > > We have traditionally allowed it from non-sleepable contexts, mainly > because the platforms I've been using it with have had such simple > clock implementations that it's not worth artifically restricting it, > or the implementation only requires a few us delay after enabling a > clock. Some on chip PLLs can be 150-300uS to get running. -- Ben ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2010-10-21 22:39 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2010-10-12 0:39 [PATCH] ARM: SAMSUNG: Bug fix spin_lock recursion in clk_enable() and clk_disable() Jaecheol Lee 2010-10-18 23:09 ` Ben Dooks 2010-10-19 1:12 ` Jassi Brar 2010-10-19 2:55 ` mhban 2010-10-19 18:57 ` Russell King - ARM Linux 2010-10-21 22:39 ` Ben Dooks
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).