[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <f552f2ab-895a-14dd-d391-a7b1c9930c88@gmail.com>
Date: Thu, 15 Dec 2022 14:30:38 +0300
From: Sergei Shtylyov <sergei.shtylyov@...il.com>
To: Wenchao Hao <haowenchao@...wei.com>,
Damien Le Moal <damien.lemoal@...nsource.wdc.com>,
linux-ide@...r.kernel.org, linux-kernel@...r.kernel.org
Cc: liuzhiqiang26@...wei.com, linfeilong@...wei.com
Subject: Re: [PATCH v2] ata:libata-eh:Cleanup ata_scsi_cmd_error_handler
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@...wei.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
Powered by blists - more mailing lists