Re: [PATCH v2] ata:libata-eh:Cleanup ata_scsi_cmd_error_handler

[Wenchao Hao <haowenchao@huawei.com>]

On 2022/12/15 19:34, Damien Le Moal wrote:
> 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
> 

Thanks a lot for review, I would update them.