[PATCH] scsi: ata: Fix a race condition between scsi error handler and ahci interrupt

[PATCH] scsi: ata: Fix a race condition between scsi error handler and ahci interrupt

[linan666]

From: Li Nan <linan122@huawei.com>

interrupt                            scsi_eh

ahci_error_intr
  =>ata_port_freeze
    =>__ata_port_freeze
      =>ahci_freeze (turn IRQ off)
    =>ata_port_abort
      =>ata_port_schedule_eh
        =>shost->host_eh_scheduled++;
        host_eh_scheduled = 1
                                     scsi_error_handler
                                       =>ata_scsi_error
                                         =>ata_scsi_port_error_handler
                                           =>ahci_error_handler
                                           . =>sata_pmp_error_handler
                                           .   =>ata_eh_thaw_port
                                           .     =>ahci_thaw (turn IRQ on)
ahci_error_intr                            .
  =>ata_port_freeze                        .
    =>__ata_port_freeze                    .
      =>ahci_freeze (turn IRQ off)         .
    =>ata_port_abort                       .
      =>ata_port_schedule_eh               .
        =>shost->host_eh_scheduled++;      .
        host_eh_scheduled = 2              .
                                           =>ata_std_end_eh
                                             =>host->host_eh_scheduled = 0;

'host_eh_scheduled' is 0 and scsi eh thread will not be scheduled again,
and the ata port remain freeze and will never be enabled.

If EH thread is already running, no need to freeze port and schedule
EH again.

Reported-by: luojian <luojian5@huawei.com>
Signed-off-by: Li Nan <linan122@huawei.com>
---
 drivers/ata/libahci.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/ata/libahci.c b/drivers/ata/libahci.c
index e2bacedf28ef..0dfb0b807324 100644
--- a/drivers/ata/libahci.c
+++ b/drivers/ata/libahci.c
@@ -1840,9 +1840,17 @@ static void ahci_error_intr(struct ata_port *ap, u32 irq_stat)
 
 	/* okay, let's hand over to EH */
 
-	if (irq_stat & PORT_IRQ_FREEZE)
+	if (irq_stat & PORT_IRQ_FREEZE) {
+		/*
+		 * EH already running, this may happen if the port is
+		 * thawed in the EH. But we cannot freeze it again
+		 * otherwise the port will never be thawed.
+		 */
+		if (ap->pflags & (ATA_PFLAG_EH_PENDING |
+			ATA_PFLAG_EH_IN_PROGRESS))
+			return;
 		ata_port_freeze(ap);
-	else if (fbs_need_dec) {
+	} else if (fbs_need_dec) {
 		ata_link_abort(link);
 		ahci_fbs_dec_intr(ap);
 	} else
-- 
2.39.2

Re: [PATCH] scsi: ata: Fix a race condition between scsi error handler and ahci interrupt

[Damien Le Moal]

On 8/10/23 10:48, linan666@huaweicloud.com wrote:
> From: Li Nan <linan122@huawei.com>
> 

Please explain the problem first instead of starting with a function call
timeline which cannot ba analized without explanations.

> interrupt                            scsi_eh
> 
> ahci_error_intr
>   =>ata_port_freeze
>     =>__ata_port_freeze
>       =>ahci_freeze (turn IRQ off)
>     =>ata_port_abort
>       =>ata_port_schedule_eh
>         =>shost->host_eh_scheduled++;
>         host_eh_scheduled = 1
>                                      scsi_error_handler
>                                        =>ata_scsi_error
>                                          =>ata_scsi_port_error_handler
>                                            =>ahci_error_handler
>                                            . =>sata_pmp_error_handler
>                                            .   =>ata_eh_thaw_port
>                                            .     =>ahci_thaw (turn IRQ on)
> ahci_error_intr                            .
>   =>ata_port_freeze                        .
>     =>__ata_port_freeze                    .
>       =>ahci_freeze (turn IRQ off)         .
>     =>ata_port_abort                       .
>       =>ata_port_schedule_eh               .
>         =>shost->host_eh_scheduled++;      .
>         host_eh_scheduled = 2              .
>                                            =>ata_std_end_eh
>                                              =>host->host_eh_scheduled = 0;
> 
> 'host_eh_scheduled' is 0 and scsi eh thread will not be scheduled again,
> and the ata port remain freeze and will never be enabled.
> 
> If EH thread is already running, no need to freeze port and schedule
> EH again.
> 
> Reported-by: luojian <luojian5@huawei.com>
> Signed-off-by: Li Nan <linan122@huawei.com>
> ---
>  drivers/ata/libahci.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/ata/libahci.c b/drivers/ata/libahci.c
> index e2bacedf28ef..0dfb0b807324 100644
> --- a/drivers/ata/libahci.c
> +++ b/drivers/ata/libahci.c
> @@ -1840,9 +1840,17 @@ static void ahci_error_intr(struct ata_port *ap, u32 irq_stat)
>  
>  	/* okay, let's hand over to EH */
>  
> -	if (irq_stat & PORT_IRQ_FREEZE)
> +	if (irq_stat & PORT_IRQ_FREEZE) {
> +		/*
> +		 * EH already running, this may happen if the port is
> +		 * thawed in the EH. But we cannot freeze it again
> +		 * otherwise the port will never be thawed.
> +		 */
> +		if (ap->pflags & (ATA_PFLAG_EH_PENDING |
> +			ATA_PFLAG_EH_IN_PROGRESS))
> +			return;

This is definitely not correct because EH may have been scheduled for a non
fatal action like a device revalidate or to get sense data for successful
commands. With this change, the port will NOT be frozen when a hard error IRQ
comes while EH is waiting to start, that is, while EH waits for all commands to
complete first.

Furthermore, if you get an IRQ that requires the port to be frozen, it means
that you had a failed command. In that case, the drive is in error state per
ATA specs and stops all communication until a read log 10h command is issued.
So you should never ever see 2 error IRQs one after the other. If you do, it
very likely means that you have buggy hardware.

How do you get into this situation ? What adapter and disk are you using ?

>  		ata_port_freeze(ap);
> -	else if (fbs_need_dec) {
> +	} else if (fbs_need_dec) {
>  		ata_link_abort(link);
>  		ahci_fbs_dec_intr(ap);
>  	} else

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH] scsi: ata: Fix a race condition between scsi error handler and ahci interrupt

[Li Nan]

在 2023/8/10 10:49, Damien Le Moal 写道:
> On 8/10/23 10:48, linan666@huaweicloud.com wrote:
>> From: Li Nan <linan122@huawei.com>
>>
> 
> Please explain the problem first instead of starting with a function call
> timeline which cannot ba analized without explanations.
> 
>> interrupt                            scsi_eh
>>
>> ahci_error_intr
>>    =>ata_port_freeze
>>      =>__ata_port_freeze
>>        =>ahci_freeze (turn IRQ off)
>>      =>ata_port_abort
>>        =>ata_port_schedule_eh
>>          =>shost->host_eh_scheduled++;
>>          host_eh_scheduled = 1
>>                                       scsi_error_handler
>>                                         =>ata_scsi_error
>>                                           =>ata_scsi_port_error_handler
>>                                             =>ahci_error_handler
>>                                             . =>sata_pmp_error_handler
>>                                             .   =>ata_eh_thaw_port
>>                                             .     =>ahci_thaw (turn IRQ on)
>> ahci_error_intr                            .
>>    =>ata_port_freeze                        .
>>      =>__ata_port_freeze                    .
>>        =>ahci_freeze (turn IRQ off)         .
>>      =>ata_port_abort                       .
>>        =>ata_port_schedule_eh               .
>>          =>shost->host_eh_scheduled++;      .
>>          host_eh_scheduled = 2              .
>>                                             =>ata_std_end_eh
>>                                               =>host->host_eh_scheduled = 0;
>>
>> 'host_eh_scheduled' is 0 and scsi eh thread will not be scheduled again,
>> and the ata port remain freeze and will never be enabled.
>>
>> If EH thread is already running, no need to freeze port and schedule
>> EH again.
>>
>> Reported-by: luojian <luojian5@huawei.com>
>> Signed-off-by: Li Nan <linan122@huawei.com>
>> ---
>>   drivers/ata/libahci.c | 12 ++++++++++--
>>   1 file changed, 10 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/ata/libahci.c b/drivers/ata/libahci.c
>> index e2bacedf28ef..0dfb0b807324 100644
>> --- a/drivers/ata/libahci.c
>> +++ b/drivers/ata/libahci.c
>> @@ -1840,9 +1840,17 @@ static void ahci_error_intr(struct ata_port *ap, u32 irq_stat)
>>   
>>   	/* okay, let's hand over to EH */
>>   
>> -	if (irq_stat & PORT_IRQ_FREEZE)
>> +	if (irq_stat & PORT_IRQ_FREEZE) {
>> +		/*
>> +		 * EH already running, this may happen if the port is
>> +		 * thawed in the EH. But we cannot freeze it again
>> +		 * otherwise the port will never be thawed.
>> +		 */
>> +		if (ap->pflags & (ATA_PFLAG_EH_PENDING |
>> +			ATA_PFLAG_EH_IN_PROGRESS))
>> +			return;
> 
> This is definitely not correct because EH may have been scheduled for a non
> fatal action like a device revalidate or to get sense data for successful
> commands. With this change, the port will NOT be frozen when a hard error IRQ
> comes while EH is waiting to start, that is, while EH waits for all commands to
> complete first.
> 

Yeah, we should find a better way to fix it. Do you have any suggesstions?

> Furthermore, if you get an IRQ that requires the port to be frozen, it means
> that you had a failed command. In that case, the drive is in error state per
> ATA specs and stops all communication until a read log 10h command is issued.
> So you should never ever see 2 error IRQs one after the other. If you do, it
> very likely means that you have buggy hardware.
> 
> How do you get into this situation ? What adapter and disk are you using ?
> 

 > How do you get into this situation ?
The first IRQ is io error, the second IRQ is disk link flash break.

 > What adapter and disk are you using ?
It is a disk developed by our company, but we think the same issue 
exists when using other disks.

>>   		ata_port_freeze(ap);
>> -	else if (fbs_need_dec) {
>> +	} else if (fbs_need_dec) {
>>   		ata_link_abort(link);
>>   		ahci_fbs_dec_intr(ap);
>>   	} else
> 

-- 
Thanks,
Nan

Re: [PATCH] scsi: ata: Fix a race condition between scsi error handler and ahci interrupt

[Damien Le Moal]

On 8/14/23 15:41, Li Nan wrote:
>> This is definitely not correct because EH may have been scheduled for a non
>> fatal action like a device revalidate or to get sense data for successful
>> commands. With this change, the port will NOT be frozen when a hard error IRQ
>> comes while EH is waiting to start, that is, while EH waits for all commands to
>> complete first.
>>
> 
> Yeah, we should find a better way to fix it. Do you have any suggesstions?
> 
>> Furthermore, if you get an IRQ that requires the port to be frozen, it means
>> that you had a failed command. In that case, the drive is in error state per
>> ATA specs and stops all communication until a read log 10h command is issued.
>> So you should never ever see 2 error IRQs one after the other. If you do, it
>> very likely means that you have buggy hardware.
>>
>> How do you get into this situation ? What adapter and disk are you using ?
>>
> 
>  > How do you get into this situation ?
> The first IRQ is io error, the second IRQ is disk link flash break.

What does "link flash break" mean ?

> 
>  > What adapter and disk are you using ?
> It is a disk developed by our company, but we think the same issue 
> exists when using other disks.

As I said, I find this situation highly suspect because if the first IRQ was to
signal an IO error that the drive reported, then per ATA specifications, the
drive should be in error mode and should NOT have transmitted any other FIS
after the SDB FIS that signaled the error. Nothing at all should come after that
error SDB FIS, until the host issues a read log 10h to get thee drive out of
error state.

If this is a prototype device, I would recommend that you take an ATA bus trace
and verify the FIS traffic. Something fishy is going on with the drive in my
opinion.

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH] scsi: ata: Fix a race condition between scsi error handler and ahci interrupt

[Li Nan]

在 2023/8/14 15:50, Damien Le Moal 写道:
> On 8/14/23 15:41, Li Nan wrote:
>>> This is definitely not correct because EH may have been scheduled for a non
>>> fatal action like a device revalidate or to get sense data for successful
>>> commands. With this change, the port will NOT be frozen when a hard error IRQ
>>> comes while EH is waiting to start, that is, while EH waits for all commands to
>>> complete first.
>>>
>>
>> Yeah, we should find a better way to fix it. Do you have any suggesstions?
>>
>>> Furthermore, if you get an IRQ that requires the port to be frozen, it means
>>> that you had a failed command. In that case, the drive is in error state per
>>> ATA specs and stops all communication until a read log 10h command is issued.
>>> So you should never ever see 2 error IRQs one after the other. If you do, it
>>> very likely means that you have buggy hardware.
>>>
>>> How do you get into this situation ? What adapter and disk are you using ?
>>>
>>
>>   > How do you get into this situation ?
>> The first IRQ is io error, the second IRQ is disk link flash break.
> 
> What does "link flash break" mean ?
> 
>>
>>   > What adapter and disk are you using ?
>> It is a disk developed by our company, but we think the same issue
>> exists when using other disks.
> 
> As I said, I find this situation highly suspect because if the first IRQ was to
> signal an IO error that the drive reported, then per ATA specifications, the
> drive should be in error mode and should NOT have transmitted any other FIS
> after the SDB FIS that signaled the error. Nothing at all should come after that
> error SDB FIS, until the host issues a read log 10h to get thee drive out of
> error state.
> 
> If this is a prototype device, I would recommend that you take an ATA bus trace
> and verify the FIS traffic. Something fishy is going on with the drive in my
> opinion.
> 

Thank you for your patient explanation. I'm sorry I didn't explain the
problem clearly before. After discussing with my colleagues who know
more about dirvers, Let me re-describe the problem.

The problem`s situation is the SATA link is quickly disconnected and 
connected. For example, when an I/O error is processed in error handling 
thread, the disk is manually removed and inserted, and the AHCI chip 
reports a hot plug interrupt.

This scenario is not just an NCQ error, but a disk is removed and 
quickly inserted before the error processing is completed. For the error 
handling process, the disk status needs to be restored after the error 
handling is complete.

-- 
Thanks,
Nan

Re: [PATCH] scsi: ata: Fix a race condition between scsi error handler and ahci interrupt

[Damien Le Moal]

On 8/14/23 22:20, Li Nan wrote:
> 
> 在 2023/8/14 15:50, Damien Le Moal 写道:
>> On 8/14/23 15:41, Li Nan wrote:
>>>> This is definitely not correct because EH may have been scheduled for a non
>>>> fatal action like a device revalidate or to get sense data for successful
>>>> commands. With this change, the port will NOT be frozen when a hard error IRQ
>>>> comes while EH is waiting to start, that is, while EH waits for all commands to
>>>> complete first.
>>>>
>>>
>>> Yeah, we should find a better way to fix it. Do you have any suggesstions?
>>>
>>>> Furthermore, if you get an IRQ that requires the port to be frozen, it means
>>>> that you had a failed command. In that case, the drive is in error state per
>>>> ATA specs and stops all communication until a read log 10h command is issued.
>>>> So you should never ever see 2 error IRQs one after the other. If you do, it
>>>> very likely means that you have buggy hardware.
>>>>
>>>> How do you get into this situation ? What adapter and disk are you using ?
>>>>
>>>
>>>   > How do you get into this situation ?
>>> The first IRQ is io error, the second IRQ is disk link flash break.
>>
>> What does "link flash break" mean ?
>>
>>>
>>>   > What adapter and disk are you using ?
>>> It is a disk developed by our company, but we think the same issue
>>> exists when using other disks.
>>
>> As I said, I find this situation highly suspect because if the first IRQ was to
>> signal an IO error that the drive reported, then per ATA specifications, the
>> drive should be in error mode and should NOT have transmitted any other FIS
>> after the SDB FIS that signaled the error. Nothing at all should come after that
>> error SDB FIS, until the host issues a read log 10h to get thee drive out of
>> error state.
>>
>> If this is a prototype device, I would recommend that you take an ATA bus trace
>> and verify the FIS traffic. Something fishy is going on with the drive in my
>> opinion.
>>
> 
> Thank you for your patient explanation. I'm sorry I didn't explain the
> problem clearly before. After discussing with my colleagues who know
> more about dirvers, Let me re-describe the problem.
> 
> The problem`s situation is the SATA link is quickly disconnected and 
> connected. For example, when an I/O error is processed in error handling 
> thread, the disk is manually removed and inserted, and the AHCI chip 
> reports a hot plug interrupt.
> 
> This scenario is not just an NCQ error, but a disk is removed and 
> quickly inserted before the error processing is completed. For the error 
> handling process, the disk status needs to be restored after the error 
> handling is complete.

In your original email, you showed:

interrupt                            scsi_eh

ahci_error_intr
  =>ata_port_freeze
    =>__ata_port_freeze
      =>ahci_freeze (turn IRQ off)
    =>ata_port_abort
      =>ata_port_schedule_eh
        =>shost->host_eh_scheduled++;
        host_eh_scheduled = 1
                                     scsi_error_handler
                                       =>ata_scsi_error
                                         =>ata_scsi_port_error_handler
                                           =>ahci_error_handler
                                           . =>sata_pmp_error_handler
                                           .   =>ata_eh_thaw_port
                                           .     =>ahci_thaw (turn IRQ on)
ahci_error_intr                            .
  =>ata_port_freeze                        .
    =>__ata_port_freeze                    .
      =>ahci_freeze (turn IRQ off)         .
    =>ata_port_abort                       .
      =>ata_port_schedule_eh               .
        =>shost->host_eh_scheduled++;      .
        host_eh_scheduled = 2              .

But here, I do not understand how host_eh_scheduled can be incremented since the
shost state should still be SHOST_RECOVERY until scsi_restart_operations() is
called at the end of scsi_error_handler(), which is after ata_std_end_eh() is
executed toward the end of ata_scsi_port_error_handler().
Not sure how what you are showing here can happen. Can you have a closer look ?

                                           =>ata_std_end_eh
                                             =>host->host_eh_scheduled = 0;

In any case, for you particular failure pattern, given that the disk "goes away"
while EH is running, I would expect the commands executed during EH (e.g. read
log 10h) to timeout, which would cause a reset and a revalidate after that. The
reset should clear the port interrupt error bits, which should allow everything
to recover after aborting all commands caught by the first EH run.

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH] scsi: ata: Fix a race condition between scsi error handler and ahci interrupt

[Li Nan]

在 2023/8/15 10:41, Damien Le Moal 写道:
> On 8/14/23 22:20, Li Nan wrote:
>>
>> 在 2023/8/14 15:50, Damien Le Moal 写道:
>>> On 8/14/23 15:41, Li Nan wrote:
>>>>> This is definitely not correct because EH may have been scheduled for a non
>>>>> fatal action like a device revalidate or to get sense data for successful
>>>>> commands. With this change, the port will NOT be frozen when a hard error IRQ
>>>>> comes while EH is waiting to start, that is, while EH waits for all commands to
>>>>> complete first.
>>>>>
>>>>
>>>> Yeah, we should find a better way to fix it. Do you have any suggesstions?
>>>>
>>>>> Furthermore, if you get an IRQ that requires the port to be frozen, it means
>>>>> that you had a failed command. In that case, the drive is in error state per
>>>>> ATA specs and stops all communication until a read log 10h command is issued.
>>>>> So you should never ever see 2 error IRQs one after the other. If you do, it
>>>>> very likely means that you have buggy hardware.
>>>>>
>>>>> How do you get into this situation ? What adapter and disk are you using ?
>>>>>
>>>>
>>>>    > How do you get into this situation ?
>>>> The first IRQ is io error, the second IRQ is disk link flash break.
>>>
>>> What does "link flash break" mean ?
>>>
>>>>
>>>>    > What adapter and disk are you using ?
>>>> It is a disk developed by our company, but we think the same issue
>>>> exists when using other disks.
>>>
>>> As I said, I find this situation highly suspect because if the first IRQ was to
>>> signal an IO error that the drive reported, then per ATA specifications, the
>>> drive should be in error mode and should NOT have transmitted any other FIS
>>> after the SDB FIS that signaled the error. Nothing at all should come after that
>>> error SDB FIS, until the host issues a read log 10h to get thee drive out of
>>> error state.
>>>
>>> If this is a prototype device, I would recommend that you take an ATA bus trace
>>> and verify the FIS traffic. Something fishy is going on with the drive in my
>>> opinion.
>>>
>>
>> Thank you for your patient explanation. I'm sorry I didn't explain the
>> problem clearly before. After discussing with my colleagues who know
>> more about dirvers, Let me re-describe the problem.
>>
>> The problem`s situation is the SATA link is quickly disconnected and
>> connected. For example, when an I/O error is processed in error handling
>> thread, the disk is manually removed and inserted, and the AHCI chip
>> reports a hot plug interrupt.
>>
>> This scenario is not just an NCQ error, but a disk is removed and
>> quickly inserted before the error processing is completed. For the error
>> handling process, the disk status needs to be restored after the error
>> handling is complete.
> 
> In your original email, you showed:
> 
> interrupt                            scsi_eh
> 
> ahci_error_intr
>    =>ata_port_freeze
>      =>__ata_port_freeze
>        =>ahci_freeze (turn IRQ off)
>      =>ata_port_abort
>        =>ata_port_schedule_eh
>          =>shost->host_eh_scheduled++;
>          host_eh_scheduled = 1
>                                       scsi_error_handler
>                                         =>ata_scsi_error
>                                           =>ata_scsi_port_error_handler
>                                             =>ahci_error_handler
>                                             . =>sata_pmp_error_handler
>                                             .   =>ata_eh_thaw_port
>                                             .     =>ahci_thaw (turn IRQ on)
> ahci_error_intr                            .
>    =>ata_port_freeze                        .
>      =>__ata_port_freeze                    .
>        =>ahci_freeze (turn IRQ off)         .
>      =>ata_port_abort                       .
>        =>ata_port_schedule_eh               .
>          =>shost->host_eh_scheduled++;      .
>          host_eh_scheduled = 2              .
> 
> But here, I do not understand how host_eh_scheduled can be incremented since the
> shost state should still be SHOST_RECOVERY until scsi_restart_operations() is
> called at the end of scsi_error_handler(), which is after ata_std_end_eh() is
> executed toward the end of ata_scsi_port_error_handler().

Yeah, shost state is still SHOST_RECOVERY during this period. But I 
don't think this will affect host_eh_scheduled++.
In scsi_host_set_state(), if (state == old state), it will return 0. So:
   ata_port_schedule_eh
    ata_std_sched_eh
     scsi_schedule_eh
      if (scsi_host_set_state(shost, SHOST_RECOVERY) == 0)  -> true
       shost->host_eh_scheduled++

> Not sure how what you are showing here can happen. Can you have a closer look ?
> 
>                                             =>ata_std_end_eh
>                                               =>host->host_eh_scheduled = 0;
> 
> In any case, for you particular failure pattern, given that the disk "goes away"
> while EH is running, I would expect the commands executed during EH (e.g. read
> log 10h) to timeout, which would cause a reset and a revalidate after that. The


If we remove the disk when EH is abount to end (after 'ahci_thaw'), the
commands which needed to be executed on the disk has already been
completed, and EH will not time out.

> reset should clear the port interrupt error bits, which should allow everything
> to recover after aborting all commands caught by the first EH run.
> 

-- 
Thanks,
Nan

Re: [PATCH] scsi: ata: Fix a race condition between scsi error handler and ahci interrupt

[Niklas Cassel]

On Thu, Aug 10, 2023 at 09:48:48AM +0800, linan666@huaweicloud.com wrote:
> From: Li Nan <linan122@huawei.com>
> 
> interrupt                            scsi_eh
> 
> ahci_error_intr
>   =>ata_port_freeze
>     =>__ata_port_freeze
>       =>ahci_freeze (turn IRQ off)
>     =>ata_port_abort
>       =>ata_port_schedule_eh
>         =>shost->host_eh_scheduled++;
>         host_eh_scheduled = 1
>                                      scsi_error_handler
>                                        =>ata_scsi_error
>                                          =>ata_scsi_port_error_handler
>                                            =>ahci_error_handler
>                                            . =>sata_pmp_error_handler
>                                            .   =>ata_eh_thaw_port
>                                            .     =>ahci_thaw (turn IRQ on)
> ahci_error_intr                            .
>   =>ata_port_freeze                        .
>     =>__ata_port_freeze                    .
>       =>ahci_freeze (turn IRQ off)         .
>     =>ata_port_abort                       .
>       =>ata_port_schedule_eh               .
>         =>shost->host_eh_scheduled++;      .
>         host_eh_scheduled = 2              .
>                                            =>ata_std_end_eh
>                                              =>host->host_eh_scheduled = 0;

Hello Li Nan,

I do not understand why the code in:
https://github.com/torvalds/linux/blob/v6.5-rc7/drivers/ata/libata-eh.c#L722-L731

does not kick in, and repeats EH.


EH_PENDING is cleared before ->error_handler() is called:
https://github.com/torvalds/linux/blob/v6.5-rc7/drivers/ata/libata-eh.c#L697

So ahci_error_intr() from the second error interrupt, which is called after
thawing the port, should have called ata_std_sched_eh(), which calls
ata_eh_set_pending(), which should have set EH_PENDING:
https://github.com/torvalds/linux/blob/v6.5-rc7/drivers/ata/libata-eh.c#L884



My only guess is that after thawing the port:
https://github.com/torvalds/linux/blob/v6.5-rc7/drivers/ata/libata-eh.c#L2807

The second error irq comes, and sets EH_PENDING,
but then this silly code might clear it:
https://github.com/torvalds/linux/blob/v6.5-rc7/drivers/ata/libata-eh.c#L2825-L2837

I think the best way would be if we could improve this "spurious error
condition check"... because if this is indeed the code that clears EH_PENDING
for you, then this code basically makes the "goto repeat" code in
ata_scsi_port_error_handler() useless...



An alternative to improving the "spurious error condition check" might be for
you to try something like:

diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
index 35e03679b0bf..82f032934ae1 100644
--- a/drivers/ata/libata-eh.c
+++ b/drivers/ata/libata-eh.c
@@ -962,7 +962,7 @@ void ata_std_end_eh(struct ata_port *ap)
 {
        struct Scsi_Host *host = ap->scsi_host;
 
-       host->host_eh_scheduled = 0;
+       host->host_eh_scheduled--;
 }
 EXPORT_SYMBOL(ata_std_end_eh);



...and see if that improves things for you.



Kind regards,
Niklas

Re: [PATCH] scsi: ata: Fix a race condition between scsi error handler and ahci interrupt

[Li Nan]

Thanks for your reply, Niklas.

在 2023/8/21 21:51, Niklas Cassel 写道:
> On Thu, Aug 10, 2023 at 09:48:48AM +0800, linan666@huaweicloud.com wrote:

[snip]

> 
> Hello Li Nan,
> 
> I do not understand why the code in:
> https://github.com/torvalds/linux/blob/v6.5-rc7/drivers/ata/libata-eh.c#L722-L731
> 
> does not kick in, and repeats EH.
> 
> 
> EH_PENDING is cleared before ->error_handler() is called:
> https://github.com/torvalds/linux/blob/v6.5-rc7/drivers/ata/libata-eh.c#L697
> 
> So ahci_error_intr() from the second error interrupt, which is called after
> thawing the port, should have called ata_std_sched_eh(), which calls
> ata_eh_set_pending(), which should have set EH_PENDING:
> https://github.com/torvalds/linux/blob/v6.5-rc7/drivers/ata/libata-eh.c#L884
> 
> 
> 
> My only guess is that after thawing the port:
> https://github.com/torvalds/linux/blob/v6.5-rc7/drivers/ata/libata-eh.c#L2807
> 
> The second error irq comes, and sets EH_PENDING,
> but then this silly code might clear it:
> https://github.com/torvalds/linux/blob/v6.5-rc7/drivers/ata/libata-eh.c#L2825-L2837
> 

Yeah, I think so.

> I think the best way would be if we could improve this "spurious error
> condition check"... because if this is indeed the code that clears EH_PENDING
> for you, then this code basically makes the "goto repeat" code in
> ata_scsi_port_error_handler() useless...
> 
> 
> An alternative to improving the "spurious error condition check" might be for
> you to try something like:
> 

We have used this solution before, but it will case WARN_ON in
ata_eh_finish() as below:

   WARNING: CPU: 1 PID: 118 at ../drivers/ata/libata-eh.c:4016 
ata_eh_finish+0x15a/0x170

   ata_do_eh+0x70/0xf0
   ata_sff_error_handler+0xde/0x170
   ata_bmdma_error_handler+0x5b/0x1e0
   ata_scsi_port_error_handler+0x5b1/0xae0
   ata_scsi_error+0xb9/0x110
   scsi_error_handler+0x137/0x8d0

> diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
> index 35e03679b0bf..82f032934ae1 100644
> --- a/drivers/ata/libata-eh.c
> +++ b/drivers/ata/libata-eh.c
> @@ -962,7 +962,7 @@ void ata_std_end_eh(struct ata_port *ap)
>   {
>          struct Scsi_Host *host = ap->scsi_host;
>   
> -       host->host_eh_scheduled = 0;
> +       host->host_eh_scheduled--;
>   }
>   EXPORT_SYMBOL(ata_std_end_eh);
> 
> 
> 
> ...and see if that improves things for you.
> 
> 
> 
> Kind regards,
> Niklas.

-- 
Thanks,
Nan

Re: [PATCH] scsi: ata: Fix a race condition between scsi error handler and ahci interrupt

[Niklas Cassel]

On Tue, Aug 22, 2023 at 05:20:33PM +0800, Li Nan wrote:
> Thanks for your reply, Niklas.
> 
> 在 2023/8/21 21:51, Niklas Cassel 写道:
> > On Thu, Aug 10, 2023 at 09:48:48AM +0800, linan666@huaweicloud.com wrote:
> 
> [snip]
> 
> > 
> > Hello Li Nan,
> > 
> > I do not understand why the code in:
> > https://github.com/torvalds/linux/blob/v6.5-rc7/drivers/ata/libata-eh.c#L722-L731
> > 
> > does not kick in, and repeats EH.
> > 
> > 
> > EH_PENDING is cleared before ->error_handler() is called:
> > https://github.com/torvalds/linux/blob/v6.5-rc7/drivers/ata/libata-eh.c#L697
> > 
> > So ahci_error_intr() from the second error interrupt, which is called after
> > thawing the port, should have called ata_std_sched_eh(), which calls
> > ata_eh_set_pending(), which should have set EH_PENDING:
> > https://github.com/torvalds/linux/blob/v6.5-rc7/drivers/ata/libata-eh.c#L884
> > 
> > 
> > 
> > My only guess is that after thawing the port:
> > https://github.com/torvalds/linux/blob/v6.5-rc7/drivers/ata/libata-eh.c#L2807
> > 
> > The second error irq comes, and sets EH_PENDING,
> > but then this silly code might clear it:
> > https://github.com/torvalds/linux/blob/v6.5-rc7/drivers/ata/libata-eh.c#L2825-L2837
> > 
> 
> Yeah, I think so.
> 
> > I think the best way would be if we could improve this "spurious error
> > condition check"... because if this is indeed the code that clears EH_PENDING
> > for you, then this code basically makes the "goto repeat" code in
> > ata_scsi_port_error_handler() useless...
> > 
> > 
> > An alternative to improving the "spurious error condition check" might be for
> > you to try something like:
> > 
> 
> We have used this solution before, but it will case WARN_ON in
> ata_eh_finish() as below:
> 
>   WARNING: CPU: 1 PID: 118 at ../drivers/ata/libata-eh.c:4016
> ata_eh_finish+0x15a/0x170

Ok.

How about if you simply move the WARN_ON to ata_scsi_port_error_handler()
as well:

diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
index 35e03679b0bf..5be2fc651131 100644
--- a/drivers/ata/libata-eh.c
+++ b/drivers/ata/libata-eh.c
@@ -741,6 +741,12 @@ void ata_scsi_port_error_handler(struct Scsi_Host *host, struct ata_port *ap)
                 */
                ap->ops->end_eh(ap);
 
+               if (!ap->scsi_host->host_eh_scheduled) {
+                       /* make sure nr_active_links is zero after EH */
+                       WARN_ON(ap->nr_active_links);
+                       ap->nr_active_links = 0;
+               }
+
                spin_unlock_irqrestore(ap->lock, flags);
                ata_eh_release(ap);
        } else {
@@ -962,7 +968,7 @@ void ata_std_end_eh(struct ata_port *ap)
 {
        struct Scsi_Host *host = ap->scsi_host;
 
-       host->host_eh_scheduled = 0;
+       host->host_eh_scheduled--;
 }
 EXPORT_SYMBOL(ata_std_end_eh);
 
@@ -3948,10 +3954,6 @@ void ata_eh_finish(struct ata_port *ap)
                        }
                }
        }
-
-       /* make sure nr_active_links is zero after EH */
-       WARN_ON(ap->nr_active_links);
-       ap->nr_active_links = 0;
 }
 
 /**



Kind regards,
Niklas

Re: [PATCH] scsi: ata: Fix a race condition between scsi error handler and ahci interrupt

[Li Nan]

在 2023/8/22 18:30, Niklas Cassel 写道:
> On Tue, Aug 22, 2023 at 05:20:33PM +0800, Li Nan wrote:
>> Thanks for your reply, Niklas.
>>
>> 在 2023/8/21 21:51, Niklas Cassel 写道:
>>> On Thu, Aug 10, 2023 at 09:48:48AM +0800, linan666@huaweicloud.com wrote:
>>
>> [snip]
>>
>>>
>>> Hello Li Nan,
>>>
>>> I do not understand why the code in:
>>> https://github.com/torvalds/linux/blob/v6.5-rc7/drivers/ata/libata-eh.c#L722-L731
>>>
>>> does not kick in, and repeats EH.
>>>
>>>
>>> EH_PENDING is cleared before ->error_handler() is called:
>>> https://github.com/torvalds/linux/blob/v6.5-rc7/drivers/ata/libata-eh.c#L697
>>>
>>> So ahci_error_intr() from the second error interrupt, which is called after
>>> thawing the port, should have called ata_std_sched_eh(), which calls
>>> ata_eh_set_pending(), which should have set EH_PENDING:
>>> https://github.com/torvalds/linux/blob/v6.5-rc7/drivers/ata/libata-eh.c#L884
>>>
>>>
>>>
>>> My only guess is that after thawing the port:
>>> https://github.com/torvalds/linux/blob/v6.5-rc7/drivers/ata/libata-eh.c#L2807
>>>
>>> The second error irq comes, and sets EH_PENDING,
>>> but then this silly code might clear it:
>>> https://github.com/torvalds/linux/blob/v6.5-rc7/drivers/ata/libata-eh.c#L2825-L2837
>>>
>>
>> Yeah, I think so.
>>
>>> I think the best way would be if we could improve this "spurious error
>>> condition check"... because if this is indeed the code that clears EH_PENDING
>>> for you, then this code basically makes the "goto repeat" code in
>>> ata_scsi_port_error_handler() useless...
>>>
>>>
>>> An alternative to improving the "spurious error condition check" might be for
>>> you to try something like:
>>>
>>
>> We have used this solution before, but it will case WARN_ON in
>> ata_eh_finish() as below:
>>
>>    WARNING: CPU: 1 PID: 118 at ../drivers/ata/libata-eh.c:4016
>> ata_eh_finish+0x15a/0x170
> 
> Ok.
> 
> How about if you simply move the WARN_ON to ata_scsi_port_error_handler()
> as well:
> 
> diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
> index 35e03679b0bf..5be2fc651131 100644
> --- a/drivers/ata/libata-eh.c
> +++ b/drivers/ata/libata-eh.c
> @@ -741,6 +741,12 @@ void ata_scsi_port_error_handler(struct Scsi_Host *host, struct ata_port *ap)
>                   */
>                  ap->ops->end_eh(ap);
>   
> +               if (!ap->scsi_host->host_eh_scheduled) {
> +                       /* make sure nr_active_links is zero after EH */
> +                       WARN_ON(ap->nr_active_links);
> +                       ap->nr_active_links = 0;
> +               }
> +
>                  spin_unlock_irqrestore(ap->lock, flags);
>                  ata_eh_release(ap);
>          } else {
> @@ -962,7 +968,7 @@ void ata_std_end_eh(struct ata_port *ap)
>   {
>          struct Scsi_Host *host = ap->scsi_host;
>   
> -       host->host_eh_scheduled = 0;
> +       host->host_eh_scheduled--;
>   }
>   EXPORT_SYMBOL(ata_std_end_eh);
>   
> @@ -3948,10 +3954,6 @@ void ata_eh_finish(struct ata_port *ap)
>                          }
>                  }
>          }
> -
> -       /* make sure nr_active_links is zero after EH */
> -       WARN_ON(ap->nr_active_links);
> -       ap->nr_active_links = 0;
>   }
>   
>   /**
> 
> 
> 
> Kind regards,
> Niklas

We have tested this patch and it can fix the bug. Thank you so much. :)

Feel free to add:

Tested-by: Li Nan <linan122@huawei.com>

-- 
Thanks,
Nan

Re: [PATCH] scsi: ata: Fix a race condition between scsi error handler and ahci interrupt

[Niklas Cassel]

On Mon, Sep 04, 2023 at 07:45:51PM +0800, Li Nan wrote:
> 
> 
> 在 2023/8/22 18:30, Niklas Cassel 写道:
> > On Tue, Aug 22, 2023 at 05:20:33PM +0800, Li Nan wrote:
> > > Thanks for your reply, Niklas.
> > > 
> > > 在 2023/8/21 21:51, Niklas Cassel 写道:
> > > > On Thu, Aug 10, 2023 at 09:48:48AM +0800, linan666@huaweicloud.com wrote:
> > > 
> > > [snip]
> > > 
> > > > 
> > > > Hello Li Nan,
> > > > 
> > > > I do not understand why the code in:
> > > > https://github.com/torvalds/linux/blob/v6.5-rc7/drivers/ata/libata-eh.c#L722-L731
> > > > 
> > > > does not kick in, and repeats EH.
> > > > 
> > > > 
> > > > EH_PENDING is cleared before ->error_handler() is called:
> > > > https://github.com/torvalds/linux/blob/v6.5-rc7/drivers/ata/libata-eh.c#L697
> > > > 
> > > > So ahci_error_intr() from the second error interrupt, which is called after
> > > > thawing the port, should have called ata_std_sched_eh(), which calls
> > > > ata_eh_set_pending(), which should have set EH_PENDING:
> > > > https://github.com/torvalds/linux/blob/v6.5-rc7/drivers/ata/libata-eh.c#L884
> > > > 
> > > > 
> > > > 
> > > > My only guess is that after thawing the port:
> > > > https://github.com/torvalds/linux/blob/v6.5-rc7/drivers/ata/libata-eh.c#L2807
> > > > 
> > > > The second error irq comes, and sets EH_PENDING,
> > > > but then this silly code might clear it:
> > > > https://github.com/torvalds/linux/blob/v6.5-rc7/drivers/ata/libata-eh.c#L2825-L2837
> > > > 
> > > 
> > > Yeah, I think so.
> > > 
> > > > I think the best way would be if we could improve this "spurious error
> > > > condition check"... because if this is indeed the code that clears EH_PENDING
> > > > for you, then this code basically makes the "goto repeat" code in
> > > > ata_scsi_port_error_handler() useless...
> > > > 
> > > > 
> > > > An alternative to improving the "spurious error condition check" might be for
> > > > you to try something like:
> > > > 
> > > 
> > > We have used this solution before, but it will case WARN_ON in
> > > ata_eh_finish() as below:
> > > 
> > >    WARNING: CPU: 1 PID: 118 at ../drivers/ata/libata-eh.c:4016
> > > ata_eh_finish+0x15a/0x170
> > 
> > Ok.
> > 
> > How about if you simply move the WARN_ON to ata_scsi_port_error_handler()
> > as well:
> > 
> > diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
> > index 35e03679b0bf..5be2fc651131 100644
> > --- a/drivers/ata/libata-eh.c
> > +++ b/drivers/ata/libata-eh.c
> > @@ -741,6 +741,12 @@ void ata_scsi_port_error_handler(struct Scsi_Host *host, struct ata_port *ap)
> >                   */
> >                  ap->ops->end_eh(ap);
> > +               if (!ap->scsi_host->host_eh_scheduled) {
> > +                       /* make sure nr_active_links is zero after EH */
> > +                       WARN_ON(ap->nr_active_links);
> > +                       ap->nr_active_links = 0;
> > +               }
> > +
> >                  spin_unlock_irqrestore(ap->lock, flags);
> >                  ata_eh_release(ap);
> >          } else {
> > @@ -962,7 +968,7 @@ void ata_std_end_eh(struct ata_port *ap)
> >   {
> >          struct Scsi_Host *host = ap->scsi_host;
> > -       host->host_eh_scheduled = 0;
> > +       host->host_eh_scheduled--;
> >   }
> >   EXPORT_SYMBOL(ata_std_end_eh);
> > @@ -3948,10 +3954,6 @@ void ata_eh_finish(struct ata_port *ap)
> >                          }
> >                  }
> >          }
> > -
> > -       /* make sure nr_active_links is zero after EH */
> > -       WARN_ON(ap->nr_active_links);
> > -       ap->nr_active_links = 0;
> >   }
> >   /**
> > 
> > 
> > 
> > Kind regards,
> > Niklas
> 
> We have tested this patch and it can fix the bug. Thank you so much. :)

Awesome! :)

Please send out a real patch, so that it is easier for the maintainer to
apply.

No need to give any credit to me.


Kind regards,
Niklas

Re: [PATCH] scsi: ata: Fix a race condition between scsi error handler and ahci interrupt

[Li Nan]

在 2023/9/4 19:57, Niklas Cassel 写道:
> On Mon, Sep 04, 2023 at 07:45:51PM +0800, Li Nan wrote:
>>
>>
>> 在 2023/8/22 18:30, Niklas Cassel 写道:
>>> On Tue, Aug 22, 2023 at 05:20:33PM +0800, Li Nan wrote:
>>>> Thanks for your reply, Niklas.
>>>>
>>>> 在 2023/8/21 21:51, Niklas Cassel 写道:
>>>>> On Thu, Aug 10, 2023 at 09:48:48AM +0800, linan666@huaweicloud.com wrote:
>>>>
>>>> [snip]
>>>>
>>>>>
>>>>> Hello Li Nan,
>>>>>
>>>>> I do not understand why the code in:
>>>>> https://github.com/torvalds/linux/blob/v6.5-rc7/drivers/ata/libata-eh.c#L722-L731
>>>>>
>>>>> does not kick in, and repeats EH.
>>>>>
>>>>>
>>>>> EH_PENDING is cleared before ->error_handler() is called:
>>>>> https://github.com/torvalds/linux/blob/v6.5-rc7/drivers/ata/libata-eh.c#L697
>>>>>
>>>>> So ahci_error_intr() from the second error interrupt, which is called after
>>>>> thawing the port, should have called ata_std_sched_eh(), which calls
>>>>> ata_eh_set_pending(), which should have set EH_PENDING:
>>>>> https://github.com/torvalds/linux/blob/v6.5-rc7/drivers/ata/libata-eh.c#L884
>>>>>
>>>>>
>>>>>
>>>>> My only guess is that after thawing the port:
>>>>> https://github.com/torvalds/linux/blob/v6.5-rc7/drivers/ata/libata-eh.c#L2807
>>>>>
>>>>> The second error irq comes, and sets EH_PENDING,
>>>>> but then this silly code might clear it:
>>>>> https://github.com/torvalds/linux/blob/v6.5-rc7/drivers/ata/libata-eh.c#L2825-L2837
>>>>>
>>>>
>>>> Yeah, I think so.
>>>>
>>>>> I think the best way would be if we could improve this "spurious error
>>>>> condition check"... because if this is indeed the code that clears EH_PENDING
>>>>> for you, then this code basically makes the "goto repeat" code in
>>>>> ata_scsi_port_error_handler() useless...
>>>>>
>>>>>
>>>>> An alternative to improving the "spurious error condition check" might be for
>>>>> you to try something like:
>>>>>
>>>>
>>>> We have used this solution before, but it will case WARN_ON in
>>>> ata_eh_finish() as below:
>>>>
>>>>     WARNING: CPU: 1 PID: 118 at ../drivers/ata/libata-eh.c:4016
>>>> ata_eh_finish+0x15a/0x170
>>>
>>> Ok.
>>>
>>> How about if you simply move the WARN_ON to ata_scsi_port_error_handler()
>>> as well:
>>>
>>> diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
>>> index 35e03679b0bf..5be2fc651131 100644
>>> --- a/drivers/ata/libata-eh.c
>>> +++ b/drivers/ata/libata-eh.c
>>> @@ -741,6 +741,12 @@ void ata_scsi_port_error_handler(struct Scsi_Host *host, struct ata_port *ap)
>>>                    */
>>>                   ap->ops->end_eh(ap);
>>> +               if (!ap->scsi_host->host_eh_scheduled) {
>>> +                       /* make sure nr_active_links is zero after EH */
>>> +                       WARN_ON(ap->nr_active_links);
>>> +                       ap->nr_active_links = 0;
>>> +               }
>>> +
>>>                   spin_unlock_irqrestore(ap->lock, flags);
>>>                   ata_eh_release(ap);
>>>           } else {
>>> @@ -962,7 +968,7 @@ void ata_std_end_eh(struct ata_port *ap)
>>>    {
>>>           struct Scsi_Host *host = ap->scsi_host;
>>> -       host->host_eh_scheduled = 0;
>>> +       host->host_eh_scheduled--;
>>>    }
>>>    EXPORT_SYMBOL(ata_std_end_eh);
>>> @@ -3948,10 +3954,6 @@ void ata_eh_finish(struct ata_port *ap)
>>>                           }
>>>                   }
>>>           }
>>> -
>>> -       /* make sure nr_active_links is zero after EH */
>>> -       WARN_ON(ap->nr_active_links);
>>> -       ap->nr_active_links = 0;
>>>    }
>>>    /**
>>>
>>>
>>>
>>> Kind regards,
>>> Niklas
>>
>> We have tested this patch and it can fix the bug. Thank you so much. :)
> 
> Awesome! :)
> 
> Please send out a real patch, so that it is easier for the maintainer to
> apply.
> 
> No need to give any credit to me.
> 
> 
> Kind regards,
> Niklas

It is my pleasure. I will send v2 later.

-- 
Thanks,
Nan