Re: [PATCH] drivers: base: add support to skip power management in device/driver model

[Ulf Hansson]

On Thu, 7 Feb 2019 at 11:36, Sudeep Holla <sudeep.holla@xxxxxxx> wrote:
>
> On Thu, Feb 07, 2019 at 10:53:56AM +0100, Ulf Hansson wrote:
> > On Wed, 6 Feb 2019 at 16:09, Sudeep Holla <sudeep.holla@xxxxxxx> wrote:
> > >
> > > All device objects in the driver model contain fields that control the
> > > handling of various power management activities. However, it's not
> > > always useful. There are few instances where pseudo devices are added
> > > to the model just to take advantage of many other features like
> > > kobjects, udev events, and so on. One such example is cpu devices and
> > > their caches.
> > >
> > > The sysfs for the cpu caches are managed by adding devices with cpu
> > > as the parent in cpu_device_create() when secondary cpu is brought
> > > online. Generally when the secondary CPUs are hotplugged back in as part
> > > of resume from suspend-to-ram, we call cpu_device_create() from the cpu
> > > hotplug state machine while the cpu device associated with that CPU is
> > > not yet ready to be resumed as the device_resume() call happens bit
> > > later. It's not really needed to set the flag is_prepared for cpu
> > > devices as they are mostly pseudo device and hotplug framework deals
> > > with state machine and not managed through the cpu device.
> >
> > What's the point of removing and then re-adding the sysfs attributes
> > for the cpu caches, at each hotplug off/on sequence? To me that sounds
> > inefficient and unnecessary, no?
> >
>
> Oh well, you must look at PowerPC pHyp which can change cache nodes for
> suspend/resume operation too, let alone hotplug operations. So, we can't
> assume the same CPUs with exact same caches are always hotplugged back
> in. That's may happen in embedded and other static platforms but not
> everywhere. Anyway that's my understand as why it's done in hotplug
> patch. So it's not so simple to call it inefficient and unnecessary IMO.

Okay, I see.

>
> > If you avoid this, would that solve this problem?
> >
>
> May be, but as mentioned above we can't really. Also this change will
> help to avoid creating unnecessary power sysfs which is mainly runtime
> pm related for some of the devices created. CPU/caches was just one
> example which triggered this, but this can be more useful. We can avoid
> adding them to dpm list.

Well, to me the approach you suggest sounds prone to errors and I am
afraid people may abuse it. Moreover, I don't know if there is other
problems with it, let's see what Rafael thinks about it.

Instead I think we should make the PM core to deal with this scenario,
as all it boils down to, is to allow a device to be unregistered and
registered during system suspend/resume, with a parent device that is
"persistent" during the sequence.

Perhaps we could even just drop the corresponding printed warning,
"cache: parent cpu1 should not be sleeping", in device_pm_add() as I
wonder if it's really a necessary print.

Kind regards
Uffe