[PATCH 1/5] regmap: irq: call pm_runtime_put in pm_runtime_get_sync failed case

[PATCH 1/5] regmap: irq: call pm_runtime_put in pm_runtime_get_sync failed case

[Li Fei]

Even in failed case of pm_runtime_get_sync, the usage_count
is incremented. In order to keep the usage_count with correct
value and runtime power management to behave correctly, call
pm_runtime_put(_sync) in such case.

Signed-off-by Liu Chuansheng <chuansheng.liu@intel.com>
Signed-off-by: Li Fei <fei.li@intel.com>
---
 drivers/base/regmap/regmap-irq.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/drivers/base/regmap/regmap-irq.c b/drivers/base/regmap/regmap-irq.c
index 4706c63..020ea2b 100644
--- a/drivers/base/regmap/regmap-irq.c
+++ b/drivers/base/regmap/regmap-irq.c
@@ -184,6 +184,7 @@ static irqreturn_t regmap_irq_thread(int irq, void *d)
 		if (ret < 0) {
 			dev_err(map->dev, "IRQ thread failed to resume: %d\n",
 				ret);
+			pm_runtime_put(map->dev);
 			return IRQ_NONE;
 		}
 	}
-- 
1.7.4.1

[PATCH 2/5] mmc: core: call pm_runtime_put_sync in pm_runtime_get_sync failed case

[Li Fei]

Even in failed case of pm_runtime_get_sync, the usage_count
is incremented. In order to keep the usage_count with correct
value and runtime power management to behave correctly, call
pm_runtime_put(_sync) in such case.

Signed-off-by Liu Chuansheng <chuansheng.liu@intel.com>
Signed-off-by: Li Fei <fei.li@intel.com>
---
 drivers/mmc/core/sdio.c     |    3 +--
 drivers/mmc/core/sdio_bus.c |    4 +++-
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c
index aa0719a..45ab6f32 100644
--- a/drivers/mmc/core/sdio.c
+++ b/drivers/mmc/core/sdio.c
@@ -874,6 +874,7 @@ static void mmc_sdio_detect(struct mmc_host *host)
 
 	mmc_release_host(host);
 
+out:
 	/*
 	 * Tell PM core it's OK to power off the card now.
 	 *
@@ -887,8 +888,6 @@ static void mmc_sdio_detect(struct mmc_host *host)
 	 */
 	if (host->caps & MMC_CAP_POWER_OFF_CARD)
 		pm_runtime_put_sync(&host->card->dev);
-
-out:
 	if (err) {
 		mmc_sdio_remove(host);
 
diff --git a/drivers/mmc/core/sdio_bus.c b/drivers/mmc/core/sdio_bus.c
index 5e57048..d572b31 100644
--- a/drivers/mmc/core/sdio_bus.c
+++ b/drivers/mmc/core/sdio_bus.c
@@ -136,8 +136,10 @@ static int sdio_bus_probe(struct device *dev)
 	 */
 	if (func->card->host->caps & MMC_CAP_POWER_OFF_CARD) {
 		ret = pm_runtime_get_sync(dev);
-		if (ret < 0)
+		if (ret < 0) {
+			pm_runtime_put_sync(dev);
 			goto out;
+		}
 	}
 
 	/* Set the default block size so the driver is sure it's something
-- 
1.7.4.1

[PATCH 3/5] wl1251: call pm_runtime_put_sync in pm_runtime_get_sync failed case

[Li Fei]

Even in failed case of pm_runtime_get_sync, the usage_count
is incremented. In order to keep the usage_count with correct
value and runtime power management to behave correctly, call
pm_runtime_put(_sync) in such case.

Signed-off-by Liu Chuansheng <chuansheng.liu@intel.com>
Signed-off-by: Li Fei <fei.li@intel.com>
---
 drivers/net/wireless/ti/wl1251/sdio.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/drivers/net/wireless/ti/wl1251/sdio.c b/drivers/net/wireless/ti/wl1251/sdio.c
index e57ee48..e2b3d9c 100644
--- a/drivers/net/wireless/ti/wl1251/sdio.c
+++ b/drivers/net/wireless/ti/wl1251/sdio.c
@@ -186,8 +186,10 @@ static int wl1251_sdio_set_power(struct wl1251 *wl, bool enable)
 			wl->set_power(true);
 
 		ret = pm_runtime_get_sync(&func->dev);
-		if (ret < 0)
+		if (ret < 0) {
+			pm_runtime_put_sync(&func->dev);
 			goto out;
+		}
 
 		sdio_claim_host(func);
 		sdio_enable_func(func);
-- 
1.7.4.1

[PATCH 4/5] usb: call pm_runtime_put_sync in pm_runtime_get_sync failed case

[Li Fei]

Even in failed case of pm_runtime_get_sync, the usage_count
is incremented. In order to keep the usage_count with correct
value and runtime power management to behave correctly, call
pm_runtime_put(_sync) in such case.

Signed-off-by Liu Chuansheng <chuansheng.liu@intel.com>
Signed-off-by: Li Fei <fei.li@intel.com>
---
 drivers/usb/core/hub.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index 5480352..b68493b 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -3152,6 +3152,7 @@ int usb_port_resume(struct usb_device *udev, pm_message_t msg)
 		if (status < 0) {
 			dev_dbg(&udev->dev, "can't resume usb port, status %d\n",
 					status);
+			pm_runtime_put_sync(&port_dev->dev);
 			return status;
 		}
 	}
-- 
1.7.4.1

[PATCH 5/5] hwspinlock/core: call pm_runtime_put in pm_runtime_get_sync failed case

[Li Fei]

Even in failed case of pm_runtime_get_sync, the usage_count
is incremented. In order to keep the usage_count with correct
value and runtime power management to behave correctly, call
pm_runtime_put(_sync) in such case.

In __hwspin_lock_request, module_put is also called before
return in pm_runtime_get_sync failed case.

Signed-off-by Liu Chuansheng <chuansheng.liu@intel.com>
Signed-off-by: Li Fei <fei.li@intel.com>
---
 drivers/hwspinlock/hwspinlock_core.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/drivers/hwspinlock/hwspinlock_core.c b/drivers/hwspinlock/hwspinlock_core.c
index db713c0..5a5076d 100644
--- a/drivers/hwspinlock/hwspinlock_core.c
+++ b/drivers/hwspinlock/hwspinlock_core.c
@@ -416,6 +416,8 @@ static int __hwspin_lock_request(struct hwspinlock *hwlock)
 	ret = pm_runtime_get_sync(dev);
 	if (ret < 0) {
 		dev_err(dev, "%s: can't power on device\n", __func__);
+		pm_runtime_put(dev);
+		module_put(dev->driver->owner);
 		return ret;
 	}
 
-- 
1.7.4.1

Re: [PATCH 3/5] wl1251: call pm_runtime_put_sync in pm_runtime_get_sync failed case

[Luciano Coelho]

On Thu, 2013-02-28 at 15:51 +0800, Li Fei wrote:
> Even in failed case of pm_runtime_get_sync, the usage_count
> is incremented. In order to keep the usage_count with correct
> value and runtime power management to behave correctly, call
> pm_runtime_put(_sync) in such case.
> 
> Signed-off-by Liu Chuansheng <chuansheng.liu@intel.com>
> Signed-off-by: Li Fei <fei.li@intel.com>
> ---

Looks good.  I'll apply this via my tree.

Thanks!

--
Luca.

Re: [PATCH 4/5] usb: call pm_runtime_put_sync in pm_runtime_get_sync failed case

[Lan Tianyu]

On 2013年02月28日 15:57, Li Fei wrote:
> 
> Even in failed case of pm_runtime_get_sync, the usage_count
> is incremented. In order to keep the usage_count with correct
> value and runtime power management to behave correctly, call
> pm_runtime_put(_sync) in such case.

Hi Fei:
	It's not necessary. Because the did_runtime_put == true means the
port's usage count has already been decreased during
usb_port_suspend().So to keep usage count balance, we should increase
the usage count in the usb_port_resume() whatever.
	
> 
> Signed-off-by Liu Chuansheng <chuansheng.liu@intel.com>
> Signed-off-by: Li Fei <fei.li@intel.com>
> ---
>  drivers/usb/core/hub.c |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
> index 5480352..b68493b 100644
> --- a/drivers/usb/core/hub.c
> +++ b/drivers/usb/core/hub.c
> @@ -3152,6 +3152,7 @@ int usb_port_resume(struct usb_device *udev, pm_message_t msg)
>  		if (status < 0) {
>  			dev_dbg(&udev->dev, "can't resume usb port, status %d\n",
>  					status);
> +			pm_runtime_put_sync(&port_dev->dev);
>  			return status;
>  		}
>  	}
> 
Hi Alan:
	Further thinking, the device should be disconnected since the port
can't be resumed and the device will not work normally. Something like
following. Does this make sense?
---
diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index d5d3de4..cf36b11 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -3170,6 +3170,7 @@ int usb_port_resume(struct usb_device *udev,
pm_message_t msg)
                if (status < 0) {
                        dev_dbg(&udev->dev, "can't resume usb port,
status %d\n",
                                        status);
+                       hub_port_logical_disconnect(hub, port1);
                        return status;
                }
        }


-- 
Best regards
Tianyu Lan

RE: [PATCH 4/5] usb: call pm_runtime_put_sync in pm_runtime_get_sync failed case

[Li, Fei]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 2769 bytes --]

> -----Original Message-----
> From: Lan, Tianyu
> Sent: Thursday, February 28, 2013 4:37 PM
> To: Li, Fei; stern@rowland.harvard.edu
> Cc: gregkh@linuxfoundation.org; sarah.a.sharp@linux.intel.com; rjw@sisk.pl;
> linux-usb@vger.kernel.org; linux-kernel@vger.kernel.org; Liu, Chuansheng
> Subject: Re: [PATCH 4/5] usb: call pm_runtime_put_sync in
> pm_runtime_get_sync failed case
> 
> On 2013年02月28日 15:57, Li Fei wrote:
> >
> > Even in failed case of pm_runtime_get_sync, the usage_count
> > is incremented. In order to keep the usage_count with correct
> > value and runtime power management to behave correctly, call
> > pm_runtime_put(_sync) in such case.
> 
> Hi Fei:
> 	It's not necessary. Because the did_runtime_put == true means the
> port's usage count has already been decreased during
> usb_port_suspend().So to keep usage count balance, we should increase
> the usage count in the usb_port_resume() whatever.

Thanks for your reminder.
Sorry, I forget to keep did_runtime_put as false in pm_runtime_get_sync
failed case. I'll submit patch V2 to update it.

> >
> > Signed-off-by Liu Chuansheng <chuansheng.liu@intel.com>
> > Signed-off-by: Li Fei <fei.li@intel.com>
> > ---
> >  drivers/usb/core/hub.c |    1 +
> >  1 files changed, 1 insertions(+), 0 deletions(-)
> >
> > diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
> > index 5480352..b68493b 100644
> > --- a/drivers/usb/core/hub.c
> > +++ b/drivers/usb/core/hub.c
> > @@ -3152,6 +3152,7 @@ int usb_port_resume(struct usb_device *udev,
> pm_message_t msg)
> >  		if (status < 0) {
> >  			dev_dbg(&udev->dev, "can't resume usb port, status %d\n",
> >  					status);
> > +			pm_runtime_put_sync(&port_dev->dev);
> >  			return status;
> >  		}
> >  	}
> >
> Hi Alan:
> 	Further thinking, the device should be disconnected since the port
> can't be resumed and the device will not work normally. Something like
> following. Does this make sense?
> ---
> diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
> index d5d3de4..cf36b11 100644
> --- a/drivers/usb/core/hub.c
> +++ b/drivers/usb/core/hub.c
> @@ -3170,6 +3170,7 @@ int usb_port_resume(struct usb_device *udev,
> pm_message_t msg)
>                 if (status < 0) {
>                         dev_dbg(&udev->dev, "can't resume usb port,
> status %d\n",
>                                         status);
> +                       hub_port_logical_disconnect(hub, port1);

IMHO, this can't keep the usage_count balance.

Best Regards,
Li Fei 

>                         return status;
>                 }
>         }
> 
> 
> --
> Best regards
> Tianyu Lan
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

[PATCH 4/5 V2] usb: call pm_runtime_put_sync in pm_runtime_get_sync failed case

[Li Fei]

Even in failed case of pm_runtime_get_sync, the usage_count
is incremented. In order to keep the usage_count with correct
value and runtime power management to behave correctly, call
pm_runtime_put(_sync) in such case.

Signed-off-by Liu Chuansheng <chuansheng.liu@intel.com>
Signed-off-by: Li Fei <fei.li@intel.com>
---
 drivers/usb/core/hub.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index 5480352..f72dede 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -3148,12 +3148,13 @@ int usb_port_resume(struct usb_device *udev, pm_message_t msg)
 
 	if (port_dev->did_runtime_put) {
 		status = pm_runtime_get_sync(&port_dev->dev);
-		port_dev->did_runtime_put = false;
 		if (status < 0) {
 			dev_dbg(&udev->dev, "can't resume usb port, status %d\n",
 					status);
+			pm_runtime_put_sync(&port_dev->dev);
 			return status;
 		}
+		port_dev->did_runtime_put = false;
 	}
 
 	/* Skip the initial Clear-Suspend step for a remote wakeup */
-- 
1.7.4.1

Re: [PATCH 4/5] usb: call pm_runtime_put_sync in pm_runtime_get_sync failed case

[Alan Stern]

On Thu, 28 Feb 2013, Lan Tianyu wrote:

> Hi Alan:
> 	Further thinking, the device should be disconnected since the port
> can't be resumed and the device will not work normally. Something like
> following. Does this make sense?
> ---
> diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
> index d5d3de4..cf36b11 100644
> --- a/drivers/usb/core/hub.c
> +++ b/drivers/usb/core/hub.c
> @@ -3170,6 +3170,7 @@ int usb_port_resume(struct usb_device *udev,
> pm_message_t msg)
>                 if (status < 0) {
>                         dev_dbg(&udev->dev, "can't resume usb port,
> status %d\n",
>                                         status);
> +                       hub_port_logical_disconnect(hub, port1);
>                         return status;
>                 }
>         }

I don't know.  If you do this, will the port ever get powered on again?  
This sort of thing is hard to test.

As far as the device is concerned, it won't make much difference.  
Either way, the device won't work.

Alan Stern

Re: [PATCH 4/5 V2] usb: call pm_runtime_put_sync in pm_runtime_get_sync failed case

[Alan Stern]

On Thu, 28 Feb 2013, Li Fei wrote:

> 
> Even in failed case of pm_runtime_get_sync, the usage_count
> is incremented. In order to keep the usage_count with correct
> value and runtime power management to behave correctly, call
> pm_runtime_put(_sync) in such case.
> 
> Signed-off-by Liu Chuansheng <chuansheng.liu@intel.com>
> Signed-off-by: Li Fei <fei.li@intel.com>
> ---
>  drivers/usb/core/hub.c |    3 ++-
>  1 files changed, 2 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
> index 5480352..f72dede 100644
> --- a/drivers/usb/core/hub.c
> +++ b/drivers/usb/core/hub.c
> @@ -3148,12 +3148,13 @@ int usb_port_resume(struct usb_device *udev, pm_message_t msg)
>  
>  	if (port_dev->did_runtime_put) {
>  		status = pm_runtime_get_sync(&port_dev->dev);
> -		port_dev->did_runtime_put = false;
>  		if (status < 0) {
>  			dev_dbg(&udev->dev, "can't resume usb port, status %d\n",
>  					status);
> +			pm_runtime_put_sync(&port_dev->dev);
>  			return status;
>  		}
> +		port_dev->did_runtime_put = false;
>  	}

I don't see much point in this.  After a failed resume, the port's 
runtime PM status is undefined.  Whether or not you do a 
pm_runtime_put_sync won't make any difference.

Alan Stern

RE: [PATCH 4/5 V2] usb: call pm_runtime_put_sync in pm_runtime_get_sync failed case

[Liu, Chuansheng]

> -----Original Message-----
> From: Alan Stern [mailto:stern@rowland.harvard.edu]
> Sent: Thursday, February 28, 2013 11:17 PM
> To: Li, Fei
> Cc: gregkh@linuxfoundation.org; Lan, Tianyu; sarah.a.sharp@linux.intel.com;
> rjw@sisk.pl; linux-usb@vger.kernel.org; linux-kernel@vger.kernel.org; Liu,
> Chuansheng
> Subject: Re: [PATCH 4/5 V2] usb: call pm_runtime_put_sync in
> pm_runtime_get_sync failed case
> 
> On Thu, 28 Feb 2013, Li Fei wrote:
> 
> >
> > Even in failed case of pm_runtime_get_sync, the usage_count
> > is incremented. In order to keep the usage_count with correct
> > value and runtime power management to behave correctly, call
> > pm_runtime_put(_sync) in such case.
> >
> > Signed-off-by Liu Chuansheng <chuansheng.liu@intel.com>
> > Signed-off-by: Li Fei <fei.li@intel.com>
> > ---
> >  drivers/usb/core/hub.c |    3 ++-
> >  1 files changed, 2 insertions(+), 1 deletions(-)
> >
> > diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
> > index 5480352..f72dede 100644
> > --- a/drivers/usb/core/hub.c
> > +++ b/drivers/usb/core/hub.c
> > @@ -3148,12 +3148,13 @@ int usb_port_resume(struct usb_device *udev,
> pm_message_t msg)
> >
> >  	if (port_dev->did_runtime_put) {
> >  		status = pm_runtime_get_sync(&port_dev->dev);
> > -		port_dev->did_runtime_put = false;
> >  		if (status < 0) {
> >  			dev_dbg(&udev->dev, "can't resume usb port, status %d\n",
> >  					status);
> > +			pm_runtime_put_sync(&port_dev->dev);
> >  			return status;
> >  		}
> > +		port_dev->did_runtime_put = false;
> >  	}
> 
> I don't see much point in this.  After a failed resume, the port's
> runtime PM status is undefined.  Whether or not you do a
> pm_runtime_put_sync won't make any difference.
In case of failed resume, calling pm_runtime_put_sync() is just for decrease the dev->power.usage_count,
because pm_runtime_get_sync() always increase the dev->power.usage_count even failed.

If not pairing runtime_get/put, after that case, the device can not enter runtime suspend any more due to dev->power.usage_count > 0 always.
Is it making sense?

Thanks.
> 
> Alan Stern

Re: [PATCH 4/5 V2] usb: call pm_runtime_put_sync in pm_runtime_get_sync failed case

[Rafael J. Wysocki]

On Friday, March 01, 2013 12:38:07 AM Liu, Chuansheng wrote:
> 
> > -----Original Message-----
> > From: Alan Stern [mailto:stern@rowland.harvard.edu]
> > Sent: Thursday, February 28, 2013 11:17 PM
> > To: Li, Fei
> > Cc: gregkh@linuxfoundation.org; Lan, Tianyu; sarah.a.sharp@linux.intel.com;
> > rjw@sisk.pl; linux-usb@vger.kernel.org; linux-kernel@vger.kernel.org; Liu,
> > Chuansheng
> > Subject: Re: [PATCH 4/5 V2] usb: call pm_runtime_put_sync in
> > pm_runtime_get_sync failed case
> > 
> > On Thu, 28 Feb 2013, Li Fei wrote:
> > 
> > >
> > > Even in failed case of pm_runtime_get_sync, the usage_count
> > > is incremented. In order to keep the usage_count with correct
> > > value and runtime power management to behave correctly, call
> > > pm_runtime_put(_sync) in such case.
> > >
> > > Signed-off-by Liu Chuansheng <chuansheng.liu@intel.com>
> > > Signed-off-by: Li Fei <fei.li@intel.com>
> > > ---
> > >  drivers/usb/core/hub.c |    3 ++-
> > >  1 files changed, 2 insertions(+), 1 deletions(-)
> > >
> > > diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
> > > index 5480352..f72dede 100644
> > > --- a/drivers/usb/core/hub.c
> > > +++ b/drivers/usb/core/hub.c
> > > @@ -3148,12 +3148,13 @@ int usb_port_resume(struct usb_device *udev,
> > pm_message_t msg)
> > >
> > >  	if (port_dev->did_runtime_put) {
> > >  		status = pm_runtime_get_sync(&port_dev->dev);
> > > -		port_dev->did_runtime_put = false;
> > >  		if (status < 0) {
> > >  			dev_dbg(&udev->dev, "can't resume usb port, status %d\n",
> > >  					status);
> > > +			pm_runtime_put_sync(&port_dev->dev);
> > >  			return status;
> > >  		}
> > > +		port_dev->did_runtime_put = false;
> > >  	}
> > 
> > I don't see much point in this.  After a failed resume, the port's
> > runtime PM status is undefined.  Whether or not you do a
> > pm_runtime_put_sync won't make any difference.
> In case of failed resume, calling pm_runtime_put_sync() is just for decrease the dev->power.usage_count,
> because pm_runtime_get_sync() always increase the dev->power.usage_count even failed.
> 
> If not pairing runtime_get/put, after that case, the device can not enter runtime suspend any more due to dev->power.usage_count > 0 always.
> Is it making sense?

Well, not really.

Before returning an error code, rpm_callback() assigns that code to
dev->power.runtime_error and that will effectively disable runtime PM for dev
going forward anyway.

Thanks,
Rafael


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

RE: [PATCH 4/5 V2] usb: call pm_runtime_put_sync in pm_runtime_get_sync failed case

[Liu, Chuansheng]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 3785 bytes --]



> -----Original Message-----
> From: Rafael J. Wysocki [mailto:rjw@sisk.pl]
> Sent: Friday, March 01, 2013 8:51 AM
> To: Liu, Chuansheng
> Cc: Alan Stern; Li, Fei; gregkh@linuxfoundation.org; Lan, Tianyu;
> sarah.a.sharp@linux.intel.com; linux-usb@vger.kernel.org;
> linux-kernel@vger.kernel.org
> Subject: Re: [PATCH 4/5 V2] usb: call pm_runtime_put_sync in
> pm_runtime_get_sync failed case
> 
> On Friday, March 01, 2013 12:38:07 AM Liu, Chuansheng wrote:
> >
> > > -----Original Message-----
> > > From: Alan Stern [mailto:stern@rowland.harvard.edu]
> > > Sent: Thursday, February 28, 2013 11:17 PM
> > > To: Li, Fei
> > > Cc: gregkh@linuxfoundation.org; Lan, Tianyu;
> sarah.a.sharp@linux.intel.com;
> > > rjw@sisk.pl; linux-usb@vger.kernel.org; linux-kernel@vger.kernel.org; Liu,
> > > Chuansheng
> > > Subject: Re: [PATCH 4/5 V2] usb: call pm_runtime_put_sync in
> > > pm_runtime_get_sync failed case
> > >
> > > On Thu, 28 Feb 2013, Li Fei wrote:
> > >
> > > >
> > > > Even in failed case of pm_runtime_get_sync, the usage_count
> > > > is incremented. In order to keep the usage_count with correct
> > > > value and runtime power management to behave correctly, call
> > > > pm_runtime_put(_sync) in such case.
> > > >
> > > > Signed-off-by Liu Chuansheng <chuansheng.liu@intel.com>
> > > > Signed-off-by: Li Fei <fei.li@intel.com>
> > > > ---
> > > >  drivers/usb/core/hub.c |    3 ++-
> > > >  1 files changed, 2 insertions(+), 1 deletions(-)
> > > >
> > > > diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
> > > > index 5480352..f72dede 100644
> > > > --- a/drivers/usb/core/hub.c
> > > > +++ b/drivers/usb/core/hub.c
> > > > @@ -3148,12 +3148,13 @@ int usb_port_resume(struct usb_device
> *udev,
> > > pm_message_t msg)
> > > >
> > > >  	if (port_dev->did_runtime_put) {
> > > >  		status = pm_runtime_get_sync(&port_dev->dev);
> > > > -		port_dev->did_runtime_put = false;
> > > >  		if (status < 0) {
> > > >  			dev_dbg(&udev->dev, "can't resume usb port,
> status %d\n",
> > > >  					status);
> > > > +			pm_runtime_put_sync(&port_dev->dev);
> > > >  			return status;
> > > >  		}
> > > > +		port_dev->did_runtime_put = false;
> > > >  	}
> > >
> > > I don't see much point in this.  After a failed resume, the port's
> > > runtime PM status is undefined.  Whether or not you do a
> > > pm_runtime_put_sync won't make any difference.
> > In case of failed resume, calling pm_runtime_put_sync() is just for decrease
> the dev->power.usage_count,
> > because pm_runtime_get_sync() always increase the
> dev->power.usage_count even failed.
> >
> > If not pairing runtime_get/put, after that case, the device can not enter
> runtime suspend any more due to dev->power.usage_count > 0 always.
> > Is it making sense?
> 
> Well, not really.
> 
> Before returning an error code, rpm_callback() assigns that code to
> dev->power.runtime_error and that will effectively disable runtime PM for dev
> going forward anyway.
Thanks your pointing out.
dev->power.runtime_error!=0 will really block the runtime PM resume/suspend to continue.

But in case of rpm_resume return error when dev->power.disable_depth > 0, the dev->power.runtime_error
is not set yet. Is it the case? Thanks your comments again.

And another case is when user called pm_runtime_set_status to clear the runtime_error after dev->power.runtime_error
is set during pm_runtime_get_sync(), the runtime_resume/suspend() can be tried again? But the dev->power.usage_count is still wrong?
Thanks your correction again:)
> 
> Thanks,
> Rafael
> 
> 
> --
> I speak only for myself.
> Rafael J. Wysocki, Intel Open Source Technology Center.
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

RE: [PATCH 4/5 V2] usb: call pm_runtime_put_sync in pm_runtime_get_sync failed case

[Liu, Chuansheng]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 1935 bytes --]



> -----Original Message-----
> From: Li, Fei
> Sent: Thursday, February 28, 2013 5:06 PM
> To: gregkh@linuxfoundation.org; Lan, Tianyu; stern@rowland.harvard.edu;
> sarah.a.sharp@linux.intel.com
> Cc: rjw@sisk.pl; linux-usb@vger.kernel.org; linux-kernel@vger.kernel.org; Liu,
> Chuansheng; Li, Fei
> Subject: [PATCH 4/5 V2] usb: call pm_runtime_put_sync in
> pm_runtime_get_sync failed case
> 
> 
> Even in failed case of pm_runtime_get_sync, the usage_count
> is incremented. In order to keep the usage_count with correct
> value and runtime power management to behave correctly, call
> pm_runtime_put(_sync) in such case.
> 
> Signed-off-by Liu Chuansheng <chuansheng.liu@intel.com>
> Signed-off-by: Li Fei <fei.li@intel.com>
> ---
>  drivers/usb/core/hub.c |    3 ++-
>  1 files changed, 2 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
> index 5480352..f72dede 100644
> --- a/drivers/usb/core/hub.c
> +++ b/drivers/usb/core/hub.c
> @@ -3148,12 +3148,13 @@ int usb_port_resume(struct usb_device *udev,
> pm_message_t msg)
> 
>  	if (port_dev->did_runtime_put) {
>  		status = pm_runtime_get_sync(&port_dev->dev);
> -		port_dev->did_runtime_put = false;
>  		if (status < 0) {
>  			dev_dbg(&udev->dev, "can't resume usb port, status %d\n",
>  					status);
> +			pm_runtime_put_sync(&port_dev->dev);
Rechecked the usb similar codes, in usb_autoresume_device() and usb_autopm_get_interface(),
when pm_runtime_get_sync() failed, the paired pm_runtime_put_sync() will be called.
Alan and Rafael, is it reasonable to consider this cleanup patch also? Thanks.

>  			return status;
>  		}
> +		port_dev->did_runtime_put = false;
>  	}
> 
>  	/* Skip the initial Clear-Suspend step for a remote wakeup */
> --
> 1.7.4.1
> 
> 

ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH 4/5 V2] usb: call pm_runtime_put_sync in pm_runtime_get_sync failed case

[Rafael J. Wysocki]

On Friday, March 01, 2013 12:59:23 AM Liu, Chuansheng wrote:
> 
> > -----Original Message-----
> > From: Rafael J. Wysocki [mailto:rjw@sisk.pl]
> > Sent: Friday, March 01, 2013 8:51 AM
> > To: Liu, Chuansheng
> > Cc: Alan Stern; Li, Fei; gregkh@linuxfoundation.org; Lan, Tianyu;
> > sarah.a.sharp@linux.intel.com; linux-usb@vger.kernel.org;
> > linux-kernel@vger.kernel.org
> > Subject: Re: [PATCH 4/5 V2] usb: call pm_runtime_put_sync in
> > pm_runtime_get_sync failed case
> > 
> > On Friday, March 01, 2013 12:38:07 AM Liu, Chuansheng wrote:
> > >
> > > > -----Original Message-----
> > > > From: Alan Stern [mailto:stern@rowland.harvard.edu]
> > > > Sent: Thursday, February 28, 2013 11:17 PM
> > > > To: Li, Fei
> > > > Cc: gregkh@linuxfoundation.org; Lan, Tianyu;
> > sarah.a.sharp@linux.intel.com;
> > > > rjw@sisk.pl; linux-usb@vger.kernel.org; linux-kernel@vger.kernel.org; Liu,
> > > > Chuansheng
> > > > Subject: Re: [PATCH 4/5 V2] usb: call pm_runtime_put_sync in
> > > > pm_runtime_get_sync failed case
> > > >
> > > > On Thu, 28 Feb 2013, Li Fei wrote:
> > > >
> > > > >
> > > > > Even in failed case of pm_runtime_get_sync, the usage_count
> > > > > is incremented. In order to keep the usage_count with correct
> > > > > value and runtime power management to behave correctly, call
> > > > > pm_runtime_put(_sync) in such case.
> > > > >
> > > > > Signed-off-by Liu Chuansheng <chuansheng.liu@intel.com>
> > > > > Signed-off-by: Li Fei <fei.li@intel.com>
> > > > > ---
> > > > >  drivers/usb/core/hub.c |    3 ++-
> > > > >  1 files changed, 2 insertions(+), 1 deletions(-)
> > > > >
> > > > > diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
> > > > > index 5480352..f72dede 100644
> > > > > --- a/drivers/usb/core/hub.c
> > > > > +++ b/drivers/usb/core/hub.c
> > > > > @@ -3148,12 +3148,13 @@ int usb_port_resume(struct usb_device
> > *udev,
> > > > pm_message_t msg)
> > > > >
> > > > >  	if (port_dev->did_runtime_put) {
> > > > >  		status = pm_runtime_get_sync(&port_dev->dev);
> > > > > -		port_dev->did_runtime_put = false;
> > > > >  		if (status < 0) {
> > > > >  			dev_dbg(&udev->dev, "can't resume usb port,
> > status %d\n",
> > > > >  					status);
> > > > > +			pm_runtime_put_sync(&port_dev->dev);
> > > > >  			return status;
> > > > >  		}
> > > > > +		port_dev->did_runtime_put = false;
> > > > >  	}
> > > >
> > > > I don't see much point in this.  After a failed resume, the port's
> > > > runtime PM status is undefined.  Whether or not you do a
> > > > pm_runtime_put_sync won't make any difference.
> > > In case of failed resume, calling pm_runtime_put_sync() is just for decrease
> > the dev->power.usage_count,
> > > because pm_runtime_get_sync() always increase the
> > dev->power.usage_count even failed.
> > >
> > > If not pairing runtime_get/put, after that case, the device can not enter
> > runtime suspend any more due to dev->power.usage_count > 0 always.
> > > Is it making sense?
> > 
> > Well, not really.
> > 
> > Before returning an error code, rpm_callback() assigns that code to
> > dev->power.runtime_error and that will effectively disable runtime PM for dev
> > going forward anyway.
> Thanks your pointing out.
> dev->power.runtime_error!=0 will really block the runtime PM resume/suspend to continue.
> 
> But in case of rpm_resume return error when dev->power.disable_depth > 0, the dev->power.runtime_error
> is not set yet. Is it the case?

Yes, it is.

> And another case is when user called pm_runtime_set_status to clear the runtime_error after dev->power.runtime_error
> is set during pm_runtime_get_sync(), the runtime_resume/suspend() can be tried again? But the dev->power.usage_count is still wrong?

If you clear runtime_error using pm_runtime_set_status(), you can correct the
reference counter as well.

But I agree that with runtime PM disabled it is actually useful to keep the
reference counter balanced appropriately so that you don't need to special
case that.  All depends on how runtime PM is used in the given piece of code.

That's why I didn't comment your other patches.  That said, I didn't look at
them in detail either.

Thanks,
Rafael


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH 4/5 V2] usb: call pm_runtime_put_sync in pm_runtime_get_sync failed case

[Rafael J. Wysocki]

On Friday, March 01, 2013 02:07:54 AM Liu, Chuansheng wrote:
> 
> > -----Original Message-----
> > From: Li, Fei
> > Sent: Thursday, February 28, 2013 5:06 PM
> > To: gregkh@linuxfoundation.org; Lan, Tianyu; stern@rowland.harvard.edu;
> > sarah.a.sharp@linux.intel.com
> > Cc: rjw@sisk.pl; linux-usb@vger.kernel.org; linux-kernel@vger.kernel.org; Liu,
> > Chuansheng; Li, Fei
> > Subject: [PATCH 4/5 V2] usb: call pm_runtime_put_sync in
> > pm_runtime_get_sync failed case
> > 
> > 
> > Even in failed case of pm_runtime_get_sync, the usage_count
> > is incremented. In order to keep the usage_count with correct
> > value and runtime power management to behave correctly, call
> > pm_runtime_put(_sync) in such case.
> > 
> > Signed-off-by Liu Chuansheng <chuansheng.liu@intel.com>
> > Signed-off-by: Li Fei <fei.li@intel.com>
> > ---
> >  drivers/usb/core/hub.c |    3 ++-
> >  1 files changed, 2 insertions(+), 1 deletions(-)
> > 
> > diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
> > index 5480352..f72dede 100644
> > --- a/drivers/usb/core/hub.c
> > +++ b/drivers/usb/core/hub.c
> > @@ -3148,12 +3148,13 @@ int usb_port_resume(struct usb_device *udev,
> > pm_message_t msg)
> > 
> >  	if (port_dev->did_runtime_put) {
> >  		status = pm_runtime_get_sync(&port_dev->dev);
> > -		port_dev->did_runtime_put = false;
> >  		if (status < 0) {
> >  			dev_dbg(&udev->dev, "can't resume usb port, status %d\n",
> >  					status);
> > +			pm_runtime_put_sync(&port_dev->dev);
> Rechecked the usb similar codes, in usb_autoresume_device() and usb_autopm_get_interface(),
> when pm_runtime_get_sync() failed, the paired pm_runtime_put_sync() will be called.
> Alan and Rafael, is it reasonable to consider this cleanup patch also? Thanks.

You can very well use pm_runtime_put_noidle() here too.  Then, it will
be kind of clear what it's for.

> 
> >  			return status;
> >  		}
> > +		port_dev->did_runtime_put = false;
> >  	}
> > 
> >  	/* Skip the initial Clear-Suspend step for a remote wakeup */
> > --
> > 1.7.4.1

Thanks,
Rafael


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

RE: [PATCH 4/5 V2] usb: call pm_runtime_put_sync in pm_runtime_get_sync failed case

[Liu, Chuansheng]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 2912 bytes --]



> -----Original Message-----
> From: Rafael J. Wysocki [mailto:rjw@sisk.pl]
> Sent: Friday, March 01, 2013 10:22 AM
> To: Liu, Chuansheng
> Cc: Li, Fei; gregkh@linuxfoundation.org; Lan, Tianyu;
> stern@rowland.harvard.edu; sarah.a.sharp@linux.intel.com;
> linux-usb@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH 4/5 V2] usb: call pm_runtime_put_sync in
> pm_runtime_get_sync failed case
> 
> On Friday, March 01, 2013 02:07:54 AM Liu, Chuansheng wrote:
> >
> > > -----Original Message-----
> > > From: Li, Fei
> > > Sent: Thursday, February 28, 2013 5:06 PM
> > > To: gregkh@linuxfoundation.org; Lan, Tianyu; stern@rowland.harvard.edu;
> > > sarah.a.sharp@linux.intel.com
> > > Cc: rjw@sisk.pl; linux-usb@vger.kernel.org; linux-kernel@vger.kernel.org;
> Liu,
> > > Chuansheng; Li, Fei
> > > Subject: [PATCH 4/5 V2] usb: call pm_runtime_put_sync in
> > > pm_runtime_get_sync failed case
> > >
> > >
> > > Even in failed case of pm_runtime_get_sync, the usage_count
> > > is incremented. In order to keep the usage_count with correct
> > > value and runtime power management to behave correctly, call
> > > pm_runtime_put(_sync) in such case.
> > >
> > > Signed-off-by Liu Chuansheng <chuansheng.liu@intel.com>
> > > Signed-off-by: Li Fei <fei.li@intel.com>
> > > ---
> > >  drivers/usb/core/hub.c |    3 ++-
> > >  1 files changed, 2 insertions(+), 1 deletions(-)
> > >
> > > diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
> > > index 5480352..f72dede 100644
> > > --- a/drivers/usb/core/hub.c
> > > +++ b/drivers/usb/core/hub.c
> > > @@ -3148,12 +3148,13 @@ int usb_port_resume(struct usb_device
> *udev,
> > > pm_message_t msg)
> > >
> > >  	if (port_dev->did_runtime_put) {
> > >  		status = pm_runtime_get_sync(&port_dev->dev);
> > > -		port_dev->did_runtime_put = false;
> > >  		if (status < 0) {
> > >  			dev_dbg(&udev->dev, "can't resume usb port,
> status %d\n",
> > >  					status);
> > > +			pm_runtime_put_sync(&port_dev->dev);
> > Rechecked the usb similar codes, in usb_autoresume_device() and
> usb_autopm_get_interface(),
> > when pm_runtime_get_sync() failed, the paired pm_runtime_put_sync() will
> be called.
> > Alan and Rafael, is it reasonable to consider this cleanup patch also? Thanks.
> 
> You can very well use pm_runtime_put_noidle() here too.  Then, it will
> be kind of clear what it's for.
Thanks. Your advice really express we want to do. Will update the patch soon.

> 
> >
> > >  			return status;
> > >  		}
> > > +		port_dev->did_runtime_put = false;
> > >  	}
> > >
> > >  	/* Skip the initial Clear-Suspend step for a remote wakeup */
> > > --
> > > 1.7.4.1
> 
> Thanks,
> Rafael
> 
> 
> --
> I speak only for myself.
> Rafael J. Wysocki, Intel Open Source Technology Center.
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

[PATCH 4/5 V3] usb: call pm_runtime_put_noidle in pm_runtime_get_sync failed case

[Li Fei]

Even in failed case of pm_runtime_get_sync, the usage_count
is incremented. In order to keep the usage_count with correct
value and runtime power management to behave correctly, call
pm_runtime_put(_sync/noidle) in such case.

Signed-off-by Liu Chuansheng <chuansheng.liu@intel.com>
Signed-off-by: Li Fei <fei.li@intel.com>
---
 drivers/usb/core/hub.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index 5480352..4a6c055 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -3148,12 +3148,13 @@ int usb_port_resume(struct usb_device *udev, pm_message_t msg)
 
 	if (port_dev->did_runtime_put) {
 		status = pm_runtime_get_sync(&port_dev->dev);
-		port_dev->did_runtime_put = false;
 		if (status < 0) {
 			dev_dbg(&udev->dev, "can't resume usb port, status %d\n",
 					status);
+			pm_runtime_put_noidle(&port_dev->dev);
 			return status;
 		}
+		port_dev->did_runtime_put = false;
 	}
 
 	/* Skip the initial Clear-Suspend step for a remote wakeup */
-- 
1.7.4.1

[PATCH 4/5 V3] usb: call pm_runtime_put_noidle in pm_runtime_get_sync failed case

[Li Fei]

Even in failed case of pm_runtime_get_sync, the usage_count
is incremented. In order to keep the usage_count with correct
value and runtime power management to behave correctly, call
pm_runtime_put(_sync/noidle) in such case.

Signed-off-by Liu Chuansheng <chuansheng.liu@intel.com>
Signed-off-by: Li Fei <fei.li@intel.com>
---
 drivers/usb/core/hub.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index 5480352..4a6c055 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -3148,12 +3148,13 @@ int usb_port_resume(struct usb_device *udev, pm_message_t msg)
 
 	if (port_dev->did_runtime_put) {
 		status = pm_runtime_get_sync(&port_dev->dev);
-		port_dev->did_runtime_put = false;
 		if (status < 0) {
 			dev_dbg(&udev->dev, "can't resume usb port, status %d\n",
 					status);
+			pm_runtime_put_noidle(&port_dev->dev);
 			return status;
 		}
+		port_dev->did_runtime_put = false;
 	}
 
 	/* Skip the initial Clear-Suspend step for a remote wakeup */
-- 
1.7.4.1

Re: [PATCH 1/5] regmap: irq: call pm_runtime_put in pm_runtime_get_sync failed case

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 358 bytes --]

On Thu, Feb 28, 2013 at 03:37:11PM +0800, Li Fei wrote:
> 
> Even in failed case of pm_runtime_get_sync, the usage_count
> is incremented. In order to keep the usage_count with correct
> value and runtime power management to behave correctly, call
> pm_runtime_put(_sync) in such case.

Oh, that is a surprising interface...  anyway, applied thanks.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 3/5] wl1251: call pm_runtime_put_sync in pm_runtime_get_sync failed case

[Luciano Coelho]

On Thu, 2013-02-28 at 15:51 +0800, Li Fei wrote:
> Even in failed case of pm_runtime_get_sync, the usage_count
> is incremented. In order to keep the usage_count with correct
> value and runtime power management to behave correctly, call
> pm_runtime_put(_sync) in such case.
> 
> Signed-off-by Liu Chuansheng <chuansheng.liu@intel.com>
> Signed-off-by: Li Fei <fei.li@intel.com>
> ---

Applied, thanks!

-- 
Luca.

Re: [PATCH 5/5] hwspinlock/core: call pm_runtime_put in pm_runtime_get_sync failed case

[Ohad Ben-Cohen]

Hi Li,

On Thu, Feb 28, 2013 at 10:02 AM, Li Fei <fei.li@intel.com> wrote:
>
> Even in failed case of pm_runtime_get_sync, the usage_count
> is incremented. In order to keep the usage_count with correct
> value and runtime power management to behave correctly, call
> pm_runtime_put(_sync) in such case.

Is it better then to call pm_runtime_put_noidle instead? This way
we're sure to only take care of usage_count without ever calling any
underlying pm handler.

Thanks,
Ohad.

> In __hwspin_lock_request, module_put is also called before
> return in pm_runtime_get_sync failed case.
>
> Signed-off-by Liu Chuansheng <chuansheng.liu@intel.com>
> Signed-off-by: Li Fei <fei.li@intel.com>
> ---
>  drivers/hwspinlock/hwspinlock_core.c |    2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/hwspinlock/hwspinlock_core.c b/drivers/hwspinlock/hwspinlock_core.c
> index db713c0..5a5076d 100644
> --- a/drivers/hwspinlock/hwspinlock_core.c
> +++ b/drivers/hwspinlock/hwspinlock_core.c
> @@ -416,6 +416,8 @@ static int __hwspin_lock_request(struct hwspinlock *hwlock)
>         ret = pm_runtime_get_sync(dev);
>         if (ret < 0) {
>                 dev_err(dev, "%s: can't power on device\n", __func__);
> +               pm_runtime_put(dev);
> +               module_put(dev->driver->owner);
>                 return ret;
>         }
>
> --
> 1.7.4.1
>
>
>

Re: [PATCH 5/5] hwspinlock/core: call pm_runtime_put in pm_runtime_get_sync failed case

[Rafael J. Wysocki]

On Friday, April 05, 2013 09:27:40 AM Ohad Ben-Cohen wrote:
> Hi Li,
> 
> On Thu, Feb 28, 2013 at 10:02 AM, Li Fei <fei.li@intel.com> wrote:
> >
> > Even in failed case of pm_runtime_get_sync, the usage_count
> > is incremented. In order to keep the usage_count with correct
> > value and runtime power management to behave correctly, call
> > pm_runtime_put(_sync) in such case.
> 
> Is it better then to call pm_runtime_put_noidle instead? This way
> we're sure to only take care of usage_count without ever calling any
> underlying pm handler.

Both would break code that does

 pm_runtime_get_sync(dev);

 <device access>

 pm_runtime_put(dev);

without checking the result of pm_runtime_get_sync() - which BTW is completely
unnecessary in the majority of cases.

So no, it's not a good idea at all.

Thanks,
Rafael


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH 5/5] hwspinlock/core: call pm_runtime_put in pm_runtime_get_sync failed case

[Rafael J. Wysocki]

On Friday, April 05, 2013 01:39:58 PM Rafael J. Wysocki wrote:
> On Friday, April 05, 2013 09:27:40 AM Ohad Ben-Cohen wrote:
> > Hi Li,
> > 
> > On Thu, Feb 28, 2013 at 10:02 AM, Li Fei <fei.li@intel.com> wrote:
> > >
> > > Even in failed case of pm_runtime_get_sync, the usage_count
> > > is incremented. In order to keep the usage_count with correct
> > > value and runtime power management to behave correctly, call
> > > pm_runtime_put(_sync) in such case.
> > 
> > Is it better then to call pm_runtime_put_noidle instead? This way
> > we're sure to only take care of usage_count without ever calling any
> > underlying pm handler.
> 
> Both would break code that does
> 
>  pm_runtime_get_sync(dev);
> 
>  <device access>
> 
>  pm_runtime_put(dev);
> 
> without checking the result of pm_runtime_get_sync() - which BTW is completely
> unnecessary in the majority of cases.

Sorry, scratch that.  I should have had a closer look at the context.

Yes, it better to call pm_runtime_put_noidle() in this case.

Thanks,
Rafael


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

RE: [PATCH 5/5] hwspinlock/core: call pm_runtime_put in pm_runtime_get_sync failed case

[Li, Fei]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 1720 bytes --]

> -----Original Message-----
> From: Rafael J. Wysocki [mailto:rjw@sisk.pl]
> Sent: Friday, April 05, 2013 7:42 PM
> To: Ohad Ben-Cohen; Liu, Chuansheng
> Cc: Li, Fei; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH 5/5] hwspinlock/core: call pm_runtime_put in
> pm_runtime_get_sync failed case
> 
> On Friday, April 05, 2013 01:39:58 PM Rafael J. Wysocki wrote:
> > On Friday, April 05, 2013 09:27:40 AM Ohad Ben-Cohen wrote:
> > > Hi Li,
> > >
> > > On Thu, Feb 28, 2013 at 10:02 AM, Li Fei <fei.li@intel.com> wrote:
> > > >
> > > > Even in failed case of pm_runtime_get_sync, the usage_count
> > > > is incremented. In order to keep the usage_count with correct
> > > > value and runtime power management to behave correctly, call
> > > > pm_runtime_put(_sync) in such case.
> > >
> > > Is it better then to call pm_runtime_put_noidle instead? This way
> > > we're sure to only take care of usage_count without ever calling any
> > > underlying pm handler.
> >
> > Both would break code that does
> >
> >  pm_runtime_get_sync(dev);
> >
> >  <device access>
> >
> >  pm_runtime_put(dev);
> >
> > without checking the result of pm_runtime_get_sync() - which BTW is
> completely
> > unnecessary in the majority of cases.
> 
> Sorry, scratch that.  I should have had a closer look at the context.
> 
> Yes, it better to call pm_runtime_put_noidle() in this case.
> 
Thanks for your feedback.
I'll upload patch V2 for this topic.

Thanks,
Fei

> Thanks,
> Rafael
> 
> 
> --
> I speak only for myself.
> Rafael J. Wysocki, Intel Open Source Technology Center.
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

[PATCH 5/5 V2] hwspinlock/core: call pm_runtime_put in pm_runtime_get_sync failed case

[Li Fei]

Even in failed case of pm_runtime_get_sync, the usage_count
is incremented. In order to keep the usage_count with correct
value and runtime power management to behave correctly, call
pm_runtime_put(_sync) in such case.

In __hwspin_lock_request, module_put is also called before
return in pm_runtime_get_sync failed case.

Signed-off-by Liu Chuansheng <chuansheng.liu@intel.com>
Signed-off-by: Li Fei <fei.li@intel.com>
---
 drivers/hwspinlock/hwspinlock_core.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/drivers/hwspinlock/hwspinlock_core.c b/drivers/hwspinlock/hwspinlock_core.c
index db713c0..461a0d7 100644
--- a/drivers/hwspinlock/hwspinlock_core.c
+++ b/drivers/hwspinlock/hwspinlock_core.c
@@ -416,6 +416,8 @@ static int __hwspin_lock_request(struct hwspinlock *hwlock)
 	ret = pm_runtime_get_sync(dev);
 	if (ret < 0) {
 		dev_err(dev, "%s: can't power on device\n", __func__);
+		pm_runtime_put_noidle(dev);
+		module_put(dev->driver->owner);
 		return ret;
 	}
 
-- 
1.7.4.1

Re: [PATCH 5/5 V2] hwspinlock/core: call pm_runtime_put in pm_runtime_get_sync failed case

[Ohad Ben-Cohen]

On Fri, Apr 5, 2013 at 4:20 PM, Li Fei <fei.li@intel.com> wrote:
> Even in failed case of pm_runtime_get_sync, the usage_count
> is incremented. In order to keep the usage_count with correct
> value and runtime power management to behave correctly, call
> pm_runtime_put(_sync) in such case.
>
> In __hwspin_lock_request, module_put is also called before
> return in pm_runtime_get_sync failed case.
>
> Signed-off-by Liu Chuansheng <chuansheng.liu@intel.com>
> Signed-off-by: Li Fei <fei.li@intel.com>

Applied (after fixing the commit log to reflect the recent change), thanks!

Re: [PATCH 2/5] mmc: core: call pm_runtime_put_sync in pm_runtime_get_sync failed case

[Ohad Ben-Cohen]

Hi Li,

On Thu, Feb 28, 2013 at 9:44 AM, Li Fei <fei.li@intel.com> wrote:
> Even in failed case of pm_runtime_get_sync, the usage_count
> is incremented. In order to keep the usage_count with correct
> value and runtime power management to behave correctly, call
> pm_runtime_put(_sync) in such case.

As with the remoteproc case, it is probably better to call the
put_noidle variant here. This way you are sure not to erroneously
invoke any underlying pm handler where your only intention is to fix
usage_count.

Thanks,
Ohad.

RE: [PATCH 2/5] mmc: core: call pm_runtime_put_sync in pm_runtime_get_sync failed case

[Li, Fei]

> 
> Hi Li,
> 
> On Thu, Feb 28, 2013 at 9:44 AM, Li Fei <fei.li@intel.com> wrote:
> > Even in failed case of pm_runtime_get_sync, the usage_count
> > is incremented. In order to keep the usage_count with correct
> > value and runtime power management to behave correctly, call
> > pm_runtime_put(_sync) in such case.
> 
> As with the remoteproc case, it is probably better to call the
> put_noidle variant here. This way you are sure not to erroneously
> invoke any underlying pm handler where your only intention is to fix
> usage_count.

Thanks for your check and feedback, and will update it in V2 soon.

Regards,
Fei
> 
> Thanks,
> Ohad.

[PATCH 2/5 V2] mmc: core: call pm_runtime_put_noidle in pm_runtime_get_sync failed case

[Li Fei]

Even in failed case of pm_runtime_get_sync, the usage_count
is incremented. In order to keep the usage_count with correct
value and runtime power management to behave correctly, call
pm_runtime_put_noidle in such case.

Signed-off-by Liu Chuansheng <chuansheng.liu@intel.com>
Signed-off-by: Li Fei <fei.li@intel.com>
---
 drivers/mmc/core/sdio.c     |    4 +++-
 drivers/mmc/core/sdio_bus.c |    3 +--
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c
index aa0719a..6889a82 100644
--- a/drivers/mmc/core/sdio.c
+++ b/drivers/mmc/core/sdio.c
@@ -861,8 +861,10 @@ static void mmc_sdio_detect(struct mmc_host *host)
 	/* Make sure card is powered before detecting it */
 	if (host->caps & MMC_CAP_POWER_OFF_CARD) {
 		err = pm_runtime_get_sync(&host->card->dev);
-		if (err < 0)
+		if (err < 0) {
+			pm_runtime_put_noidle(&host->card->dev);
 			goto out;
+		}
 	}
 
 	mmc_claim_host(host);
diff --git a/drivers/mmc/core/sdio_bus.c b/drivers/mmc/core/sdio_bus.c
index 5e57048..7bfefb5 100644
--- a/drivers/mmc/core/sdio_bus.c
+++ b/drivers/mmc/core/sdio_bus.c
@@ -137,7 +137,7 @@ static int sdio_bus_probe(struct device *dev)
 	if (func->card->host->caps & MMC_CAP_POWER_OFF_CARD) {
 		ret = pm_runtime_get_sync(dev);
 		if (ret < 0)
-			goto out;
+			goto disable_runtimepm;
 	}
 
 	/* Set the default block size so the driver is sure it's something
@@ -157,7 +157,6 @@ static int sdio_bus_probe(struct device *dev)
 disable_runtimepm:
 	if (func->card->host->caps & MMC_CAP_POWER_OFF_CARD)
 		pm_runtime_put_noidle(dev);
-out:
 	return ret;
 }
 
-- 
1.7.4.1

Re: [PATCH 2/5 V2] mmc: core: call pm_runtime_put_noidle in pm_runtime_get_sync failed case

[Ohad Ben-Cohen]

On Mon, Apr 8, 2013 at 4:36 AM, Li Fei <fei.li@intel.com> wrote:
> Even in failed case of pm_runtime_get_sync, the usage_count
> is incremented. In order to keep the usage_count with correct
> value and runtime power management to behave correctly, call
> pm_runtime_put_noidle in such case.
>
> Signed-off-by Liu Chuansheng <chuansheng.liu@intel.com>
> Signed-off-by: Li Fei <fei.li@intel.com>

Acked-by: Ohad Ben-Cohen <ohad@wizery.com>

BTW, Li, could you please move to _noidle in those other places where
your previous patch was already applied? I think we have at least the
12xx driver (cc'ing Luca).

Thanks!
Ohad.

Re: [PATCH 2/5 V2] mmc: core: call pm_runtime_put_noidle in pm_runtime_get_sync failed case

[Chris Ball]

Hi,

On Mon, Apr 08 2013, Ohad Ben-Cohen wrote:
> On Mon, Apr 8, 2013 at 4:36 AM, Li Fei <fei.li@intel.com> wrote:
>> Even in failed case of pm_runtime_get_sync, the usage_count
>> is incremented. In order to keep the usage_count with correct
>> value and runtime power management to behave correctly, call
>> pm_runtime_put_noidle in such case.
>>
>> Signed-off-by Liu Chuansheng <chuansheng.liu@intel.com>
>> Signed-off-by: Li Fei <fei.li@intel.com>
>
> Acked-by: Ohad Ben-Cohen <ohad@wizery.com>

Thanks, pushed this patch to mmc-next for 3.10.

- Chris.
-- 
Chris Ball   <cjb@laptop.org>   <http://printf.net/>
One Laptop Per Child