[PATCH 0/3] ata: pata_ep93xx: COMPILE_TEST and ARM fixups

[PATCH 0/3] ata: pata_ep93xx: COMPILE_TEST and ARM fixups

[Rosen Penev]

This small series cleans up pata_ep93xx to allow COMPILE_TEST on
non-ARM platforms. Patch 1 avoids including ARM-specific asm headers,
patch 2 switches to unsigned long for the data variable, and patch 3
enables COMPILE_TEST.

Rosen Penev (3):
  ata: pata_ep93xx: avoid asm on non ARM
  ata: pata_ep93xx: use unsigned long for data
  ata: pata_ep93xx: add COMPILE_TEST support

 drivers/ata/Kconfig       | 2 +-
 drivers/ata/pata_ep93xx.c | 7 ++++++-
 2 files changed, 7 insertions(+), 2 deletions(-)

-- 
2.54.0

[PATCH 1/3] ata: pata_ep93xx: avoid asm on non ARM

[Rosen Penev]

The raw ARM asm delay loop prevents COMPILE_TEST builds on
non-ARM architectures. Guard it with CONFIG_ARM and provide a
cpu_relax() fallback for compilation on other architectures.

Assisted-by: Opencode:Big-pickle
Signed-off-by: Rosen Penev <rosenp@gmail.com>
---
 drivers/ata/pata_ep93xx.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/ata/pata_ep93xx.c b/drivers/ata/pata_ep93xx.c
index b2b9e0058333..f7e87d6af264 100644
--- a/drivers/ata/pata_ep93xx.c
+++ b/drivers/ata/pata_ep93xx.c
@@ -204,6 +204,7 @@ static void ep93xx_pata_enable_pio(void __iomem *base, int pio_mode)
  */
 static void ep93xx_pata_delay(unsigned long count)
 {
+#ifdef CONFIG_ARM
 	__asm__ volatile (
 		"0:\n"
 		"mov r0, r0\n"
@@ -212,6 +213,10 @@ static void ep93xx_pata_delay(unsigned long count)
 		: "=r" (count)
 		: "0" (count)
 	);
+#else
+	while (count--)
+		cpu_relax();
+#endif
 }
 
 static unsigned long ep93xx_pata_wait_for_iordy(void __iomem *base,
-- 
2.54.0

Re: [PATCH 1/3] ata: pata_ep93xx: avoid asm on non ARM

[sashiko-bot]

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

Pre-existing issues:
- [High] Missing condition code clobber in inline assembly.
- [High] Double free and invalid pointer dereference on DMA probe error path.
--

commit 161745bc82337864b2d7b61cafb8fabbb474d029
Author: Rosen Penev <rosenp@gmail.com>

ata: pata_ep93xx: avoid asm on non ARM

The raw ARM asm delay loop prevents COMPILE_TEST builds on
non-ARM architectures. Guard it with CONFIG_ARM and provide a
cpu_relax() fallback for compilation on other architectures.

> diff --git a/drivers/ata/pata_ep93xx.c b/drivers/ata/pata_ep93xx.c
> index b2b9e0058333e..f7e87d6af2641 100644
> --- a/drivers/ata/pata_ep93xx.c
> +++ b/drivers/ata/pata_ep93xx.c
> @@ -204,6 +204,7 @@ static void ep93xx_pata_enable_pio(void __iomem *base, int pio_mode)
>   */
>  static void ep93xx_pata_delay(unsigned long count)
>  {
> +#ifdef CONFIG_ARM
>  	__asm__ volatile (
>  		"0:\n"
>  		"mov r0, r0\n"
>  		"subs %0, %1, #1\n"
>  		"bge 0b\n"
>  		: "=r" (count)
>  		: "0" (count)
>  	);

[Severity: High]
This is a pre-existing issue, but since this inline assembly uses the
"subs" instruction which alters the ARM condition code flags, does it
need a "cc" clobber?

Without the "cc" clobber, the compiler might assume that condition flags
set prior to the inline assembly remain valid across it. Because
ep93xx_pata_delay() is a small static function, it's likely to be
inlined into callers like ep93xx_pata_wait_for_iordy(). Could this lead
to unpredictable control flow corruption if the compiler schedules
flag-dependent instructions across the assembly block?

> +#else
> +	while (count--)
> +		cpu_relax();
> +#endif
>  }

[Severity: High]
This is a pre-existing issue, but I noticed a potential double free and
invalid pointer dereference on the DMA probe error path in this driver.

If dma_request_chan(&pdev->dev, "tx") fails during ep93xx_pata_dma_init(),
it returns an ERR_PTR which is stored in drv_data->dma_tx_channel.

The error path jumps to fail_release_rx, which releases dma_rx_channel but
doesn't clear the pointer, and then falls through to ep93xx_pata_release_dma():

drivers/ata/pata_ep93xx.c:ep93xx_pata_dma_init() {
    ...
fail_release_rx:
	dma_release_channel(drv_data->dma_rx_channel);
fail_release_dma:
	ep93xx_pata_release_dma(drv_data);
    ...
}

Inside ep93xx_pata_release_dma(), the code attempts to free both channels:

drivers/ata/pata_ep93xx.c:ep93xx_pata_release_dma() {
    ...
	if (drv_data->dma_rx_channel) {
		dma_release_channel(drv_data->dma_rx_channel);
		drv_data->dma_rx_channel = NULL;
	}
	if (drv_data->dma_tx_channel) {
		dma_release_channel(drv_data->dma_tx_channel);
		drv_data->dma_tx_channel = NULL;
	}
    ...
}

Since drv_data->dma_rx_channel was never set to NULL, does this cause a
double free of dma_rx_channel?

Additionally, since drv_data->dma_tx_channel holds an ERR_PTR rather than
NULL, this check evaluates to true and dma_release_channel() is called with
the ERR_PTR. Could this cause a kernel panic when the DMA subsystem attempts
to dereference the error pointer?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260530003627.42473-1-rosenp@gmail.com?part=1

[PATCH 2/3] ata: pata_ep93xx: use unsigned long for data

[Rosen Penev]

An int is being encoded as a void pointer but that breaks on 64-bit
systems as the type needs to match pointer size.

Signed-off-by: Rosen Penev <rosenp@gmail.com>
---
 drivers/ata/pata_ep93xx.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/ata/pata_ep93xx.c b/drivers/ata/pata_ep93xx.c
index f7e87d6af264..1663dcd00a93 100644
--- a/drivers/ata/pata_ep93xx.c
+++ b/drivers/ata/pata_ep93xx.c
@@ -977,7 +977,7 @@ static int ep93xx_pata_probe(struct platform_device *pdev)
 
 		match = soc_device_match(ep93xx_soc_table);
 		if (match)
-			ap->udma_mask = (unsigned int) match->data;
+			ap->udma_mask = (unsigned long) match->data;
 		else
 			ap->udma_mask = ATA_UDMA2;
 	}
-- 
2.54.0

Re: [PATCH 2/3] ata: pata_ep93xx: use unsigned long for data

[sashiko-bot]

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

Pre-existing issues:
- [High] Double free and invalid pointer dereference on DMA TX channel request failure
--

commit 29937e8c8082c869105691e9f38d16e45bbd37fb
Author: Rosen Penev <rosenp@gmail.com>

ata: pata_ep93xx: use unsigned long for data

An int is being encoded as a void pointer but that breaks on 64-bit
systems as the type needs to match pointer size.

> diff --git a/drivers/ata/pata_ep93xx.c b/drivers/ata/pata_ep93xx.c
> index f7e87d6af2641..1663dcd00a932 100644
> --- a/drivers/ata/pata_ep93xx.c
> +++ b/drivers/ata/pata_ep93xx.c
> @@ -977,7 +977,7 @@ static int ep93xx_pata_probe(struct platform_device *pdev)
>  
>  		match = soc_device_match(ep93xx_soc_table);
>  		if (match)
> -			ap->udma_mask = (unsigned int) match->data;
> +			ap->udma_mask = (unsigned long) match->data;

[Severity: High]
This is a pre-existing issue, but while reviewing this file I noticed a
potential double free and invalid pointer dereference in the DMA error
handling paths.

If dma_request_chan() for the "tx" channel fails in ep93xx_pata_dma_init(),
it returns an ERR_PTR which is stored in drv_data->dma_tx_channel, and jumps
to fail_release_rx:

drivers/ata/pata_ep93xx.c:ep93xx_pata_dma_init() {
    ...
	drv_data->dma_tx_channel = dma_request_chan(&pdev->dev, "tx");
	if (IS_ERR(drv_data->dma_tx_channel)) {
		ret = dev_err_probe(dev, PTR_ERR(drv_data->dma_tx_channel),
				    "tx DMA setup failed\n");
		goto fail_release_rx;
	}
    ...
fail_release_rx:
	dma_release_channel(drv_data->dma_rx_channel);
fail_release_dma:
	ep93xx_pata_release_dma(drv_data);
}

The fail_release_rx path calls dma_release_channel() on the rx channel but
doesn't set drv_data->dma_rx_channel to NULL. It then falls through to
fail_release_dma which calls ep93xx_pata_release_dma():

drivers/ata/pata_ep93xx.c:ep93xx_pata_release_dma() {
	if (drv_data->dma_rx_channel) {
		dma_release_channel(drv_data->dma_rx_channel);
		drv_data->dma_rx_channel = NULL;
	}
	if (drv_data->dma_tx_channel) {
		dma_release_channel(drv_data->dma_tx_channel);
		drv_data->dma_tx_channel = NULL;
	}
}

Will this cause dma_release_channel() to be called a second time on the rx
channel?

Also, since drv_data->dma_tx_channel holds an ERR_PTR rather than NULL,
won't the second if block try to dereference that ERR_PTR when
dma_release_channel() is called on it?

>  		else
>  			ap->udma_mask = ATA_UDMA2;
>  	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260530003627.42473-1-rosenp@gmail.com?part=2

[PATCH 3/3] ata: pata_ep93xx: add COMPILE_TEST support

[Rosen Penev]

Now that the build failures have been fixed, we can add COMPILE_TEST so
the buildbots can find potentially more problems.

Signed-off-by: Rosen Penev <rosenp@gmail.com>
---
 drivers/ata/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/ata/Kconfig b/drivers/ata/Kconfig
index 462cbb01567c..6e25e9fcadb2 100644
--- a/drivers/ata/Kconfig
+++ b/drivers/ata/Kconfig
@@ -692,7 +692,7 @@ config PATA_EFAR
 
 config PATA_EP93XX
 	tristate "Cirrus Logic EP93xx PATA support"
-	depends on ARCH_EP93XX
+	depends on ARCH_EP93XX || COMPILE_TEST
 	select PATA_TIMINGS
 	help
 	  This option enables support for the PATA controller in
-- 
2.54.0

Re: [PATCH 3/3] ata: pata_ep93xx: add COMPILE_TEST support

[sashiko-bot]

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

Pre-existing issues:
- [High] Double free of 'rx' DMA channel and invalid pointer dereference of `ERR_PTR` in the 'tx' DMA channel error path.
--

commit b636a1b70934f2b41b7dce1e54946eb6d1bed3b4
Author: Rosen Penev <rosenp@gmail.com>

ata: pata_ep93xx: add COMPILE_TEST support

This commit enables COMPILE_TEST support for the Cirrus Logic EP93xx PATA
controller. This allows the buildbots to compile the driver on other
architectures to find potential issues.

> diff --git a/drivers/ata/Kconfig b/drivers/ata/Kconfig
> index fff305ec1e788..28ca856ecc754 100644
> --- a/drivers/ata/Kconfig
> +++ b/drivers/ata/Kconfig
> @@ -692,7 +692,7 @@ config PATA_EFAR
>  
>  config PATA_EP93XX
>  	tristate "Cirrus Logic EP93xx PATA support"
> -	depends on ARCH_EP93XX
> +	depends on ARCH_EP93XX || COMPILE_TEST

[Severity: High]
This isn't a bug introduced by this patch, but enabling COMPILE_TEST might
expose a pre-existing error handling issue in drivers/ata/pata_ep93xx.c.

In ep93xx_pata_dma_init(), if dma_request_chan() for "tx" fails, it stores
the ERR_PTR in drv_data->dma_tx_channel and jumps to fail_release_rx:

drivers/ata/pata_ep93xx.c:ep93xx_pata_dma_init() {
    ...
	drv_data->dma_tx_channel = dma_request_chan(&pdev->dev, "tx");
	if (IS_ERR(drv_data->dma_tx_channel)) {
		ret = dev_err_probe(dev, PTR_ERR(drv_data->dma_tx_channel),
				    "tx DMA setup failed\n");
		goto fail_release_rx;
	}
    ...
fail_release_rx:
	dma_release_channel(drv_data->dma_rx_channel);
fail_release_dma:
	ep93xx_pata_release_dma(drv_data);

	return ret;
}

Could this sequence lead to a double-free and an invalid pointer dereference?

The fail_release_rx path releases dma_rx_channel but doesn't clear the
pointer to NULL before falling through to ep93xx_pata_release_dma(). Also,
dma_tx_channel holds an ERR_PTR which is non-NULL.

When ep93xx_pata_release_dma() is called, it checks if the pointers are
non-NULL:

drivers/ata/pata_ep93xx.c:ep93xx_pata_release_dma() {
    ...
	if (drv_data->dma_rx_channel) {
		dma_release_channel(drv_data->dma_rx_channel);
		drv_data->dma_rx_channel = NULL;
	}
	if (drv_data->dma_tx_channel) {
		dma_release_channel(drv_data->dma_tx_channel);
		drv_data->dma_tx_channel = NULL;
	}
    ...
}

Would this result in dma_release_channel() being called a second time on
the rx channel, and being called with the ERR_PTR for the tx channel?

>  	select PATA_TIMINGS
>  	help
>  	  This option enables support for the PATA controller in

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260530003627.42473-1-rosenp@gmail.com?part=3