Received-SPF: pass (google.com: domain of linux-kernel+bounces-119969-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Message-ID: <31bc6c0a-c0e7-4055-82ec-33da51368e0f@gmx.de>
Date: Tue, 26 Mar 2024 21:59:58 +0100
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v11] platform/x86: add lenovo wmi camera button driver
To: Andy Shevchenko <andriy.shevchenko@intel.com>, Ai Chao <aichao@kylinos.cn>
Cc: hdegoede@redhat.com, ilpo.jarvinen@linux.intel.com,
 linux-kernel@vger.kernel.org, platform-driver-x86@vger.kernel.org
References: <20240322064750.267422-1-aichao@kylinos.cn>
 <ZgGmfV9ciPdtbGO1@smile.fi.intel.com>
Content-Language: en-US
From: Armin Wolf <W_Armin@gmx.de>
In-Reply-To: <ZgGmfV9ciPdtbGO1@smile.fi.intel.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
UI-OutboundReport: notjunk:1;M01:P0:6whmCGcjBL4=;WQUegEFlWSWKLFd1WnF7kbFQd5h
 k/IQesRrup/X+zbEcmwoZPUNAMpl1EWvCiODx7HXYgE5xfF6/lNWiUbTMkLNkcwIQovCzHHbM
 p2irvQnpMdDSl0AXntwih23Oic/pzZvjg4ev5Ir/2J4DER/tuRNJnkZ6gs5n/3/H1ZhJaX2XR
 7UB1AvWhSbhqncDtBY5tVLT16calfxU32z6jhV62eaMwodLBL5Ytl0UUz3V4XM13oROJznAPe
 d1s+m9Dj1vtDI+O6pHRF1NazzF9e29Z+t/+ix6fXrG2LkE8h8XUGpdhfrpzRc1SGYrAwwUpAB
 PnDgwcMp8B+StSZ1siGFclMCqNljCpbfQCvyOPiiZrWatukkjgJuNymJs/Lzx+FX8fKloeVaX
 Rs6PlAddgcq2m2b6AoLy1g+TAQ/YSH6O30b0wVG+GWGICKdQPr0kW/ly0KimOl/p9LMnJAEjA
 BLPc8VDTckR7noF+SNj/L++Kfn9cIWo7rpkF8ZWPE/0UYQb5jRS0xGQeDIM3P0SBLVpbm/rVz
 OYKbNsy+E7ZiX604uIfSsscZ/vibD8QM57wKg/oJTbYUyLMHtCleVtBtOmsd2WaBZqF+Dr1ym
 fzGHoHapqWtRX02F/POcQCpBO/MqJTgQ581ZDOeps2b9/cRT7zu25Od7UM1gfqC5OtKjvP9C/
 iHeVGJCeENEfPLGNIA1EaNIpYTWwiIdr3Zx3ZpVFmxTDJM0ZUuzTNrnjtqjxg6Caeeiy8z900
 Af4GK2YAzQtkPCIsO0Ta6ONm+K82CspNckbrhM8mm0x8uW7J4WhxyGJMNcdtBM+GHARbsictj
 ZEJlH/a/UOR471or5JPq2CWVlr8BIBR/6S6jwAxyI0np4=

Am 25.03.24 um 17:29 schrieb Andy Shevchenko:

> On Fri, Mar 22, 2024 at 02:47:50PM +0800, Ai Chao wrote:
>> Add lenovo generic wmi driver to support camera button.
> WMI
>
>> The Camera button is a GPIO device. This driver receives ACPI notifyi
>> when the camera button is switched on/off. This driver is used in
>> Lenovo A70, it is a Computer integrated machine.
>> +config LENOVO_WMI_CAMERA
>> +	tristate "Lenovo WMI Camera Button driver"
>> +	depends on ACPI_WMI
>> +	depends on INPUT
> No COMPILE_TEST?
>
>> +	help
>> +	  This driver provides support for Lenovo camera button. The Camera
>> +	  button is a GPIO device. This driver receives ACPI notify when the
>> +	  camera button is switched on/off.
>> +
>> +	  To compile this driver as a module, choose M here: the module
>> +	  will be called lenovo-wmi-camera.
> ...
>
>> +#include <linux/acpi.h>
>> +#include <linux/device.h>
>> +#include <linux/input.h>
>> +#include <linux/module.h>
>> +#include <linux/mutex.h>
> + types.h
>
>> +#include <linux/wmi.h>
> ...
>
>> +struct lenovo_wmi_priv {
>> +	struct input_dev *idev;
>> +	struct mutex notify_lock;	/* lenovo wmi camera button notify lock */
> WMI
>
>> +};
> ...
>
>> +	/* obj->buffer.pointer[0] is camera mode:
>> +	 *      0 camera close
>> +	 *      1 camera open
>> +	 */
> /*
>   * The correct multi-line comment style
>   * is depicted here.
>   */
>
> ...
>
>> +	keycode = (camera_mode == SW_CAMERA_ON ?
>> +		   KEY_CAMERA_ACCESS_ENABLE : KEY_CAMERA_ACCESS_DISABLE);
> Useless parentheses.
>
> ...
>
>> +	ret = input_register_device(priv->idev);
>> +	if (ret)
>> +		return ret;
>> +	mutex_init(&priv->notify_lock);
> Your mutex should be initialized before use. Have you tested that?

Hi,

i suggested that the mutex be initialized after calling input_register_device().
The reason for this is that the mutex is only used inside the WMI notify callback,
and the WMI driver core will only call it after probe() has returned.

So imho it should be safe.

Thanks,
Armin Wolf

>
> ...
>
>> +static struct wmi_driver lenovo_wmi_driver = {
>> +	.driver = {
>> +		.name = "lenovo-wmi-camera",
>> +		.probe_type = PROBE_PREFER_ASYNCHRONOUS,
>> +	},
>> +	.id_table = lenovo_wmi_id_table,
>> +	.no_singleton = true,
>> +	.probe = lenovo_wmi_probe,
>> +	.notify = lenovo_wmi_notify,
>> +	.remove = lenovo_wmi_remove,
>> +};
>> +
> Unneeded blank line.
>
>> +module_wmi_driver(lenovo_wmi_driver);
> ...
>
>> +MODULE_DEVICE_TABLE(wmi, lenovo_wmi_id_table);
> Please, move it closer to the respective table.
>