| Message ID | 20150224034036.31400.6133.stgit@notabene.brown (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
* NeilBrown <neilb@suse.de> [150223 19:45]: > A construct like: > > if (pm_runtime_suspended(twl->dev)) > pm_runtime_get_sync(twl->dev); > > is against the spirit of the runtime_pm interface as it > makes the internal refcounting useless. > > In this case it is also racy, particularly as 'put_autosuspend' > is use to drop a reference. > When that happens a timer is started and the device is > runtime-suspended after the timeout. > If the above code runs in this window, the device will not be > found to be suspended so no pm_runtime reference is taken. > When the timer expires the device will be suspended, which is > against the intention of the code. > > So be more direct is taking and dropping references. > If twl->linkstat is VBUS_VALID or ID_GROUND, then hold a > pm_runtime reference, otherwise don't. Looks good to me, thanks for fixing it. I've tested this series on beagleboard-xm by plugging and unplugging devices and switching between host and peripheral mode, things still idle properly for off-idle. So please feel free to add: Tested-by: Tony Lindgren <tony@atomide.com> > Signed-off-by: NeilBrown <neilb@suse.de> > --- > drivers/phy/phy-twl4030-usb.c | 20 +++++++++++++------- > 1 file changed, 13 insertions(+), 7 deletions(-) > > diff --git a/drivers/phy/phy-twl4030-usb.c b/drivers/phy/phy-twl4030-usb.c > index 8e87f54671f3..97c59074233f 100644 > --- a/drivers/phy/phy-twl4030-usb.c > +++ b/drivers/phy/phy-twl4030-usb.c > @@ -536,8 +536,13 @@ static irqreturn_t twl4030_usb_irq(int irq, void *_twl) > > mutex_lock(&twl->lock); > if (status >= 0 && status != twl->linkstat) { > + status_changed = > + (twl->linkstat == OMAP_MUSB_VBUS_VALID || > + twl->linkstat == OMAP_MUSB_ID_GROUND) > + != > + (status == OMAP_MUSB_VBUS_VALID || > + status == OMAP_MUSB_ID_GROUND); > twl->linkstat = status; > - status_changed = true; > } > mutex_unlock(&twl->lock); > > @@ -555,13 +560,10 @@ static irqreturn_t twl4030_usb_irq(int irq, void *_twl) > */ > if ((status == OMAP_MUSB_VBUS_VALID) || > (status == OMAP_MUSB_ID_GROUND)) { > - if (pm_runtime_suspended(twl->dev)) > - pm_runtime_get_sync(twl->dev); > + pm_runtime_get_sync(twl->dev); > } else { > - if (pm_runtime_active(twl->dev)) { > - pm_runtime_mark_last_busy(twl->dev); > - pm_runtime_put_autosuspend(twl->dev); > - } > + pm_runtime_mark_last_busy(twl->dev); > + pm_runtime_put_autosuspend(twl->dev); > } > omap_musb_mailbox(status); > } > @@ -768,6 +770,10 @@ static int twl4030_usb_remove(struct platform_device *pdev) > > /* disable complete OTG block */ > twl4030_usb_clear_bits(twl, POWER_CTRL, POWER_CTRL_OTG_ENAB); > + > + if (twl->linkstat == OMAP_MUSB_VBUS_VALID || > + twl->linkstat == OMAP_MUSB_ID_GROUND) > + pm_runtime_put_noidle(twl->dev); > pm_runtime_mark_last_busy(twl->dev); > pm_runtime_put(twl->dev); > > > -- 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! > + status_changed = > + (twl->linkstat == OMAP_MUSB_VBUS_VALID || > + twl->linkstat == OMAP_MUSB_ID_GROUND) > + != > + (status == OMAP_MUSB_VBUS_VALID || > + status == OMAP_MUSB_ID_GROUND); > twl->linkstat = status; ... > @@ -768,6 +770,10 @@ static int twl4030_usb_remove(struct platform_device *pdev) > > /* disable complete OTG block */ > twl4030_usb_clear_bits(twl, POWER_CTRL, POWER_CTRL_OTG_ENAB); > + > + if (twl->linkstat == OMAP_MUSB_VBUS_VALID || > + twl->linkstat == OMAP_MUSB_ID_GROUND) > + pm_runtime_put_noidle(twl->dev); > pm_runtime_mark_last_busy(twl->dev); inline function returning (x == OMAP_MUSB_VBUS_VALID || x == OMAP_MUSB_ID_GROUND) would really help readability here. Thanks, Pavel
On Mon, 2 Mar 2015 22:03:59 +0100 Pavel Machek <pavel@ucw.cz> wrote: > Hi! > > > + status_changed = > > + (twl->linkstat == OMAP_MUSB_VBUS_VALID || > > + twl->linkstat == OMAP_MUSB_ID_GROUND) > > + != > > + (status == OMAP_MUSB_VBUS_VALID || > > + status == OMAP_MUSB_ID_GROUND); > > twl->linkstat = status; > ... > > @@ -768,6 +770,10 @@ static int twl4030_usb_remove(struct platform_device *pdev) > > > > /* disable complete OTG block */ > > twl4030_usb_clear_bits(twl, POWER_CTRL, POWER_CTRL_OTG_ENAB); > > + > > + if (twl->linkstat == OMAP_MUSB_VBUS_VALID || > > + twl->linkstat == OMAP_MUSB_ID_GROUND) > > + pm_runtime_put_noidle(twl->dev); > > pm_runtime_mark_last_busy(twl->dev); > > inline function returning (x == OMAP_MUSB_VBUS_VALID || x == > OMAP_MUSB_ID_GROUND) would really help readability here. > > Thanks, > Pavel Good idea. I've done that. The function is called "cable_present()". Thanks, NeilBrown
diff --git a/drivers/phy/phy-twl4030-usb.c b/drivers/phy/phy-twl4030-usb.c index 8e87f54671f3..97c59074233f 100644 --- a/drivers/phy/phy-twl4030-usb.c +++ b/drivers/phy/phy-twl4030-usb.c @@ -536,8 +536,13 @@ static irqreturn_t twl4030_usb_irq(int irq, void *_twl) mutex_lock(&twl->lock); if (status >= 0 && status != twl->linkstat) { + status_changed = + (twl->linkstat == OMAP_MUSB_VBUS_VALID || + twl->linkstat == OMAP_MUSB_ID_GROUND) + != + (status == OMAP_MUSB_VBUS_VALID || + status == OMAP_MUSB_ID_GROUND); twl->linkstat = status; - status_changed = true; } mutex_unlock(&twl->lock); @@ -555,13 +560,10 @@ static irqreturn_t twl4030_usb_irq(int irq, void *_twl) */ if ((status == OMAP_MUSB_VBUS_VALID) || (status == OMAP_MUSB_ID_GROUND)) { - if (pm_runtime_suspended(twl->dev)) - pm_runtime_get_sync(twl->dev); + pm_runtime_get_sync(twl->dev); } else { - if (pm_runtime_active(twl->dev)) { - pm_runtime_mark_last_busy(twl->dev); - pm_runtime_put_autosuspend(twl->dev); - } + pm_runtime_mark_last_busy(twl->dev); + pm_runtime_put_autosuspend(twl->dev); } omap_musb_mailbox(status); } @@ -768,6 +770,10 @@ static int twl4030_usb_remove(struct platform_device *pdev) /* disable complete OTG block */ twl4030_usb_clear_bits(twl, POWER_CTRL, POWER_CTRL_OTG_ENAB); + + if (twl->linkstat == OMAP_MUSB_VBUS_VALID || + twl->linkstat == OMAP_MUSB_ID_GROUND) + pm_runtime_put_noidle(twl->dev); pm_runtime_mark_last_busy(twl->dev); pm_runtime_put(twl->dev);
A construct like: if (pm_runtime_suspended(twl->dev)) pm_runtime_get_sync(twl->dev); is against the spirit of the runtime_pm interface as it makes the internal refcounting useless. In this case it is also racy, particularly as 'put_autosuspend' is use to drop a reference. When that happens a timer is started and the device is runtime-suspended after the timeout. If the above code runs in this window, the device will not be found to be suspended so no pm_runtime reference is taken. When the timer expires the device will be suspended, which is against the intention of the code. So be more direct is taking and dropping references. If twl->linkstat is VBUS_VALID or ID_GROUND, then hold a pm_runtime reference, otherwise don't. Signed-off-by: NeilBrown <neilb@suse.de> --- drivers/phy/phy-twl4030-usb.c | 20 +++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-) -- 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