Skip to contentSkip to conversation

usb: ulpi: don't register drivers if bus doesn't exist

PATCH In review Lu Baolu 3 participants 4 messages May 21, 2015 – May 24, 2015

Subsystems (2)
lore
Share & tools
lore

Lu Baolu

author
#0 lore raw

PATCH 1/1 usb: ulpi: don't register drivers if bus doesn't exist

ULPI registers its bus at module_init, so if the bus fails to register, the module will fail to load and all will be well in the world.

However, if the ULPI code is built-in rather than a module, the bus initialization may fail, but we'd still try to register drivers later onto a non-existent bus, which will panic the kernel.

Fix that by checking that the bus was indeed initialized before trying to register drivers on top of it.

---
 drivers/usb/common/ulpi.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)
drivers/usb/common/ulpi.c +5 -1
index 0e6f968..af52b46 100644
@@ -132,6 +132,10 @@ int ulpi_register_driver(struct ulpi_driver *drv)
 	if (!drv->probe)
 		return -EINVAL;
 
+	/* Was the bus registered successfully? */
+	if (unlikely(WARN_ON(!ulpi_bus.p)))
+		return -ENODEV;
+
 	drv->driver.bus = &ulpi_bus;
 
 	return driver_register(&drv->driver);
@@ -242,7 +246,7 @@ static int __init ulpi_init(void)
 {
 	return bus_register(&ulpi_bus);
 }
-module_init(ulpi_init);
+subsys_initcall(ulpi_init);
 
 static void __exit ulpi_exit(void)
 {
Signature · 1 line
2.1.4
author
#1 lore raw

Hi Heikki, Sasha and others,

Please ignore this patch. I should not squash these two patches into one and sign it off behalf of other people. I apologize for this and I'm sorry if this lets you feel affronted.

Thanks,
Baolu

Quoted 43 lines · Lu Baolu

ULPI registers its bus at module_init, so if the bus fails to register, the module will fail to load and all will be well in the world.

However, if the ULPI code is built-in rather than a module, the bus initialization may fail, but we'd still try to register drivers later onto a non-existent bus, which will panic the kernel.

Fix that by checking that the bus was indeed initialized before trying to register drivers on top of it.

---
  drivers/usb/common/ulpi.c | 6 +++++-
  1 file changed, 5 insertions(+), 1 deletion(-)
drivers/usb/common/ulpi.c +5 -1
index 0e6f968..af52b46 100644
@@ -132,6 +132,10 @@ int ulpi_register_driver(struct ulpi_driver *drv)
  	if (!drv->probe)
  		return -EINVAL;
 
+	/* Was the bus registered successfully? */
+	if (unlikely(WARN_ON(!ulpi_bus.p)))
+		return -ENODEV;
+
  	drv->driver.bus = &ulpi_bus;
 
  	return driver_register(&drv->driver);
@@ -242,7 +246,7 @@ static int __init ulpi_init(void)
  {
  	return bus_register(&ulpi_bus);
  }
-module_init(ulpi_init);
+subsys_initcall(ulpi_init);
 
  static void __exit ulpi_exit(void)
  {
maintainer
#2 lore raw
Quoted 6 lines · Baolu

Hi Heikki, Sasha and others,

Please ignore this patch. I should not squash these two patches into one and sign it off behalf of other people. I apologize for this and I'm sorry if this lets
you feel affronted.

No worries. So we'll use patch from Sasha to fix the panic when the bus is missing, and your's to fix the execution order.

If you prepare v2 of your original patch, I'll ACK it.

Thanks,

Signature · 1 line
heikki
#3 lore raw
In reply to Lu Baolu
Quoted 24 lines · Lu Baolu

ULPI registers its bus at module_init, so if the bus fails to register, the module will fail to load and all will be well in the world.

However, if the ULPI code is built-in rather than a module, the bus initialization may fail, but we'd still try to register drivers later onto a non-existent bus, which will panic the kernel.

Fix that by checking that the bus was indeed initialized before trying to register drivers on top of it.

---
 drivers/usb/common/ulpi.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)
drivers/usb/common/ulpi.c +0 -0
index 0e6f968..af52b46 100644

drivers/usb/common/ulpi.c int ulpi_register_driver(struct ulpi_driver *drv) · line 136

Quoted 5 lines
drivers/usb/common/ulpi.c +2 -0
@@ -132,6 +132,10 @@ int ulpi_register_driver(struct ulpi_driver *drv)
 	if (!drv->probe)
 		return -EINVAL;
 
+	/* Was the bus registered successfully? */
+	if (unlikely(WARN_ON(!ulpi_bus.p)))

never use unlikely/likely unless you can actually measure the difference if the marking is not used.

Hint, on driver probe time, you can't.

thanks,

greg k-h