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
