| Message ID | 20130606121647.GR23140@pengutronix.de |
|---|---|
| State | New |
| Headers | show |
On Thu, Jun 06, 2013 at 02:16:47PM +0200, Uwe Kleine-König wrote: > Hello > > [added jstultz to Cc:] > > On Thu, Jun 06, 2013 at 02:55:26PM +0800, Peter Chen wrote: > > For tickless system, the jiffies may be updated long time (>20ms). > ... may not be updated for a long time ... ? > > > At high loading system, the current waiting method will cause waiting > > timeout, and cause a kernel dump at below case. > > After timeout = jiffies + msecs_to_jiffies(10), > > the timer interrupt occurs, it updates jiffies (eg, + 2 jiffies), > > then return back from interrupt, the time between above operations > > are tiny, the PLL is still not locked, but the timeout condition is satisfied. > Hmm, I admit I didn't follow the tickless stuff, but still I wonder if > the analysis is right. I thought on tickless jiffies are updated as > before by the boot cpu that cannot run in tickless mode? > > Anyhow, this only affects the commit log, not the problem. > I add jiffies print at irq_exit (kernel/softirq.c), and it is not updated every jiffies. Meanwhile, I ran out this PLL Lock timeout issue with iperf test using usb ethernet gadget at current code (v3.5.7 kernel). After using udelay, this issue is disappeared.
On Fri, Jun 07, 2013 at 10:53:20AM +0800, Peter Chen wrote: > On Thu, Jun 06, 2013 at 02:16:47PM +0200, Uwe Kleine-König wrote: > > Hello > > > > [added jstultz to Cc:] > > > > On Thu, Jun 06, 2013 at 02:55:26PM +0800, Peter Chen wrote: > > > For tickless system, the jiffies may be updated long time (>20ms). > > ... may not be updated for a long time ... ? > > > > > At high loading system, the current waiting method will cause waiting > > > timeout, and cause a kernel dump at below case. > > > After timeout = jiffies + msecs_to_jiffies(10), > > > the timer interrupt occurs, it updates jiffies (eg, + 2 jiffies), > > > then return back from interrupt, the time between above operations > > > are tiny, the PLL is still not locked, but the timeout condition is satisfied. > > Hmm, I admit I didn't follow the tickless stuff, but still I wonder if > > the analysis is right. I thought on tickless jiffies are updated as > > before by the boot cpu that cannot run in tickless mode? > > > > Anyhow, this only affects the commit log, not the problem. > > > > I add jiffies print at irq_exit (kernel/softirq.c), and it > is not updated every jiffies. Oh, I have not considered cpu idle condition. I think this problem may occur the condition that irq and softirq consume too much time. > Meanwhile, I ran out this PLL Lock timeout issue with > iperf test using usb ethernet gadget at current code (v3.5.7 kernel). > After using udelay, this issue is disappeared. > > -- > > Best Regards, > Peter Chen
On Fri, Jun 07, 2013 at 11:33:07AM +0800, Peter Chen wrote: > On Fri, Jun 07, 2013 at 10:53:20AM +0800, Peter Chen wrote: > > On Thu, Jun 06, 2013 at 02:16:47PM +0200, Uwe Kleine-König wrote: > > > Hello > > > > > > [added jstultz to Cc:] > > > > > > On Thu, Jun 06, 2013 at 02:55:26PM +0800, Peter Chen wrote: > > > > For tickless system, the jiffies may be updated long time (>20ms). > > > ... may not be updated for a long time ... ? > > > > > > > At high loading system, the current waiting method will cause waiting > > > > timeout, and cause a kernel dump at below case. > > > > After timeout = jiffies + msecs_to_jiffies(10), > > > > the timer interrupt occurs, it updates jiffies (eg, + 2 jiffies), > > > > then return back from interrupt, the time between above operations > > > > are tiny, the PLL is still not locked, but the timeout condition is satisfied. > > > Hmm, I admit I didn't follow the tickless stuff, but still I wonder if > > > the analysis is right. I thought on tickless jiffies are updated as > > > before by the boot cpu that cannot run in tickless mode? > > > > > > Anyhow, this only affects the commit log, not the problem. > > > > > > > I add jiffies print at irq_exit (kernel/softirq.c), and it > > is not updated every jiffies. > > Oh, I have not considered cpu idle condition. I think this > problem may occur the condition that irq and softirq consume > too much time. That was my thought, too. With my patch this time at least only happens after the pll is programmed and the issue might be solved, too?! (Maybe apart from Russell's comment.) Best regards Uwe
On 06/06/2013 05:16 AM, Uwe Kleine-König wrote: > Hello > > [added jstultz to Cc:] > > On Thu, Jun 06, 2013 at 02:55:26PM +0800, Peter Chen wrote: >> For tickless system, the jiffies may be updated long time (>20ms). > ... may not be updated for a long time ... ? > >> At high loading system, the current waiting method will cause waiting >> timeout, and cause a kernel dump at below case. >> After timeout = jiffies + msecs_to_jiffies(10), >> the timer interrupt occurs, it updates jiffies (eg, + 2 jiffies), >> then return back from interrupt, the time between above operations >> are tiny, the PLL is still not locked, but the timeout condition is satisfied. > Hmm, I admit I didn't follow the tickless stuff, but still I wonder if > the analysis is right. I thought on tickless jiffies are updated as > before by the boot cpu that cannot run in tickless mode? > > Anyhow, this only affects the commit log, not the problem. > >> Signed-off-by: Peter Chen <peter.chen@freescale.com> >> --- >> arch/arm/mach-imx/clk-pllv3.c | 9 ++++++--- >> 1 files changed, 6 insertions(+), 3 deletions(-) >> >> diff --git a/arch/arm/mach-imx/clk-pllv3.c b/arch/arm/mach-imx/clk-pllv3.c >> index 36aac94..eefc6c2 100644 >> --- a/arch/arm/mach-imx/clk-pllv3.c >> +++ b/arch/arm/mach-imx/clk-pllv3.c >> @@ -16,6 +16,7 @@ >> #include <linux/slab.h> >> #include <linux/jiffies.h> >> #include <linux/err.h> >> +#include <linux/delay.h> >> #include "clk.h" >> >> #define PLL_NUM_OFFSET 0x10 >> @@ -50,7 +51,7 @@ struct clk_pllv3 { >> static int clk_pllv3_prepare(struct clk_hw *hw) >> { >> struct clk_pllv3 *pll = to_clk_pllv3(hw); >> - unsigned long timeout = jiffies + msecs_to_jiffies(10); >> + int count = 100; >> u32 val; >> >> val = readl_relaxed(pll->base); >> @@ -62,9 +63,11 @@ static int clk_pllv3_prepare(struct clk_hw *hw) >> writel_relaxed(val, pll->base); >> >> /* Wait for PLL to lock */ >> - while (!(readl_relaxed(pll->base) & BM_PLL_LOCK)) >> - if (time_after(jiffies, timeout)) >> + while (!(readl_relaxed(pll->base) & BM_PLL_LOCK)) { >> + udelay(100); >> + if (--count == 0) >> return -ETIMEDOUT; >> + } > Maybe it's enough to do timeout = jiffies + msecs_to_jiffies(10); just > after the pll is reprogrammed? i.e. > > diff --git a/arch/arm/mach-imx/clk-pllv3.c b/arch/arm/mach-imx/clk-pllv3.c > index d09bc3d..37f734e 100644 > --- a/arch/arm/mach-imx/clk-pllv3.c > +++ b/arch/arm/mach-imx/clk-pllv3.c > @@ -48,7 +48,7 @@ struct clk_pllv3 { > static int clk_pllv3_prepare(struct clk_hw *hw) > { > struct clk_pllv3 *pll = to_clk_pllv3(hw); > - unsigned long timeout = jiffies + msecs_to_jiffies(10); > + unsigned long timeout; > u32 val; > > val = readl_relaxed(pll->base); > @@ -59,6 +59,8 @@ static int clk_pllv3_prepare(struct clk_hw *hw) > val &= ~BM_PLL_POWER; > writel_relaxed(val, pll->base); > > + timeout = jiffies + msecs_to_jiffies(10); > + > /* Wait for PLL to lock */ > while (!(readl_relaxed(pll->base) & BM_PLL_LOCK)) > if (time_after(jiffies, timeout)) > > Then at least the pll tries to look while the process is interrupted. > > What is msecs_to_jiffies(10) for you? John, would you expect the problem > here that Peter describes? No, I'm not sure I see how being tickless would cause such a problem (Adding Frederic here in case he spots something). The only issues that pop up for me right away is: 1) The one Uwe noted, where jiffies may be incremented from the early timeout assignment to before the wait loop begins. 2) That at HZ=100, msecs_to_jiffies(10) is just 1, so if you were to catch jiffies right before it was updated, you would end up waiting only 10ms before timing out. That's still seems like plenty of time, but I don't know the hardware details here. thanks -john
On Fri, Jun 07, 2013 at 09:47:26AM -0700, John Stultz wrote: > On 06/06/2013 05:16 AM, Uwe Kleine-König wrote: > >Hello > > > >[added jstultz to Cc:] > > > >On Thu, Jun 06, 2013 at 02:55:26PM +0800, Peter Chen wrote: > >>For tickless system, the jiffies may be updated long time (>20ms). > >... may not be updated for a long time ... ? > > > >>At high loading system, the current waiting method will cause waiting > >>timeout, and cause a kernel dump at below case. > >>After timeout = jiffies + msecs_to_jiffies(10), > >>the timer interrupt occurs, it updates jiffies (eg, + 2 jiffies), > >>then return back from interrupt, the time between above operations > >>are tiny, the PLL is still not locked, but the timeout condition is satisfied. > >Hmm, I admit I didn't follow the tickless stuff, but still I wonder if > >the analysis is right. I thought on tickless jiffies are updated as > >before by the boot cpu that cannot run in tickless mode? > > > >Anyhow, this only affects the commit log, not the problem. > >>Signed-off-by: Peter Chen <peter.chen@freescale.com> > >>--- > >> arch/arm/mach-imx/clk-pllv3.c | 9 ++++++--- > >> 1 files changed, 6 insertions(+), 3 deletions(-) > >> > >>diff --git a/arch/arm/mach-imx/clk-pllv3.c b/arch/arm/mach-imx/clk-pllv3.c > >>index 36aac94..eefc6c2 100644 > >>--- a/arch/arm/mach-imx/clk-pllv3.c > >>+++ b/arch/arm/mach-imx/clk-pllv3.c > >>@@ -16,6 +16,7 @@ > >> #include <linux/slab.h> > >> #include <linux/jiffies.h> > >> #include <linux/err.h> > >>+#include <linux/delay.h> > >> #include "clk.h" > >> #define PLL_NUM_OFFSET 0x10 > >>@@ -50,7 +51,7 @@ struct clk_pllv3 { > >> static int clk_pllv3_prepare(struct clk_hw *hw) > >> { > >> struct clk_pllv3 *pll = to_clk_pllv3(hw); > >>- unsigned long timeout = jiffies + msecs_to_jiffies(10); > >>+ int count = 100; > >> u32 val; > >> val = readl_relaxed(pll->base); > >>@@ -62,9 +63,11 @@ static int clk_pllv3_prepare(struct clk_hw *hw) > >> writel_relaxed(val, pll->base); > >> /* Wait for PLL to lock */ > >>- while (!(readl_relaxed(pll->base) & BM_PLL_LOCK)) > >>- if (time_after(jiffies, timeout)) > >>+ while (!(readl_relaxed(pll->base) & BM_PLL_LOCK)) { > >>+ udelay(100); > >>+ if (--count == 0) > >> return -ETIMEDOUT; > >>+ } > >Maybe it's enough to do timeout = jiffies + msecs_to_jiffies(10); just > >after the pll is reprogrammed? i.e. > > > >diff --git a/arch/arm/mach-imx/clk-pllv3.c b/arch/arm/mach-imx/clk-pllv3.c > >index d09bc3d..37f734e 100644 > >--- a/arch/arm/mach-imx/clk-pllv3.c > >+++ b/arch/arm/mach-imx/clk-pllv3.c > >@@ -48,7 +48,7 @@ struct clk_pllv3 { > > static int clk_pllv3_prepare(struct clk_hw *hw) > > { > > struct clk_pllv3 *pll = to_clk_pllv3(hw); > >- unsigned long timeout = jiffies + msecs_to_jiffies(10); > >+ unsigned long timeout; > > u32 val; > > val = readl_relaxed(pll->base); > >@@ -59,6 +59,8 @@ static int clk_pllv3_prepare(struct clk_hw *hw) > > val &= ~BM_PLL_POWER; > > writel_relaxed(val, pll->base); > >+ timeout = jiffies + msecs_to_jiffies(10); > >+ > > /* Wait for PLL to lock */ > > while (!(readl_relaxed(pll->base) & BM_PLL_LOCK)) > > if (time_after(jiffies, timeout)) > > > >Then at least the pll tries to look while the process is interrupted. > > > >What is msecs_to_jiffies(10) for you? John, would you expect the problem > >here that Peter describes? > > No, I'm not sure I see how being tickless would cause such a problem > (Adding Frederic here in case he spots something). I must confess I don't understand very well the problem description :-s > > The only issues that pop up for me right away is: > 1) The one Uwe noted, where jiffies may be incremented from the > early timeout assignment to before the wait loop begins. > > 2) That at HZ=100, msecs_to_jiffies(10) is just 1, so if you were to > catch jiffies right before it was updated, you would end up waiting > only 10ms before timing out. That's still seems like plenty of time, > but I don't know the hardware details here. > > thanks > -john >
diff --git a/arch/arm/mach-imx/clk-pllv3.c b/arch/arm/mach-imx/clk-pllv3.c index d09bc3d..37f734e 100644 --- a/arch/arm/mach-imx/clk-pllv3.c +++ b/arch/arm/mach-imx/clk-pllv3.c @@ -48,7 +48,7 @@ struct clk_pllv3 { static int clk_pllv3_prepare(struct clk_hw *hw) { struct clk_pllv3 *pll = to_clk_pllv3(hw); - unsigned long timeout = jiffies + msecs_to_jiffies(10); + unsigned long timeout; u32 val; val = readl_relaxed(pll->base); @@ -59,6 +59,8 @@ static int clk_pllv3_prepare(struct clk_hw *hw) val &= ~BM_PLL_POWER; writel_relaxed(val, pll->base); + timeout = jiffies + msecs_to_jiffies(10); + /* Wait for PLL to lock */ while (!(readl_relaxed(pll->base) & BM_PLL_LOCK)) if (time_after(jiffies, timeout))