[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <ZPXGM0YUPP4+n0Rz@x1-carbon>
Date: Mon, 4 Sep 2023 11:57:41 +0000
From: Niklas Cassel <Niklas.Cassel@....com>
To: Li Nan <linan666@...weicloud.com>
CC: "dlemoal@...nel.org" <dlemoal@...nel.org>,
"linux-ide@...r.kernel.org" <linux-ide@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"yukuai3@...wei.com" <yukuai3@...wei.com>,
"yi.zhang@...wei.com" <yi.zhang@...wei.com>,
"houtao1@...wei.com" <houtao1@...wei.com>,
"yangerkun@...wei.com" <yangerkun@...wei.com>
Subject: Re: [PATCH] scsi: ata: Fix a race condition between scsi error
handler and ahci interrupt
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@...weicloud.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. :)
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
Powered by blists - more mailing lists