| Message ID | 20200731202154.11382-2-ddadap@nvidia.com (mailing list archive) |
|---|---|
| State | Rejected, archived |
| Headers | show |
| Series | [v2,1/2] platform/x86: Add driver for ACPI WMAA EC-based backlight control | expand |
> -----Original Message----- > From: Daniel Dadap <ddadap@nvidia.com> > Sent: Friday, July 31, 2020 3:22 PM > To: platform-driver-x86@vger.kernel.org; Limonciello, Mario; > pobrn@protonmail.com > Cc: andy@infradead.org; dvhart@infradead.org; aplattner@nvidia.com; Daniel > Dadap > Subject: [PATCH v2 2/2] platform/x86: wmi: fail wmi_driver_register when no > GUID is found > > > [EXTERNAL EMAIL] > > If a driver registers with WMI, and none of the GUIDs in its ID table > is present on the system, then that driver will not be probed and > automatically loaded. However, it is still possible to load such a > driver explicitly on a system which doesn't include the relevant > hardware. > > Update wmi_driver_register to test for the presence of at least one > GUID from the driver's ID table at driver registration time, and > fail registration if none are found. > > Signed-off-by: Daniel Dadap <ddadap@nvidia.com> > --- > drivers/platform/x86/wmi.c | 18 ++++++++++++++++++ > 1 file changed, 18 insertions(+) > > diff --git a/drivers/platform/x86/wmi.c b/drivers/platform/x86/wmi.c > index 941739db7199..19aa23d1cf8e 100644 > --- a/drivers/platform/x86/wmi.c > +++ b/drivers/platform/x86/wmi.c > @@ -130,6 +130,21 @@ static bool find_guid(const char *guid_string, struct > wmi_block **out) > return false; > } > > +static bool find_driver_guid(const struct wmi_driver *wdriver) > +{ > + const struct wmi_device_id *id; > + > + if (wdriver == NULL) > + return false; > + > + for (id = wdriver->id_table; *id->guid_string; id++) { > + if (find_guid(id->guid_string, NULL)) > + return true; > + } > + > + return false; > +} > + As a point of preference, why not in this function return -ENODEV directly rather than it be boolean and map all errors to -ENODEV? > static const void *find_guid_context(struct wmi_block *wblock, > struct wmi_driver *wdriver) > { > @@ -1419,6 +1434,9 @@ static int acpi_wmi_probe(struct platform_device > *device) > int __must_check __wmi_driver_register(struct wmi_driver *driver, > struct module *owner) > { > + if (!find_driver_guid(driver)) > + return -ENODEV; > + Then the logic here can be something like: ret = find_driver_guid(driver); if (ret) return ret; > driver->driver.owner = owner; > driver->driver.bus = &wmi_bus_type; > > -- > 2.18.4
2020. július 31., péntek 22:21 keltezéssel, Daniel Dadap <ddadap@nvidia.com> írta: > If a driver registers with WMI, and none of the GUIDs in its ID table > is present on the system, then that driver will not be probed and > automatically loaded. However, it is still possible to load such a > driver explicitly on a system which doesn't include the relevant > hardware. > > Update wmi_driver_register to test for the presence of at least one > GUID from the driver's ID table at driver registration time, and > fail registration if none are found. > > Signed-off-by: Daniel Dadap <ddadap@nvidia.com> > --- > drivers/platform/x86/wmi.c | 18 ++++++++++++++++++ > 1 file changed, 18 insertions(+) > > diff --git a/drivers/platform/x86/wmi.c b/drivers/platform/x86/wmi.c > index 941739db7199..19aa23d1cf8e 100644 > --- a/drivers/platform/x86/wmi.c > +++ b/drivers/platform/x86/wmi.c > @@ -130,6 +130,21 @@ static bool find_guid(const char *guid_string, struct wmi_block **out) > return false; > } > > +static bool find_driver_guid(const struct wmi_driver *wdriver) > +{ > + const struct wmi_device_id *id; > + > + if (wdriver == NULL) > + return false; > + > + for (id = wdriver->id_table; *id->guid_string; id++) { > + if (find_guid(id->guid_string, NULL)) > + return true; > + } > + > + return false; > +} > + > static const void *find_guid_context(struct wmi_block *wblock, > struct wmi_driver *wdriver) > { > @@ -1419,6 +1434,9 @@ static int acpi_wmi_probe(struct platform_device *device) > int __must_check __wmi_driver_register(struct wmi_driver *driver, > struct module *owner) > { > + if (!find_driver_guid(driver)) > + return -ENODEV; > + > driver->driver.owner = owner; > driver->driver.bus = &wmi_bus_type; > > -- > 2.18.4 Can you elaborate as to why this change in behaviour of the wmi driver is needed? If I understand it correctly you want to prevent WMI drivers from successfully loading when there are no matching GUIDs. Is that correct? Barnabás Pőcze
On 7/31/20 3:56 PM, Barnabás Pőcze wrote: > > 2020. július 31., péntek 22:21 keltezéssel, Daniel Dadap <ddadap@nvidia.com> írta: >> If a driver registers with WMI, and none of the GUIDs in its ID table >> is present on the system, then that driver will not be probed and >> automatically loaded. However, it is still possible to load such a >> driver explicitly on a system which doesn't include the relevant >> hardware. >> >> Update wmi_driver_register to test for the presence of at least one >> GUID from the driver's ID table at driver registration time, and >> fail registration if none are found. >> >> Signed-off-by: Daniel Dadap <ddadap@nvidia.com> >> --- >> drivers/platform/x86/wmi.c | 18 ++++++++++++++++++ >> 1 file changed, 18 insertions(+) >> >> diff --git a/drivers/platform/x86/wmi.c b/drivers/platform/x86/wmi.c >> index 941739db7199..19aa23d1cf8e 100644 >> --- a/drivers/platform/x86/wmi.c >> +++ b/drivers/platform/x86/wmi.c >> @@ -130,6 +130,21 @@ static bool find_guid(const char *guid_string, struct wmi_block **out) >> return false; >> } >> >> +static bool find_driver_guid(const struct wmi_driver *wdriver) >> +{ >> + const struct wmi_device_id *id; >> + >> + if (wdriver == NULL) >> + return false; >> + >> + for (id = wdriver->id_table; *id->guid_string; id++) { >> + if (find_guid(id->guid_string, NULL)) >> + return true; >> + } >> + >> + return false; >> +} >> + >> static const void *find_guid_context(struct wmi_block *wblock, >> struct wmi_driver *wdriver) >> { >> @@ -1419,6 +1434,9 @@ static int acpi_wmi_probe(struct platform_device *device) >> int __must_check __wmi_driver_register(struct wmi_driver *driver, >> struct module *owner) >> { >> + if (!find_driver_guid(driver)) >> + return -ENODEV; >> + >> driver->driver.owner = owner; >> driver->driver.bus = &wmi_bus_type; >> >> -- >> 2.18.4 > > > Can you elaborate as to why this change in behaviour of the wmi > driver is needed? If I understand it correctly you want to prevent > WMI drivers from successfully loading when there are no matching > GUIDs. Is that correct? Currently, a driver that registers with WMI can be explicitly loaded, even on a system that lacks the relevant hardware. It's not a huge deal if this happens, since most people won't bother explicitly loading a driver their system can't use, and since the probe routine will never be called, the module just sits around uselessly loaded into the kernel. I discovered this behavior when testing the version of the patch reworked to register with WMI, and thought it would be nice to not allow useless modules to be loaded, but if anybody disagrees with this change, it's certainly not essential, and I'm happy to leave the behavior the way it currently is. > > Barnabás Pőcze >
On 7/31/20 3:52 PM, Limonciello, Mario wrote: >> -----Original Message----- >> From: Daniel Dadap <ddadap@nvidia.com> >> Sent: Friday, July 31, 2020 3:22 PM >> To: platform-driver-x86@vger.kernel.org; Limonciello, Mario; >> pobrn@protonmail.com >> Cc: andy@infradead.org; dvhart@infradead.org; aplattner@nvidia.com; Daniel >> Dadap >> Subject: [PATCH v2 2/2] platform/x86: wmi: fail wmi_driver_register when no >> GUID is found >> >> >> [EXTERNAL EMAIL] >> >> If a driver registers with WMI, and none of the GUIDs in its ID table >> is present on the system, then that driver will not be probed and >> automatically loaded. However, it is still possible to load such a >> driver explicitly on a system which doesn't include the relevant >> hardware. >> >> Update wmi_driver_register to test for the presence of at least one >> GUID from the driver's ID table at driver registration time, and >> fail registration if none are found. >> >> Signed-off-by: Daniel Dadap <ddadap@nvidia.com> >> --- >> drivers/platform/x86/wmi.c | 18 ++++++++++++++++++ >> 1 file changed, 18 insertions(+) >> >> diff --git a/drivers/platform/x86/wmi.c b/drivers/platform/x86/wmi.c >> index 941739db7199..19aa23d1cf8e 100644 >> --- a/drivers/platform/x86/wmi.c >> +++ b/drivers/platform/x86/wmi.c >> @@ -130,6 +130,21 @@ static bool find_guid(const char *guid_string, struct >> wmi_block **out) >> return false; >> } >> >> +static bool find_driver_guid(const struct wmi_driver *wdriver) >> +{ >> + const struct wmi_device_id *id; >> + >> + if (wdriver == NULL) >> + return false; >> + >> + for (id = wdriver->id_table; *id->guid_string; id++) { >> + if (find_guid(id->guid_string, NULL)) >> + return true; >> + } >> + >> + return false; >> +} >> + > As a point of preference, why not in this function return -ENODEV directly > rather than it be boolean and map all errors to -ENODEV? Sure, if others think this behavior change is useful/desirable, I'd be happy to change find_driver_guid() to return an error code directly, and maybe return -EINVAL in the wdriver == NULL case. >> static const void *find_guid_context(struct wmi_block *wblock, >> struct wmi_driver *wdriver) >> { >> @@ -1419,6 +1434,9 @@ static int acpi_wmi_probe(struct platform_device >> *device) >> int __must_check __wmi_driver_register(struct wmi_driver *driver, >> struct module *owner) >> { >> + if (!find_driver_guid(driver)) >> + return -ENODEV; >> + > Then the logic here can be something like: > > ret = find_driver_guid(driver); > if (ret) > return ret; > >> driver->driver.owner = owner; >> driver->driver.bus = &wmi_bus_type; >> >> -- >> 2.18.4 > S
Hi, On 7/31/20 10:21 PM, Daniel Dadap wrote: > If a driver registers with WMI, and none of the GUIDs in its ID table > is present on the system, then that driver will not be probed and > automatically loaded. However, it is still possible to load such a > driver explicitly on a system which doesn't include the relevant > hardware. > > Update wmi_driver_register to test for the presence of at least one > GUID from the driver's ID table at driver registration time, and > fail registration if none are found. This would make the WMI bus different from all the other kernel bus subsystems where one can happily load drivers even if there is no hardware using them. And this would also break being able to manually bind a different (hopefully compatible but different) guid device through /sys/bus/wmi/drivers/foo/bind So NACK to this one from me. Note please do send a new version of patch 1/2 of this sets addressing Andy's remarks to the other similar patch you did. Regards, Hans > > Signed-off-by: Daniel Dadap <ddadap@nvidia.com> > --- > drivers/platform/x86/wmi.c | 18 ++++++++++++++++++ > 1 file changed, 18 insertions(+) > > diff --git a/drivers/platform/x86/wmi.c b/drivers/platform/x86/wmi.c > index 941739db7199..19aa23d1cf8e 100644 > --- a/drivers/platform/x86/wmi.c > +++ b/drivers/platform/x86/wmi.c > @@ -130,6 +130,21 @@ static bool find_guid(const char *guid_string, struct wmi_block **out) > return false; > } > > +static bool find_driver_guid(const struct wmi_driver *wdriver) > +{ > + const struct wmi_device_id *id; > + > + if (wdriver == NULL) > + return false; > + > + for (id = wdriver->id_table; *id->guid_string; id++) { > + if (find_guid(id->guid_string, NULL)) > + return true; > + } > + > + return false; > +} > + > static const void *find_guid_context(struct wmi_block *wblock, > struct wmi_driver *wdriver) > { > @@ -1419,6 +1434,9 @@ static int acpi_wmi_probe(struct platform_device *device) > int __must_check __wmi_driver_register(struct wmi_driver *driver, > struct module *owner) > { > + if (!find_driver_guid(driver)) > + return -ENODEV; > + > driver->driver.owner = owner; > driver->driver.bus = &wmi_bus_type; > >
On 11/10/20 3:34 AM, Hans de Goede wrote: > External email: Use caution opening links or attachments > > > Hi, > > On 7/31/20 10:21 PM, Daniel Dadap wrote: >> If a driver registers with WMI, and none of the GUIDs in its ID table >> is present on the system, then that driver will not be probed and >> automatically loaded. However, it is still possible to load such a >> driver explicitly on a system which doesn't include the relevant >> hardware. >> >> Update wmi_driver_register to test for the presence of at least one >> GUID from the driver's ID table at driver registration time, and >> fail registration if none are found. > This would make the WMI bus different from all the other kernel > bus subsystems where one can happily load drivers even if there > is no hardware using them. > > And this would also break being able to manually bind a different > (hopefully compatible but different) guid device through > /sys/bus/wmi/drivers/foo/bind > > So NACK to this one from me. > > Note please do send a new version of patch 1/2 of this sets addressing > Andy's remarks to the other similar patch you did. Not a problem. I'll remove this patch from the series and update the other one. > Regards, > > Hans > > > > > >> Signed-off-by: Daniel Dadap <ddadap@nvidia.com> >> --- >> drivers/platform/x86/wmi.c | 18 ++++++++++++++++++ >> 1 file changed, 18 insertions(+) >> >> diff --git a/drivers/platform/x86/wmi.c b/drivers/platform/x86/wmi.c >> index 941739db7199..19aa23d1cf8e 100644 >> --- a/drivers/platform/x86/wmi.c >> +++ b/drivers/platform/x86/wmi.c >> @@ -130,6 +130,21 @@ static bool find_guid(const char *guid_string, struct wmi_block **out) >> return false; >> } >> >> +static bool find_driver_guid(const struct wmi_driver *wdriver) >> +{ >> + const struct wmi_device_id *id; >> + >> + if (wdriver == NULL) >> + return false; >> + >> + for (id = wdriver->id_table; *id->guid_string; id++) { >> + if (find_guid(id->guid_string, NULL)) >> + return true; >> + } >> + >> + return false; >> +} >> + >> static const void *find_guid_context(struct wmi_block *wblock, >> struct wmi_driver *wdriver) >> { >> @@ -1419,6 +1434,9 @@ static int acpi_wmi_probe(struct platform_device *device) >> int __must_check __wmi_driver_register(struct wmi_driver *driver, >> struct module *owner) >> { >> + if (!find_driver_guid(driver)) >> + return -ENODEV; >> + >> driver->driver.owner = owner; >> driver->driver.bus = &wmi_bus_type; >> >>
diff --git a/drivers/platform/x86/wmi.c b/drivers/platform/x86/wmi.c index 941739db7199..19aa23d1cf8e 100644 --- a/drivers/platform/x86/wmi.c +++ b/drivers/platform/x86/wmi.c @@ -130,6 +130,21 @@ static bool find_guid(const char *guid_string, struct wmi_block **out) return false; } +static bool find_driver_guid(const struct wmi_driver *wdriver) +{ + const struct wmi_device_id *id; + + if (wdriver == NULL) + return false; + + for (id = wdriver->id_table; *id->guid_string; id++) { + if (find_guid(id->guid_string, NULL)) + return true; + } + + return false; +} + static const void *find_guid_context(struct wmi_block *wblock, struct wmi_driver *wdriver) { @@ -1419,6 +1434,9 @@ static int acpi_wmi_probe(struct platform_device *device) int __must_check __wmi_driver_register(struct wmi_driver *driver, struct module *owner) { + if (!find_driver_guid(driver)) + return -ENODEV; + driver->driver.owner = owner; driver->driver.bus = &wmi_bus_type;
If a driver registers with WMI, and none of the GUIDs in its ID table is present on the system, then that driver will not be probed and automatically loaded. However, it is still possible to load such a driver explicitly on a system which doesn't include the relevant hardware. Update wmi_driver_register to test for the presence of at least one GUID from the driver's ID table at driver registration time, and fail registration if none are found. Signed-off-by: Daniel Dadap <ddadap@nvidia.com> --- drivers/platform/x86/wmi.c | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+)