Messages in this thread | | | Subject | Re: [PATCH v2] platform/x86: Add Driver to set up lid GPEs on MS Surface device | From | Maximilian Luz <> | Date | Wed, 16 Sep 2020 19:54:13 +0200 |
| |
On 9/16/20 7:13 PM, Barnabás Pőcze wrote: ... >>>> + }s >>>> + >>>> + return 0; >>>> +} >>>> [...] >>>> +static int surface_gpe_probe(struct platform_device *pdev) >>>> +{ >>>> + struct surface_lid_device *lid; >>>> + u32 gpe_number; >>>> + int status; >>>> + >>>> + status = device_property_read_u32(&pdev->dev, "gpe", &gpe_number); >>>> + if (status) >>>> + return -ENODEV; >>> >>> 'device_property_read_u32()' returns an error code, you could simply return that >>> instead of hiding it. >> >> My thought there was that if the "gpe" property isn't present or of a >> different type, this is not a device that we want to/can handle. Thus >> the -ENODEV. Although I think a debug print statement may be useful >> here. >> > > I see, I just wanted to bring to your attention that 'device_property_read_u32()' > returns various standard error codes and you could simply return those.
I think one could also argue that module-loading should have taken care of filtering out devices that we don't load on, so -ENODEV would be redundant here. At least if one neglects that a user could try to manually bind the driver to a device. Following that thought, I guess it makes more sense to return the actual value here.
>> [...] >>>> + >>>> + lid->gpe_number = gpe_number; >>>> + platform_set_drvdata(pdev, lid); >>>> + >>>> + status = surface_lid_enable_wakeup(&pdev->dev, false); >>>> + if (status) { >>>> + acpi_disable_gpe(NULL, gpe_number); >>>> + platform_set_drvdata(pdev, NULL); >>> >>> Why is 'platform_set_drvdata(pdev, NULL)' needed? >> >> Is this not required for clean-up once the driver data has been set? Or >> does the driver-base take care of that for us when the driver is >> removed/fails to probe? My reasoning was that I don't want to leave >> stuff around for any other driver to trip on (and rather have that >> driver oops on a NULL-pointer). If the driver-core already takes care of >> NULL-ing that, that line is not needed. Unfortunately that behavior >> doesn't seem to be explained in the documentation. >> > > I'm not aware that it would be required. As a matter of fact, only two x86 > platform drivers (intel_pmc_core, ideapad-laptop) do any cleanup of driver data. > There are much more hits (536) for "set_drvdata(.* NULL" when scanning all drivers. > There are 4864 hits for "set_drvdata(.*" that have no 'NULL' in them. > > There is drivers/base/dd.c:really_probe(), which seems to be the place where driver > probes are actually called. And it calls 'dev_set_drvdata(dev, NULL)' if the probe > fails. And it also sets the driver data to NULL in '__device_release_driver()', > so I'm pretty sure the driver core takes care of it.
I see, thanks! Would make sense that the core takes care of that.
>>>> + return status; >>>> + } >>>> + >>>> + return 0; >>>> +} >> [...] >>>> +static int __init surface_gpe_init(void) >>>> +{ >>>> + const struct dmi_system_id *match; >>>> + const struct property_entry *props; >>>> + struct platform_device *pdev; >>>> + struct fwnode_handle *fwnode; >>>> + int status; >>>> + >>>> + match = dmi_first_match(dmi_lid_device_table); >>>> + if (!match) { >>>> + pr_info(KBUILD_MODNAME": no device detected, exiting\n"); >>> >>> If you put >>> #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt >>> before including any headers, you can simply write 'pr_info("no device...")' and it'll >>> be prefixed by the module name. This is the "usual" way of achieving what you want. >> >> Right, thanks! >> >>>> + return 0; >>> >>> Shouldn't it return -ENODEV? >> >> How does module auto-loading behave with a -ENODEV return value in init? >> I know that in the driver's probe callback it signals that the driver >> isn't intended for the device. Is this the same for modules or would a >> user get an error message in the kernel log? As I couldn't find any >> documentation on this, I assumed it didn't behave the same and would >> emit an error message. >> >> The reason I don't want to emit an error message here is that the module >> can be loaded for devices that it's not intended (and that's not >> something we can fix with a better MODULE_ALIAS as Microsoft cleverly >> named their 5th generation Surface Pro "Surface Pro", without any >> version number). Mainly, I don't want users to get a random error >> message that doesn't indicate an actual error. >> > > I wasn't sure, so I tested it. It prints the "no device" message, but that's it, > no more indication of the -ENODEV error in the kernel log during automatic > module loading at boot. > > You could print "no compatible Microsoft Surface device found, exitig" (or something > similar). I think this provides enough information for any user to decide if > they should be concerned or not.
I ran the same test (with same results) earlier today and also did some digging: From what I can tell, udev is responsible for auto-loading and the code doing that can be found at [1]. This code seems to, by default, log any errors as debug output. Only in verbose mode it logs them as error, with the exception of -ENODEV, which then is specifically logged only as notice.
It also seems to be used by a couple of other modules this way. So I guess that's the expected use-case for -ENODEV in module-init and pretty much guarantees the behavior I've wanted.
[1]: https://github.com/systemd/systemd/blob/6d95e7d9b263c94e94704e3125cb790840b76ca2/src/shared/module-util.c#L58-L64
Thanks again. If there are no other comments, I'll likely submit a v3 addressing the issues tomorrow.
Regards, Max
| |