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

From: Ming Lei <tom.leiming@gmail.com>
To: "Rafael J. Wysocki" <rjw@sisk.pl>
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 11:19:56 +0800	[thread overview]
Message-ID: <CACVXFVPMaTT4s7aS8mdNjgm3i3GFJ6gqFhFxZKuooF0mqi45rA@mail.gmail.com> (raw)
In-Reply-To: <201208092141.35589.rjw@sisk.pl>

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.

>
>> >> 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 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.

> 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?

IMO, it is a bit weird to just store a function pointer data(not for
callback) in
driver object, but anyway, it is better than before, :-)

Thanks,
-- 
Ming Lei