On 2022/12/15 16:54, Niklas Cassel wrote:
> On Thu, Dec 15, 2022 at 01:04:16PM +0800, Wenchao Hao wrote:
>> If ap->ops->error_handler is NULL, just go out and release the
>> spinlock. This patch is just a cleanup, which would not change
>> the origin error handle logic.
>>
>> 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..66ca3ac7cd58 100644
>> --- a/drivers/ata/libata-eh.c
>> +++ b/drivers/ata/libata-eh.c
>> @@ -565,6 +565,8 @@ void ata_scsi_cmd_error_handler(struct Scsi_Host *host, struct ata_port *ap,
>> {
>> int i;
>> unsigned long flags;
>> + struct scsi_cmnd *scmd, *tmp;
>> + int nr_timedout = 0;
>>
>> /* make sure sff pio task is not running */
>> ata_sff_flush_pio_task(ap);
>> @@ -584,62 +586,60 @@ 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->error_handler)
>> + goto out;
>
> Nice cleanup!
>
> However, I don't think there is any point in taking the spin lock if there is
> no error_handler.
>
> So I think that you can do similar to the skip_eh label in ata_port_detach():
> https://github.com/torvalds/linux/blob/master/drivers/ata/libata-core.c#L5904
>
>
> Kind regards,
> Niklas
>
I did not want to change the origin logic. But it looks unnecessary to take
the spin lock, would update.
>>
>> - if (ap->ops->lost_interrupt)
>> - ap->ops->lost_interrupt(ap);
>> + /* 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);
>>
>> - 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
>> + * 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
>> + * 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);
>>
>> - /* initialize eh_tries */
>> - ap->eh_tries = ATA_EH_MAX_TRIES;
>> - }
>> + /* initialize eh_tries */
>> + ap->eh_tries = ATA_EH_MAX_TRIES;
>> +out:
>> spin_unlock_irqrestore(ap->lock, flags);
>>
>> }
>> --
>> 2.32.0
>> .
