Re: Thread Safety Analysis and the Linux kernel

[Peter Zijlstra <peterz@infradead.org>]

On Fri, Mar 07, 2025 at 09:52:04AM +0100, Peter Zijlstra wrote:

> Yeah, so IIRC I once proposed a guard that takes a NULL pointer to mean
> not take the lock, but people had a bit of a fit.
> 
> It would've allowed writing the thing like:
> 
> 	{
> 		guard(device)(parent);
> 		device_release_driver(dev);
> 	}

So the below does compile... Greg, how revolted are you? :-)

diff --git a/drivers/base/core.c b/drivers/base/core.c
index 5a1f05198114..7c95e7800b89 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -4796,33 +4796,30 @@ void device_shutdown(void)
 		spin_unlock(&devices_kset->list_lock);
 
 		/* hold lock to avoid race with probe/release */
-		if (parent)
-			device_lock(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);
+		{
+			guard(device_cond)(parent);
+			guard(device)(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 (parent)
-			device_unlock(parent);
-
 		put_device(dev);
 		put_device(parent);
 
diff --git a/include/linux/cleanup.h b/include/linux/cleanup.h
index ec00e3f7af2b..bf72fec6f99b 100644
--- a/include/linux/cleanup.h
+++ b/include/linux/cleanup.h
@@ -300,7 +300,7 @@ static __maybe_unused const bool class_##_name##_is_conditional = _is_cond
 #define DEFINE_GUARD_COND(_name, _ext, _condlock) \
 	__DEFINE_CLASS_IS_CONDITIONAL(_name##_ext, true); \
 	EXTEND_CLASS(_name, _ext, \
-		     ({ void *_t = _T; if (_T && !(_condlock)) _t = NULL; _t; }), \
+		     ({ void *_t = (_condlock) ? _T : NULL; _t; }), \
 		     class_##_name##_t _T) \
 	static inline void * class_##_name##_ext##_lock_ptr(class_##_name##_t *_T) \
 	{ return class_##_name##_lock_ptr(_T); }
diff --git a/include/linux/device.h b/include/linux/device.h
index 80a5b3268986..4e7ebbb7fb64 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -1046,6 +1046,7 @@ static inline void device_unlock(struct device *dev)
 }
 
 DEFINE_GUARD(device, struct device *, device_lock(_T), device_unlock(_T))
+DEFINE_GUARD_COND(device, _cond, (_T ? (device_lock(_T), true) : false))
 
 static inline void device_lock_assert(struct device *dev)
 {