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: GsGU8td6rsDgQNg+lUwNxj3Tg9eURHJc4+MUhsh08q1Ze7KKFobe9O6+m8NlDNThDUUxtoKouG
 POVpZ4Be3kHGxdyiHReetQt5JgF2zXsYe2uyfa+qX/VhXjuoKxPf6ELUL6/2SaAUMiF8fr7N7W
 4bVFhF/Yszc4RPpHXxGQ01FoGd5lHj4OSTFUk6zkugQ7KA8MpJJxVZCHtCvr3r9sv/PAMIdpEF
 ShcxmjYCV2iQPwejpI3UoA2kbzquQlUyIutWa97Ulc352OSXeYA1cUbb5tkCUuzizx6yF+U4mf
 C6k=
IronPort-SDR: ZCNjqxJSGblhtf9bqvVuPah0tDWUAg13IF7iIc1k4twCVl4Tq1GCZS5qTzlA+vRmUKbqLKAIeB
 cGAD32FX6gUG8CmFfwTlat8xIgF9XCJWvsZ4ANzppIqZX2aqt72BN+uRKt5oC8ap1mJQxMi+PH
 wPrJa1/bsYPK2XmSN5/ftZh6XaS0Lw3SRQpqeHd0Fuf9HxlPMfZ8zNqB2G2+r8GpGhfpwsQPet
 TR0wVke1lyNKjKqL8BOuSab0VERMrzCzq0u06pa0FyCdHIbn6yYPoYJLAMlf4zdh9cnJb2Pbwf
 qHM=
WDCIronportException: Internal
Message-ID: <d991d4fd-3b5f-f0ef-5c96-2352a08e1262@opensource.wdc.com>
Date:   Thu, 15 Dec 2022 20:34:09 +0900
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.5.1
Subject: Re: [PATCH v2] ata:libata-eh:Cleanup ata_scsi_cmd_error_handler
Content-Language: en-US
To:     Sergei Shtylyov <sergei.shtylyov@gmail.com>,
        Wenchao Hao <haowenchao@huawei.com>, linux-ide@vger.kernel.org,
        linux-kernel@vger.kernel.org
Cc:     liuzhiqiang26@huawei.com, linfeilong@huawei.com
References: <20221215105747.1927412-1-haowenchao@huawei.com>
 <f552f2ab-895a-14dd-d391-a7b1c9930c88@gmail.com>
From:   Damien Le Moal <damien.lemoal@opensource.wdc.com>
Organization: Western Digital Research
In-Reply-To: <f552f2ab-895a-14dd-d391-a7b1c9930c88@gmail.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 12/15/22 20:30, Sergei Shtylyov wrote:
> Hello!
> 
>   The subject should really look like "ata: libata-eh: Cleanup
> ata_scsi_cmd_error_handler()"...
> 
> On 12/15/22 1:57 PM, Wenchao Hao wrote:
> 
>> If ap->ops->error_handler is NULL, just return.
>>
>> V2:
>> - Check ap->ops->error_handler without taking the spin lock
> 
>    This normally belongs under ---...

I should read all emails before replying. I commented exactly the same :)

> 
>> Signed-off-by: Wenchao Hao <haowenchao@huawei.com>
>> ---
>>  drivers/ata/libata-eh.c | 96 ++++++++++++++++++++---------------------
>>  1 file changed, 48 insertions(+), 48 deletions(-)
>>
>> diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
>> index 34303ce67c14..c445c272e77f 100644
>> --- a/drivers/ata/libata-eh.c
>> +++ b/drivers/ata/libata-eh.c
> [...]
>> @@ -584,62 +589,57 @@ void ata_scsi_cmd_error_handler(struct Scsi_Host *host, struct ata_port *ap,
>>  	 * timed out iff its associated qc is active and not failed.
>>  	 */
>>  	spin_lock_irqsave(ap->lock, flags);
>> -	if (ap->ops->error_handler) {
>> -		struct scsi_cmnd *scmd, *tmp;
>> -		int nr_timedout = 0;
>> -
>> -		/* This must occur under the ap->lock as we don't want
>> -		   a polled recovery to race the real interrupt handler
>> -
>> -		   The lost_interrupt handler checks for any completed but
>> -		   non-notified command and completes much like an IRQ handler.
>>  
>> -		   We then fall into the error recovery code which will treat
>> -		   this as if normal completion won the race */
>> -
>> -		if (ap->ops->lost_interrupt)
>> -			ap->ops->lost_interrupt(ap);
>> +	/* This must occur under the ap->lock as we don't want
> 
>    The multi-line comments should start with /* on its own line...
> 
>> +	 * a polled recovery to race the real interrupt handler
>> +	 *
>> +	 * The lost_interrupt handler checks for any completed but
>> +	 * non-notified command and completes much like an IRQ handler.
>> +	 *
>> +	 * We then fall into the error recovery code which will treat
>> +	 * this as if normal completion won the race
>> +	 */
>> +	if (ap->ops->lost_interrupt)
>> +		ap->ops->lost_interrupt(ap);
>>  
>> -		list_for_each_entry_safe(scmd, tmp, eh_work_q, eh_entry) {
>> -			struct ata_queued_cmd *qc;
>> +	list_for_each_entry_safe(scmd, tmp, eh_work_q, eh_entry) {
>> +		struct ata_queued_cmd *qc;
>>  
>> -			ata_qc_for_each_raw(ap, qc, i) {
>> -				if (qc->flags & ATA_QCFLAG_ACTIVE &&
>> -				    qc->scsicmd == scmd)
>> -					break;
>> -			}
>> +		ata_qc_for_each_raw(ap, qc, i) {
>> +			if (qc->flags & ATA_QCFLAG_ACTIVE &&
>> +			    qc->scsicmd == scmd)
>> +				break;
>> +		}
>>  
>> -			if (i < ATA_MAX_QUEUE) {
>> -				/* the scmd has an associated qc */
>> -				if (!(qc->flags & ATA_QCFLAG_FAILED)) {
>> -					/* which hasn't failed yet, timeout */
>> -					qc->err_mask |= AC_ERR_TIMEOUT;
>> -					qc->flags |= ATA_QCFLAG_FAILED;
>> -					nr_timedout++;
>> -				}
>> -			} else {
>> -				/* Normal completion occurred after
>> -				 * SCSI timeout but before this point.
>> -				 * Successfully complete it.
>> -				 */
>> -				scmd->retries = scmd->allowed;
>> -				scsi_eh_finish_cmd(scmd, &ap->eh_done_q);
>> +		if (i < ATA_MAX_QUEUE) {
>> +			/* the scmd has an associated qc */
>> +			if (!(qc->flags & ATA_QCFLAG_FAILED)) {
>> +				/* which hasn't failed yet, timeout */
>> +				qc->err_mask |= AC_ERR_TIMEOUT;
>> +				qc->flags |= ATA_QCFLAG_FAILED;
>> +				nr_timedout++;
>>  			}
>> +		} else {
>> +			/* Normal completion occurred after
> 
>    Here as well...
> 
>> +			 * SCSI timeout but before this point.
>> +			 * Successfully complete it.
>> +			 */
>> +			scmd->retries = scmd->allowed;
>> +			scsi_eh_finish_cmd(scmd, &ap->eh_done_q);
>>  		}
>> +	}
>>  
>> -		/* If we have timed out qcs.  They belong to EH from
>> -		 * this point but the state of the controller is
>> -		 * unknown.  Freeze the port to make sure the IRQ
>> -		 * handler doesn't diddle with those qcs.  This must
>> -		 * be done atomically w.r.t. setting QCFLAG_FAILED.
>> -		 */
>> -		if (nr_timedout)
>> -			__ata_port_freeze(ap);
>> -
>> +	/* If we have timed out qcs.  They belong to EH from
> 
>    And here...
> 
> [...]
> 
> MBR, Sergey

-- 
Damien Le Moal
Western Digital Research