| Message ID | 20221215105747.1927412-1-haowenchao@huawei.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] ata:libata-eh:Cleanup ata_scsi_cmd_error_handler | expand |
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 ---... > 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
On 12/15/22 19:57, Wenchao Hao wrote: > If ap->ops->error_handler is NULL, just return. > > V2: > - Check ap->ops->error_handler without taking the spin lock Changelog needs to go after the "---" below your SoB. > > 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 > @@ -565,10 +565,15 @@ 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); > > + if (!ap->ops->error_handler) > + return; > + > /* synchronize with host lock and sort out timeouts */ > > /* For new EH, all qcs are finished in one of three ways - > @@ -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 While at it, please fix the comment style. Multi-line comments should start with a "/*" empty 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 > + * 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 Same here. Fix the comment style please. > + * 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; > spin_unlock_irqrestore(ap->lock, flags); > > }
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
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.
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 @@ -565,10 +565,15 @@ 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); + if (!ap->ops->error_handler) + return; + /* synchronize with host lock and sort out timeouts */ /* For new EH, all qcs are finished in one of three ways - @@ -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 + * 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; spin_unlock_irqrestore(ap->lock, flags); }
If ap->ops->error_handler is NULL, just return. V2: - Check ap->ops->error_handler without taking the spin lock Signed-off-by: Wenchao Hao <haowenchao@huawei.com> --- drivers/ata/libata-eh.c | 96 ++++++++++++++++++++--------------------- 1 file changed, 48 insertions(+), 48 deletions(-)