Re: MX28: leds/pwm: Using pwm driven led as heartbeat leads to kernel warning

Re: MX28: leds/pwm: Using pwm driven led as heartbeat leads to kernel warning

[Stefan Wahren]

Hello again,

Am 27.03.2014 18:14, schrieb Stefan Wahren:
> Hello,
>
> i'm using a i.MX28 custom board with two leds and Kernel 3.14-rc5.
>
> Here is the relevant dts part:
>
> pwmleds {
>         compatible = "pwm-leds";
>         pinctrl-names = "default";
>         pinctrl-0 = <&pwm0_pins_b>; /* MX28_PAD_AUART1_RX__PWM_0 */
>        
>         led0 {
>             label = "duckbill:red";
>             pwms = <&pwm 0 7812500>;
>             max-brightness = <127>;
>             linux,default-trigger = "none";
>         };
> };
>
> gpioleds {
>         compatible = "gpio-leds";
>         pinctrl-names = "default";
>         pinctrl-0 = <&led_pins>;
>
>         led1 {
>             label = "duckbill:green";
>             gpios = <&gpio3 5 0>;
>             linux,default-trigger = "heartbeat";
>         };
> };
>
> The gpio driven led works perfect as heartbeat. But if i change the
> trigger of the pwm
> driven led from none to heartbeat, it results always in a kernel warning
> and the pwm led
> keeps on:
>
> [  248.916168] WARNING: CPU: 0 PID: 0 at kernel/locking/mutex.c:856
> mutex_trylock+0x184/0x1a4()
> [  248.924745] DEBUG_LOCKS_WARN_ON(in_interrupt())
> [  248.929136] Modules linked in:
> [  248.932517] CPU: 0 PID: 0 Comm: swapper Not tainted 3.14.0-rc5 #18
> [  248.938781] [<c0015420>] (unwind_backtrace) from [<c0012cb0>]
> (show_stack+0x10/0x14)
> [  248.946672] [<c0012cb0>] (show_stack) from [<c001daf8>]
> (warn_slowpath_common+0x6c/0x8c)
> [  248.954902] [<c001daf8>] (warn_slowpath_common) from [<c001dbac>]
> (warn_slowpath_fmt+0x30/0x40)
> [  248.963742] [<c001dbac>] (warn_slowpath_fmt) from [<c045df74>]
> (mutex_trylock+0x184/0x1a4)
> [  248.972147] [<c045df74>] (mutex_trylock) from [<c0360950>]
> (clk_prepare_lock+0xc/0xec)
> [  248.980132] [<c0360950>] (clk_prepare_lock) from [<c0362020>]
> (clk_get_rate+0xc/0x68)
> [  248.988105] [<c0362020>] (clk_get_rate) from [<c028d07c>]
> (mxs_pwm_config+0x20/0x198)
> [  248.996098] [<c028d07c>] (mxs_pwm_config) from [<c028bde8>]
> (pwm_config+0x60/0x70)
> [  249.003824] [<c028bde8>] (pwm_config) from [<c034b61c>]
> (__led_pwm_set+0x1c/0x3c)
> [  249.011442] [<c034b61c>] (__led_pwm_set) from [<c034bc3c>]
> (led_heartbeat_function+0x70/0x110)
> [  249.020114] [<c034bc3c>] (led_heartbeat_function) from [<c00292f0>]
> (call_timer_fn+0x7c/0x164)
> [  249.028856] [<c00292f0>] (call_timer_fn) from [<c00295c8>]
> (run_timer_softirq+0x1f0/0x260)
> [  249.037267] [<c00295c8>] (run_timer_softirq) from [<c002255c>]
> (__do_softirq+0xc4/0x2f0)
> [  249.045507] [<c002255c>] (__do_softirq) from [<c0022890>]
> (irq_exit+0xa4/0x10c)
> [  249.052966] [<c0022890>] (irq_exit) from [<c0010240>]
> (handle_IRQ+0x34/0x84)
> [  249.060079] [<c0010240>] (handle_IRQ) from [<c0013524>]
> (__irq_svc+0x44/0x54)
> [  249.067358] [<c0013524>] (__irq_svc) from [<c00107f8>]
> (arch_cpu_idle+0x40/0x48)
> [  249.074916] [<c00107f8>] (arch_cpu_idle) from [<c005deb8>]
> (cpu_startup_entry+0x70/0x198)
> [  249.083258] [<c005deb8>] (cpu_startup_entry) from [<c060aac8>]
> (start_kernel+0x2a8/0x2f8)
>
> Best regards
> Stefan Wahren
>

the problem is still reproducable in Mainline 3.14.0 and gives the same
warning message.
Also i add linux-leds in CC.

Are there any advices to narrow down the problem?

Is this problem specific to i.MX28 PWM (i don't have other kind of
hardware)?

Can anybody reproduce this problem?

Kind regards
Stefan Wahren

Re: MX28: leds/pwm: Using pwm driven led as heartbeat leads to kernel warning

[Shawn Guo]

On Fri, Apr 04, 2014 at 03:54:52PM +0200, Stefan Wahren wrote:
> the problem is still reproducable in Mainline 3.14.0 and gives the same
> warning message.
> Also i add linux-leds in CC.
> 
> Are there any advices to narrow down the problem?
> 
> Is this problem specific to i.MX28 PWM (i don't have other kind of
> hardware)?

I don't think it's specific to i.MX28 PWM.  Since the .config hook of
pwm_ops will be called from interrupt context in this case, any pwm
drivers that do sleeping calls in the hook will have this problem, e.g.
the clk APIs that hold mutex like clk_prepare() and clk_get_rate() etc.

Shawn

Re: MX28: leds/pwm: Using pwm driven led as heartbeat leads to kernel warning

[Stefan Wahren]

Hi Shawn,

Am 04.04.2014 16:26, schrieb Shawn Guo:
> On Fri, Apr 04, 2014 at 03:54:52PM +0200, Stefan Wahren wrote:
>> the problem is still reproducable in Mainline 3.14.0 and gives the same
>> warning message.
>> Also i add linux-leds in CC.
>>
>> Are there any advices to narrow down the problem?
>>
>> Is this problem specific to i.MX28 PWM (i don't have other kind of
>> hardware)?
> I don't think it's specific to i.MX28 PWM.  Since the .config hook of
> pwm_ops will be called from interrupt context in this case, any pwm
> drivers that do sleeping calls in the hook will have this problem, e.g.
> the clk APIs that hold mutex like clk_prepare() and clk_get_rate() etc.
>
> Shawn

that sounds like the problem should be easier to reproduce.

Is there anything i can do to support bugfixing, beside sending the patch?

Stefan

Re: MX28: leds/pwm: Using pwm driven led as heartbeat leads to kernel warning

[Alexandre Belloni]

On 04/04/2014 at 22:26:58 +0800, Shawn Guo wrote :
> On Fri, Apr 04, 2014 at 03:54:52PM +0200, Stefan Wahren wrote:
> > the problem is still reproducable in Mainline 3.14.0 and gives the same
> > warning message.
> > Also i add linux-leds in CC.
> > 
> > Are there any advices to narrow down the problem?
> > 
> > Is this problem specific to i.MX28 PWM (i don't have other kind of
> > hardware)?
> 
> I don't think it's specific to i.MX28 PWM.  Since the .config hook of
> pwm_ops will be called from interrupt context in this case, any pwm
> drivers that do sleeping calls in the hook will have this problem, e.g.
> the clk APIs that hold mutex like clk_prepare() and clk_get_rate() etc.
> 

Yeah, I'm wondering whether we should set pwm->chip->can_sleep then.

Thierry, do you have an opinion ?

-- 
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Re: MX28: leds/pwm: Using pwm driven led as heartbeat leads to kernel warning

[Shawn Guo]

On Fri, Apr 04, 2014 at 06:44:47PM +0200, Alexandre Belloni wrote:
> On 04/04/2014 at 22:26:58 +0800, Shawn Guo wrote :
> > On Fri, Apr 04, 2014 at 03:54:52PM +0200, Stefan Wahren wrote:
> > > the problem is still reproducable in Mainline 3.14.0 and gives the same
> > > warning message.
> > > Also i add linux-leds in CC.
> > > 
> > > Are there any advices to narrow down the problem?
> > > 
> > > Is this problem specific to i.MX28 PWM (i don't have other kind of
> > > hardware)?
> > 
> > I don't think it's specific to i.MX28 PWM.  Since the .config hook of
> > pwm_ops will be called from interrupt context in this case, any pwm
> > drivers that do sleeping calls in the hook will have this problem, e.g.
> > the clk APIs that hold mutex like clk_prepare() and clk_get_rate() etc.
> > 
> 
> Yeah, I'm wondering whether we should set pwm->chip->can_sleep then.

Right, per kerneldoc of struct pwm_chip, we should set can_sleep.

/**
 * struct pwm_chip - abstract a PWM controller
 * ...
 * @can_sleep: must be true if the .config(), .enable() or .disable()
 *             operations may sleep
 */

But how does setting the flag make a difference there?  I see pwm core
does nothing about the flag, or am I missing something?

Shawn

Re: MX28: leds/pwm: Using pwm driven led as heartbeat leads to kernel warning

[Alexandre Belloni]

On 08/04/2014 at 15:34:04 +0800, Shawn Guo wrote :
> On Fri, Apr 04, 2014 at 06:44:47PM +0200, Alexandre Belloni wrote:
> > On 04/04/2014 at 22:26:58 +0800, Shawn Guo wrote :
> > > On Fri, Apr 04, 2014 at 03:54:52PM +0200, Stefan Wahren wrote:
> > > > the problem is still reproducable in Mainline 3.14.0 and gives the same
> > > > warning message.
> > > > Also i add linux-leds in CC.
> > > > 
> > > > Are there any advices to narrow down the problem?
> > > > 
> > > > Is this problem specific to i.MX28 PWM (i don't have other kind of
> > > > hardware)?
> > > 
> > > I don't think it's specific to i.MX28 PWM.  Since the .config hook of
> > > pwm_ops will be called from interrupt context in this case, any pwm
> > > drivers that do sleeping calls in the hook will have this problem, e.g.
> > > the clk APIs that hold mutex like clk_prepare() and clk_get_rate() etc.
> > > 
> > 
> > Yeah, I'm wondering whether we should set pwm->chip->can_sleep then.
> 
> Right, per kerneldoc of struct pwm_chip, we should set can_sleep.
> 
> /**
>  * struct pwm_chip - abstract a PWM controller
>  * ...
>  * @can_sleep: must be true if the .config(), .enable() or .disable()
>  *             operations may sleep
>  */
> 
> But how does setting the flag make a difference there?  I see pwm core
> does nothing about the flag, or am I missing something?
> 

The driver using the PWM should test the flag, have a look at:
http://lxr.free-electrons.com/source/drivers/leds/leds-pwm.c#L183


-- 
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Re: MX28: leds/pwm: Using pwm driven led as heartbeat leads to kernel warning

[Shawn Guo]

On Tue, Apr 08, 2014 at 09:59:16AM +0200, Alexandre Belloni wrote:
> The driver using the PWM should test the flag, have a look at:
> http://lxr.free-electrons.com/source/drivers/leds/leds-pwm.c#L183

Ah, right, that' how the flag works.  Thanks, Alexandre.  I send a patch
to set the flag for pwm-mxs driver shortly.

Shawn

Re: MX28: leds/pwm: Using pwm driven led as heartbeat leads to kernel warning

[Alexandre Belloni]

On 08/04/2014 at 16:18:49 +0800, Shawn Guo wrote :
> On Tue, Apr 08, 2014 at 09:59:16AM +0200, Alexandre Belloni wrote:
> > The driver using the PWM should test the flag, have a look at:
> > http://lxr.free-electrons.com/source/drivers/leds/leds-pwm.c#L183
> 
> Ah, right, that' how the flag works.  Thanks, Alexandre.  I send a patch
> to set the flag for pwm-mxs driver shortly.
> 

Actually, I had a closer look and I believe using clk_prepare_enable()
in .config is wrong. Let me prepare a patch.


-- 
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Re: MX28: leds/pwm: Using pwm driven led as heartbeat leads to kernel warning

[Shawn Guo]

On Tue, Apr 08, 2014 at 10:41:50AM +0200, Alexandre Belloni wrote:
> On 08/04/2014 at 16:18:49 +0800, Shawn Guo wrote :
> > On Tue, Apr 08, 2014 at 09:59:16AM +0200, Alexandre Belloni wrote:
> > > The driver using the PWM should test the flag, have a look at:
> > > http://lxr.free-electrons.com/source/drivers/leds/leds-pwm.c#L183
> > 
> > Ah, right, that' how the flag works.  Thanks, Alexandre.  I send a patch
> > to set the flag for pwm-mxs driver shortly.
> > 
> 
> Actually, I had a closer look and I believe using clk_prepare_enable()
> in .config is wrong. Let me prepare a patch.

Yes, we can split clk_prepare_enable() into clk_prepare() and
clk_enable(), call the former in .probe() and only the later in .config
hook.

But it only fixes the problem partially.  If you look at Stefan's report
closely, you can see the warning actually comes from the clk_get_rate()
call which holds a mutex too.

Shawn

Re: MX28: leds/pwm: Using pwm driven led as heartbeat leads to kernel warning

[Alexandre Belloni]

On 08/04/2014 at 16:49:52 +0800, Shawn Guo wrote :
> On Tue, Apr 08, 2014 at 10:41:50AM +0200, Alexandre Belloni wrote:
> > On 08/04/2014 at 16:18:49 +0800, Shawn Guo wrote :
> > > On Tue, Apr 08, 2014 at 09:59:16AM +0200, Alexandre Belloni wrote:
> > > > The driver using the PWM should test the flag, have a look at:
> > > > http://lxr.free-electrons.com/source/drivers/leds/leds-pwm.c#L183
> > > 
> > > Ah, right, that' how the flag works.  Thanks, Alexandre.  I send a patch
> > > to set the flag for pwm-mxs driver shortly.
> > > 
> > 
> > Actually, I had a closer look and I believe using clk_prepare_enable()
> > in .config is wrong. Let me prepare a patch.
> 
> Yes, we can split clk_prepare_enable() into clk_prepare() and
> clk_enable(), call the former in .probe() and only the later in .config
> hook.
> 
> But it only fixes the problem partially.  If you look at Stefan's report
> closely, you can see the warning actually comes from the clk_get_rate()
> call which holds a mutex too.
> 

Right, I missed that one. I would say it is still worth doing that
change anyway as using test_bit(PWMF_ENABLED, &pwm->flags) is
unnecessary because clk_enable()/clk_disable() maintain a usage count.

-- 
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com