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;
IronPort-SDR: 6wr8axaauB9WPyA7o9s1RWf4l6O0fgfwRpLHwMcTepSOF33lpekhjb7EwbQVqfiQqxOOqY0/9P
 SOBTE0ylkgZuJM404wIq9IP3udhlRucvUB7cz84EsuwqXUnma2yuxX+sVSS9zOVO/bcVLuJnwG
 Y/YM2/mpIIJuN6xxbF6B0Q3n416JmzDc4euNSGAsv5OE884nxNImQKypbvIQjNObNzwwSEkoB6
 HOGZlwXA/LdFrXNbMn9fSVntmpSuGblsrIlNpEyNfs1vMeaJsf2C5gUt9WLiv6Jsn7rFKAp9B8
 IiBECI+Ded6ScvRIz9BRwUNj
IronPort-SDR: PnJHSdcvilVMLVnz2CT/Bs6QjMreamRw59vLKq6M/+XnMg6Jr2SaKEYdHfbGDmntLYz0cvWLo5
 4ceZKwk4fyM/1y0cSlwsXoLv2zFNW70dz84jRWVVx/aCwf9GNuL6pBhOJlRVOwImhEt+mBneBx
 lkTwlOxYzaxKoM4s/HKIv+Hjx1hVUZ0tcoScc5fKomR7qHwSioE+MXzcGHMNAk5/A7ualQkHIb
 umrqeR6noA2jPTZvvPKtUhjRTtbRniT9EjmMlcnVZ/xQcessqOZGw7ct5yPelAHLao5FXEMhjw
 yv0=
WDCIronportException: Internal
Message-ID: <3105a674-f976-95c8-f1d9-35034d91c5d4@opensource.wdc.com>
Date:   Tue, 6 Sep 2022 08:23:54 +0900
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.2.1
Subject: Re: [PATCH v2 2/6] scsi: libsas: Add sas_ata_device_link_abort()
Content-Language: en-US
To:     John Garry <john.garry@huawei.com>, jejb@linux.ibm.com,
        martin.petersen@oracle.com, jinpu.wang@cloud.ionos.com,
        yangxingui@huawei.com
Cc:     linux-scsi@vger.kernel.org, linux-kernel@vger.kernel.org,
        linuxarm@huawei.com, hare@suse.de
References: <1660747934-60059-1-git-send-email-john.garry@huawei.com>
 <1660747934-60059-3-git-send-email-john.garry@huawei.com>
 <0bb6b134-7bad-7c39-ad6d-25d57bd343eb@opensource.wdc.com>
 <018080d5-fbc4-7f04-0d3f-587f2397cd20@huawei.com>
 <baf63982-810c-85eb-b28f-99ab0517c6ba@opensource.wdc.com>
 <eb3465a2-335e-a605-ba8a-4cce790b5b02@huawei.com>
 <4b471300-a912-c3c0-ead4-7165c57cbbf8@huawei.com>
From:   Damien Le Moal <damien.lemoal@opensource.wdc.com>
Organization: Western Digital Research
In-Reply-To: <4b471300-a912-c3c0-ead4-7165c57cbbf8@huawei.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 9/3/22 01:19, John Garry wrote:
> Hi Damien,
> 
>>>>
>>>> But the pm8001 manual and current driver indicate that the
>>>> OPC_INB_SATA_ABORT command should be sent after read log ext when
>>>> handling NCQ error, regardless of an autopsy. I send OPC_INB_SATA_ABORT
>>>> in ata_eh_reset() -> pm8001_I_T_nexus_reset() -> pm8001_send_abort_all()
>>> You lost me: ata_eh_recover() will call ata_eh_reset() only if the 
>>> ATA_EH_RESET
>>> action flag is set. So are you saying that even though it is not 
>>> needed, you
>>> still need to set ATA_EH_RESET for pm8001 ?
>>
>> As below, it was the only location I found suitable to call 
>> pm8001_send_abort_all().
>>
>> However I am not really sure it is required now. For pm8001 NCQ error 
>> handling we require 2x steps:
>> - read log ext
>> - Send OPC_INB_SATA_ABORT - we do this in pm8001_send_abort_all()
>>
>> pm8001_send_abort_all() sends OPC_INB_SATA_ABORT in "device abort all" 
>> mode, meaning any IO in the HBA is aborted for the device. But we are 
>> also earlier in EH sending OPC_INB_SATA_ABORT for individual IOs in 
>> sas_eh_handle_sas_errors() -> sas_scsi_find_task() -> 
>> pm8001_abort_task() -> sas_execute_internal_abort_single() -> ... 
>> send_abort_task()
>>
>> So I don't think that the pm8001_send_abort_all() call has any effect, 
>> as we're already aborting any outstanding IO earlier.
>>
>> Admittedly the order of the 2x steps is different, but 
>> OPC_INB_SATA_ABORT does not send any protocol message to the disk, so 
>> would not affect anything subsequently read with read log ext.
>>
>> Having said all that, it may be wise to still send 
>> pm8001_send_abort_all()..
> 
> Have you had a chance to consider all this yet?
> 
> I am now starting to think that it is not necessary to call 
> pm8001_send_abort_all(), and instead rely only on 
> sas_eh_handle_sas_errors() -> sas_scsi_find_task() -> 
> pm8001_abort_task() -> sas_execute_internal_abort_single() -> ... -> 
> send_abort_task() to abort each outstanding IO. Then we would not have 
> the issue of forcing the reset in $subject (to lead to calling 
> pm8001_send_abort_all()).
> 
> This idea could simply be tested by stubbing pm8001_send_abort_all()
> (and dropping the |= ATA_EH_RESET in $subject).

Send a re-spinned patch and I will test :)

> 
> Thanks,
> John
> 

-- 
Damien Le Moal
Western Digital Research