Re: [PATCH v12 3/7] input: serio: Add driver for ASUS Transformer dock keyboard and touchpad

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] input: serio: serio ports registered before notifier in asus_ec_kbc_probe()
--

commit 03f99aa77c8833478d88304ecd5af6dc8d7131c2
Author: Michał Mirosław <mirq-linux@rere.qmqm.pl>

input: serio: Add driver for ASUS Transformer dock keyboard and touchpad

This commit adds a serio driver for the ASUS Transformer dock keyboard and
touchpad controller, forwarding EC notifications to serio ports and
remapping several non-standard keycodes.

> diff --git a/drivers/input/serio/asus-transformer-ec-kbc.c b/drivers/input/serio/asus-transformer-ec-kbc.c
> new file mode 100644
> index 0000000000000..fa7f7c3f037db
> --- /dev/null
> +++ b/drivers/input/serio/asus-transformer-ec-kbc.c
> @@ -0,0 +1,264 @@

[ ... ]

> +static int asus_ec_register_serio(struct platform_device *pdev, int idx,
> +				  const char *name, int cmd)
> +{
> +	struct asus_ec_kbc_data *priv = platform_get_drvdata(pdev);
> +	struct i2c_client *parent = priv->parent;
> +	struct serio *port = kzalloc_obj(*port);
> +
> +	if (!port)
> +		return -ENOMEM;
> +
> +	priv->sdev[idx] = port;
> +	port->dev.parent = &pdev->dev;
> +	port->id.type = SERIO_8042;
> +	port->id.extra = cmd & 0xFF;
> +	port->write = asus_ec_serio_write;
> +	port->port_data = (void *)priv;
> +	snprintf(port->name, sizeof(port->name), "%s %s",
> +		 priv->ec->model, name);
> +	snprintf(port->phys, sizeof(port->phys), "i2c-%u-%04x/serio%d",
> +		 i2c_adapter_id(parent->adapter), parent->addr, idx);
> +
> +	serio_register_port(port);
> +
> +	return devm_add_action_or_reset(&pdev->dev, asus_ec_serio_remove, port);
> +}
> +
> +static void asus_ec_notifier_chain_unregister(void *data)
> +{
> +	struct asus_ec_kbc_data *priv = data;
> +	struct asusec_core *ec = priv->ec;
> +
> +	blocking_notifier_chain_unregister(&ec->notify_list, &priv->nb);
> +}
> +
> +static int asus_ec_kbc_probe(struct platform_device *pdev)
> +{
> +	struct asusec_core *ec = dev_get_drvdata(pdev->dev.parent);
> +	struct asus_ec_kbc_data *priv;
> +	int error;
> +
> +	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	platform_set_drvdata(pdev, priv);
> +
> +	priv->ec = ec;
> +	priv->parent = to_i2c_client(pdev->dev.parent);
> +
> +	error = asus_ec_register_serio(pdev, 0, "Keyboard", 0);
> +	if (error)
> +		return error;
> +
> +	error = asus_ec_register_serio(pdev, 1, "Touchpad", I8042_CMD_AUX_SEND);
> +	if (error)
> +		return error;
> +
> +	priv->nb.notifier_call = asus_ec_kbc_notify;
> +
> +	error = blocking_notifier_chain_register(&ec->notify_list, &priv->nb);
> +	if (error)
> +		return dev_err_probe(&pdev->dev, error,
> +				     "failed to register blocking notifier chain");
> +
> +	error = devm_add_action_or_reset(&pdev->dev,
> +					 asus_ec_notifier_chain_unregister,
> +					 priv);

[Severity: Medium]
Can registering the serio ports before registering the notifier in
asus_ec_kbc_probe() cause a race with serio driver probing?

Inside asus_ec_register_serio(), serio_register_port() immediately queues
SERIO_REGISTER_PORT onto system_long_wq (serio_event_work). If
serio_event_work runs on another CPU before
blocking_notifier_chain_register() completes, serio_add_port() will bind
atkbd or psmouse, which immediately send PS/2 reset and identification
commands via asus_ec_serio_write() and wait for responses via
serio_interrupt().

Since the parent MFD driver has already enabled its IRQ handler
asus_ec_interrupt(), the EC response will be read from ASUSEC_READ_BUF and
passed to blocking_notifier_call_chain(&ddata->ec.notify_list, ...) before
priv->nb is on the list, dropping the response bytes and causing
ps2_command() timeouts.

Similarly, on driver unbind, devm will run
asus_ec_notifier_chain_unregister() before asus_ec_serio_remove() calls
serio_unregister_port(), disconnecting the receive path while the serio
ports and their drivers are still active.

Should the notifier be registered (or gated via serio open/close or
start/stop callbacks with priv->sdev[] initialized beforehand) so that the
receive path is active whenever the serio ports are registered?

> +	if (error)
> +		return error;
> +
> +	return 0;
> +}

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261019100406.103687-1-clamor95@gmail.com?part=3