Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <5d37add3-41ce-e2af-b45a-d701eaf36a6c@huaweicloud.com>
Date:   Mon, 4 Sep 2023 19:45:51 +0800
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101
 Thunderbird/91.10.0
Subject: Re: [PATCH] scsi: ata: Fix a race condition between scsi error
 handler and ahci interrupt
To:     Niklas Cassel <Niklas.Cassel@wdc.com>
Cc:     "dlemoal@kernel.org" <dlemoal@kernel.org>,
        "linux-ide@vger.kernel.org" <linux-ide@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "yukuai3@huawei.com" <yukuai3@huawei.com>,
        "yi.zhang@huawei.com" <yi.zhang@huawei.com>,
        "houtao1@huawei.com" <houtao1@huawei.com>,
        "yangerkun@huawei.com" <yangerkun@huawei.com>
References: <20230810014848.2148316-1-linan666@huaweicloud.com>
 <ZONr0f26IT/QKsSu@x1-carbon>
 <f33816ef-ce26-1501-99b6-c75f91c6d8f5@huaweicloud.com>
 <ZOSONbLwc6RtQnS0@x1-carbon>
From:   Li Nan <linan666@huaweicloud.com>
In-Reply-To: <ZOSONbLwc6RtQnS0@x1-carbon>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk


在 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