Hi Greg,
On 12/29/21 6:04 PM, Greg Kroah-Hartman wrote:
On Wed, Dec 29, 2021 at 12:51:59PM +0800, Lu Baolu wrote:
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.
What null pointer is being sent to sysfs_remove_link()? For which link?
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.
How are you triggering this failure path and how was it tested?
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.
Fixes: 1901fb2604fbc ("Driver core: fix "driver" symlink timing")
Cc: stable@xxxxxxxxxxxxxxx
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.
Signed-off-by: Lu Baolu <baolu.lu@xxxxxxxxxxxxxxx>
---
drivers/base/dd.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index 68ea1f949daa..9eaaff2f556c 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -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;
Why not call the notifier chain here? Did you verify that this change
still works properly?
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.
thanks,
greg k-h
Best regards,
baolu
