Re: Do we need asynchronous pm_runtime_get()? (was: Re: bisected regression ...)

From: "Rafael J. Wysocki" <rjw@sisk.pl>
To: Ming Lei <tom.leiming@gmail.com>
Cc: Alan Stern <stern@rowland.harvard.edu>,
	linux-pci@vger.kernel.org, USB list <linux-usb@vger.kernel.org>,
	Linux PM list <linux-pm@vger.kernel.org>
Subject: Re: Do we need asynchronous pm_runtime_get()? (was: Re: bisected regression ...)
Date: Fri, 10 Aug 2012 22:29:36 +0200	[thread overview]
Message-ID: <201208102229.36360.rjw@sisk.pl> (raw)
In-Reply-To: <CACVXFVPMaTT4s7aS8mdNjgm3i3GFJ6gqFhFxZKuooF0mqi45rA@mail.gmail.com>

On Friday, August 10, 2012, Ming Lei wrote:
> On Fri, Aug 10, 2012 at 3:41 AM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> >
> > It just isn't guaranteed that the subsystem callback won't do anything
> > after driver->runtime_resume completion.  I agree that it isn't likely
> > to happen.
> 
> In fact, the subsystem callback should make sure that don't happen, see
> below comments on .runtime_resume:
> 
>  * @runtime_resume: Put the device into the fully active state in response to a
>  *      wakeup event generated by hardware or at the request of software.  If
>  *      necessary, put the device into the full-power state and restore its
>  *      registers, so that it is fully operational.
> 
> So once driver->runtime_resume completes, the device should be fully operational
> from the view of driver.

This comment only applies literally to drivers whose callbacks are run directly
by the PM core.  If subsystems and/or PM domains are involved, the interactions
between different layers of callbacks obviously have to be taken into account.

Please note, however, that this comment doesn't say anything about processing
I/O by the callback (hint: the callback is _not_ _supposed_ to do that).

> >> >> Firstly, introduce one extra pointer in device may increase memory
> >> >> consume for device allocation,
> >> >
> >> > Yes, it does, which may or may not matter depending on the actual size of
> >> > struct device and the CPU cache line size on the given machine, right?
> >>
> >> It may double memory allocation size in some cases. And it is very possible
> >> since there are so many device objects in system.
> >
> > Numbers, please?  If you don't have them, it's just waving your hands.
> 
> It is easily observed and proved. Suppose sizeof(struct foo_dev) is 508bytes,
> it will become 516bytes after your patch applies on 64bit arch, so
> ksize(foo_dev_ptr)
> will become 1024 and the memory consumption of the object is doubled.

I meant real numbers, not made-up ones.

> >> I have explained it before, it is enough to keep the pointer read only
> >> since driver can maintain its internal state in its specific device instance
> >> (for example, usb_interface objects) and decide what to do in 'func'
> >> for situations, right?
> >
> > Yes, it is.  I actually have a patch that does something similar (I'll post it
> > shortly).
> 
> I have seen your patch which moves the 'func' from device into device_driver.
> It is much better than before.

Oh, thanks for letting me know.

> > Of course, it is based on the assumption that func() will always be the same
> > pointer for the given driver, which doesn't seem to be proven, but perhaps
> > it is sufficient.  At least I'm not aware of use cases where it wouldn't be.
> 
> Since you have moved 'func' into device_driver, and you still thought the
> pointer can't be changed after it is set, so why not implement it as callback?

I don't understand what you mean.

It _is_ a callback now in fact.

Thanks,
Rafael