On Mon, 7 May 2018, I wrote:
On Sun, 6 May 2018, Greg Kroah-Hartman wrote:
Why not just have an "bus is registered" flag in your driver
register function that refuses to let drivers register with the
driver core if it isn't set?
Perhaps that should happen in the core driver_register() function.
BUG_ON is frowned upon, after all. Would that be acceptable?
I don't understand what you mean here, perhaps make a patch to show it?
As an alternative to your suggestion (add flag to avoid the BUG_ON):
--- a/drivers/base/driver.c
+++ b/drivers/base/driver.c
@@ -148,7 +148,10 @@ int driver_register(struct device_driver *drv)
int ret;
struct device_driver *other;
- BUG_ON(!drv->bus->p);
+ if (!drv->bus->p) {
+ WARN_ONCE(1, "Cannot register driver with invalid bus\n");
+ return -EPROBE_DEFER;
+ }
if ((drv->bus->probe && drv->probe) ||
(drv->bus->remove && drv->remove) ||
That rushed example I gave above seems to be confusing the issue. Sorry
about that. (See sioux-core.c for the code that motivated it.)
This example is the sort of flag removal that I had in mind --
diff --git a/drivers/base/driver.c b/drivers/base/driver.c
index ba912558a510..4ee22fb3db92 100644
--- a/drivers/base/driver.c
+++ b/drivers/base/driver.c
@@ -148,7 +148,8 @@ int driver_register(struct device_driver *drv)
int ret;
struct device_driver *other;
- BUG_ON(!drv->bus->p);
+ if (!drv->bus->p)
+ return -ENODEV;
if ((drv->bus->probe && drv->probe) ||
(drv->bus->remove && drv->remove) ||
diff --git a/drivers/visorbus/visorbus_main.c b/drivers/visorbus/visorbus_main.c
index 0b2434cc4ecd..73ff294fd449 100644
--- a/drivers/visorbus/visorbus_main.c
+++ b/drivers/visorbus/visorbus_main.c
@@ -19,8 +19,6 @@ static const guid_t visor_vbus_channel_guid = VISOR_VBUS_CHANNEL_GUID;
#define LINESIZE 99
#define POLLJIFFIES_NORMALCHANNEL 10
-/* stores whether bus_registration was successful */
-static bool initialized;
static struct dentry *visorbus_debugfs_dir;
/*
@@ -965,9 +963,6 @@ static int visordriver_probe_device(struct device *xdev)
*/
int visorbus_register_visor_driver(struct visor_driver *drv)
{
- /* can't register on a nonexistent bus */
- if (!initialized)
- return -ENODEV;
if (!drv->probe)
return -EINVAL;
if (!drv->remove)
@@ -1212,7 +1207,6 @@ int visorbus_init(void)
err = bus_register(&visorbus_type);
if (err < 0)
return err;
- initialized = true;
bus_device_info_init(&chipset_driverinfo, "chipset", "visorchipset");
return 0;
}
@@ -1229,6 +1223,5 @@ void visorbus_exit(void)
visorbus_remove_instance(dev);
}
bus_unregister(&visorbus_type);
- initialized = false;
debugfs_remove_recursive(visorbus_debugfs_dir);
}
It's very hard to find examples of this pattern, where a flag is used to
inhibit driver_register() calls. As a method of avoiding the BUG_ON this
pattern is not popular.
Hence, the opportunities for flag removal that I mentioned are similarly
rare. So this kind of change is not something I would propose.
Instead I would prefer either of the two solution I've previously sent,
though any one of the 3 would actually resolve the bug reported by
Michael.
In anycase, I'm happy to work on a new solution if it would be more
acceptable. Would you please explain how changing the link order would
avoid the modprobe crash? I still can't figure it out.
--
--
To unsubscribe from this list: send the line "unsubscribe linux-m68k" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
