Re: [PATCH v2] can: mcp251xfd: Add ACPI device ID table for MCP251XFD

[sashiko-bot@kernel.org]

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

New issues:
- [Medium] Undocumented removal of `netdev_info` print in `mcp251xfd_handle_txatif`.

Pre-existing issues:
- [High] Infinite loop / interrupt storm in `mcp251xfd_irq` due to unhandled `TXATIF` interrupt flag.
- [High] NULL pointer dereference when retrieving match data if the device is bound manually via sysfs `driver_override`.
--

commit ac99a03d44f8633aff78ad69c67bdd4c8da1aa29
Author: guoqi0226 <guoqi0226@163.com>

can: mcp251xfd: Add ACPI device ID table for MCP251XFD

This patch adds an ACPI match table for the MCP251XFD CAN controller,
enabling driver binding on ACPI-enumerated SPI platforms.

> diff --git a/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c b/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c
> index 92a86083c8962..0319673707289 100644
> --- a/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c
> +++ b/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c

[ ... ]

> @@ -982,7 +983,6 @@ static int mcp251xfd_handle_rxovif(struct mcp251xfd_priv *priv)
>  
>  static int mcp251xfd_handle_txatif(struct mcp251xfd_priv *priv)
>  {
> -	netdev_info(priv->ndev, "%s\n", __func__);
>  
>  	return 0;
>  }

[Severity: Medium]
Is this removal intentional? The commit message only mentions adding an
ACPI match table, but this modifies the interrupt handler by removing
a debug print. If it is intentional, should it be mentioned in the
commit message?

[Severity: High]
This is a pre-existing issue, but does this empty function cause an
infinite interrupt loop?

Because TXATIF is not part of MCP251XFD_REG_INT_IF_CLEARABLE_MASK, the
hardware flag must be manually cleared in the FIFOSTA register. Since
mcp251xfd_handle_txatif() returns 0 without clearing the flag, won't
the do-while loop in mcp251xfd_irq() continually read TXATIF as pending
and loop infinitely?

[ ... ]

> @@ -2212,6 +2212,14 @@ static const struct spi_device_id mcp251xfd_id_table[] = {
>  };
>  MODULE_DEVICE_TABLE(spi, mcp251xfd_id_table);
>  
> +#ifdef CONFIG_ACPI
> +static const struct acpi_device_id mcp251xfd_acpi_match[] = {
> +	{ "MCHP15FD", (kernel_ulong_t)&mcp251xfd_devtype_data_mcp251xfd},
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(acpi, mcp251xfd_acpi_match);
> +#endif
> +
>  static int mcp251xfd_probe(struct spi_device *spi)
>  {
>  	struct net_device *ndev;

[Severity: High]
This is a pre-existing issue, but is it safe to unconditionally
dereference the match data retrieved in this function?

Later in mcp251xfd_probe(), the driver does:

priv->devtype_data = *(struct mcp251xfd_devtype_data *)spi_get_device_match_data(spi);

If the driver is bound manually via the sysfs driver_override attribute
without matching any table entries, spi_get_device_match_data() will
return NULL, which would cause a panic when dereferenced. Should there
be a check for NULL before dereferencing it?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260616085359.89406-1-guoqi0226@163.com?part=1