| Message ID | 902E09E6452B0E43903E4F2D568737AB2C8C9F@DNCE04.ent.ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
"Strashko, Grygorii" <grygorii.strashko@ti.com> writes: > Hi Kevin, > > yep, [1] is the same fix - thanks. > Hi Kalle, > > i've applied these changes and PM runtime fix on top of git://git.kernel.org/pub/scm/linux/kernel/git/tmlind/linux-omap.git (omap2plus_defconfig) > Could you check it with your use case, pls? (just to be sure that idea is right) I think the ideas is right, but [1] introduces more potential problems since it disables the IRQ at the INTC well before being disabled at the device. With runtime PM autosuspend timeouts, that means any IRQs firing during the autosuspend delay will be lost, which basically nullifies the use of autosuspend. Since Shubhrajyoti didn't respond to my runtime PM comments on the original patch, I'll respin this patch by moving the INTC disable/enable into the runtime PM callbacks and make the changelog a bit more clear. Grygorii, that pm_runtime_resume() change is needed to. Can you spin a patch with just that change with a nice descriptive changelog about why it is needed? Thanks. Kevin > > diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c > index a0e49f6..cb09e20 100644 > --- a/drivers/i2c/busses/i2c-omap.c > +++ b/drivers/i2c/busses/i2c-omap.c > @@ -586,6 +586,9 @@ omap_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num) > if (IS_ERR_VALUE(r)) > goto out; > > + /* We have the bus, enable IRQ */ > + enable_irq(dev->irq); > + > r = omap_i2c_wait_for_bb(dev); > if (r < 0) > goto out; > @@ -606,6 +609,7 @@ omap_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num) > r = num; > > omap_i2c_wait_for_bb(dev); > + disable_irq(dev->irq); > out: > pm_runtime_put(dev->dev); > return r; > @@ -1060,6 +1064,9 @@ omap_i2c_probe(struct platform_device *pdev) > omap_i2c_isr; > r = request_irq(dev->irq, isr, IRQF_NO_SUSPEND, pdev->name, dev); > > + /* We enable IRQ only when request for I2C from master */ > + disable_irq(dev->irq); > + > if (r) { > dev_err(dev->dev, "failure requesting irq %i\n", dev->irq); > goto err_unuse_clocks; > @@ -1182,7 +1189,23 @@ static int omap_i2c_runtime_resume(struct device *dev) > } > #endif /* CONFIG_PM_RUNTIME */ > > +static int omap_i2c_suspend(struct device *dev) > +{ > + int ret; > + > + /* > + * Enable I2C device, so it will be accessible during > + * later stages of suspending when device Runtime PM is disabled. > + * I2C device will be turned off at "noirq" suspend stage. > + */ > + ret = pm_runtime_resume(dev); > + if (ret < 0) > + return ret; > + return 0; > +} > + > static struct dev_pm_ops omap_i2c_pm_ops = { > + SET_SYSTEM_SLEEP_PM_OPS(omap_i2c_suspend, NULL) > SET_RUNTIME_PM_OPS(omap_i2c_runtime_suspend, > omap_i2c_runtime_resume, NULL) > }; > > - Grygorii > ________________________________________ > From: Kevin Hilman [khilman@deeprootsystems.com] > Sent: Friday, October 12, 2012 5:34 PM > To: Strashko, Grygorii > Cc: Kalle Jokiniemi; linux-i2c@vger.kernel.org; w.sang@pengutronix.de; ben-linux@fluff.org; tony@atomide.com; linux-omap@vger.kernel.org; Datta, Shubhrajyoti; Kankroliwala, Huzefa > Subject: Re: [PATCH v3] ARM: OMAP: i2c: fix interrupt flood during resume > > "Strashko, Grygorii" <grygorii.strashko@ti.com> writes: > >> Hi All, >> >> Sorry, for the late reply. >> + CC Huzefa Kankroliwala - who is I2C driver owner on Android Kernel 3.4. > > Hi Grygorii, thanks for reviewing. I was hoping you would have some > ideas here as this was sounding familiar to something you had > mentioned elsewhere. > >> Regarding this patch - from my point of view, it fixes corner case and not an issue in general. >> Let take a look on resume sequence: >> - platform resume >> - syscore resume >> - resume_noirq >> - enable IRQs - resume_device_irqs() >> |- at this point IRQ handler will be invoked if IRQ state is IRQS_PENDING. >> |- so, the I2C device IRQ handler may be called at time when I2C adapter IRQ is still disabled and, as result, the I2C device IRQ-handler may fail. (I2C device and I2C adapter may use different physical IRQ lines) >> - resume_late >> |- enable I2C bus IRQ >> >> Possibly, the better way is to enable/disable I2C bus IRQ when needed - in our case in omap_i2c_xfer(). >> We use such approach in Android kernel 3.4 >> (http://git.omapzoom.org/?p=kernel/omap.git;a=commitdiff;h=1445a4d3b587c164bd30d108b61760aaaa07365e) > > I agree, that should work and cover the cases where I2C is used by other > processors also. Shubhrajyoti already posted something similar[1] but > it needed some rework (comments from Russell and myself.) > > Huzefa, Shubhrajyoti, who can rework this idea for the upstream and/or > follow up with the earlier patch[1]? > > Wolfram, I guess for now lets hold off on $SUBJECT patch. Seems we can > come up with a broader solution. Thanks. > > Kevin > > [1] http://lists.infradead.org/pipermail/linux-arm-kernel/2012-October/124427.html -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Oct 12, 2012 at 10:13 PM, Kevin Hilman <khilman@deeprootsystems.com> wrote: > "Strashko, Grygorii" <grygorii.strashko@ti.com> writes: > >> Hi Kevin, >> >> yep, [1] is the same fix - thanks. > >> Hi Kalle, >> >> i've applied these changes and PM runtime fix on top of git://git.kernel.org/pub/scm/linux/kernel/git/tmlind/linux-omap.git (omap2plus_defconfig) >> Could you check it with your use case, pls? (just to be sure that idea is right) > > I think the ideas is right, but [1] introduces more potential problems > since it disables the IRQ at the INTC well before being disabled at the > device. I fail to see this point. Current driver supports master mode only. So unless there is a msg queued by the core. May be no interrupts should fire. what I mean msg -> conf -> intr -> completion/error -> autosuspend. > With runtime PM autosuspend timeouts, that means any IRQs > firing during the autosuspend delay will be lost, which basically > nullifies the use of autosuspend. so I do not expect any interrupts between completion/error -> autosuspend. > > Since Shubhrajyoti didn't respond to my runtime PM comments on the > original patch, my apologies for the delay. > I'll respin this patch by moving the INTC disable/enable > into the runtime PM callbacks and make the changelog a bit more clear. > > Grygorii, that pm_runtime_resume() change is needed to. Can you spin a > patch with just that change with a nice descriptive changelog about why > it is needed? Thanks. > > Kevin -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Shubhrajyoti Datta <omaplinuxkernel@gmail.com> writes: > On Fri, Oct 12, 2012 at 10:13 PM, Kevin Hilman > <khilman@deeprootsystems.com> wrote: >> "Strashko, Grygorii" <grygorii.strashko@ti.com> writes: >> >>> Hi Kevin, >>> >>> yep, [1] is the same fix - thanks. >> >>> Hi Kalle, >>> >>> i've applied these changes and PM runtime fix on top of git://git.kernel.org/pub/scm/linux/kernel/git/tmlind/linux-omap.git (omap2plus_defconfig) >>> Could you check it with your use case, pls? (just to be sure that idea is right) >> >> I think the ideas is right, but [1] introduces more potential problems >> since it disables the IRQ at the INTC well before being disabled at the >> device. > I fail to see this point. Current driver supports master mode only. > So unless there is a msg queued by the core. May be no interrupts should fire. > > what I mean > > msg -> conf -> intr -> completion/error -> autosuspend. > >> With runtime PM autosuspend timeouts, that means any IRQs >> firing during the autosuspend delay will be lost, which basically >> nullifies the use of autosuspend. > > so I do not expect any interrupts between completion/error -> autosuspend. Runtime PM is designed for concurrent users (hence the usecounting.) The I2C driver may not fully support this, since there is a single bus to share, but the usage of runtime PM currently allows multiple users to do runtime PM get/put (though in this driver they will block in the wait_for_bb.) So the fundamental problem in doing the enable/disable IRQ in the xfer function, and not the runtime PM callbacks is that you're ignoring the runtime PM usecount. You're assuming that the 'get' is *always* incrementing the usecount from zero to 1, and the 'put' is *always* a transition from 1 to zero. This may be the case in current usage and tests, but does not have to be the case. Even if that never happens in practice, it can in theory, and to me leads to confusing code. It simply doesn't make sense in terms of readability to disable the IRQ at the INTC in xfer, and disable IRQs at the device level in the runtime PM callback. To put it more simply: anything that needs to be done when the I2C is about to be idled/enabled should be done in the runtime PM callbacks. Please have a look at the patch I just sent[1]. In addition to making it more readable (IMO), it elminates the need for an extra disable_irq() in probe. Kevin [1] http://marc.info/?l=linux-omap&m=135006723121147&w=2 -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Saturday 13 October 2012 12:34 AM, Kevin Hilman wrote: > Shubhrajyoti Datta <omaplinuxkernel@gmail.com> writes: > >> On Fri, Oct 12, 2012 at 10:13 PM, Kevin Hilman >> <khilman@deeprootsystems.com> wrote: >>> "Strashko, Grygorii" <grygorii.strashko@ti.com> writes: >>> >>>> Hi Kevin, >>>> >>>> yep, [1] is the same fix - thanks. >>>> Hi Kalle, >>>> >>>> i've applied these changes and PM runtime fix on top of git://git.kernel.org/pub/scm/linux/kernel/git/tmlind/linux-omap.git (omap2plus_defconfig) >>>> Could you check it with your use case, pls? (just to be sure that idea is right) >>> I think the ideas is right, but [1] introduces more potential problems >>> since it disables the IRQ at the INTC well before being disabled at the >>> device. >> I fail to see this point. Current driver supports master mode only. >> So unless there is a msg queued by the core. May be no interrupts should fire. >> >> what I mean >> >> msg -> conf -> intr -> completion/error -> autosuspend. >> >>> With runtime PM autosuspend timeouts, that means any IRQs >>> firing during the autosuspend delay will be lost, which basically >>> nullifies the use of autosuspend. >> so I do not expect any interrupts between completion/error -> autosuspend. > Runtime PM is designed for concurrent users (hence the usecounting.) > The I2C driver may not fully support this, since there is a single bus > to share, but the usage of runtime PM currently allows multiple users to > do runtime PM get/put (though in this driver they will block in the > wait_for_bb.) > > So the fundamental problem in doing the enable/disable IRQ in the xfer > function, and not the runtime PM callbacks is that you're ignoring the > runtime PM usecount. You're assuming that the 'get' is *always* > incrementing the usecount from zero to 1, and the 'put' is *always* a > transition from 1 to zero. This may be the case in current usage and > tests, but does not have to be the case. > > Even if that never happens in practice, it can in theory, and to me > leads to confusing code. It simply doesn't make sense in terms of > readability to disable the IRQ at the INTC in xfer, and disable IRQs at > the device level in the runtime PM callback. Agree. > > To put it more simply: anything that needs to be done when the I2C is > about to be idled/enabled should be done in the runtime PM callbacks. > > Please have a look at the patch I just sent[1]. In addition to making > it more readable (IMO), it elminates the need for an extra disable_irq() > in probe. thanks. > > Kevin > > [1] http://marc.info/?l=linux-omap&m=135006723121147&w=2 -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, pe, 2012-10-12 kello 14:46 +0000, Strashko, Grygorii kirjoitti: > Hi Kevin, > > yep, [1] is the same fix - thanks. > > Hi Kalle, > > i've applied these changes and PM runtime fix on top of git://git.kernel.org/pub/scm/linux/kernel/git/tmlind/linux-omap.git (omap2plus_defconfig) > Could you check it with your use case, pls? (just to be sure that idea is right) Odd, it's not working. I'll add some debug prints to see what happens there. - Kalle > > diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c > index a0e49f6..cb09e20 100644 > --- a/drivers/i2c/busses/i2c-omap.c > +++ b/drivers/i2c/busses/i2c-omap.c > @@ -586,6 +586,9 @@ omap_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num) > if (IS_ERR_VALUE(r)) > goto out; > > + /* We have the bus, enable IRQ */ > + enable_irq(dev->irq); > + > r = omap_i2c_wait_for_bb(dev); > if (r < 0) > goto out; > @@ -606,6 +609,7 @@ omap_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num) > r = num; > > omap_i2c_wait_for_bb(dev); > + disable_irq(dev->irq); > out: > pm_runtime_put(dev->dev); > return r; > @@ -1060,6 +1064,9 @@ omap_i2c_probe(struct platform_device *pdev) > omap_i2c_isr; > r = request_irq(dev->irq, isr, IRQF_NO_SUSPEND, pdev->name, dev); > > + /* We enable IRQ only when request for I2C from master */ > + disable_irq(dev->irq); > + > if (r) { > dev_err(dev->dev, "failure requesting irq %i\n", dev->irq); > goto err_unuse_clocks; > @@ -1182,7 +1189,23 @@ static int omap_i2c_runtime_resume(struct device *dev) > } > #endif /* CONFIG_PM_RUNTIME */ > > +static int omap_i2c_suspend(struct device *dev) > +{ > + int ret; > + > + /* > + * Enable I2C device, so it will be accessible during > + * later stages of suspending when device Runtime PM is disabled. > + * I2C device will be turned off at "noirq" suspend stage. > + */ > + ret = pm_runtime_resume(dev); > + if (ret < 0) > + return ret; > + return 0; > +} > + > static struct dev_pm_ops omap_i2c_pm_ops = { > + SET_SYSTEM_SLEEP_PM_OPS(omap_i2c_suspend, NULL) > SET_RUNTIME_PM_OPS(omap_i2c_runtime_suspend, > omap_i2c_runtime_resume, NULL) > }; > > - Grygorii > ________________________________________ > From: Kevin Hilman [khilman@deeprootsystems.com] > Sent: Friday, October 12, 2012 5:34 PM > To: Strashko, Grygorii > Cc: Kalle Jokiniemi; linux-i2c@vger.kernel.org; w.sang@pengutronix.de; ben-linux@fluff.org; tony@atomide.com; linux-omap@vger.kernel.org; Datta, Shubhrajyoti; Kankroliwala, Huzefa > Subject: Re: [PATCH v3] ARM: OMAP: i2c: fix interrupt flood during resume > > "Strashko, Grygorii" <grygorii.strashko@ti.com> writes: > > > Hi All, > > > > Sorry, for the late reply. > > + CC Huzefa Kankroliwala - who is I2C driver owner on Android Kernel 3.4. > > Hi Grygorii, thanks for reviewing. I was hoping you would have some > ideas here as this was sounding familiar to something you had > mentioned elsewhere. > > > Regarding this patch - from my point of view, it fixes corner case and not an issue in general. > > Let take a look on resume sequence: > > - platform resume > > - syscore resume > > - resume_noirq > > - enable IRQs - resume_device_irqs() > > |- at this point IRQ handler will be invoked if IRQ state is IRQS_PENDING. > > |- so, the I2C device IRQ handler may be called at time when I2C adapter IRQ is still disabled and, as result, the I2C device IRQ-handler may fail. (I2C device and I2C adapter may use different physical IRQ lines) > > - resume_late > > |- enable I2C bus IRQ > > > > Possibly, the better way is to enable/disable I2C bus IRQ when needed - in our case in omap_i2c_xfer(). > > We use such approach in Android kernel 3.4 > > (http://git.omapzoom.org/?p=kernel/omap.git;a=commitdiff;h=1445a4d3b587c164bd30d108b61760aaaa07365e) > > I agree, that should work and cover the cases where I2C is used by other > processors also. Shubhrajyoti already posted something similar[1] but > it needed some rework (comments from Russell and myself.) > > Huzefa, Shubhrajyoti, who can rework this idea for the upstream and/or > follow up with the earlier patch[1]? > > Wolfram, I guess for now lets hold off on $SUBJECT patch. Seems we can > come up with a broader solution. Thanks. > > Kevin > > [1] http://lists.infradead.org/pipermail/linux-arm-kernel/2012-October/124427.html -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Oct 15, 2012 at 2:46 PM, Kalle Jokiniemi <kalle.jokiniemi@jollamobile.com> wrote: > ma, 2012-10-15 kello 09:21 +0300, Kalle Jokiniemi kirjoitti: >> Hi, >> >> pe, 2012-10-12 kello 14:46 +0000, Strashko, Grygorii kirjoitti: >> > Hi Kevin, >> > >> > yep, [1] is the same fix - thanks. >> > >> > Hi Kalle, >> > >> > i've applied these changes and PM runtime fix on top of git://git.kernel.org/pub/scm/linux/kernel/git/tmlind/linux-omap.git (omap2plus_defconfig) >> > Could you check it with your use case, pls? (just to be sure that idea is right) >> >> Odd, it's not working. I'll add some debug prints to see what happens >> there. > > Well, seems after enabling irq 23 in the resume_noirq, someone does > i2c_xfer and there is consequent flood of i2c_xfers and interrupts. If there is continuous xfers, you could enable debug LL and see who is queuing the transfers. > > Not sure now how these IRQ numbers get mapped these days, my debug print > says it's irq number 72 (UART1 from TRM) that is flooding (although it's > printed from the i2c-omap isr function, so it's still I2C bus irq...). Can you do a cat /proc/interrupts > > The irq enabling (in resume_noirq) is still slightly progressing after > the flooding starts, but my watchdog kicks in before we get to the > finish. > > Attached my debug prints patch and log. I used also "no_console_suspend" > boot option. > > - Kalle > -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
ma, 2012-10-15 kello 15:41 +0530, Shubhrajyoti Datta kirjoitti: > On Mon, Oct 15, 2012 at 2:46 PM, Kalle Jokiniemi > <kalle.jokiniemi@jollamobile.com> wrote: > > ma, 2012-10-15 kello 09:21 +0300, Kalle Jokiniemi kirjoitti: > >> Hi, > >> > >> pe, 2012-10-12 kello 14:46 +0000, Strashko, Grygorii kirjoitti: > >> > Hi Kevin, > >> > > >> > yep, [1] is the same fix - thanks. > >> > > >> > Hi Kalle, > >> > > >> > i've applied these changes and PM runtime fix on top of git://git.kernel.org/pub/scm/linux/kernel/git/tmlind/linux-omap.git (omap2plus_defconfig) > >> > Could you check it with your use case, pls? (just to be sure that idea is right) > >> > >> Odd, it's not working. I'll add some debug prints to see what happens > >> there. > > > > Well, seems after enabling irq 23 in the resume_noirq, someone does > > i2c_xfer and there is consequent flood of i2c_xfers and interrupts. > If there is continuous xfers, you could enable debug LL and see who is > queuing the > transfers. > > > > > Not sure now how these IRQ numbers get mapped these days, my debug print > > says it's irq number 72 (UART1 from TRM) that is flooding (although it's > > printed from the i2c-omap isr function, so it's still I2C bus irq...). > > Can you do a cat /proc/interrupts Yes :) [root@localhost proc]# cat interrupts CPU0 20: 0 INTC gpmc 23: 2 INTC TWL4030-PIH 25: 0 INTC l3-debug-irq 26: 0 INTC l3-app-irq 28: 48157 INTC DMA 40: 0 INTC omap-iommu.0 52: 0 INTC dsp_wdt 53: 807807 INTC gp_timer 65: 0 INTC omap-sham 72: 5490 INTC omap_i2c 73: 85 INTC omap_i2c 77: 0 INTC omap_i2c 90: 1069 INTC OMAP UART2 102: 55940 INTC mmc0 179: 6142 GPIO omap2-onenand 306: 44666 PRCM pm_wkup 315: 4 PRCM hwmod_io, pm_io 338: 0 twl4030 twl4030_gpio 343: 2 twl4030 twl4030_power 346: 0 twl4030 twl4030_pwrbutton 348: 2 twl4030 twl4030_usb 349: 0 twl4030 rtc0 Hmm, I did not notice that PIH handler before, makes sense now that it triggers the flood (irq 23) as it is really the one that passes the interrupts to other handlers. - Kalle > > > > > > The irq enabling (in resume_noirq) is still slightly progressing after > > the flooding starts, but my watchdog kicks in before we get to the > > finish. > > > > Attached my debug prints patch and log. I used also "no_console_suspend" > > boot option. > > > > - Kalle > > -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c index a0e49f6..cb09e20 100644 --- a/drivers/i2c/busses/i2c-omap.c +++ b/drivers/i2c/busses/i2c-omap.c @@ -586,6 +586,9 @@ omap_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num) if (IS_ERR_VALUE(r)) goto out; + /* We have the bus, enable IRQ */ + enable_irq(dev->irq); + r = omap_i2c_wait_for_bb(dev); if (r < 0) goto out; @@ -606,6 +609,7 @@ omap_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num) r = num; omap_i2c_wait_for_bb(dev); + disable_irq(dev->irq); out: pm_runtime_put(dev->dev); return r; @@ -1060,6 +1064,9 @@ omap_i2c_probe(struct platform_device *pdev) omap_i2c_isr; r = request_irq(dev->irq, isr, IRQF_NO_SUSPEND, pdev->name, dev); + /* We enable IRQ only when request for I2C from master */ + disable_irq(dev->irq); + if (r) { dev_err(dev->dev, "failure requesting irq %i\n", dev->irq); goto err_unuse_clocks; @@ -1182,7 +1189,23 @@ static int omap_i2c_runtime_resume(struct device *dev) } #endif /* CONFIG_PM_RUNTIME */ +static int omap_i2c_suspend(struct device *dev) +{ + int ret; + + /* + * Enable I2C device, so it will be accessible during + * later stages of suspending when device Runtime PM is disabled. + * I2C device will be turned off at "noirq" suspend stage. + */ + ret = pm_runtime_resume(dev); + if (ret < 0) + return ret; + return 0; +} + static struct dev_pm_ops omap_i2c_pm_ops = { + SET_SYSTEM_SLEEP_PM_OPS(omap_i2c_suspend, NULL) SET_RUNTIME_PM_OPS(omap_i2c_runtime_suspend, omap_i2c_runtime_resume, NULL) };