Re: [PATCH v2 1/3] Input: applespi - force PIO mode on MacBook8,1

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Incomplete restoration of `spi->controller->can_dma` leads to an execute-after-free on module unload if the original callback was NULL.
- [High] Architectural violation and data race due to mutating shared SPI host controller, which is applied too late to prevent DMA timeouts.
--

--- Patch [1]: [PATCH v2 1/3] Input: applespi - force PIO mode on MacBook8,1 ---
commit c3d66aabf4cd47b8a9f7af8039fad4fc085aef5f
Author: Shih-Yuan Lee <fourdollars@debian.org>

Input: applespi - force PIO mode on MacBook8,1

This patch introduces a DMI quirk to force PIO mode on MacBook8,1 laptops.
It overrides the shared SPI controller's can_dma callback to avoid timeout
errors during DMA handshakes, and attempts to restore the original callback
when the driver is removed.

[ ... ]
> --- a/drivers/input/keyboard/applespi.c
> +++ b/drivers/input/keyboard/applespi.c
[ ... ]
> @@ -1788,6 +1800,19 @@ static int applespi_probe(struct spi_device *spi)
>  	debugfs_create_file("tp_dim", 0400, applespi->debugfs_root, applespi,
>  			    &applespi_tp_dim_fops);
>  
> +	/*
> +	 * MacBook8,1's SPI host controller DMA is broken (timeout errors).
> +	 * Force PIO mode by overriding the controller's can_dma callback.
> +	 *
> +	 * Since we modify the shared controller's callback, we save the
> +	 * original pointer and restore it in applespi_remove().
> +	 */
> +	applespi->original_can_dma = spi->controller->can_dma;
> +	if (dmi_match(DMI_PRODUCT_NAME, "MacBook8,1")) {
> +		dev_info(&spi->dev, "Disabling DMA for MacBook8,1 SPI to force PIO mode\n");
> +		spi->controller->can_dma = applespi_can_not_dma;
> +	}
> +
>  	return 0;
>  }

[Severity: High]
Is this too late to prevent the DMA timeouts?

The override occurs at the very end of applespi_probe(). However, early SPI
transfers triggered earlier in the function via applespi_setup_spi() and
applespi_init(), as well as any reads triggered by ACPI GPE interrupts that
were already enabled, will still attempt to use DMA and fail.

Furthermore, does mutating the shared SPI host controller's method table from
a peripheral client driver cause data races with concurrent SPI core
operations? Typically, controller-level hardware quirks need to be implemented
in the SPI host controller driver itself rather than the client driver to
maintain encapsulation and prevent races.

[ ... ]
> @@ -1822,6 +1847,9 @@ static void applespi_remove(struct spi_device *spi)
>  
>  	applespi_drain_reads(applespi);
>  
> +	if (applespi->original_can_dma)
> +		spi->controller->can_dma = applespi->original_can_dma;
> +
>  	debugfs_remove_recursive(applespi->debugfs_root);
>  }

[Severity: High]
Will this leave a dangling pointer in the host controller if the original
callback was NULL?

If the host controller does not natively provide a can_dma callback,
applespi->original_can_dma will be NULL. In that scenario, this conditional
check evaluates to false and skips the restoration entirely. 

Once the applespi module is unloaded and unmapped from memory, the shared
host controller will be left pointing to the freed applespi_can_not_dma
function. Any subsequent SPI transfers on the host controller would then
dereference this freed pointer, resulting in an execute-after-free and kernel
panic.

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