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@xxxxxxxxxx>
>>> ---
>>> 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.
