[PATCH v3] ata: sata_dwc_460ex: Don't enable interrupt if libATA core registration fails

[PATCH v3] ata: sata_dwc_460ex: Don't enable interrupt if libATA core registration fails

[Nikolay Borisov]

Recent 4bbc16a353a9 commit changed the timing when interrupts are
enabled. However it didn't account for the libATA registration step. So
it's possible that interrupts are enabled when the device is not
registered with libATA core.

Resolve the issue by ensuring that interrupts are enabled only if the rest of the
initialization is successful.

Fixes: 4bbc16a353a9 ("ata: sata_dwc_460ex: enable SATA interrupts only after IRQ handler is registered")
Signed-off-by: Nikolay Borisov <nik.borisov@suse.com>
---


Changes since v1:
	* Added proper goto label
Changes since v2:
    * Rework logic so that interrupts are enabled if everything is successful
	and don't call phy_exit otherwise, as the driver is quite broken w.r.t to it.

 drivers/ata/sata_dwc_460ex.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/ata/sata_dwc_460ex.c b/drivers/ata/sata_dwc_460ex.c
index 8e3fc713891a..e74335dec42a 100644
--- a/drivers/ata/sata_dwc_460ex.c
+++ b/drivers/ata/sata_dwc_460ex.c
@@ -1185,11 +1185,13 @@ static int sata_dwc_probe(struct platform_device *ofdev)
 	 * error_handler() to execute a dummy Softreset EH session
 	 */
 	err = ata_host_activate(host, irq, sata_dwc_isr, 0, &sata_dwc_sht);
-	if (err)
+	if (err) {
 		dev_err(dev, "failed to activate host");
+	} else {
+		/* Enable SATA Interrupts */
+		sata_dwc_enable_interrupts(hsdev);
+	}
 
-	/* Enable SATA Interrupts */
-	sata_dwc_enable_interrupts(hsdev);
 	return 0;
 
 error_out:
--
2.55.0

Re: [PATCH v3] ata: sata_dwc_460ex: Don't enable interrupt if libATA core registration fails

[Damien Le Moal]

On 2026/09/17 18:35, Nikolay Borisov wrote:
> Recent 4bbc16a353a9 commit changed the timing when interrupts are
> enabled. However it didn't account for the libATA registration step. So
> it's possible that interrupts are enabled when the device is not
> registered with libATA core.
> 
> Resolve the issue by ensuring that interrupts are enabled only if the rest of the
> initialization is successful.
> 
> Fixes: 4bbc16a353a9 ("ata: sata_dwc_460ex: enable SATA interrupts only after IRQ handler is registered")
> Signed-off-by: Nikolay Borisov <nik.borisov@suse.com>
> ---
> 
> 
> Changes since v1:
> 	* Added proper goto label
> Changes since v2:
>     * Rework logic so that interrupts are enabled if everything is successful
> 	and don't call phy_exit otherwise, as the driver is quite broken w.r.t to it.
> 
>  drivers/ata/sata_dwc_460ex.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/ata/sata_dwc_460ex.c b/drivers/ata/sata_dwc_460ex.c
> index 8e3fc713891a..e74335dec42a 100644
> --- a/drivers/ata/sata_dwc_460ex.c
> +++ b/drivers/ata/sata_dwc_460ex.c
> @@ -1185,11 +1185,13 @@ static int sata_dwc_probe(struct platform_device *ofdev)
>  	 * error_handler() to execute a dummy Softreset EH session
>  	 */
>  	err = ata_host_activate(host, irq, sata_dwc_isr, 0, &sata_dwc_sht);
> -	if (err)
> +	if (err) {
>  		dev_err(dev, "failed to activate host");

Why not simply "goto err_out;" here ?

> +	} else {
> +		/* Enable SATA Interrupts */
> +		sata_dwc_enable_interrupts(hsdev);
> +	}
>  
> -	/* Enable SATA Interrupts */
> -	sata_dwc_enable_interrupts(hsdev);
>  	return 0;
>  
>  error_out:
> --
> 2.55.0


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v3] ata: sata_dwc_460ex: Don't enable interrupt if libATA core registration fails

[Nikolay Borisov]

On 17.09.26 г. 16:18 ч., Damien Le Moal wrote:
> On 2026/09/17 18:35, Nikolay Borisov wrote:
>> Recent 4bbc16a353a9 commit changed the timing when interrupts are
>> enabled. However it didn't account for the libATA registration step. So
>> it's possible that interrupts are enabled when the device is not
>> registered with libATA core.
>>
>> Resolve the issue by ensuring that interrupts are enabled only if the rest of the
>> initialization is successful.
>>
>> Fixes: 4bbc16a353a9 ("ata: sata_dwc_460ex: enable SATA interrupts only after IRQ handler is registered")
>> Signed-off-by: Nikolay Borisov <nik.borisov@suse.com>
>> ---
>>
>>
>> Changes since v1:
>> 	* Added proper goto label
>> Changes since v2:
>>      * Rework logic so that interrupts are enabled if everything is successful
>> 	and don't call phy_exit otherwise, as the driver is quite broken w.r.t to it.
>>
>>   drivers/ata/sata_dwc_460ex.c | 8 +++++---
>>   1 file changed, 5 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/ata/sata_dwc_460ex.c b/drivers/ata/sata_dwc_460ex.c
>> index 8e3fc713891a..e74335dec42a 100644
>> --- a/drivers/ata/sata_dwc_460ex.c
>> +++ b/drivers/ata/sata_dwc_460ex.c
>> @@ -1185,11 +1185,13 @@ static int sata_dwc_probe(struct platform_device *ofdev)
>>   	 * error_handler() to execute a dummy Softreset EH session
>>   	 */
>>   	err = ata_host_activate(host, irq, sata_dwc_isr, 0, &sata_dwc_sht);
>> -	if (err)
>> +	if (err) {
>>   		dev_err(dev, "failed to activate host");
> 
> Why not simply "goto err_out;" here ?

That was in V2 however Niklas discovered some issue w.r.t to phy_exit. 
https://lore.kernel.org/all/aqGFjsuIMRIy5xfv@ryzen/
> 
>> +	} else {
>> +		/* Enable SATA Interrupts */
>> +		sata_dwc_enable_interrupts(hsdev);
>> +	}
>>   
>> -	/* Enable SATA Interrupts */
>> -	sata_dwc_enable_interrupts(hsdev);
>>   	return 0;
>>   
>>   error_out:
>> --
>> 2.55.0
> 
> 

Re: [PATCH v3] ata: sata_dwc_460ex: Don't enable interrupt if libATA core registration fails

[Niklas Cassel]

On Thu, Sep 17, 2026 at 04:26:41PM +0300, Nikolay Borisov wrote:
> 
> 
> On 17.09.26 г. 16:18 ч., Damien Le Moal wrote:
> > On 2026/09/17 18:35, Nikolay Borisov wrote:
> > > Recent 4bbc16a353a9 commit changed the timing when interrupts are
> > > enabled. However it didn't account for the libATA registration step. So
> > > it's possible that interrupts are enabled when the device is not
> > > registered with libATA core.
> > > 
> > > Resolve the issue by ensuring that interrupts are enabled only if the rest of the
> > > initialization is successful.
> > > 
> > > Fixes: 4bbc16a353a9 ("ata: sata_dwc_460ex: enable SATA interrupts only after IRQ handler is registered")
> > > Signed-off-by: Nikolay Borisov <nik.borisov@suse.com>
> > > ---
> > > 
> > > 
> > > Changes since v1:
> > > 	* Added proper goto label
> > > Changes since v2:
> > >      * Rework logic so that interrupts are enabled if everything is successful
> > > 	and don't call phy_exit otherwise, as the driver is quite broken w.r.t to it.
> > > 
> > >   drivers/ata/sata_dwc_460ex.c | 8 +++++---
> > >   1 file changed, 5 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/ata/sata_dwc_460ex.c b/drivers/ata/sata_dwc_460ex.c
> > > index 8e3fc713891a..e74335dec42a 100644
> > > --- a/drivers/ata/sata_dwc_460ex.c
> > > +++ b/drivers/ata/sata_dwc_460ex.c
> > > @@ -1185,11 +1185,13 @@ static int sata_dwc_probe(struct platform_device *ofdev)
> > >   	 * error_handler() to execute a dummy Softreset EH session
> > >   	 */
> > >   	err = ata_host_activate(host, irq, sata_dwc_isr, 0, &sata_dwc_sht);
> > > -	if (err)
> > > +	if (err) {
> > >   		dev_err(dev, "failed to activate host");
> > 
> > Why not simply "goto err_out;" here ?
> 
> That was in V2 however Niklas discovered some issue w.r.t to phy_exit.
> https://lore.kernel.org/all/aqGFjsuIMRIy5xfv@ryzen/

I have since merged a series, which is queued on ibata/linux.git (for-7.4):
https://lore.kernel.org/linux-ide/20260915084127.692494-8-cassel@kernel.org/

See:
https://lore.kernel.org/linux-ide/20260915084127.692494-13-cassel@kernel.org/

Where, if ata_host_activate() failed after ata_host_start() returned success
(which will have called call->port_start()), it will call ata_host_undo_start()
which will call ->port_stop().


Thus, the Sashiko comment on your v2:
https://lore.kernel.org/linux-ide/20260909113821.A72011F00A3A@smtp.kernel.org/

is no longer valid.


Please send your V2 again (as a V4) just so that we can get Sashiko to review
it again.

Preferably use:
$ git format-patch --base libata/for-7.4

So that it adds the proper base commit that you depend on.


Kind regards,
Niklas