If ap->ops->error_handler is NULL just return. This patch also
fixes some comment style issue.
---
v3:
- Start with a "/*" empty line for multi-line comments.
- Correct the commit subject
v2:
- Check ap->ops->error_handler without taking the spin lock
Signed-off-by: Wenchao Hao <[email protected]>
---
drivers/ata/libata-eh.c | 101 +++++++++++++++++++++-------------------
1 file changed, 52 insertions(+), 49 deletions(-)
diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
index 34303ce67c14..56820b8e953a 100644
--- a/drivers/ata/libata-eh.c
+++ b/drivers/ata/libata-eh.c
@@ -565,13 +565,19 @@ 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 -
+ /*
+ * For new EH, all qcs are finished in one of three ways -
* normal completion, error completion, and SCSI timeout.
* Both completions can race against SCSI timeout. When normal
* completion wins, the qc never reaches EH. When error
@@ -584,62 +590,59 @@ 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);
}
--
2.32.0
On 2022/12/15 23:37, Wenchao Hao wrote:
> If ap->ops->error_handler is NULL just return. This patch also
> fixes some comment style issue.
>
> ---
> v3:
> - Start with a "/*" empty line for multi-line comments.
> - Correct the commit subject
>
> v2:
> - Check ap->ops->error_handler without taking the spin lock
>
> Signed-off-by: Wenchao Hao <[email protected]>
friendly pinging...
> ---
> drivers/ata/libata-eh.c | 101 +++++++++++++++++++++-------------------
> 1 file changed, 52 insertions(+), 49 deletions(-)
>
> diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
> index 34303ce67c14..56820b8e953a 100644
> --- a/drivers/ata/libata-eh.c
> +++ b/drivers/ata/libata-eh.c
> @@ -565,13 +565,19 @@ 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 -
> + /*
> + * For new EH, all qcs are finished in one of three ways -
> * normal completion, error completion, and SCSI timeout.
> * Both completions can race against SCSI timeout. When normal
> * completion wins, the qc never reaches EH. When error
> @@ -584,62 +590,59 @@ 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);
>
> }
On Thu, Dec 15, 2022 at 11:37:49PM +0800, Wenchao Hao wrote:
> If ap->ops->error_handler is NULL just return. This patch also
> fixes some comment style issue.
>
> ---
> v3:
> - Start with a "/*" empty line for multi-line comments.
> - Correct the commit subject
>
> v2:
> - Check ap->ops->error_handler without taking the spin lock
>
> Signed-off-by: Wenchao Hao <[email protected]>
> ---
> drivers/ata/libata-eh.c | 101 +++++++++++++++++++++-------------------
> 1 file changed, 52 insertions(+), 49 deletions(-)
>
> diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
> index 34303ce67c14..56820b8e953a 100644
> --- a/drivers/ata/libata-eh.c
> +++ b/drivers/ata/libata-eh.c
> @@ -565,13 +565,19 @@ 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 -
> + /*
> + * For new EH, all qcs are finished in one of three ways -
> * normal completion, error completion, and SCSI timeout.
> * Both completions can race against SCSI timeout. When normal
> * completion wins, the qc never reaches EH. When error
> @@ -584,62 +590,59 @@ 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;
Personally, I would prefer a newline between the spin_unlock_irqrestore()
and ap->eh_tries = ATA_EH_MAX_TRIES;
> spin_unlock_irqrestore(ap->lock, flags);
>
You could also drop this superfluous newline.
> }
> --
> 2.32.0
>
With or without the above nits fixed:
Reviewed-by: Niklas Cassel <[email protected]>
On 12/16/22 00:37, Wenchao Hao wrote:
> If ap->ops->error_handler is NULL just return. This patch also
> fixes some comment style issue.
>
Missing a Signed-off-by tag... Please resend with one.
> ---
> v3:
> - Start with a "/*" empty line for multi-line comments.
> - Correct the commit subject
>
> v2:
> - Check ap->ops->error_handler without taking the spin lock
>
> Signed-off-by: Wenchao Hao <[email protected]>
> ---
> drivers/ata/libata-eh.c | 101 +++++++++++++++++++++-------------------
> 1 file changed, 52 insertions(+), 49 deletions(-)
>
> diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
> index 34303ce67c14..56820b8e953a 100644
> --- a/drivers/ata/libata-eh.c
> +++ b/drivers/ata/libata-eh.c
> @@ -565,13 +565,19 @@ 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 -
> + /*
> + * For new EH, all qcs are finished in one of three ways -
> * normal completion, error completion, and SCSI timeout.
> * Both completions can race against SCSI timeout. When normal
> * completion wins, the qc never reaches EH. When error
> @@ -584,62 +590,59 @@ 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);
>
> }
--
Damien Le Moal
Western Digital Research
On 12/16/22 00:37, Wenchao Hao wrote:
> If ap->ops->error_handler is NULL just return. This patch also
> fixes some comment style issue.
>
> ---
> v3:
> - Start with a "/*" empty line for multi-line comments.
> - Correct the commit subject
>
> v2:
> - Check ap->ops->error_handler without taking the spin lock
>
> Signed-off-by: Wenchao Hao <[email protected]>
Ah, so your SoB is here. Wrong patch format. The changelog needs to go
right below the "---" below here and your SoB above it. You added a "---"
above, which tells git that this is the end of the commit message and so
your SoB below it is ignored. I fixed that up when applying to for-6.3,
together with Niklas suggested edits. In the future, please format your
patches correctly.
> ---
> drivers/ata/libata-eh.c | 101 +++++++++++++++++++++-------------------
> 1 file changed, 52 insertions(+), 49 deletions(-)
>
> diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
> index 34303ce67c14..56820b8e953a 100644
> --- a/drivers/ata/libata-eh.c
> +++ b/drivers/ata/libata-eh.c
> @@ -565,13 +565,19 @@ 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 -
> + /*
> + * For new EH, all qcs are finished in one of three ways -
> * normal completion, error completion, and SCSI timeout.
> * Both completions can race against SCSI timeout. When normal
> * completion wins, the qc never reaches EH. When error
> @@ -584,62 +590,59 @@ 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);
>
> }
--
Damien Le Moal
Western Digital Research
On 2023/1/4 12:29, Damien Le Moal wrote:
> On 12/16/22 00:37, Wenchao Hao wrote:
>> If ap->ops->error_handler is NULL just return. This patch also
>> fixes some comment style issue.
>>
>> ---
>> v3:
>> - Start with a "/*" empty line for multi-line comments.
>> - Correct the commit subject
>>
>> v2:
>> - Check ap->ops->error_handler without taking the spin lock
>>
>> Signed-off-by: Wenchao Hao <[email protected]>
>
> Ah, so your SoB is here. Wrong patch format. The changelog needs to go
> right below the "---" below here and your SoB above it. You added a "---"
> above, which tells git that this is the end of the commit message and so
> your SoB below it is ignored. I fixed that up when applying to for-6.3,
> together with Niklas suggested edits. In the future, please format your
> patches correctly.
OK, I would use correct format in future patches.
>
>> ---
>> drivers/ata/libata-eh.c | 101 +++++++++++++++++++++-------------------
>> 1 file changed, 52 insertions(+), 49 deletions(-)
>>
>> diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
>> index 34303ce67c14..56820b8e953a 100644
>> --- a/drivers/ata/libata-eh.c
>> +++ b/drivers/ata/libata-eh.c
>> @@ -565,13 +565,19 @@ 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 -
>> + /*
>> + * For new EH, all qcs are finished in one of three ways -
>> * normal completion, error completion, and SCSI timeout.
>> * Both completions can race against SCSI timeout. When normal
>> * completion wins, the qc never reaches EH. When error
>> @@ -584,62 +590,59 @@ 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);
>>
>> }
>