Re: [PATCH v3 1/5] Input: applespi - use unified wait queue with timeouts for drain

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [Medium] The patch attempts to prevent indefinite lockups during driver unbind or PM transitions by adding a timeout to the drain waits. However, upon timeout, it explicitly falls back to waiting unconditionally on the exact same condition, neutralizing the protection and still causing an indefinite lockup.
- [Low] Unnecessary removal of scope-based cleanup `guard()`, reverting to manual spinlock management.

Pre-existing issues:
- [Critical] Missing cancel_work_sync() in applespi_remove() causes Use-After-Free of applespi_data when the workqueue executes.
--

--- Patch [1]: [PATCH v3 1/5] Input: applespi - use unified wait queue with timeouts for drain ---
commit 08fd2aeb0171b9531113cd115a28c72f63462dc0
Author: Shih-Yuan Lee <fourdollars@debian.org>

Input: applespi - use unified wait queue with timeouts for drain
    
Consolidate wait queues by renaming drain_complete to wait_queue and use
wait_event_lock_irq_timeout() with a 3-second timeout during read and write
drains to prevent indefinite lockups during driver unbind or PM transitions
if the hardware becomes unresponsive.

> diff --git a/drivers/input/keyboard/applespi.c b/drivers/input/keyboard/applespi.c
> index b5ff71cd5a701..c501bf7e517e4 100644
> --- a/drivers/input/keyboard/applespi.c
> +++ b/drivers/input/keyboard/applespi.c

[ ... ]

> @@ -1793,21 +1793,49 @@ static int applespi_probe(struct spi_device *spi)
>  
>  static void applespi_drain_writes(struct applespi_data *applespi)
>  {
> -	guard(spinlock_irqsave)(&applespi->cmd_msg_lock);
> +	unsigned long flags;
> +	long ret;
> +
> +	spin_lock_irqsave(&applespi->cmd_msg_lock, flags);

[Severity: Low]
This isn't a bug, but is it necessary to revert from guard() to manual
spin_lock_irqsave() here?

The input subsystem guidelines prefer using modern cleanup primitives like
guard() in new or refactored code. wait_event_lock_irq_timeout() should
work correctly with the hidden flags variable established by guard().

>  
>  	applespi->drain = true;
> -	wait_event_lock_irq(applespi->drain_complete, !applespi->write_active,
> -			    applespi->cmd_msg_lock);
> +	ret = wait_event_lock_irq_timeout(applespi->wait_queue,
> +					  !applespi->write_active,
> +					  applespi->cmd_msg_lock,
> +					  msecs_to_jiffies(3000));
> +	if (!ret && applespi->write_active) {
> +		dev_warn(&applespi->spi->dev,
> +			 "Timed out waiting for write drain, waiting unconditionally\n");
> +		wait_event_lock_irq(applespi->wait_queue,
> +				    !applespi->write_active,
> +				    applespi->cmd_msg_lock);

[Severity: Medium]
Does this fallback logic defeat the purpose of the timeout?

The commit message states the timeout is meant to prevent indefinite lockups,
but if a timeout occurs here, wait_event_lock_irq() waits unconditionally
on the exact same condition. 

If the hardware is genuinely unresponsive, won't this just delay the indefinite
lockup by 3 seconds rather than preventing it?

> +	}
> +
> +	spin_unlock_irqrestore(&applespi->cmd_msg_lock, flags);
>  }
>  
>  static void applespi_drain_reads(struct applespi_data *applespi)
>  {
> -	guard(spinlock_irqsave)(&applespi->cmd_msg_lock);
> +	unsigned long flags;
> +	long ret;
>  
> -	wait_event_lock_irq(applespi->drain_complete, !applespi->read_active,
> -			    applespi->cmd_msg_lock);
> +	spin_lock_irqsave(&applespi->cmd_msg_lock, flags);
> +
> +	ret = wait_event_lock_irq_timeout(applespi->wait_queue,
> +					  !applespi->read_active,
> +					  applespi->cmd_msg_lock,
> +					  msecs_to_jiffies(3000));
> +	if (!ret && applespi->read_active) {
> +		dev_warn(&applespi->spi->dev,
> +			 "Timed out waiting for read drain, waiting unconditionally\n");
> +		wait_event_lock_irq(applespi->wait_queue,
> +				    !applespi->read_active,
> +				    applespi->cmd_msg_lock);

[Severity: Medium]
Does this read drain fallback have the same indefinite lockup issue as the 
write drain when the hardware fails?

> +	}
>  
>  	applespi->suspended = true;
> +
> +	spin_unlock_irqrestore(&applespi->cmd_msg_lock, flags);
>  }
>  
>  static void applespi_remove(struct spi_device *spi)

[Severity: Critical]
This is a pre-existing issue, but does applespi_remove() miss a call to
cancel_work_sync(&applespi->work) before returning?

If the driver unbinds while a touchpad info SPI transaction is in flight,
applespi_drain_reads() will block until the read finishes. 

When the SPI read completes, its handler schedules applespi->work, clears
read_active, and wakes up the drain. 

If applespi_remove() finishes without cancelling the work, devres will
free applespi_data, which could lead to a use-after-free when the scheduled
applespi_worker() executes.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260720162219.32324-1-fourdollars@debian.org?part=1