| Message ID | 20221215050416.1891113-1-haowenchao@huawei.com |
|---|---|
| State | New |
| Headers | show |
| Series | ata:libata-eh:Cleanup ata_scsi_cmd_error_handler | expand |
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@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..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 > > - 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 >
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@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..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 >> .
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; - 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); }
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@huawei.com> --- drivers/ata/libata-eh.c | 96 ++++++++++++++++++++--------------------- 1 file changed, 48 insertions(+), 48 deletions(-)