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

[Simon Horman <horms@kernel.org>]

From: 'Simon Horman' <horms@kernel.org>

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://sashiko.dev
---
p54spi: convert to devicetree

This commit converts the Prism54 SPI driver from using hardcoded GPIO
module parameters to using the GPIO descriptor interface and device tree
probing.

> diff --git a/drivers/net/wireless/intersil/p54/p54spi.c b/drivers/net/wireless/intersil/p54/p54spi.c
> index 9d66dcae54e02..d18be25450285 100644
> --- a/drivers/net/wireless/intersil/p54/p54spi.c
> +++ b/drivers/net/wireless/intersil/p54/p54spi.c

[ ... ]

> @@ -30,19 +31,6 @@
>  MODULE_FIRMWARE("3826.arm");
>  MODULE_FIRMWARE("3826.eeprom");
>  
> -/* gpios should be handled in board files and provided via platform data,
> - * but because it's currently impossible for p54spi to have a header file
> - * in include/linux, let's use module parameters for now
> - */
> -
> -static int p54spi_gpio_power = 97;
> -module_param(p54spi_gpio_power, int, 0444);
> -MODULE_PARM_DESC(p54spi_gpio_power, "gpio number for power line");
> -
> -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)
>  {

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?

[ ... ]

> @@ -608,31 +596,20 @@ static int p54spi_probe(struct spi_device *spi)
>  		goto err_free;
>  	}
>  
> -	ret = gpio_request(p54spi_gpio_power, "p54spi power");
> -	if (ret < 0) {
> -		dev_err(&priv->spi->dev, "power GPIO request failed: %d", ret);
> +	priv->gpio_powerdown = gpiod_get_optional(&spi->dev, "powerdown", GPIOD_OUT_HIGH);
> +	if (IS_ERR(priv->gpio_powerdown)) {
> +		ret = dev_err_probe(&priv->spi->dev, PTR_ERR(priv->gpio_powerdown),
> +				    "powerdown GPIO request failed\n");
>  		goto err_free;
>  	}
>  
> -	ret = gpio_request(p54spi_gpio_irq, "p54spi irq");
> -	if (ret < 0) {
> -		dev_err(&priv->spi->dev, "irq GPIO request failed: %d", ret);
> -		goto err_free_gpio_power;
> -	}
> -
> -	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?

>  	if (ret < 0) {
>  		dev_err(&priv->spi->dev, "request_irq() failed");
> -		goto err_free_gpio_irq;
> +		goto err_free_gpio_power;
>  	}