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

[Bjorn Helgaas]

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@xxxxxxxxxx/).
> 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