Re: [PATCH] scsi: ata: Fix a race condition between scsi error handler and ahci interrupt

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 





在 2023/9/4 19:57, Niklas Cassel 写道:
On Mon, Sep 04, 2023 at 07:45:51PM +0800, Li Nan wrote:


在 2023/8/22 18:30, Niklas Cassel 写道:
On Tue, Aug 22, 2023 at 05:20:33PM +0800, Li Nan wrote:
Thanks for your reply, Niklas.

在 2023/8/21 21:51, Niklas Cassel 写道:
On Thu, Aug 10, 2023 at 09:48:48AM +0800, linan666@xxxxxxxxxxxxxxx wrote:

[snip]


Hello Li Nan,

I do not understand why the code in:
https://github.com/torvalds/linux/blob/v6.5-rc7/drivers/ata/libata-eh.c#L722-L731

does not kick in, and repeats EH.


EH_PENDING is cleared before ->error_handler() is called:
https://github.com/torvalds/linux/blob/v6.5-rc7/drivers/ata/libata-eh.c#L697

So ahci_error_intr() from the second error interrupt, which is called after
thawing the port, should have called ata_std_sched_eh(), which calls
ata_eh_set_pending(), which should have set EH_PENDING:
https://github.com/torvalds/linux/blob/v6.5-rc7/drivers/ata/libata-eh.c#L884



My only guess is that after thawing the port:
https://github.com/torvalds/linux/blob/v6.5-rc7/drivers/ata/libata-eh.c#L2807

The second error irq comes, and sets EH_PENDING,
but then this silly code might clear it:
https://github.com/torvalds/linux/blob/v6.5-rc7/drivers/ata/libata-eh.c#L2825-L2837


Yeah, I think so.

I think the best way would be if we could improve this "spurious error
condition check"... because if this is indeed the code that clears EH_PENDING
for you, then this code basically makes the "goto repeat" code in
ata_scsi_port_error_handler() useless...


An alternative to improving the "spurious error condition check" might be for
you to try something like:


We have used this solution before, but it will case WARN_ON in
ata_eh_finish() as below:

    WARNING: CPU: 1 PID: 118 at ../drivers/ata/libata-eh.c:4016
ata_eh_finish+0x15a/0x170

Ok.

How about if you simply move the WARN_ON to ata_scsi_port_error_handler()
as well:

diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
index 35e03679b0bf..5be2fc651131 100644
--- a/drivers/ata/libata-eh.c
+++ b/drivers/ata/libata-eh.c
@@ -741,6 +741,12 @@ void ata_scsi_port_error_handler(struct Scsi_Host *host, struct ata_port *ap)
                   */
                  ap->ops->end_eh(ap);
+               if (!ap->scsi_host->host_eh_scheduled) {
+                       /* make sure nr_active_links is zero after EH */
+                       WARN_ON(ap->nr_active_links);
+                       ap->nr_active_links = 0;
+               }
+
                  spin_unlock_irqrestore(ap->lock, flags);
                  ata_eh_release(ap);
          } else {
@@ -962,7 +968,7 @@ void ata_std_end_eh(struct ata_port *ap)
   {
          struct Scsi_Host *host = ap->scsi_host;
-       host->host_eh_scheduled = 0;
+       host->host_eh_scheduled--;
   }
   EXPORT_SYMBOL(ata_std_end_eh);
@@ -3948,10 +3954,6 @@ void ata_eh_finish(struct ata_port *ap)
                          }
                  }
          }
-
-       /* make sure nr_active_links is zero after EH */
-       WARN_ON(ap->nr_active_links);
-       ap->nr_active_links = 0;
   }
   /**



Kind regards,
Niklas

We have tested this patch and it can fix the bug. Thank you so much. :)

Awesome! :)

Please send out a real patch, so that it is easier for the maintainer to
apply.

No need to give any credit to me.


Kind regards,
Niklas

It is my pleasure. I will send v2 later.

--
Thanks,
Nan




[Index of Archives]     [Linux Filesystems]     [Linux SCSI]     [Linux RAID]     [Git]     [Kernel Newbies]     [Linux Newbie]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Samba]     [Device Mapper]

  Powered by Linux