Messages in this thread | | | Date | Mon, 4 Sep 2023 19:45:51 +0800 | Subject | Re: [PATCH] scsi: ata: Fix a race condition between scsi error handler and ahci interrupt | From | Li Nan <> |
| |
在 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@huaweicloud.com 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. :)
Feel free to add:
Tested-by: Li Nan <linan122@huawei.com>
-- Thanks, Nan
| |