driver core: Fix driver_sysfs_remove() order in really_probe()

The driver_sysfs_remove() should always be called after successful driver_sysfs_add(). Otherwise, NULL pointers will be passed to the sysfs_remove_link(), where it is decoded as searching sysfs root.

---
 drivers/base/dd.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

index 68ea1f949daa..9eaaff2f556c 100644
@@ -577,14 +577,14 @@ static int really_probe(struct device *dev, struct device_driver *drv)
 	if (dev->bus->dma_configure) {
 		ret = dev->bus->dma_configure(dev);
 		if (ret)
-			goto probe_failed;
+			goto pinctrl_bind_failed;
 	}
 
 	ret = driver_sysfs_add(dev);
 	if (ret) {
 		pr_err("%s: driver_sysfs_add(%s) failed\n",
 		       __func__, dev_name(dev));
-		goto probe_failed;
+		goto sysfs_failed;
 	}
 
 	if (dev->pm_domain && dev->pm_domain->activate) {

@@ -657,6 +657,8 @@ static int really_probe(struct device *dev, struct device_driver *drv)
 	else if (drv->remove)
 		drv->remove(dev);
 probe_failed:
+	driver_sysfs_remove(dev);
+sysfs_failed:
 	if (dev->bus)
 		blocking_notifier_call_chain(&dev->bus->p->bus_notifier,
 					     BUS_NOTIFY_DRIVER_NOT_BOUND, dev);

@@ -666,7 +668,6 @@ static int really_probe(struct device *dev, struct device_driver *drv)
 	arch_teardown_dma_ops(dev);
 	kfree(dev->dma_range_map);
 	dev->dma_range_map = NULL;
-	driver_sysfs_remove(dev);
 	dev->driver = NULL;
 	dev_set_drvdata(dev, NULL);
 	if (dev->pm_domain && dev->pm_domain->dismiss)

2.25.1

What null pointer is being sent to sysfs_remove_link()? For which link?

How are you triggering this failure path and how was it tested?

Hi Greg,

Oh, my fault. Thank you for pointing this out.

The device and driver sysfs nodes have already been created, so there's no null pointers. The out-of-order call of driver_sysfs_remove() just tries to remove some nonexistent nodes under the device and driver sysfs nodes. It is allowed by the sysfs layer.

I hacked the a driver to return failure in dma_configure() callback. I didn't see any failure. But I mistakenly thought that driver_sysfs_remove() could possibly delete some sysfs entries by mistake. That's not true. Sorry for the noise.

This patch only improves the readability of really_probe() and it does not fix any bugs. I will remove above tags and resent a version if you think this improvement is valuable.

The BUS_NOTIFY_DRIVER_NOT_BOUND event is listened in two places in the tree.

$ git grep BUS_NOTIFY_DRIVER_NOT_BOUND -- :^drivers/base/dd.c :^include
drivers/acpi/acpi_lpss.c:       case BUS_NOTIFY_DRIVER_NOT_BOUND:
drivers/base/power/clock_ops.c: case BUS_NOTIFY_DRIVER_NOT_BOUND:

The usage pattern is setting up something in BUS_NOTIFY_BIND_DRIVER and doing the cleanup in BUS_NOTIFY_DRIVER_NOT_BOUND or BUS_NOTIFY_UNBIND_DRIVER. The right order of these events should be

[failure case]
- BUS_NOTIFY_BIND_DRIVER: driver is about to be bound
- BUS_NOTIFY_DRIVER_NOT_BOUND: driver failed to be bound

or

[successful case]
- BUS_NOTIFY_BIND_DRIVER: driver is about to be bound
- BUS_NOTIFY_BOUND_DRIVER: driver bound to device
- BUS_NOTIFY_UNBIND_DRIVER: driver is about to be unbound
- BUS_NOTIFY_UNBOUND_DRIVER: driver is unbound from the device

Without above change, when dma_configure() returns failure, the listener could get a BUS_NOTIFY_DRIVER_NOT_BOUND without BUS_NOTIFY_BIND_DRIVER.

Please guide me if my understanding is wrong.

Best regards,
baolu