Re: [PATCH 2/5] driver core: separate function to shutdown one device

Bjorn Helgaas
2026-03-11T21:37:30Z

On Wed, Mar 11, 2026 at 11:00:11AM -0700, Bart Van Assche wrote:
> On 3/11/26 10:12 AM, David Jeffery wrote:
> > +static void shutdown_one_device(struct device *dev)
> > +{
> > + /* hold lock to avoid race with probe/release */
> > + if (dev->parent && dev->bus && dev->bus->need_parent_lock)
> > + device_lock(dev->parent);
> > + device_lock(dev);
> > +
> > + /* Don't allow any more runtime suspends */
> > + pm_runtime_get_noresume(dev);
> > + pm_runtime_barrier(dev);
> > +
> > + if (dev->class && dev->class->shutdown_pre) {
> > + if (initcall_debug)
> > + dev_info(dev, "shutdown_pre\n");
> > + dev->class->shutdown_pre(dev);
> > + }
> > + if (dev->bus && dev->bus->shutdown) {
> > + if (initcall_debug)
> > + dev_info(dev, "shutdown\n");
> > + dev->bus->shutdown(dev);
> > + } else if (dev->driver && dev->driver->shutdown) {
> > + if (initcall_debug)
> > + dev_info(dev, "shutdown\n");
> > + dev->driver->shutdown(dev);
> > + }
> > +
> > + device_unlock(dev);
> > + if (dev->parent && dev->bus && dev->bus->need_parent_lock)
> > + device_unlock(dev->parent);
> > +
> > + put_device(dev->parent);
> > + put_device(dev);
> > +}
>
> Please keep the following code in the caller:
>
> if (dev->parent && dev->bus && dev->bus->need_parent_lock)
> device_lock(dev->parent);
>
> if (dev->parent && dev->bus && dev->bus->need_parent_lock)
> device_unlock(dev->parent);
>
> put_device(dev->parent);
> put_device(dev);

Can you elaborate on this a little bit? I can see that doing this in
the caller is simpler in some ways, although there are two callers
that would need this. Maybe it's just the lock anti-pattern below?

> Additionally, please make sure that the caller is made compatible with
> lock context analysis (see also
> https://lore.kernel.org/all/20250206181711.1902989-1-elver@google.com/).
> All that is required to make this code compatible with lock context
> analysis is to organize it as follows:
>
> if (dev->parent && dev->bus && dev->bus->need_parent_lock) {
> device_lock(dev->parent);
> shutdown_one_device(dev);
> device_unlock(dev->parent);
> } else {
> shutdown_one_device(dev);
> }

I guess avoiding the "conditional acquisition and later conditional
release" pattern mentioned at [1] is what makes this compatible with
lock context analysis?

I guess this is another way of expressing the "no conditionally held
locks" rule [2], which is more concise and fits better in my pea
brain.

[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/dev-tools/context-analysis.rst?id=v7.0-rc1#n42
[2] https://clang.llvm.org/docs/ThreadSafetyAnalysis.html#no-conditionally-held-locks