[PATCH] ata: sata_gemini: unwind clocks on IDE pinctrl errors

[PATCH] ata: sata_gemini: unwind clocks on IDE pinctrl errors

[Myeonghun Pak]

gemini_sata_bridge_init() prepares and enables both SATA PCLKs, then
disables them again while keeping the clocks prepared for later bridge
start and stop operations. If gemini_setup_ide_pins() fails after that,
gemini_sata_probe() returns directly and skips the existing
out_unprep_clk unwind path.

Route the IDE pinctrl failure through out_unprep_clk so the clocks
prepared by gemini_sata_bridge_init() are unprepared before probe
fails.

Fixes: d872ced29d5f ("ata: sata_gemini: Introduce explicit IDE pin control")
Co-developed-by: Ijae Kim <ae878000@gmail.com>
Signed-off-by: Ijae Kim <ae878000@gmail.com>
Signed-off-by: Myeonghun Pak <mhun512@gmail.com>
---
 drivers/ata/sata_gemini.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/ata/sata_gemini.c b/drivers/ata/sata_gemini.c
index 582c9708275c..92984b34fd3b 100644
--- a/drivers/ata/sata_gemini.c
+++ b/drivers/ata/sata_gemini.c
@@ -355,7 +355,7 @@ static int gemini_sata_probe(struct platform_device *pdev)
 	if (sg->ide_pins) {
 		ret = gemini_setup_ide_pins(dev);
 		if (ret)
-			return ret;
+			goto out_unprep_clk;
 	}
 
 	dev_info(dev, "set up the Gemini IDE/SATA nexus\n");
-- 
2.47.1

Re: [PATCH] ata: sata_gemini: unwind clocks on IDE pinctrl errors

[sashiko-bot]

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

Pre-existing issues:
- [High] Probe deferral is permanently masked because devm_clk_get errors are converted to -ENODEV.
- [High] The sata_gemini bridge state (sg) can be freed while still in use by consumers like pata_ftide010, leading to a Use-After-Free on driver unbind.
- [High] Unconditional clock disable during port stop can lead to negative enable counts if port start partially fails.
--

commit 41dfc4226f4d13085ec0bbac5ed9062fd893a436
Author: Myeonghun Pak <mhun512@gmail.com>

ata: sata_gemini: unwind clocks on IDE pinctrl errors

gemini_sata_bridge_init() prepares and enables both SATA PCLKs, then
disables them again while keeping the clocks prepared for later bridge
start and stop operations. This patch routes the IDE pinctrl failure
through out_unprep_clk so the clocks prepared by gemini_sata_bridge_init()
are correctly unprepared before probe fails.

> diff --git a/drivers/ata/sata_gemini.c b/drivers/ata/sata_gemini.c
> --- a/drivers/ata/sata_gemini.c
> +++ b/drivers/ata/sata_gemini.c
> @@ -353,7 +353,7 @@ static int gemini_sata_probe(struct platform_device *pdev)
>  	if (sg->ide_pins) {
>  		ret = gemini_setup_ide_pins(dev);
>  		if (ret)
> -			return ret;
> +			goto out_unprep_clk;

[Severity: High]
This is a pre-existing issue, but I noticed that probe deferral is
permanently masked when getting the clocks.

In gemini_sata_bridge_init(), which is called earlier in probe, errors
from devm_clk_get() are overwritten:

drivers/ata/sata_gemini.c:gemini_sata_bridge_init() {
    sg->sata0_pclk = devm_clk_get(dev, "SATA0_PCLK");
    if (IS_ERR(sg->sata0_pclk)) {
        dev_err(dev, "no SATA0 PCLK");
        return -ENODEV;
    }
    ...
}

If devm_clk_get() returns -EPROBE_DEFER because the clock provider isn't
ready, does returning -ENODEV cause the SATA controller to permanently
fail to probe?

[Severity: High]
This isn't a bug introduced by this patch, but there appears to be a
lifetime issue between sata_gemini and pata_ftide010.

When pata_ftide010 retrieves the bridge state:

drivers/ata/pata_ftide010.c:pata_ftide010_gemini_init() {
    ...
    sg = gemini_sata_bridge_get();
    if (IS_ERR(sg))
        return PTR_ERR(sg);
    ftide->sg = sg;
    ...
}

The bridge is returned from a singleton:

drivers/ata/sata_gemini.c:gemini_sata_bridge_get() {
    if (sg_singleton)
        return sg_singleton;
    ...
}

Since the memory is allocated with devm_kzalloc() in gemini_sata_probe()
and no device link or reference count is taken, can this lead to a
use-after-free if sata_gemini is unbound via sysfs while pata_ftide010
is still active?

[Severity: High]
This is also a pre-existing issue, but the clock disable logic might become
unbalanced during port stop.

In pata_ftide010_gemini_port_stop(), the bridge is unconditionally
stopped:

drivers/ata/pata_ftide010.c:pata_ftide010_gemini_port_stop() {
    ...
    if (ftide->master_to_sata1) {
        dev_info(dev, "SATA1 (master) stop\n");
        gemini_sata_stop_bridge(sg, 1);
    }
    ...
}

Which disables the clock:

drivers/ata/sata_gemini.c:gemini_sata_stop_bridge() {
    ...
    else if (bridge == 1)
        clk_disable(sg->sata1_pclk);
    ...
}

If port start partially fails (e.g., due to a PHY timeout in
gemini_sata_setup_bridge()) and the clock was already disabled in the
error path of gemini_sata_start_bridge(), does this unconditional stop
cause a kernel warning or panic due to negative enable counts?

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

Re: [PATCH] ata: sata_gemini: unwind clocks on IDE pinctrl errors

[Niklas Cassel]

On Fri, Jun 26, 2026 at 05:58:37PM +0900, Myeonghun Pak wrote:
> gemini_sata_bridge_init() prepares and enables both SATA PCLKs, then
> disables them again while keeping the clocks prepared for later bridge
> start and stop operations. If gemini_setup_ide_pins() fails after that,
> gemini_sata_probe() returns directly and skips the existing
> out_unprep_clk unwind path.
> 
> Route the IDE pinctrl failure through out_unprep_clk so the clocks
> prepared by gemini_sata_bridge_init() are unprepared before probe
> fails.
> 
> Fixes: d872ced29d5f ("ata: sata_gemini: Introduce explicit IDE pin control")
> Co-developed-by: Ijae Kim <ae878000@gmail.com>
> Signed-off-by: Ijae Kim <ae878000@gmail.com>
> Signed-off-by: Myeonghun Pak <mhun512@gmail.com>
> ---
>  drivers/ata/sata_gemini.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/ata/sata_gemini.c b/drivers/ata/sata_gemini.c
> index 582c9708275c..92984b34fd3b 100644
> --- a/drivers/ata/sata_gemini.c
> +++ b/drivers/ata/sata_gemini.c
> @@ -355,7 +355,7 @@ static int gemini_sata_probe(struct platform_device *pdev)
>  	if (sg->ide_pins) {
>  		ret = gemini_setup_ide_pins(dev);
>  		if (ret)
> -			return ret;
> +			goto out_unprep_clk;
>  	}
>  
>  	dev_info(dev, "set up the Gemini IDE/SATA nexus\n");
> -- 
> 2.47.1
> 

Reviewed-by: Niklas Cassel <cassel@kernel.org>

Re: [PATCH] ata: sata_gemini: unwind clocks on IDE pinctrl errors

[Damien Le Moal]

On 6/26/26 17:58, Myeonghun Pak wrote:
> gemini_sata_bridge_init() prepares and enables both SATA PCLKs, then
> disables them again while keeping the clocks prepared for later bridge
> start and stop operations. If gemini_setup_ide_pins() fails after that,
> gemini_sata_probe() returns directly and skips the existing
> out_unprep_clk unwind path.
> 
> Route the IDE pinctrl failure through out_unprep_clk so the clocks
> prepared by gemini_sata_bridge_init() are unprepared before probe
> fails.
> 
> Fixes: d872ced29d5f ("ata: sata_gemini: Introduce explicit IDE pin control")
> Co-developed-by: Ijae Kim <ae878000@gmail.com>
> Signed-off-by: Ijae Kim <ae878000@gmail.com>

One of the above 2 is redundant I think.

> Signed-off-by: Myeonghun Pak <mhun512@gmail.com>
> ---
>  drivers/ata/sata_gemini.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/ata/sata_gemini.c b/drivers/ata/sata_gemini.c
> index 582c9708275c..92984b34fd3b 100644
> --- a/drivers/ata/sata_gemini.c
> +++ b/drivers/ata/sata_gemini.c
> @@ -355,7 +355,7 @@ static int gemini_sata_probe(struct platform_device *pdev)
>  	if (sg->ide_pins) {
>  		ret = gemini_setup_ide_pins(dev);
>  		if (ret)
> -			return ret;
> +			goto out_unprep_clk;
>  	}
>  
>  	dev_info(dev, "set up the Gemini IDE/SATA nexus\n");


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH] ata: sata_gemini: unwind clocks on IDE pinctrl errors

[Niklas Cassel]

On Fri, Jun 26, 2026 at 08:32:02PM +0900, Damien Le Moal wrote:
> On 6/26/26 17:58, Myeonghun Pak wrote:
> > gemini_sata_bridge_init() prepares and enables both SATA PCLKs, then
> > disables them again while keeping the clocks prepared for later bridge
> > start and stop operations. If gemini_setup_ide_pins() fails after that,
> > gemini_sata_probe() returns directly and skips the existing
> > out_unprep_clk unwind path.
> > 
> > Route the IDE pinctrl failure through out_unprep_clk so the clocks
> > prepared by gemini_sata_bridge_init() are unprepared before probe
> > fails.
> > 
> > Fixes: d872ced29d5f ("ata: sata_gemini: Introduce explicit IDE pin control")
> > Co-developed-by: Ijae Kim <ae878000@gmail.com>
> > Signed-off-by: Ijae Kim <ae878000@gmail.com>
> 
> One of the above 2 is redundant I think.

The tags are correct:
https://docs.kernel.org/process/submitting-patches.html#when-to-use-acked-by-cc-and-co-developed-by

"Since Co-developed-by: denotes authorship, every Co-developed-by: must be
immediately followed by a Signed-off-by: of the associated co-author."



Kind regards,
Niklas

Re: [PATCH] ata: sata_gemini: unwind clocks on IDE pinctrl errors

[Damien Le Moal]

On 6/26/26 20:35, Niklas Cassel wrote:
> On Fri, Jun 26, 2026 at 08:32:02PM +0900, Damien Le Moal wrote:
>> On 6/26/26 17:58, Myeonghun Pak wrote:
>>> gemini_sata_bridge_init() prepares and enables both SATA PCLKs, then
>>> disables them again while keeping the clocks prepared for later bridge
>>> start and stop operations. If gemini_setup_ide_pins() fails after that,
>>> gemini_sata_probe() returns directly and skips the existing
>>> out_unprep_clk unwind path.
>>>
>>> Route the IDE pinctrl failure through out_unprep_clk so the clocks
>>> prepared by gemini_sata_bridge_init() are unprepared before probe
>>> fails.
>>>
>>> Fixes: d872ced29d5f ("ata: sata_gemini: Introduce explicit IDE pin control")
>>> Co-developed-by: Ijae Kim <ae878000@gmail.com>
>>> Signed-off-by: Ijae Kim <ae878000@gmail.com>
>>
>> One of the above 2 is redundant I think.
> 
> The tags are correct:
> https://docs.kernel.org/process/submitting-patches.html#when-to-use-acked-by-cc-and-co-developed-by
> 
> "Since Co-developed-by: denotes authorship, every Co-developed-by: must be
> immediately followed by a Signed-off-by: of the associated co-author."

OK.

> 
> 
> 
> Kind regards,
> Niklas


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH] ata: sata_gemini: unwind clocks on IDE pinctrl errors

[Linus Walleij]

On Fri, Jun 26, 2026 at 11:00 AM Myeonghun Pak <mhun512@gmail.com> wrote:

> gemini_sata_bridge_init() prepares and enables both SATA PCLKs, then
> disables them again while keeping the clocks prepared for later bridge
> start and stop operations. If gemini_setup_ide_pins() fails after that,
> gemini_sata_probe() returns directly and skips the existing
> out_unprep_clk unwind path.
>
> Route the IDE pinctrl failure through out_unprep_clk so the clocks
> prepared by gemini_sata_bridge_init() are unprepared before probe
> fails.
>
> Fixes: d872ced29d5f ("ata: sata_gemini: Introduce explicit IDE pin control")
> Co-developed-by: Ijae Kim <ae878000@gmail.com>
> Signed-off-by: Ijae Kim <ae878000@gmail.com>
> Signed-off-by: Myeonghun Pak <mhun512@gmail.com>

Thanks!
Reviewed-by: Linus Walleij <linusw@kernel.org>

Yours,
Linus Walleij

Re: [PATCH] ata: sata_gemini: unwind clocks on IDE pinctrl errors

[Damien Le Moal]

On 6/26/26 17:58, Myeonghun Pak wrote:
> gemini_sata_bridge_init() prepares and enables both SATA PCLKs, then
> disables them again while keeping the clocks prepared for later bridge
> start and stop operations. If gemini_setup_ide_pins() fails after that,
> gemini_sata_probe() returns directly and skips the existing
> out_unprep_clk unwind path.
> 
> Route the IDE pinctrl failure through out_unprep_clk so the clocks
> prepared by gemini_sata_bridge_init() are unprepared before probe
> fails.
> 
> Fixes: d872ced29d5f ("ata: sata_gemini: Introduce explicit IDE pin control")
> Co-developed-by: Ijae Kim <ae878000@gmail.com>
> Signed-off-by: Ijae Kim <ae878000@gmail.com>
> Signed-off-by: Myeonghun Pak <mhun512@gmail.com>

Applied to for-7.2-fixes. Thanks!

-- 
Damien Le Moal
Western Digital Research