[PATCH v3 1/3] Revert "drivers: core: synchronize really_probe() and dev_uevent()"

[PATCH v3 1/3] Revert "drivers: core: synchronize really_probe() and dev_uevent()"

[Dmitry Torokhov]

This reverts commit c0a40097f0bc81deafc15f9195d1fb54595cd6d0.

Probing a device can take arbitrary long time. In the field we observed
that, for example, probing a bad micro-SD cards in an external USB card
reader (or maybe cards were good but cables were flaky) sometimes takes
longer than 2 minutes due to multiple retries at various levels of the
stack. We can not block uevent_show() method for that long because udev
is reading that attribute very often and that blocks udev and interferes
with booting of the system.

The change that introduced locking was concerned with dev_uevent()
racing with unbinding the driver. However we can handle it without
locking (which will be done in subsequent patch).

There was also claim that synchronization with probe() is needed to
properly load USB drivers, however this is a red herring: the change
adding the lock was introduced in May of last year and USB loading and
probing worked properly for many years before that.

Revert the harmful locking.

Cc: stable@vger.kernel.org
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/base/core.c | 3 ---
 1 file changed, 3 deletions(-)

v3: no changes.

v2: added Cc: stable, no code changes.

diff --git a/drivers/base/core.c b/drivers/base/core.c
index d2f9d3a59d6b..f9c1c623bca5 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -2726,11 +2726,8 @@ static ssize_t uevent_show(struct device *dev, struct device_attribute *attr,
 	if (!env)
 		return -ENOMEM;
 
-	/* Synchronize with really_probe() */
-	device_lock(dev);
 	/* let the kset specific function add its keys */
 	retval = kset->uevent_ops->uevent(&dev->kobj, env);
-	device_unlock(dev);
 	if (retval)
 		goto out;
 
-- 
2.49.0.rc0.332.g42c0ae87b1-goog

[PATCH v3 2/3] driver core: introduce device_set_driver() helper

[Dmitry Torokhov]

In preparation to closing a race when reading driver pointer in
dev_uevent() code, instead of setting device->driver pointer directly
introduce device_set_driver() helper.

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---

v3: new patch introducing device_set_driver() helper (suggested by
Rafael).

 drivers/base/base.h | 6 ++++++
 drivers/base/core.c | 2 +-
 drivers/base/dd.c   | 7 +++----
 3 files changed, 10 insertions(+), 5 deletions(-)

diff --git a/drivers/base/base.h b/drivers/base/base.h
index 0042e4774b0c..eb203cf8370b 100644
--- a/drivers/base/base.h
+++ b/drivers/base/base.h
@@ -180,6 +180,12 @@ int driver_add_groups(const struct device_driver *drv, const struct attribute_gr
 void driver_remove_groups(const struct device_driver *drv, const struct attribute_group **groups);
 void device_driver_detach(struct device *dev);
 
+static inline void device_set_driver(struct device *dev, const struct device_driver *drv)
+{
+	// FIXME - this cast should not be needed "soon"
+	dev->driver = (struct device_driver *)drv;
+}
+
 int devres_release_all(struct device *dev);
 void device_block_probing(void);
 void device_unblock_probing(void);
diff --git a/drivers/base/core.c b/drivers/base/core.c
index f9c1c623bca5..b000ee61c149 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -3697,7 +3697,7 @@ int device_add(struct device *dev)
 	device_pm_remove(dev);
 	dpm_sysfs_remove(dev);
  DPMError:
-	dev->driver = NULL;
+	device_set_driver(dev, NULL);
 	bus_remove_device(dev);
  BusError:
 	device_remove_attrs(dev);
diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index f0e4b4aba885..b526e0e0f52d 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -550,7 +550,7 @@ static void device_unbind_cleanup(struct device *dev)
 	arch_teardown_dma_ops(dev);
 	kfree(dev->dma_range_map);
 	dev->dma_range_map = NULL;
-	dev->driver = NULL;
+	device_set_driver(dev, NULL);
 	dev_set_drvdata(dev, NULL);
 	if (dev->pm_domain && dev->pm_domain->dismiss)
 		dev->pm_domain->dismiss(dev);
@@ -629,8 +629,7 @@ static int really_probe(struct device *dev, const struct device_driver *drv)
 	}
 
 re_probe:
-	// FIXME - this cast should not be needed "soon"
-	dev->driver = (struct device_driver *)drv;
+	device_set_driver(dev, drv);
 
 	/* If using pinctrl, bind pins now before probing */
 	ret = pinctrl_bind_pins(dev);
@@ -1014,7 +1013,7 @@ static int __device_attach(struct device *dev, bool allow_async)
 		if (ret == 0)
 			ret = 1;
 		else {
-			dev->driver = NULL;
+			device_set_driver(dev, NULL);
 			ret = 0;
 		}
 	} else {
-- 
2.49.0.rc0.332.g42c0ae87b1-goog

Re: [PATCH v3 2/3] driver core: introduce device_set_driver() helper

[Masami Hiramatsu]

On Mon, 10 Mar 2025 22:24:15 -0700
Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:

> In preparation to closing a race when reading driver pointer in
> dev_uevent() code, instead of setting device->driver pointer directly
> introduce device_set_driver() helper.
> 
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>

This looks good to me.

Reviewed-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>

Thanks,

> ---
> 
> v3: new patch introducing device_set_driver() helper (suggested by
> Rafael).
> 
>  drivers/base/base.h | 6 ++++++
>  drivers/base/core.c | 2 +-
>  drivers/base/dd.c   | 7 +++----
>  3 files changed, 10 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/base/base.h b/drivers/base/base.h
> index 0042e4774b0c..eb203cf8370b 100644
> --- a/drivers/base/base.h
> +++ b/drivers/base/base.h
> @@ -180,6 +180,12 @@ int driver_add_groups(const struct device_driver *drv, const struct attribute_gr
>  void driver_remove_groups(const struct device_driver *drv, const struct attribute_group **groups);
>  void device_driver_detach(struct device *dev);
>  
> +static inline void device_set_driver(struct device *dev, const struct device_driver *drv)
> +{
> +	// FIXME - this cast should not be needed "soon"
> +	dev->driver = (struct device_driver *)drv;
> +}
> +
>  int devres_release_all(struct device *dev);
>  void device_block_probing(void);
>  void device_unblock_probing(void);
> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index f9c1c623bca5..b000ee61c149 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -3697,7 +3697,7 @@ int device_add(struct device *dev)
>  	device_pm_remove(dev);
>  	dpm_sysfs_remove(dev);
>   DPMError:
> -	dev->driver = NULL;
> +	device_set_driver(dev, NULL);
>  	bus_remove_device(dev);
>   BusError:
>  	device_remove_attrs(dev);
> diff --git a/drivers/base/dd.c b/drivers/base/dd.c
> index f0e4b4aba885..b526e0e0f52d 100644
> --- a/drivers/base/dd.c
> +++ b/drivers/base/dd.c
> @@ -550,7 +550,7 @@ static void device_unbind_cleanup(struct device *dev)
>  	arch_teardown_dma_ops(dev);
>  	kfree(dev->dma_range_map);
>  	dev->dma_range_map = NULL;
> -	dev->driver = NULL;
> +	device_set_driver(dev, NULL);
>  	dev_set_drvdata(dev, NULL);
>  	if (dev->pm_domain && dev->pm_domain->dismiss)
>  		dev->pm_domain->dismiss(dev);
> @@ -629,8 +629,7 @@ static int really_probe(struct device *dev, const struct device_driver *drv)
>  	}
>  
>  re_probe:
> -	// FIXME - this cast should not be needed "soon"
> -	dev->driver = (struct device_driver *)drv;
> +	device_set_driver(dev, drv);
>  
>  	/* If using pinctrl, bind pins now before probing */
>  	ret = pinctrl_bind_pins(dev);
> @@ -1014,7 +1013,7 @@ static int __device_attach(struct device *dev, bool allow_async)
>  		if (ret == 0)
>  			ret = 1;
>  		else {
> -			dev->driver = NULL;
> +			device_set_driver(dev, NULL);
>  			ret = 0;
>  		}
>  	} else {
> -- 
> 2.49.0.rc0.332.g42c0ae87b1-goog
> 


-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>

[PATCH v3 3/3] driver core: fix potential NULL pointer dereference in dev_uevent()

[Dmitry Torokhov]

If userspace reads "uevent" device attribute at the same time as another
threads unbinds the device from its driver, change to dev->driver from a
valid pointer to NULL may result in crash. Fix this by using READ_ONCE()
when fetching the pointer, and take bus' drivers klist lock to make sure
driver instance will not disappear while we access it.

Use WRITE_ONCE() when setting the driver pointer to ensure there is no
tearing.

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---

v3: addressed Rafael's feedback and split device_set_driver() helper
into a separate patch.
 
v2: addressed Rafael's feedback by introducing device_set_driver()
helper that does WRITE_ONCE() to prevent tearing. 

I added Cc: stable however I do not think we need to worry too much
about backporting it to [very] old kernels: the race window is very
small, and in real life we do not unbind devices that often.

I believe there are more questionable places where we read dev->driver
pointer, those need to be adjusted separately.


 drivers/base/base.h | 13 ++++++++++++-
 drivers/base/bus.c  |  2 +-
 drivers/base/core.c | 33 +++++++++++++++++++++++++++++++--
 3 files changed, 44 insertions(+), 4 deletions(-)

diff --git a/drivers/base/base.h b/drivers/base/base.h
index eb203cf8370b..123031a757d9 100644
--- a/drivers/base/base.h
+++ b/drivers/base/base.h
@@ -73,6 +73,7 @@ static inline void subsys_put(struct subsys_private *sp)
 		kset_put(&sp->subsys);
 }
 
+struct subsys_private *bus_to_subsys(const struct bus_type *bus);
 struct subsys_private *class_to_subsys(const struct class *class);
 
 struct driver_private {
@@ -182,8 +183,18 @@ void device_driver_detach(struct device *dev);
 
 static inline void device_set_driver(struct device *dev, const struct device_driver *drv)
 {
+	/*
+	 * Majority (all?) read accesses to dev->driver happens either
+	 * while holding device lock or in bus/driver code that is only
+	 * invoked when the device is bound to a driver and there is no
+	 * concern of the pointer being changed while it is being read.
+	 * However when reading device's uevent file we read driver pointer
+	 * without taking device lock (so we do not block there for
+	 * arbitrary amount of time). We use WRITE_ONCE() here to prevent
+	 * tearing so that READ_ONCE() can safely be used in uevent code.
+	 */
 	// FIXME - this cast should not be needed "soon"
-	dev->driver = (struct device_driver *)drv;
+	WRITE_ONCE(dev->driver, (struct device_driver *)drv);
 }
 
 int devres_release_all(struct device *dev);
diff --git a/drivers/base/bus.c b/drivers/base/bus.c
index 5ea3b03af9ba..5e75e1bce551 100644
--- a/drivers/base/bus.c
+++ b/drivers/base/bus.c
@@ -57,7 +57,7 @@ static int __must_check bus_rescan_devices_helper(struct device *dev,
  * NULL.  A call to subsys_put() must be done when finished with the pointer in
  * order for it to be properly freed.
  */
-static struct subsys_private *bus_to_subsys(const struct bus_type *bus)
+struct subsys_private *bus_to_subsys(const struct bus_type *bus)
 {
 	struct subsys_private *sp = NULL;
 	struct kobject *kobj;
diff --git a/drivers/base/core.c b/drivers/base/core.c
index b000ee61c149..cbc0099d8ef2 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -2624,6 +2624,35 @@ static const char *dev_uevent_name(const struct kobject *kobj)
 	return NULL;
 }
 
+/*
+ * Try filling "DRIVER=<name>" uevent variable for a device. Because this
+ * function may race with binding and unbinding the device from a driver,
+ * we need to be careful. Binding is generally safe, at worst we miss the
+ * fact that the device is already bound to a driver (but the driver
+ * information that is delivered through uevents is best-effort, it may
+ * become obsolete as soon as it is generated anyways). Unbinding is more
+ * risky as driver pointer is transitioning to NULL, so READ_ONCE() should
+ * be used to make sure we are dealing with the same pointer, and to
+ * ensure that driver structure is not going to disappear from under us
+ * we take bus' drivers klist lock. The assumption that only registered
+ * driver can be bound to a device, and to unregister a driver bus code
+ * will take the same lock.
+ */
+static void dev_driver_uevent(const struct device *dev, struct kobj_uevent_env *env)
+{
+	struct subsys_private *sp = bus_to_subsys(dev->bus);
+
+	if (sp) {
+		scoped_guard(spinlock, &sp->klist_drivers.k_lock) {
+			struct device_driver *drv = READ_ONCE(dev->driver);
+			if (drv)
+				add_uevent_var(env, "DRIVER=%s", drv->name);
+		}
+
+		subsys_put(sp);
+	}
+}
+
 static int dev_uevent(const struct kobject *kobj, struct kobj_uevent_env *env)
 {
 	const struct device *dev = kobj_to_dev(kobj);
@@ -2655,8 +2684,8 @@ static int dev_uevent(const struct kobject *kobj, struct kobj_uevent_env *env)
 	if (dev->type && dev->type->name)
 		add_uevent_var(env, "DEVTYPE=%s", dev->type->name);
 
-	if (dev->driver)
-		add_uevent_var(env, "DRIVER=%s", dev->driver->name);
+	/* Add "DRIVER=%s" variable if the device is bound to a driver */
+	dev_driver_uevent(dev, env);
 
 	/* Add common DT information about the device */
 	of_device_uevent(dev, env);
-- 
2.49.0.rc0.332.g42c0ae87b1-goog

Re: [PATCH v3 3/3] driver core: fix potential NULL pointer dereference in dev_uevent()

[Masami Hiramatsu]

On Mon, 10 Mar 2025 22:24:16 -0700
Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:

> If userspace reads "uevent" device attribute at the same time as another
> threads unbinds the device from its driver, change to dev->driver from a
> valid pointer to NULL may result in crash. Fix this by using READ_ONCE()
> when fetching the pointer, and take bus' drivers klist lock to make sure
> driver instance will not disappear while we access it.
> 
> Use WRITE_ONCE() when setting the driver pointer to ensure there is no
> tearing.
> 

This looks good to me.

Reviewed-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>

BTW, since other drivers still assigns dev->driver directly instead
of using device_set_driver(). Would we need another patch to replace
those?

Thank you,

> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---
> 
> v3: addressed Rafael's feedback and split device_set_driver() helper
> into a separate patch.
>  
> v2: addressed Rafael's feedback by introducing device_set_driver()
> helper that does WRITE_ONCE() to prevent tearing. 
> 
> I added Cc: stable however I do not think we need to worry too much
> about backporting it to [very] old kernels: the race window is very
> small, and in real life we do not unbind devices that often.
> 
> I believe there are more questionable places where we read dev->driver
> pointer, those need to be adjusted separately.
> 
> 
>  drivers/base/base.h | 13 ++++++++++++-
>  drivers/base/bus.c  |  2 +-
>  drivers/base/core.c | 33 +++++++++++++++++++++++++++++++--
>  3 files changed, 44 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/base/base.h b/drivers/base/base.h
> index eb203cf8370b..123031a757d9 100644
> --- a/drivers/base/base.h
> +++ b/drivers/base/base.h
> @@ -73,6 +73,7 @@ static inline void subsys_put(struct subsys_private *sp)
>  		kset_put(&sp->subsys);
>  }
>  
> +struct subsys_private *bus_to_subsys(const struct bus_type *bus);
>  struct subsys_private *class_to_subsys(const struct class *class);
>  
>  struct driver_private {
> @@ -182,8 +183,18 @@ void device_driver_detach(struct device *dev);
>  
>  static inline void device_set_driver(struct device *dev, const struct device_driver *drv)
>  {
> +	/*
> +	 * Majority (all?) read accesses to dev->driver happens either
> +	 * while holding device lock or in bus/driver code that is only
> +	 * invoked when the device is bound to a driver and there is no
> +	 * concern of the pointer being changed while it is being read.
> +	 * However when reading device's uevent file we read driver pointer
> +	 * without taking device lock (so we do not block there for
> +	 * arbitrary amount of time). We use WRITE_ONCE() here to prevent
> +	 * tearing so that READ_ONCE() can safely be used in uevent code.
> +	 */
>  	// FIXME - this cast should not be needed "soon"
> -	dev->driver = (struct device_driver *)drv;
> +	WRITE_ONCE(dev->driver, (struct device_driver *)drv);
>  }
>  
>  int devres_release_all(struct device *dev);
> diff --git a/drivers/base/bus.c b/drivers/base/bus.c
> index 5ea3b03af9ba..5e75e1bce551 100644
> --- a/drivers/base/bus.c
> +++ b/drivers/base/bus.c
> @@ -57,7 +57,7 @@ static int __must_check bus_rescan_devices_helper(struct device *dev,
>   * NULL.  A call to subsys_put() must be done when finished with the pointer in
>   * order for it to be properly freed.
>   */
> -static struct subsys_private *bus_to_subsys(const struct bus_type *bus)
> +struct subsys_private *bus_to_subsys(const struct bus_type *bus)
>  {
>  	struct subsys_private *sp = NULL;
>  	struct kobject *kobj;
> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index b000ee61c149..cbc0099d8ef2 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -2624,6 +2624,35 @@ static const char *dev_uevent_name(const struct kobject *kobj)
>  	return NULL;
>  }
>  
> +/*
> + * Try filling "DRIVER=<name>" uevent variable for a device. Because this
> + * function may race with binding and unbinding the device from a driver,
> + * we need to be careful. Binding is generally safe, at worst we miss the
> + * fact that the device is already bound to a driver (but the driver
> + * information that is delivered through uevents is best-effort, it may
> + * become obsolete as soon as it is generated anyways). Unbinding is more
> + * risky as driver pointer is transitioning to NULL, so READ_ONCE() should
> + * be used to make sure we are dealing with the same pointer, and to
> + * ensure that driver structure is not going to disappear from under us
> + * we take bus' drivers klist lock. The assumption that only registered
> + * driver can be bound to a device, and to unregister a driver bus code
> + * will take the same lock.
> + */
> +static void dev_driver_uevent(const struct device *dev, struct kobj_uevent_env *env)
> +{
> +	struct subsys_private *sp = bus_to_subsys(dev->bus);
> +
> +	if (sp) {
> +		scoped_guard(spinlock, &sp->klist_drivers.k_lock) {
> +			struct device_driver *drv = READ_ONCE(dev->driver);
> +			if (drv)
> +				add_uevent_var(env, "DRIVER=%s", drv->name);
> +		}
> +
> +		subsys_put(sp);
> +	}
> +}
> +
>  static int dev_uevent(const struct kobject *kobj, struct kobj_uevent_env *env)
>  {
>  	const struct device *dev = kobj_to_dev(kobj);
> @@ -2655,8 +2684,8 @@ static int dev_uevent(const struct kobject *kobj, struct kobj_uevent_env *env)
>  	if (dev->type && dev->type->name)
>  		add_uevent_var(env, "DEVTYPE=%s", dev->type->name);
>  
> -	if (dev->driver)
> -		add_uevent_var(env, "DRIVER=%s", dev->driver->name);
> +	/* Add "DRIVER=%s" variable if the device is bound to a driver */
> +	dev_driver_uevent(dev, env);
>  
>  	/* Add common DT information about the device */
>  	of_device_uevent(dev, env);
> -- 
> 2.49.0.rc0.332.g42c0ae87b1-goog
> 


-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>

Re: [PATCH v3 1/3] Revert "drivers: core: synchronize really_probe() and dev_uevent()"

[Masami Hiramatsu]

On Mon, 10 Mar 2025 22:24:14 -0700
Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:

> This reverts commit c0a40097f0bc81deafc15f9195d1fb54595cd6d0.
> 
> Probing a device can take arbitrary long time. In the field we observed
> that, for example, probing a bad micro-SD cards in an external USB card
> reader (or maybe cards were good but cables were flaky) sometimes takes
> longer than 2 minutes due to multiple retries at various levels of the
> stack. We can not block uevent_show() method for that long because udev
> is reading that attribute very often and that blocks udev and interferes
> with booting of the system.
> 
> The change that introduced locking was concerned with dev_uevent()
> racing with unbinding the driver. However we can handle it without
> locking (which will be done in subsequent patch).
> 
> There was also claim that synchronization with probe() is needed to
> properly load USB drivers, however this is a red herring: the change
> adding the lock was introduced in May of last year and USB loading and
> probing worked properly for many years before that.
> 
> Revert the harmful locking.
> 

Reviewed-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>

Thanks,

> Cc: stable@vger.kernel.org
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---
>  drivers/base/core.c | 3 ---
>  1 file changed, 3 deletions(-)
> 
> v3: no changes.
> 
> v2: added Cc: stable, no code changes.
> 
> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index d2f9d3a59d6b..f9c1c623bca5 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -2726,11 +2726,8 @@ static ssize_t uevent_show(struct device *dev, struct device_attribute *attr,
>  	if (!env)
>  		return -ENOMEM;
>  
> -	/* Synchronize with really_probe() */
> -	device_lock(dev);
>  	/* let the kset specific function add its keys */
>  	retval = kset->uevent_ops->uevent(&dev->kobj, env);
> -	device_unlock(dev);
>  	if (retval)
>  		goto out;
>  
> -- 
> 2.49.0.rc0.332.g42c0ae87b1-goog
> 


-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>

Re: [PATCH v3 1/3] Revert "drivers: core: synchronize really_probe() and dev_uevent()"

[Aditya Garg]

> On 11 Mar 2025, at 10:54 AM, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
> 
> This reverts commit c0a40097f0bc81deafc15f9195d1fb54595cd6d0.
> 
> Probing a device can take arbitrary long time. In the field we observed
> that, for example, probing a bad micro-SD cards in an external USB card
> reader (or maybe cards were good but cables were flaky) sometimes takes
> longer than 2 minutes due to multiple retries at various levels of the
> stack. We can not block uevent_show() method for that long because udev
> is reading that attribute very often and that blocks udev and interferes
> with booting of the system.
> 
> The change that introduced locking was concerned with dev_uevent()
> racing with unbinding the driver. However we can handle it without
> locking (which will be done in subsequent patch).
> 
> There was also claim that synchronization with probe() is needed to
> properly load USB drivers, however this is a red herring: the change
> adding the lock was introduced in May of last year and USB loading and
> probing worked properly for many years before that.
> 
> Revert the harmful locking.
> 
> Cc: stable@vger.kernel.org
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---

Hi

For sometime users of the appletbdrm driver used for Touch Bar support on T2 Macs (https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/drm/tiny/appletbdrm.c?h=v7.0-rc4) have been facing a regression that caused the Touch Bar to no longer work on resume. A person tried to bisect and this commit was found to be the reason.

The person has tried to explain the whole situation in this GitHub issue: https://github.com/t2linux/wiki/issues/635#issuecomment-4092907335

And this seems to be the most plausible explanation: https://github.com/t2linux/wiki/issues/635#issuecomment-4071720148

Thanks
Aditya

Re: [PATCH v3 1/3] Revert "drivers: core: synchronize really_probe() and dev_uevent()"

[Dmitry Torokhov]

On Fri, Mar 20, 2026 at 06:16:33AM +0000, Aditya Garg wrote:
> 
> 
> > On 11 Mar 2025, at 10:54 AM, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
> > 
> > This reverts commit c0a40097f0bc81deafc15f9195d1fb54595cd6d0.
> > 
> > Probing a device can take arbitrary long time. In the field we observed
> > that, for example, probing a bad micro-SD cards in an external USB card
> > reader (or maybe cards were good but cables were flaky) sometimes takes
> > longer than 2 minutes due to multiple retries at various levels of the
> > stack. We can not block uevent_show() method for that long because udev
> > is reading that attribute very often and that blocks udev and interferes
> > with booting of the system.
> > 
> > The change that introduced locking was concerned with dev_uevent()
> > racing with unbinding the driver. However we can handle it without
> > locking (which will be done in subsequent patch).
> > 
> > There was also claim that synchronization with probe() is needed to
> > properly load USB drivers, however this is a red herring: the change
> > adding the lock was introduced in May of last year and USB loading and
> > probing worked properly for many years before that.
> > 
> > Revert the harmful locking.
> > 
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> > ---
> 
> Hi
> 

> For sometime users of the appletbdrm driver used for Touch Bar support
> on T2 Macs
> (https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/drm/tiny/appletbdrm.c?h=v7.0-rc4)
> have been facing a regression that caused the Touch Bar to no longer
> work on resume. A person tried to bisect and this commit was found to
> be the reason.
> 
> The person has tried to explain the whole situation in this GitHub
> issue:
> https://github.com/t2linux/wiki/issues/635#issuecomment-4092907335
> 
> And this seems to be the most plausible explanation:
> https://github.com/t2linux/wiki/issues/635#issuecomment-4071720148

Hi,

I believe the person who did the root cause analysis told exactly how to
fix the issue. Your driver should not assume that the device is in given
configuration and if the device needs to be transitioned into particular
configuration which given timing it is on your driver to implement it.

It appears you have rule that triggers on "ADD" event for the device to
not only load the driver, but also toggle the configuration, and relied
on it be executed only after initial probe (that sets configuration 0)
is done. This is not supported and should not be expected to work. There
are dedicated "bind" and "unbind" events to signal when a device binds
or unbinds from a driver, but again, the proper fix is actually in your
driver's probe to configure the device properly.

Thanks.

-- 
Dmitry