Re: [PATCH 2/4] Input: psmouse - clean up locking around disable_work_sync()

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Moving disable_work_sync() to the top of psmouse_disconnect() breaks the early-abort synchronization in psmouse_resync(), causing delays and queuing stray reconnect events.
--

--- Patch [2]: [PATCH 2/4] Input: psmouse - clean up locking around disable_work_sync() ---
commit f2e43a3d4ae9250b89f797899728eb8d61c0ef21
Author: Dmitry Torokhov <[email protected]>

Input: psmouse - clean up locking around disable_work_sync()
    
In the past, psmouse_disconnect() used cancel_work_sync(). Because
cancel_work_sync() must be called with the psmouse_mutex dropped, and we
needed to prevent psmouse_receive_byte() from re-queueing the work
behind our back, the code transitioned the device to PSMOUSE_CMD_MODE
while holding the mutex, then dropped the mutex and cancelled the work.

When cancel_work_sync() was replaced with disable_work_sync() in this
path, the mutex juggling remained. However, disable_work_sync()
inherently prevents the work from being executed or re-queued, making
the mutex juggling unnecessary.

Clean this up by moving disable_work_sync() to the very top of
psmouse_disconnect(), before we acquire psmouse_mutex.

> diff --git a/drivers/input/mouse/psmouse-base.c b/drivers/input/mouse/psmouse-base.c
> index 916a14b9b6a7d..3fcc1d293c313 100644
> --- a/drivers/input/mouse/psmouse-base.c
> +++ b/drivers/input/mouse/psmouse-base.c
> @@ -1458,15 +1458,12 @@ static void psmouse_disconnect(struct serio *serio)
>  	struct psmouse *psmouse = psmouse_from_serio(serio);
>  	struct psmouse *parent = NULL;
>  
> +	disable_work_sync(&psmouse->resync_work);
> +
>  	mutex_lock(&psmouse_mutex);
>  
>  	psmouse_set_state(psmouse, PSMOUSE_CMD_MODE);
>  
> -	/* make sure we don't have a resync in progress */
> -	mutex_unlock(&psmouse_mutex);
> -	disable_work_sync(&psmouse->resync_work);
> -	mutex_lock(&psmouse_mutex);
> -

[Severity: High]
By moving disable_work_sync() before psmouse_set_state(..., PSMOUSE_CMD_MODE),
does this break the early-abort synchronization in psmouse_resync()?

If a hardware resync event races with driver unbind, disable_work_sync() will
block waiting for the already-running psmouse_resync() worker:

drivers/input/mouse/psmouse-base.c:psmouse_resync() {
	mutex_lock(&psmouse_mutex);

	if (psmouse->state != PSMOUSE_RESYNCING)
		goto out;

Since psmouse_disconnect() hasn't updated the state yet, psmouse_resync() will
observe PSMOUSE_RESYNCING, proceed to poll the disconnected hardware (causing
delays), and upon failure, call serio_reconnect():

	if (failed) {
		psmouse_set_state(psmouse, PSMOUSE_IGNORE);
		psmouse_info(psmouse,
			     "resync failed, issuing reconnect request\n");
		serio_reconnect(serio);
	}

Could this queued reconnect event cause the driver to spontaneously re-bind
after the disconnect finishes?

>  	if (serio->parent && serio->id.type == SERIO_PS_PSTHRU) {
>  		parent = psmouse_from_serio(serio->parent);
>  		psmouse_deactivate(parent);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2