| Message ID | 1334842101-20670-1-git-send-email-shubhrajyoti@ti.com |
|---|---|
| State | New |
| Headers | show |
On Thu, Apr 19, 2012 at 06:58:13PM +0530, Shubhrajyoti D wrote: > Currently the i2c driver calls the pm_runtime_enable and never > the disable. This may cause a warning when pm_runtime_enable > checks for the count match.Attempting to fix the same by calling > pm_runtime_disable in the error and the remove path. Why "attempting"? > > Cc: Kevin Hilman <khilman@ti.com> > Cc: Rajendra Nayak <rnayak@ti.com> > Signed-off-by: Shubhrajyoti D <shubhrajyoti@ti.com> > --- > 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 4f4188d..c851672 100644 > --- a/drivers/i2c/busses/i2c-omap.c > +++ b/drivers/i2c/busses/i2c-omap.c > @@ -1090,6 +1090,7 @@ err_unuse_clocks: > omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, 0); > pm_runtime_put(dev->dev); > iounmap(dev->base); > + pm_runtime_disable(&pdev->dev); > err_free_mem: > platform_set_drvdata(pdev, NULL); > kfree(dev); > @@ -1110,6 +1111,7 @@ omap_i2c_remove(struct platform_device *pdev) > free_irq(dev->irq, dev); > i2c_del_adapter(&dev->adapter); > omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, 0); > + pm_runtime_disable(&pdev->dev); > iounmap(dev->base); > kfree(dev); > mem = platform_get_resource(pdev, IORESOURCE_MEM, 0); > -- > 1.7.5.4 >
On Thu, Apr 19, 2012 at 06:58:18PM +0530, Shubhrajyoti D wrote: > If PM runtime get_sync fails return with the error > so that no further reads/writes goes through the interface. > This will avoid possible abort. Add a error message in case > of failure with the cause of the failure. I don't think the error message is especially helpful. You also use different string (probably typo). > > Signed-off-by: Shubhrajyoti D <shubhrajyoti@ti.com> > --- > drivers/i2c/busses/i2c-omap.c | 19 ++++++++++++++++--- > 1 files changed, 16 insertions(+), 3 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c > index 44e8cfa..d555dcd 100644 > --- a/drivers/i2c/busses/i2c-omap.c > +++ b/drivers/i2c/busses/i2c-omap.c > @@ -585,7 +585,11 @@ omap_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num) > int i; > int r; > > - pm_runtime_get_sync(dev->dev); > + r = pm_runtime_get_sync(dev->dev); > + if (r < 0) { > + dev_err(dev->dev, "pm_runtime_get_sync failed %d\n", r); > + return r; > + } > > r = omap_i2c_wait_for_bb(dev); > if (r < 0) > @@ -1011,7 +1015,11 @@ omap_i2c_probe(struct platform_device *pdev) > dev->regs = (u8 *)reg_map_ip_v1; > > pm_runtime_enable(dev->dev); > - pm_runtime_get_sync(dev->dev); > + r = pm_runtime_get_sync(dev->dev); > + if (r < 0) { > + dev_err(dev->dev, "pm_runtime_get_sync failed:%d\n", r); > + return r; > + } Smatch says: drivers/i2c/busses/i2c-omap.c:1021 omap_i2c_probe() warn: 'mem->start' was not released on error In fact, you are leaking quite more. > @@ -1103,12 +1111,17 @@ omap_i2c_remove(struct platform_device *pdev) > { > struct omap_i2c_dev *dev = platform_get_drvdata(pdev); > struct resource *mem; > + int ret; > > platform_set_drvdata(pdev, NULL); > > free_irq(dev->irq, dev); > i2c_del_adapter(&dev->adapter); > - pm_runtime_get_sync(&pdev->dev); > + ret = pm_runtime_get_sync(&pdev->dev); > + if (ret < 0) { > + dev_err(dev->dev, "pm_runtime_get_sync failed %d\n", ret); > + return ret; > + } I am no too familiar with runtime_pm. Is it really so bad to fail remove, when get_sync has an error? Why is it checked and e.g. pm_runtime_put later is not? Any pointers? > omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, 0); > pm_runtime_put(&pdev->dev); > pm_runtime_disable(&pdev->dev); > -- > 1.7.5.4 > Thanks, Wolfram
On Thu, Apr 19, 2012 at 06:58:20PM +0530, Shubhrajyoti D wrote: > Currently in the 1.153 errata handling while waiting for transmitter > underflow if NACK is got the XUDF flag is also set. > The flag is set after wait for the condition is over. What does XUDF mean? Please update the commit description, I can't know all terminology for all architectures.
On Thu, Apr 19, 2012 at 06:58:19PM +0530, Shubhrajyoti D wrote: > From: Tasslehoff Kjappfot <tasskjapp@gmail.com> > > i2c_probe set the dev->errata flag, but omap_i2c_init cleared the flag again. > Move the errata handling to i2c_probe. > > Signed-off-by: Tasslehoff Kjappfot <tasskjapp@gmail.com> > Signed-off-by: Shubhrajyoti D <shubhrajyoti@ti.com> Subject looks bogus to me. You are handling I207 here. > --- > drivers/i2c/busses/i2c-omap.c | 10 +++++----- > 1 files changed, 5 insertions(+), 5 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c > index d555dcd..9323201 100644 > --- a/drivers/i2c/busses/i2c-omap.c > +++ b/drivers/i2c/busses/i2c-omap.c > @@ -427,11 +427,6 @@ static int omap_i2c_init(struct omap_i2c_dev *dev) > /* Take the I2C module out of reset: */ > omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, OMAP_I2C_CON_EN); > > - dev->errata = 0; > - > - if (dev->flags & OMAP_I2C_FLAG_APPLY_ERRATA_I207) > - dev->errata |= I2C_OMAP_ERRATA_I207; > - > /* Enable interrupts */ > dev->iestate = (OMAP_I2C_IE_XRDY | OMAP_I2C_IE_RRDY | > OMAP_I2C_IE_ARDY | OMAP_I2C_IE_NACK | > @@ -1023,6 +1018,11 @@ omap_i2c_probe(struct platform_device *pdev) > > dev->rev = omap_i2c_read_reg(dev, OMAP_I2C_REV_REG) & 0xff; > > + dev->errata = 0; > + > + if (dev->flags & OMAP_I2C_FLAG_APPLY_ERRATA_I207) > + dev->errata |= I2C_OMAP_ERRATA_I207; > + > if (dev->rev <= OMAP_I2C_REV_ON_3430) > dev->errata |= I2C_OMAP3_1P153; > > -- > 1.7.5.4 >
On Thu, Apr 19, 2012 at 06:58:17PM +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] [<c0013078>] (dump_backtrace+0x0/0x110) from [<c026c158>] (dump_stack+0x18/0x1c) > [ 154.752716] r6:00000070 r5:c002c43c r4:df9b9e98 r3:df9b8000 > [ 154.764465] [<c026c140>] (dump_stack+0x0/0x1c) from [<c0041a2c>] (warn_slowpath_common+0x5c/0x6c) > [ 154.768341] [<c00419d0>] (warn_slowpath_common+0x0/0x6c) from [<c0041ae0>] (warn_slowpath_fmt+0x38/0x40) > [ 154.776153] r8:00000180 r7:c0361594 r6:c0379b48 r5:00080003 r4:e0838b00 > [ 154.790771] r3:00000009 > [ 154.791778] [<c0041aa8>] (warn_slowpath_fmt+0x0/0x40) from [<c002c43c>] (l3_interrupt_handler+0x1b4/0x1c4) > [ 154.803710] r3:c0361598 r2:c02ef74c > [ 154.807403] [<c002c288>] (l3_interrupt_handler+0x0/0x1c4) from [<c0085f44>] (handle_irq_event_percpu+0x58/0 > [ 154.818237] r8:0000002a r7:00000000 r6:00000000 r5:df808054 r4:df8893c0 > [ 154.825378] [<c0085eec>] (handle_irq_event_percpu+0x0/0x188) from [<c00860b8>] (handle_irq_event+0x44/0x64) > [ 154.835662] [<c0086074>] (handle_irq_event+0x0/0x64) from [<c0088ec0>] (handle_fasteoi_irq+0xa4/0x10c) > [ 154.845458] r6:0000002a r5:df808054 r4:df808000 r3:c034a150 > [ 154.846466] [<c0088e1c>] (handle_fasteoi_irq+0x0/0x10c) from [<c0085ed0>] (generic_handle_irq+0x30/0x38) > [ 154.854278] r5:c034aa48 r4:0000002a > [ 154.862091] [<c0085ea0>] (generic_handle_irq+0x0/0x38) from [<c000fd38>] (handle_IRQ+0x60/0xc0) > [ 154.874450] r4:c034ea70 r3:000001f8 > [ 154.878234] [<c000fcd8>] (handle_IRQ+0x0/0xc0) from [<c0008478>] (gic_handle_irq+0x20/0x5c) > [ 154.887023] r7:ffffff40 r6:df9b9fb0 r5:c034e2b4 r4:0000001a > [ 154.887054] [<c0008458>] (gic_handle_irq+0x0/0x5c) from [<c000ea80>] (__irq_usr+0x40/0x60) > [ 154.901153] Exception stack(0xdf9b9fb0 to 0xdf9b9ff8) > [ 154.907104] 9fa0: beaf1f04 4006be00 0000000f 0000000c > [ 154.915710] 9fc0: 4006c000 00000000 00008034 ffffff40 00000007 00000000 00000000 0007b8d7 > [ 154.916778] 9fe0: 00000000 beaf1b68 0000d23c 4005baf0 80000010 ffffffff > [ 154.931335] r6:ffffffff r5:80000010 r4:4005baf0 r3:beaf1f04 > [ 154.937316] ---[ end trace 1b75b31a2719ed21 ]-- > > Cc: <stable@vger.kernel.org> > Signed-off-by: Shubhrajyoti D <shubhrajyoti@ti.com> Is this really the correct solution? I do wonder that every driver using runtime PM should enable the clocks on their own. That should be done by the core, I'd say; it is not unusual that drivers need to write to registers in remove(). If it is correct, can I get some acks? > --- > 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 >
On Thu, Apr 19, 2012 at 06:58:11PM +0530, Shubhrajyoti D wrote: > This patch series seperates the fixes from other > changes from [2] I also noticed this one in my build. Can you check? WARNING: modpost: Found 1 section mismatch(es).
The subject is too generic. (I don't want to be negative always: The following description of the problem is good :)) On Thu, Apr 19, 2012 at 06:58:15PM +0530, Shubhrajyoti D wrote: > Currently in probe > pm_runtime_put(dev->dev); Indentation wrong. > > ... > /* i2c device drivers may be active on return from add_adapter() */ > adap->nr = pdev->id; > r = i2c_add_numbered_adapter(adap); > if (r) { > dev_err(dev->dev, "failure adding adapter\n"); > goto err_free_irq; > } > ... > > return 0; > > err_free_irq: > free_irq(dev->irq, dev); > err_unuse_clocks: > omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, 0); > pm_runtime_put(dev->dev); > > This may access the i2c registers without the clocks. > Attempting to fix the same by moving the pm_rintime_put after the error check. Drop "Attempting". > > Signed-off-by: Shubhrajyoti D <shubhrajyoti@ti.com> > --- > drivers/i2c/busses/i2c-omap.c | 4 ++-- > 1 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c > index bf07ffd..1777d79 100644 > --- a/drivers/i2c/busses/i2c-omap.c > +++ b/drivers/i2c/busses/i2c-omap.c > @@ -1061,8 +1061,6 @@ omap_i2c_probe(struct platform_device *pdev) > dev_info(dev->dev, "bus %d rev%d.%d.%d at %d kHz\n", pdev->id, > dev->dtrev, dev->rev >> 4, dev->rev & 0xf, dev->speed); > > - pm_runtime_put(dev->dev); > - > adap = &dev->adapter; > i2c_set_adapdata(adap, dev); > adap->owner = THIS_MODULE; > @@ -1082,6 +1080,8 @@ omap_i2c_probe(struct platform_device *pdev) > > of_i2c_register_devices(adap); > > + pm_runtime_put(dev->dev); > + > return 0; > > err_free_irq: > -- > 1.7.5.4 >
On Thu, Apr 19, 2012 at 06:58:16PM +0530, Shubhrajyoti D wrote: > By definition, wait_for_completion_timeout() returns an unsigned value and > therefore, it is not necessary to check if the return value is less than zero > as this is not possible. > > This is based on a patch from Jon Hunter <jon-hunter@ti.com> > Changes from his patch > - Declare a long as the wait_for_completion_timeout returns long. > > Original patch is > http://git.omapzoom.org/?p=kernel/omap.git;a=commitdiff;h=ea02cece7b0000bc736e60c4188a11aaa74bc6e6 The original patch has a signed-off, so you need to include it here as well. > > Cc : Jon Hunter <jon-hunter@ti.com> > Signed-off-by: Shubhrajyoti D <shubhrajyoti@ti.com> > --- > drivers/i2c/busses/i2c-omap.c | 10 ++++------ > 1 files changed, 4 insertions(+), 6 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c > index 1777d79..fec8d5c 100644 > --- a/drivers/i2c/busses/i2c-omap.c > +++ b/drivers/i2c/busses/i2c-omap.c > @@ -473,7 +473,7 @@ static int omap_i2c_xfer_msg(struct i2c_adapter *adap, > struct i2c_msg *msg, int stop) > { > struct omap_i2c_dev *dev = i2c_get_adapdata(adap); > - int r; > + unsigned long timeout; > u16 w; > > dev_dbg(dev->dev, "addr: 0x%04x, len: %d, flags: 0x%x, stop: %d\n", > @@ -541,12 +541,10 @@ static int omap_i2c_xfer_msg(struct i2c_adapter *adap, > * REVISIT: We should abort the transfer on signals, but the bus goes > * into arbitration and we're currently unable to recover from it. > */ > - r = wait_for_completion_timeout(&dev->cmd_complete, > - OMAP_I2C_TIMEOUT); > + timeout = wait_for_completion_timeout(&dev->cmd_complete, > + OMAP_I2C_TIMEOUT); > dev->buf_len = 0; > - if (r < 0) > - return r; > - if (r == 0) { > + if (timeout == 0) { > dev_err(dev->dev, "controller timed out\n"); > omap_i2c_init(dev); > return -ETIMEDOUT; > -- > 1.7.5.4 >
On Monday 23 April 2012 09:50 PM, Wolfram Sang wrote: > On Thu, Apr 19, 2012 at 06:58:18PM +0530, Shubhrajyoti D wrote: >> If PM runtime get_sync fails return with the error >> so that no further reads/writes goes through the interface. >> This will avoid possible abort. Add a error message in case >> of failure with the cause of the failure. > I don't think the error message is especially helpful. You also use different > string (probably typo). Will correct and resend > >> Signed-off-by: Shubhrajyoti D <shubhrajyoti@ti.com> >> --- >> drivers/i2c/busses/i2c-omap.c | 19 ++++++++++++++++--- >> 1 files changed, 16 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c >> index 44e8cfa..d555dcd 100644 >> --- a/drivers/i2c/busses/i2c-omap.c >> +++ b/drivers/i2c/busses/i2c-omap.c >> @@ -585,7 +585,11 @@ omap_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num) >> int i; >> int r; >> >> - pm_runtime_get_sync(dev->dev); >> + r = pm_runtime_get_sync(dev->dev); >> + if (r < 0) { >> + dev_err(dev->dev, "pm_runtime_get_sync failed %d\n", r); >> + return r; >> + } >> >> r = omap_i2c_wait_for_bb(dev); >> if (r < 0) >> @@ -1011,7 +1015,11 @@ omap_i2c_probe(struct platform_device *pdev) >> dev->regs = (u8 *)reg_map_ip_v1; >> >> pm_runtime_enable(dev->dev); >> - pm_runtime_get_sync(dev->dev); >> + r = pm_runtime_get_sync(dev->dev); >> + if (r < 0) { >> + dev_err(dev->dev, "pm_runtime_get_sync failed:%d\n", r); >> + return r; >> + } > Smatch says: > > drivers/i2c/busses/i2c-omap.c:1021 omap_i2c_probe() warn: 'mem->start' was not released on error > > In fact, you are leaking quite more. Ops will correct, thanks. > >> @@ -1103,12 +1111,17 @@ omap_i2c_remove(struct platform_device *pdev) >> { >> struct omap_i2c_dev *dev = platform_get_drvdata(pdev); >> struct resource *mem; >> + int ret; >> >> platform_set_drvdata(pdev, NULL); >> >> free_irq(dev->irq, dev); >> i2c_del_adapter(&dev->adapter); >> - pm_runtime_get_sync(&pdev->dev); >> + ret = pm_runtime_get_sync(&pdev->dev); >> + if (ret < 0) { >> + dev_err(dev->dev, "pm_runtime_get_sync failed %d\n", ret); >> + return ret; >> + } > I am no too familiar with runtime_pm. Is it really so bad to fail remove, when > get_sync has an error? Why is it checked and e.g. pm_runtime_put later is not? > Any pointers? If the get_sync fails the clock were not enabled so checking to prevent register access. > >> omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, 0); >> pm_runtime_put(&pdev->dev); >> pm_runtime_disable(&pdev->dev); >> -- >> 1.7.5.4 >> > Thanks, > > Wolfram >
On Monday 23 April 2012 10:12 PM, Wolfram Sang wrote: > On Thu, Apr 19, 2012 at 06:58:20PM +0530, Shubhrajyoti D wrote: >> Currently in the 1.153 errata handling while waiting for transmitter >> underflow if NACK is got the XUDF flag is also set. >> The flag is set after wait for the condition is over. > What does XUDF mean? Please update the commit description, I can't know > all terminology for all architectures. > Transmitter underflow ,will update the commit description and resend.
On Monday 23 April 2012 10:13 PM, Wolfram Sang wrote: > On Thu, Apr 19, 2012 at 06:58:19PM +0530, Shubhrajyoti D wrote: >> From: Tasslehoff Kjappfot <tasskjapp@gmail.com> >> >> i2c_probe set the dev->errata flag, but omap_i2c_init cleared the flag again. >> Move the errata handling to i2c_probe. >> >> Signed-off-by: Tasslehoff Kjappfot <tasskjapp@gmail.com> >> Signed-off-by: Shubhrajyoti D <shubhrajyoti@ti.com> > Subject looks bogus to me. You are handling I207 here. Actually probe set ip153 then i2c init cleared so moved all the errata handling in one place so that such errors are prevented. WIll update the changelogs and resend > >> --- >> drivers/i2c/busses/i2c-omap.c | 10 +++++----- >> 1 files changed, 5 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c >> index d555dcd..9323201 100644 >> --- a/drivers/i2c/busses/i2c-omap.c >> +++ b/drivers/i2c/busses/i2c-omap.c >> @@ -427,11 +427,6 @@ static int omap_i2c_init(struct omap_i2c_dev *dev) >> /* Take the I2C module out of reset: */ >> omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, OMAP_I2C_CON_EN); >> >> - dev->errata = 0; >> - >> - if (dev->flags & OMAP_I2C_FLAG_APPLY_ERRATA_I207) >> - dev->errata |= I2C_OMAP_ERRATA_I207; >> - >> /* Enable interrupts */ >> dev->iestate = (OMAP_I2C_IE_XRDY | OMAP_I2C_IE_RRDY | >> OMAP_I2C_IE_ARDY | OMAP_I2C_IE_NACK | >> @@ -1023,6 +1018,11 @@ omap_i2c_probe(struct platform_device *pdev) >> >> dev->rev = omap_i2c_read_reg(dev, OMAP_I2C_REV_REG) & 0xff; >> >> + dev->errata = 0; >> + >> + if (dev->flags & OMAP_I2C_FLAG_APPLY_ERRATA_I207) >> + dev->errata |= I2C_OMAP_ERRATA_I207; >> + >> if (dev->rev <= OMAP_I2C_REV_ON_3430) >> dev->errata |= I2C_OMAP3_1P153; >> >> -- >> 1.7.5.4 >>
On Monday 23 April 2012 10:35 PM, Wolfram Sang wrote: > On Thu, Apr 19, 2012 at 06:58:11PM +0530, Shubhrajyoti D wrote: >> This patch series seperates the fixes from other >> changes from [2] > I also noticed this one in my build. Can you check? > > WARNING: modpost: Found 1 section mismatch(es). I checked on the old branch didnt see the error anyways will rebase to the latest head and recheck . Thanks,
On Monday 23 April 2012 10:19 PM, Wolfram Sang wrote: >> [ 154.901153] Exception stack(0xdf9b9fb0 to 0xdf9b9ff8) >> > [ 154.907104] 9fa0: beaf1f04 4006be00 0000000f 0000000c >> > [ 154.915710] 9fc0: 4006c000 00000000 00008034 ffffff40 00000007 00000000 00000000 0007b8d7 >> > [ 154.916778] 9fe0: 00000000 beaf1b68 0000d23c 4005baf0 80000010 ffffffff >> > [ 154.931335] r6:ffffffff r5:80000010 r4:4005baf0 r3:beaf1f04 >> > [ 154.937316] ---[ end trace 1b75b31a2719ed21 ]-- >> > >> > Cc: <stable@vger.kernel.org> >> > Signed-off-by: Shubhrajyoti D <shubhrajyoti@ti.com> > Is this really the correct solution? I do wonder that every driver using > runtime PM should enable the clocks on their own. That should be done by > the core, By core you don't mean the i2c core but the pm layer right? > I'd say; it is not unusual that drivers need to write to > registers in remove(). If it is correct, can I get some acks? I did see the crash. Will wait for the pm experts to comment. >> > --- >> > drivers/i2c/busses/i2c-omap.c | 2 ++ >> > 1 files changed, 2 insertions(+), 0 deletions(-) >> >
On Tue, Apr 24, 2012 at 11:44:15PM +0530, Shubhrajyoti wrote: > On Monday 23 April 2012 10:19 PM, Wolfram Sang wrote: > >> [ 154.901153] Exception stack(0xdf9b9fb0 to 0xdf9b9ff8) > >> > [ 154.907104] 9fa0: beaf1f04 4006be00 0000000f 0000000c > >> > [ 154.915710] 9fc0: 4006c000 00000000 00008034 ffffff40 00000007 00000000 00000000 0007b8d7 > >> > [ 154.916778] 9fe0: 00000000 beaf1b68 0000d23c 4005baf0 80000010 ffffffff > >> > [ 154.931335] r6:ffffffff r5:80000010 r4:4005baf0 r3:beaf1f04 > >> > [ 154.937316] ---[ end trace 1b75b31a2719ed21 ]-- > >> > > >> > Cc: <stable@vger.kernel.org> > >> > Signed-off-by: Shubhrajyoti D <shubhrajyoti@ti.com> > > Is this really the correct solution? I do wonder that every driver using > > runtime PM should enable the clocks on their own. That should be done by > > the core, > By core you don't mean the i2c core but the pm layer right? Yes. > > I'd say; it is not unusual that drivers need to write to > > registers in remove(). If it is correct, can I get some acks? > I did see the crash. That was never a doubt. With "correct" I meant "correct solution". > Will wait for the pm experts to comment. Yup.
On Tuesday 24 April 2012 11:48 PM, Wolfram Sang wrote: > On Tue, Apr 24, 2012 at 11:44:15PM +0530, Shubhrajyoti wrote: >> On Monday 23 April 2012 10:19 PM, Wolfram Sang wrote: >>>> [ 154.901153] Exception stack(0xdf9b9fb0 to 0xdf9b9ff8) >>>>> [ 154.907104] 9fa0: beaf1f04 4006be00 0000000f 0000000c >>>>> [ 154.915710] 9fc0: 4006c000 00000000 00008034 ffffff40 00000007 00000000 00000000 0007b8d7 >>>>> [ 154.916778] 9fe0: 00000000 beaf1b68 0000d23c 4005baf0 80000010 ffffffff >>>>> [ 154.931335] r6:ffffffff r5:80000010 r4:4005baf0 r3:beaf1f04 >>>>> [ 154.937316] ---[ end trace 1b75b31a2719ed21 ]-- >>>>> >>>>> Cc: <stable@vger.kernel.org> >>>>> Signed-off-by: Shubhrajyoti D <shubhrajyoti@ti.com> >>> Is this really the correct solution? I do wonder that every driver using >>> runtime PM should enable the clocks on their own. That should be done by >>> the core, >> By core you don't mean the i2c core but the pm layer right? > Yes. > >>> I'd say; it is not unusual that drivers need to write to >>> registers in remove(). If it is correct, can I get some acks? >> I did see the crash. > That was never a doubt. With "correct" I meant "correct solution". > >> Will wait for the pm experts to comment. As far as I know I don’t think that the pm layer doesn't enable the clocks for the drivers on remove. Maybe Kevin or Rajendra can comment better.
On Thursday 26 April 2012 11:58 AM, Shubhrajyoti wrote: > On Tuesday 24 April 2012 11:48 PM, Wolfram Sang wrote: >> On Tue, Apr 24, 2012 at 11:44:15PM +0530, Shubhrajyoti wrote: >>> On Monday 23 April 2012 10:19 PM, Wolfram Sang wrote: >>>>> [ 154.901153] Exception stack(0xdf9b9fb0 to 0xdf9b9ff8) >>>>>> [ 154.907104] 9fa0: beaf1f04 4006be00 0000000f 0000000c >>>>>> [ 154.915710] 9fc0: 4006c000 00000000 00008034 ffffff40 00000007 00000000 00000000 0007b8d7 >>>>>> [ 154.916778] 9fe0: 00000000 beaf1b68 0000d23c 4005baf0 80000010 ffffffff >>>>>> [ 154.931335] r6:ffffffff r5:80000010 r4:4005baf0 r3:beaf1f04 >>>>>> [ 154.937316] ---[ end trace 1b75b31a2719ed21 ]-- >>>>>> >>>>>> Cc: <stable@vger.kernel.org> >>>>>> Signed-off-by: Shubhrajyoti D <shubhrajyoti@ti.com> >>>> Is this really the correct solution? I do wonder that every driver using >>>> runtime PM should enable the clocks on their own. That should be done by >>>> the core, >>> By core you don't mean the i2c core but the pm layer right? >> Yes. >> >>>> I'd say; it is not unusual that drivers need to write to >>>> registers in remove(). If it is correct, can I get some acks? >>> I did see the crash. >> That was never a doubt. With "correct" I meant "correct solution". >> >>> Will wait for the pm experts to comment. > As far as I know I don’t think that the pm layer doesn't enable the clocks > for the drivers on remove. Maybe Kevin or Rajendra can comment better. typo what i meant was As far as I know the pm layer doesn't enable the clocks for the drivers on remove. Maybe Kevin or Rajendra can comment better. thanks, > >
This patch series seperates the fixes from other changes from [2] [2] http://www.spinics.net/lists/linux-omap/msg68056.html The patch series does the following - Warn fixes if CONFIG_PM_RUNTIME is not selected. - In case of i2c remove register access was done without any get_sync fix the same. - Folds a patch from Tasslehoff to prevent any merge conflicts. - Prevents the XDUF flag to be set if the underflow condition is not met. - As per discussion in [1] .Adds a patch to rename the 1p153 errata and use the unique id instead as the section number in the recent errata docs has changed. [1] http://www.spinics.net/lists/linux-i2c/msg07607.html Tested on omap4sdp and omap3sdp. The following changes since commit 592fe8980688e7cba46897685d014c7fb3018a67: Merge tag 'ext4_for_linus' of git://git.kernel.org/pub/scm/linux/kernel/git/tytso/ext4 (2012-04-17 13:30:34 -0700) are available in the git repository at: git://gitorious.org/linus-tree/linus-tree.git i2c_omap-fixes Shubhrajyoti D (9): I2C: OMAP: make omap_i2c_unidle/idle functions depend on CONFIG_PM_RUNTIME I2C: OMAP: Fix the mismatch of pm_runtime enable and disable I2C: OMAP: Fix the interrupt clearing in OMAP4 I2C: OMAP: Fix the error handling I2C: OMAP: Don't check if wait_for_completion_timeout() returns less than zero I2C: OMAP: Fix the crash in i2c remove I2C: OMAP: Handle error check for pm runtime I2C: OMAP: Do not set the XUDF if the underflow is not reached I2C: OMAP: Rename the 1p153 to the erratum id i462 Tasslehoff Kjappfot (1): I2C: OMAP: fix missing handling of errata I2C_OMAP3_1P153 drivers/i2c/busses/i2c-omap.c | 132 +++++++++++++++++++++-------------------- 1 files changed, 67 insertions(+), 65 deletions(-)