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@xxxxxxx]
> > Sent: Friday, March 01, 2013 8:51 AM
> > To: Liu, Chuansheng
> > Cc: Alan Stern; Li, Fei; gregkh@xxxxxxxxxxxxxxxxxxx; Lan, Tianyu;
> > sarah.a.sharp@xxxxxxxxxxxxxxx; linux-usb@xxxxxxxxxxxxxxx;
> > linux-kernel@xxxxxxxxxxxxxxx
> > 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@xxxxxxxxxxxxxxxxxxx]
> > > > Sent: Thursday, February 28, 2013 11:17 PM
> > > > To: Li, Fei
> > > > Cc: gregkh@xxxxxxxxxxxxxxxxxxx; Lan, Tianyu;
> > sarah.a.sharp@xxxxxxxxxxxxxxx;
> > > > rjw@xxxxxxx; linux-usb@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; 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@xxxxxxxxx>
> > > > > Signed-off-by: Li Fei <fei.li@xxxxxxxxx>
> > > > > ---
> > > > >  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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/