[lkml]   [2020]   [Sep]   [16]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
SubjectRe: [PATCH v2] platform/x86: Add Driver to set up lid GPEs on MS Surface device
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.


Thanks again. If there are no other comments, I'll likely submit a v3
addressing the issues tomorrow.


 \ /
  Last update: 2020-09-16 19:57    [W:0.076 / U:18.440 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site