Re: [PATCH v5 2/2] m68k: defconfig: enable PATA_CSWARP

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Missing check for ioremap() failure in pata_cswarp_probe().
- [Medium] Missing iounmap() on the ata_host_activate() error path.
--

--- Patch [2]: [PATCH v5 2/2] m68k: defconfig: enable PATA_CSWARP ---
commit 50c3007b9ddccb0c0a488126c545810ad538a593
Author: Paolo Pisati <[email protected]>

ata: pata_cswarp: Add Amiga cslab ata support

This commit introduces the pata_cswarp driver to provide support for
the on-board IDE interface on the CS-Lab Warp Expansion Card.

> diff --git a/drivers/ata/pata_cswarp.c b/drivers/ata/pata_cswarp.c
> new file mode 100644
> index 0000000000000..55f000f427384
> --- /dev/null
> +++ b/drivers/ata/pata_cswarp.c
> @@ -0,0 +1,180 @@

[ ... ]

> +static int pata_cswarp_probe(struct zorro_dev *z,
> +			     const struct zorro_device_id *ent)
> +{

[ ... ]

> +	host = ata_host_alloc(&z->dev, 1);
> +	if (!host)
> +		return -ENXIO;
> +
> +	ap = host->ports[0];
> +	base = ioremap(board + WARP_OFFSET_ATA, 0x1800);
> +
> +	ap->ops = &pata_cswarp_ops;

[Severity: High]
Is it possible for ioremap() to fail and return a NULL pointer here?

If memory mapping fails during device probe, base would be NULL.

This NULL pointer is then used to calculate and assign various ATA port I/O
addresses (e.g., ap->ioaddr.error_addr = base + 1 * 4).

When the block layer or ATA framework later attempts to access these I/O
ports, will this result in a kernel panic due to an invalid pointer
dereference?

> +
> +	ap->pio_mask = ATA_PIO4;

[ ... ]

> +	ata_port_desc(ap, "  cmd 0x%lx ctl 0x%lx", (unsigned long)base,
> +		      (unsigned long)ap->ioaddr.ctl_addr);
> +
> +	return ata_host_activate(host, 0, NULL,
> +			  IRQF_SHARED, &pata_cswarp_sht);
> +}

[Severity: Medium]
Does this error path leak the I/O memory mapping?

If ata_host_activate() fails, it returns a non-zero error code which is
immediately returned by pata_cswarp_probe().

Since this driver uses manual memory mapping rather than devm_ioremap()
(as seen by the manual iounmap() call in pata_cswarp_remove()), should
iounmap(base) be called before returning the error?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2