Re: [PATCH 26/38] mfd: ioc3: add support for IOC4

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] mfd: NULL pointer dereference due to complex pointer in pci_device_id match table
- [Low] mfd: ioc4_cells array is not static const
- [Low] mfd: ioc3: commit message capitalization violation
--

commit 6ec254319e574e5b3a17ca8a7e4c5206dfed5e04
Author: Imre Kaloz <kaloz@kernel.org>

mfd: ioc3: add support for IOC4

This commit adds support for IOC4, which is IOC3's successor on IP35 Origin
350-class boards. It reuses the serial and PS/2 drivers of IOC3 and splits
interrupt status into two banks.

> mfd: ioc3: add support for IOC4

[Severity: Low]
Does this commit title violate the MFD subsystem formatting requirements?

The MFD subsystem conventions mandate that the description after the subsystem
prefix must always be capitalized (e.g., "mfd: ioc3: Add support...").

> diff --git a/drivers/mfd/ioc3.c b/drivers/mfd/ioc3.c
> index 5f8ac364b610e..fd50bae0f10b3 100644
> --- a/drivers/mfd/ioc3.c
> +++ b/drivers/mfd/ioc3.c

[ ... ]

> @@ -565,8 +605,11 @@ static int ioc3_setup(struct ioc3_priv_data *ipd)
>  static int ioc3_mfd_probe(struct pci_dev *pdev,
>  			  const struct pci_device_id *pci_id)
>  {
> +	const struct ioc3_variant *var = (void *)pci_id->driver_data;

[Severity: High]
Is it safe to blindly retrieve the pointer from driver_data?

If the driver is bound to a device dynamically via the sysfs 'new_id'
interface (where driver_data often defaults to 0), the 'var' pointer becomes
NULL.

>  	struct ioc3_priv_data *ipd;
> -	struct ioc3 __iomem *regs;
> +	void __iomem *base;
> +	unsigned int i;
> +	u16 cmd;
>  	int ret;
>  
>  	ret = pci_enable_device(pdev);
> @@ -576,6 +619,12 @@ static int ioc3_mfd_probe(struct pci_dev *pdev,
>  	pci_write_config_byte(pdev, PCI_LATENCY_TIMER, IOC3_LATENCY);
>  	pci_set_master(pdev);
>  
> +	if (var->pci_command) {

[Severity: High]
Does this cause a NULL pointer dereference if 'var' is NULL?

As mentioned above, if 'driver_data' is 0, 'var' will be NULL, leading to a
crash here. The MFD subsystem guidelines explicitly prohibit passing complex
pointers through match data tables and mandate using an integer ID/enum,
selecting the static const data in the probe function.

[ ... ]

> @@ -649,12 +708,152 @@ static void ioc3_mfd_remove(struct pci_dev *pdev)

[ ... ]

> +#define IOC4_SERIAL_CELL(port) {					\
> +	.name = "ioc3-serial8250",					\
> +	.resources = ioc4_uart_resources[port],				\
> +	.num_resources = ARRAY_SIZE(ioc4_uart_resources[port]),		\
> +	.platform_data = &ioc4_serial_pdata,				\
> +	.pdata_size = sizeof(ioc4_serial_pdata),			\
> +}
> +
> +static struct mfd_cell ioc4_cells[] = {

[Severity: Low]
Should this array be declared as 'static const'?

The MFD subsystem guidelines strictly require that mfd_cell arrays must be
defined as 'static const' to prevent dynamic platform data amendments and
memory safety hazards. The newly introduced 'ioc4_cells' array is missing the
'const' qualifier.

> +	IOC4_SERIAL_CELL(0),
> +	IOC4_SERIAL_CELL(1),

[ ... ]

> +static const struct ioc3_variant ioc4_variant = {
> +	.name = "IOC4",
> +	.chip = &ioc4_irq_chip,
> +	.banks = ioc4_irq_banks,
> +	.nr_banks = ARRAY_SIZE(ioc4_irq_banks),
> +	.nr_irqs = IOC4_NUM_IRQS,
> +	/* Parity and SERR# reporting, as the SN2 IOC4 core enabled them. */
> +	.pci_command = PCI_COMMAND_PARITY | PCI_COMMAND_SERR,
> +	.setup = ioc4_setup,
> +};
> +
>  static struct pci_device_id ioc3_mfd_id_table[] = {
> -	{ PCI_VENDOR_ID_SGI, PCI_DEVICE_ID_SGI_IOC3, PCI_ANY_ID, PCI_ANY_ID },
> +	{ PCI_VENDOR_ID_SGI, PCI_DEVICE_ID_SGI_IOC3, PCI_ANY_ID, PCI_ANY_ID,
> +	  0, 0, (kernel_ulong_t)&ioc3_variant },
> +	{ PCI_VENDOR_ID_SGI, 0x100a, PCI_ANY_ID, PCI_ANY_ID,
> +	  0, 0, (kernel_ulong_t)&ioc4_variant },
>  	{ 0, },
>  };

[Severity: High]
Is it safe to embed complex pointers in the match table?

The patch passes a pointer to 'ioc3_variant' / 'ioc4_variant' through the
pci_device_id match table's driver_data field. The MFD subsystem guidelines
explicitly prohibit this and mandate using an integer ID/enum, selecting the
static const data in the probe function instead.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1790857966.git.kaloz@kernel.org?part=26