Re: [PATCHv9 06/10] I2C: OMAP: Fix the crash in i2c remove
Hi Wolfram, Wolfram Sang writes: > On Wed, May 02, 2012 at 08:02:11PM +0530, Shubhrajyoti D wrote: >> In omap_i2c_remove we are accessing the I2C_CON register without >> enabling the clocks. Fix the same by enabling the clocks and disabling >> it. [...] > I'd really like a comment from the PM experts if each and every driver > has to ensure that the clocks are enabled on remove like this? Yes, this is correct. In fact, this is the goal of runtime PM. The driver itself tells the PM core (using runtime PM) when the device needs to be accessible and when it doesn't. Technically speaking, the it's up to the platform-specific runtime PM implementation to decide whether or not the clocks are actually disable or not (e.g. due to wakeup latency requirements, it might decide not to cut clocks.) Because of that, the changelog should be reworded to say something like "ensure device is accessible" instead of "enable the clocks", because the runtime PM implementation does more than just manage clocks. Kevin P.S. It's great to see you helping out maintaining i2c drivers. Thanks! P.P.S. Before you merge this, I would strongly recommend we wait for a few more Tested-bys, and a bit more description about how this was tested. We've been having quite a few problems with regressions introduced in OMAP drivers that have not been well reviewed or tested. -- 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: [PATCHv9 06/10] I2C: OMAP: Fix the crash in i2c remove
On Saturday 12 May 2012 11:40 PM, Wolfram Sang wrote:
>> Cc: Kevin Hilman
>> > Cc: Rajendra Nayak
>> > Signed-off-by: Shubhrajyoti D
> I'd really like a comment from the PM experts if each and every driver
> has to ensure that the clocks are enabled on remove like this?
Just resent cc ing linux-pm
BTW also found that
Some others are also doing the same. eg:
drivers/mmc/host/omap_hsmmc.c
static int __devexit omap_hsmmc_remove(struct platform_device *pdev)
{
struct omap_hsmmc_host *host = platform_get_drvdata(pdev);
struct resource *res;
pm_runtime_get_sync(host->dev);
mmc_remove_host(host->mmc);
2. drivers/usb/musb/musb_core.c
static void musb_shutdown(struct platform_device *pdev)
{
struct musb *musb = dev_to_musb(&pdev->dev);
unsigned long flags;
pm_runtime_get_sync(musb->controller);
Anyways will wait for feedback.
>
>> > ---
>> > drivers/i2c/busses/i2c-omap.c |
--
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: [PATCHv9 06/10] I2C: OMAP: Fix the crash in i2c remove
On Wed, May 02, 2012 at 08:02:11PM +0530, Shubhrajyoti D wrote: > In omap_i2c_remove we are accessing the I2C_CON register without > enabling the clocks. Fix the same by enabling the clocks and disabling > it. > This fixes the following crash. > [ 154.723022] [ cut here ] > [ 154.725677] WARNING: at arch/arm/mach-omap2/omap_l3_noc.c:112 > l3_interrupt_handler+0x1b4/0x1c4() > [ 154.725677] L3 custom error: MASTER:MPU TARGET:L4 PER2 > [ 154.742614] Modules linked in: i2c_omap(-) > [ 154.746948] Backtrace: > [ 154.746948] [] (dump_backtrace+0x0/0x110) from [] > (dump_stack+0x18/0x1c) > [ 154.752716] r6:0070 r5:c002c43c r4:df9b9e98 r3:df9b8000 > [ 154.764465] [] (dump_stack+0x0/0x1c) from [] > (warn_slowpath_common+0x5c/0x6c) > [ 154.768341] [] (warn_slowpath_common+0x0/0x6c) from > [] (warn_slowpath_fmt+0x38/0x40) > [ 154.776153] r8:0180 r7:c0361594 r6:c0379b48 r5:00080003 > r4:e0838b00 > [ 154.790771] r3:0009 > [ 154.791778] [] (warn_slowpath_fmt+0x0/0x40) from > [] (l3_interrupt_handler+0x1b4/0x1c4) > [ 154.803710] r3:c0361598 r2:c02ef74c > [ 154.807403] [] (l3_interrupt_handler+0x0/0x1c4) from > [] (handle_irq_event_percpu+0x58/0 > [ 154.818237] r8:002a r7: r6: r5:df808054 > r4:df8893c0 > [ 154.825378] [] (handle_irq_event_percpu+0x0/0x188) from > [] (handle_irq_event+0x44/0x64) > [ 154.835662] [] (handle_irq_event+0x0/0x64) from [] > (handle_fasteoi_irq+0xa4/0x10c) > [ 154.845458] r6:002a r5:df808054 r4:df808000 r3:c034a150 > [ 154.846466] [] (handle_fasteoi_irq+0x0/0x10c) from > [] (generic_handle_irq+0x30/0x38) > [ 154.854278] r5:c034aa48 r4:002a > [ 154.862091] [] (generic_handle_irq+0x0/0x38) from > [] (handle_IRQ+0x60/0xc0) > [ 154.874450] r4:c034ea70 r3:01f8 > [ 154.878234] [] (handle_IRQ+0x0/0xc0) from [] > (gic_handle_irq+0x20/0x5c) > [ 154.887023] r7:ff40 r6:df9b9fb0 r5:c034e2b4 r4:001a > [ 154.887054] [] (gic_handle_irq+0x0/0x5c) from [] > (__irq_usr+0x40/0x60) > [ 154.901153] Exception stack(0xdf9b9fb0 to 0xdf9b9ff8) > [ 154.907104] 9fa0: beaf1f04 > 4006be00 000f 000c > [ 154.915710] 9fc0: 4006c000 8034 ff40 0007 > 0007b8d7 > [ 154.916778] 9fe0: beaf1b68 d23c 4005baf0 8010 > [ 154.931335] r6: r5:8010 r4:4005baf0 r3:beaf1f04 > [ 154.937316] ---[ end trace 1b75b31a2719ed21 ]-- > > Cc: Kevin Hilman > Cc: Rajendra Nayak > Signed-off-by: Shubhrajyoti D I'd really like a comment from the PM experts if each and every driver has to ensure that the clocks are enabled on remove like this? > --- > drivers/i2c/busses/i2c-omap.c |2 ++ > 1 files changed, 2 insertions(+), 0 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c > index fec8d5c..44e8cfa 100644 > --- a/drivers/i2c/busses/i2c-omap.c > +++ b/drivers/i2c/busses/i2c-omap.c > @@ -1108,7 +1108,9 @@ omap_i2c_remove(struct platform_device *pdev) > > free_irq(dev->irq, dev); > i2c_del_adapter(&dev->adapter); > + pm_runtime_get_sync(&pdev->dev); > omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, 0); > + pm_runtime_put(&pdev->dev); > pm_runtime_disable(&pdev->dev); > iounmap(dev->base); > kfree(dev); > -- > 1.7.5.4 > -- Pengutronix e.K. | Wolfram Sang| Industrial Linux Solutions | http://www.pengutronix.de/ | signature.asc Description: Digital signature
[PATCHv9 06/10] I2C: OMAP: Fix the crash in i2c remove
In omap_i2c_remove we are accessing the I2C_CON register without enabling the clocks. Fix the same by enabling the clocks and disabling it. This fixes the following crash. [ 154.723022] [ cut here ] [ 154.725677] WARNING: at arch/arm/mach-omap2/omap_l3_noc.c:112 l3_interrupt_handler+0x1b4/0x1c4() [ 154.725677] L3 custom error: MASTER:MPU TARGET:L4 PER2 [ 154.742614] Modules linked in: i2c_omap(-) [ 154.746948] Backtrace: [ 154.746948] [] (dump_backtrace+0x0/0x110) from [] (dump_stack+0x18/0x1c) [ 154.752716] r6:0070 r5:c002c43c r4:df9b9e98 r3:df9b8000 [ 154.764465] [] (dump_stack+0x0/0x1c) from [] (warn_slowpath_common+0x5c/0x6c) [ 154.768341] [] (warn_slowpath_common+0x0/0x6c) from [] (warn_slowpath_fmt+0x38/0x40) [ 154.776153] r8:0180 r7:c0361594 r6:c0379b48 r5:00080003 r4:e0838b00 [ 154.790771] r3:0009 [ 154.791778] [] (warn_slowpath_fmt+0x0/0x40) from [] (l3_interrupt_handler+0x1b4/0x1c4) [ 154.803710] r3:c0361598 r2:c02ef74c [ 154.807403] [] (l3_interrupt_handler+0x0/0x1c4) from [] (handle_irq_event_percpu+0x58/0 [ 154.818237] r8:002a r7: r6: r5:df808054 r4:df8893c0 [ 154.825378] [] (handle_irq_event_percpu+0x0/0x188) from [] (handle_irq_event+0x44/0x64) [ 154.835662] [] (handle_irq_event+0x0/0x64) from [] (handle_fasteoi_irq+0xa4/0x10c) [ 154.845458] r6:002a r5:df808054 r4:df808000 r3:c034a150 [ 154.846466] [] (handle_fasteoi_irq+0x0/0x10c) from [] (generic_handle_irq+0x30/0x38) [ 154.854278] r5:c034aa48 r4:002a [ 154.862091] [] (generic_handle_irq+0x0/0x38) from [] (handle_IRQ+0x60/0xc0) [ 154.874450] r4:c034ea70 r3:01f8 [ 154.878234] [] (handle_IRQ+0x0/0xc0) from [] (gic_handle_irq+0x20/0x5c) [ 154.887023] r7:ff40 r6:df9b9fb0 r5:c034e2b4 r4:001a [ 154.887054] [] (gic_handle_irq+0x0/0x5c) from [] (__irq_usr+0x40/0x60) [ 154.901153] Exception stack(0xdf9b9fb0 to 0xdf9b9ff8) [ 154.907104] 9fa0: beaf1f04 4006be00 000f 000c [ 154.915710] 9fc0: 4006c000 8034 ff40 0007 0007b8d7 [ 154.916778] 9fe0: beaf1b68 d23c 4005baf0 8010 [ 154.931335] r6: r5:8010 r4:4005baf0 r3:beaf1f04 [ 154.937316] ---[ end trace 1b75b31a2719ed21 ]-- Cc: Kevin Hilman Cc: Rajendra Nayak Signed-off-by: Shubhrajyoti D --- drivers/i2c/busses/i2c-omap.c |2 ++ 1 files changed, 2 insertions(+), 0 deletions(-) diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c index fec8d5c..44e8cfa 100644 --- a/drivers/i2c/busses/i2c-omap.c +++ b/drivers/i2c/busses/i2c-omap.c @@ -1108,7 +1108,9 @@ omap_i2c_remove(struct platform_device *pdev) free_irq(dev->irq, dev); i2c_del_adapter(&dev->adapter); + pm_runtime_get_sync(&pdev->dev); omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, 0); + pm_runtime_put(&pdev->dev); pm_runtime_disable(&pdev->dev); iounmap(dev->base); kfree(dev); -- 1.7.5.4 -- 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
