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

On Mon, May 11, 2026, at 18:12, Simon Horman wrote:
>
> Simon says: As a pre-existing issue I don't think we need to be concerned
>             about this in the scope of this patch-set. But it may
> 	    be a potential for follow-up.
>
> 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?

I already commented on this earlier: This is indeed a theoretical
bug, but it works in practice because DMA-capable SPI controllers
tend to all use MMIO for short transfers instead of DMA.

SPI is very fragile this way, and I do have some patches to improve
that overall, but it seems out of scope for this one driver.

>> -
>> -	gpio_direction_output(p54spi_gpio_power, 0);
>> -	gpio_direction_input(p54spi_gpio_irq);
>> -
>> -	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);
>
> Simon says: This one seems more concerning in the context of this patch-set.
>
> 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?

I also commented on this: request_irq() already fails gracefully
with -EINVAL when presented with an invalid IRQ. IRQ 0 is guaranteed
to be invalid on any target that uses devicetree.

      Arnd