Message-ID: <EMZQgUl1xLN4o0hV9ZkCD563O85SuOYB5kNFZ5_hlxLQXbJCXpQfrM2afyFIr28h31tXMxD1mxE4DkA5Wy60A0Z2mDnstwF17tEdnX4IRas=@protonmail.com>
Date:   Tue, 15 Sep 2020 23:58:20 +0000
From:   Barnabás Pőcze <pobrn@...tonmail.com>
To:     Maximilian Luz <luzmaximilian@...il.com>
Cc:     Darren Hart <dvhart@...radead.org>,
        Andy Shevchenko <andy@...radead.org>,
        Hans de Goede <hdegoede@...hat.com>,
        Mika Westerberg <mika.westerberg@...ux.intel.com>,
        Gayatri Kammela <gayatri.kammela@...el.com>,
        "Rafael J. Wysocki" <rjw@...ysocki.net>,
        Len Brown <lenb@...nel.org>,
        "platform-driver-x86@...r.kernel.org" 
        <platform-driver-x86@...r.kernel.org>,
        "linux-acpi@...r.kernel.org" <linux-acpi@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2] platform/x86: Add Driver to set up lid GPEs on MS Surface device

Hi


> [...]
> +static int surface_lid_enable_wakeup(struct device *dev, bool enable)
> +{
> +	const struct surface_lid_device *lid = dev_get_drvdata(dev);
> +	int action = enable ? ACPI_GPE_ENABLE : ACPI_GPE_DISABLE;
> +	acpi_status status;
> +
> +	status = acpi_set_gpe_wake_mask(NULL, lid->gpe_number, action);
> +	if (status) {

I think 'if (ACPI_FAILURE(status))' would be better.


> +		dev_err(dev, "failed to set GPE wake mask: %d\n", status);

I'm not sure if it's technically safe to print acpi_status with the %d format
specifier since 'acpi_status' is defined as 'u32' at the moment.
 func("%lu", (unsigned long) status)
would be safer. You could also use 'acpi_format_exception()', which is possibly
the most correct approach since it assumes nothing about what 'acpi_status'
actually is.


> +		return -EINVAL;

I'm not sure if -EINVAL is the best error to return here.


> +	}
> +
> +	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.


> +
> +	status = acpi_mark_gpe_for_wake(NULL, gpe_number);
> +	if (status) {
> +		dev_err(&pdev->dev, "failed to mark GPE for wake: %d\n", status);
> +		return -EINVAL;
> +	}
> +
> +	status = acpi_enable_gpe(NULL, gpe_number);
> +	if (status) {
> +		dev_err(&pdev->dev, "failed to enable GPE: %d\n", status);
> +		return -EINVAL;
> +	}

My previous comments about ACPI and the returned value apply here as well.
Furthermore, 'acpi_mark_gpe_for_wake()' and 'acpi_enable_gpe()' both return
a value of type 'acpi_status', not 'int'.


> +
> +	lid = devm_kzalloc(&pdev->dev, sizeof(struct surface_lid_device),
> +			   GFP_KERNEL);

 lid = devm_kzalloc(..., sizeof(*lid), ...)
is preferred.


> +	if (!lid)
> +		return -ENOMEM;

Isn't that problematic that the side effects of the previous two ACPI calls are
not undone when returning here with -ENOMEM? Allocating this struct right after
querying 'gpe_number' could prevent it.


> +
> +	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?


> +		return status;
> +	}
> +
> +	return 0;
> +}
> +
> +static int surface_gpe_remove(struct platform_device *pdev)
> +{
> +	struct surface_lid_device *lid = dev_get_drvdata(&pdev->dev);
> +
> +	/* restore default behavior without this module */
> +	surface_lid_enable_wakeup(&pdev->dev, false);
> +	acpi_disable_gpe(NULL, lid->gpe_number);
> +
> +	platform_set_drvdata(pdev, NULL);

I'm wondering why this is needed?


> +	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.


> +		return 0;

Shouldn't it return -ENODEV?


> +	}
> +
> +	props = match->driver_data;
> +
> +	status = platform_driver_register(&surface_gpe_driver);
> +	if (status)
> +		return status;
> +
> +	pdev = platform_device_alloc("surface_gpe", PLATFORM_DEVID_NONE);
> +	if (!pdev) {
> +		platform_driver_unregister(&surface_gpe_driver);
> +		return -ENOMEM;
> +	}
> +
> +	fwnode = fwnode_create_software_node(props, NULL);
> +	if (IS_ERR(fwnode)) {
> +		platform_device_put(pdev);
> +		platform_driver_unregister(&surface_gpe_driver);
> +		return PTR_ERR(fwnode);
> +	}
> +
> +	pdev->dev.fwnode = fwnode;
> +
> +	status = platform_device_add(pdev);
> +	if (status) {
> +		platform_device_put(pdev);
> +		platform_driver_unregister(&surface_gpe_driver);
> +		return status;
> +	}
> +

It may be a matter of preference, but I think the 'if (err) goto X' pattern would
be better in this function (at least for the last 3 or so error paths).


> +	surface_gpe_device = pdev;
> +	return 0;
> +}
> +module_init(surface_gpe_init);
> +
> +static void __exit surface_gpe_exit(void)
> +{
> +	if (!surface_gpe_device)
> +		return;

If you returned -ENODEV in init when no DMI match is found,
then this check would be redundant.


> [...]


Regards,
Barnabás Pőcze

Powered by blists - more mailing lists