Re: [PATCH 2/3] [v5 net-next] p54spi: convert to devicetree

On Fri, May 8, 2026, at 23:32, [email protected] wrote:
>> -static int p54spi_gpio_irq = 87;
>> -module_param(p54spi_gpio_irq, int, 0444);
>> -MODULE_PARM_DESC(p54spi_gpio_irq, "gpio number for irq line");
>> -
>>  static void p54spi_spi_read(struct p54s_priv *priv, u8 address,
>>  			      void *buf, size_t len)
>>  {
>
> This problem wasn't introduced by this patch, but I noticed a pre-existing
> issue in the context here. Does the p54spi_spi_read() function perform an SPI
> transfer using a stack-allocated buffer?
>
> Looking at p54spi_spi_read(), the addr variable is allocated on the stack and
> passed as a transfer buffer to spi_sync(). The SPI framework maps transfer
> buffers for DMA. Passing a stack-allocated buffer is illegal, particularly with
> CONFIG_VMAP_STACK enabled, because stack memory resides in vmalloc space and
> cannot be safely DMA-mapped.
>
> Could this cause memory corruption or a kernel panic during the transfer?

In theory it could, as passing long buffers on stack is not
allowed for SPI drivers. In practice this seems to work because
all SPI drivers that perform DMA check for short transfers and
fall back to MMIO if possible. In this case, the spi-omap2-mcspi
driver uses DMA_MIN_BYTES=160, while the on-stack transfers are
only two or four bytes and will always be done through MMIO.

The driver has other flaws that I'm not trying to address either.

>> -
>> -	ret = request_irq(gpio_to_irq(p54spi_gpio_irq),
>> -			  p54spi_interrupt, IRQF_NO_AUTOEN, "p54spi",
>> -			  priv->spi);
>> +	ret = request_irq(spi->irq, p54spi_interrupt, IRQF_NO_AUTOEN, "p54spi", priv->spi);
>
> Is there missing validation for spi->irq before calling request_irq()?
>
> If the device tree node lacks an interrupts property, the SPI core may set
> spi->irq to 0 or a negative error code.
>
> Passing this directly to request_irq() without validation can successfully
> hijack a core system interrupt like the system timer on some architectures,
> leading to system instability or deadlocks.
>
> Should there be a check like if (spi->irq <= 0) to fail the probe gracefully?

request_irq() fails with -EINVAL in this case, which is an appropriate
return code. This is again the same behavior as before, if an invalid
GPIO, or a GPIO that is not connected to an irqchip gets passed.

     Arnd