Re: [PATCH] driver core: Ensure proper suspend/resume ordering

[Thierry Reding]

On Sat, Sep 19, 2015 at 01:07:56AM +0200, Rafael J. Wysocki wrote:
> On Fri, Sep 18, 2015 at 5:55 PM, Thierry Reding <thierry.reding@xxxxxxxxx> wrote:
[...]
> > Of course there's still the matter of some types of devices physically
> > disappearing (USB, PCI, ...).
> 
> Right.  In some cases removal is simply necessary as part of the
> cleanup, like after a surprise hot-unplug of a device, for example.
> In those cases everything that depended on the device that went away
> should be unbound from drivers at least IMO.

Agreed.

> > Force-removing drivers that depend on a device that's being unbound
> > would be a possibility to solve the problem where consumers depend on a
> > device that could physically go away. It might also be the right thing
> > to do in any case. Presumably somebody unloading a module want to do
> > just that, and refusing to do so isn't playing very nice. Of course
> > allowing random modules to be removed even if a lot of consumers might
> > depend on it may not be friendly either. Consider if you unload a GPIO
> > driver that provides a pin that's used to enable power to an eMMC that
> > might have the root filesystem.
> >
> > Then again, if you unload a module you better know what you're doing
> > anyway, so maybe that's not something we need to be concerned about.
> 
> I think that it's better to fail module unloads in such cases by
> default (to prevent simple silly mistakes from having possibly severe
> consequences), but if a "force" option is used, we should regard that
> as "the user really means it" and do as requested.  That would be very
> much analogous to the hot-unplug situation from the software
> perspective.

Sounds very reasonable to me.

> > I think this would also tie in nicely with Tomeu's patch set to do
> > on-demand probing. Essentially a [dev_]*_get() call could in turn call
> > this new "declare dependency" API, and the new API could underneath do
> > on-demand probing.
> >
> > Given that this isn't a strictly PM mechanism anymore, perhaps something
> > like:
> >
> >         int device_depend(struct device *dev, struct device *target);
> >
> > would be a more generic option.
> 
> I thought about something like link_device(dev, target, flags), where
> the last argument would indicate what the core is supposed to use the
> link for (removal handling, system suspend/resume, runtime PM etc).

Sounds good to me. I think the core isn't quite consistent on the naming
of functions, so we have things like device_register/unregister() versus
get/put_device(). I'd lean towards device_link(dev, target, flags), but
I'll go with any color you'd like the shed to have.

> And I agree that this isn't really PM-specific.
> 
> OK, thanks a lot for the feedback!
> 
> Let me think about that a bit more and I'll try to come up with a more
> detailed design description.

This sounds like it's not going to make it into v4.3 anymore, so I'll
need to think about the easiest way to (temporarily) fix up the current
regression.

Is this something that you will have time to implement yourself? If so,
please keep me in the loop and Cc me on any patches that you need
tested. If you're short on time, let me know as well and I'll see if I
can take a stab at it myself, though I'm pretty sure I'll need further
guidance along the way.

Thierry

Attachment: signature.asc
Description: PGP signature