Re: [PATCH 1/1] scsi: ufs: make sure all interrupts are processed

[Asutosh Das (asd)]

On 2/2/2018 8:53 AM, Asutosh Das (asd) wrote:
> On 1/31/2018 1:09 PM, Avri Altman wrote:
> > Hi,
> > Can you elaborate how this can even happen?
> > Isn't the interrupt aggregation capability should attend for those cases?
> >
> > Thanks,
> > Avri
> >
> > > -----Original Message-----
> > > From: linux-scsi-owner@xxxxxxxxxxxxxxx [mailto:linux-scsi-
> > > owner@xxxxxxxxxxxxxxx] On Behalf Of Asutosh Das
> > > Sent: Tuesday, January 30, 2018 6:54 AM
> > > To: subhashj@xxxxxxxxxxxxxx; cang@xxxxxxxxxxxxxx;
> > > vivek.gautam@xxxxxxxxxxxxxx; rnayak@xxxxxxxxxxxxxx;
> > > vinholikatti@xxxxxxxxx; jejb@xxxxxxxxxxxxxxxxxx;
> > > martin.petersen@xxxxxxxxxx
> > > Cc: linux-scsi@xxxxxxxxxxxxxxx; Venkat Gopalakrishnan
> > > <venkatg@xxxxxxxxxxxxxx>; Asutosh Das <asutoshd@xxxxxxxxxxxxxx>; open
> > > list <linux-kernel@xxxxxxxxxxxxxxx>
> > > Subject: [PATCH 1/1] scsi: ufs: make sure all interrupts are processed
> > >
> > > From: Venkat Gopalakrishnan <venkatg@xxxxxxxxxxxxxx>
> > >
> > > As multiple requests are submitted to the ufs host controller in 
> > > parallel there
> > > could be instances where the command completion interrupt arrives 
> > > later for a
> > > request that is already processed earlier as the corresponding 
> > > doorbell was
> > > cleared when handling the previous interrupt. Read the interrupt 
> > > status in a
> > > loop after processing the received interrupt to catch such interrupts 
> > > and handle
> > > it.
> > >
> > > Signed-off-by: Venkat Gopalakrishnan <venkatg@xxxxxxxxxxxxxx>
> > > Signed-off-by: Asutosh Das <asutoshd@xxxxxxxxxxxxxx>
> > > ---
> > > Â drivers/scsi/ufs/ufshcd.c | 27 +++++++++++++++++++--------
> > > Â 1 file changed, 19 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c index
> > > 8af2af3..58d81de 100644
> > > --- a/drivers/scsi/ufs/ufshcd.c
> > > +++ b/drivers/scsi/ufs/ufshcd.c
> > > @@ -5357,19 +5357,30 @@ static irqreturn_t ufshcd_intr(int irq, void 
> > > *__hba)
> > > ÂÂÂÂÂ u32 intr_status, enabled_intr_status;
> > > ÂÂÂÂÂ irqreturn_t retval = IRQ_NONE;
> > > ÂÂÂÂÂ struct ufs_hba *hba = __hba;
> > > +ÂÂÂ int retries = hba->nutrs;
> > >
> > > ÂÂÂÂÂ spin_lock(hba->host->host_lock);
> > > ÂÂÂÂÂ intr_status = ufshcd_readl(hba, REG_INTERRUPT_STATUS);
> > > -ÂÂÂ enabled_intr_status =
> > > -ÂÂÂÂÂÂÂ intr_status & ufshcd_readl(hba, REG_INTERRUPT_ENABLE);
> > >
> > > -ÂÂÂ if (intr_status)
> > > -ÂÂÂÂÂÂÂ ufshcd_writel(hba, intr_status, REG_INTERRUPT_STATUS);
> > > +ÂÂÂ /*
> > > +ÂÂÂÂ * There could be max of hba->nutrs reqs in flight and in worst 
> > > case
> > > +ÂÂÂÂ * if the reqs get finished 1 by 1 after the interrupt status is
> > > +ÂÂÂÂ * read, make sure we handle them by checking the interrupt status
> > > +ÂÂÂÂ * again in a loop until we process all of the reqs before 
> > > returning.
> > > +ÂÂÂÂ */
> > > +ÂÂÂ do {
> > > +ÂÂÂÂÂÂÂ enabled_intr_status =
> > > +ÂÂÂÂÂÂÂÂÂÂÂ intr_status & ufshcd_readl(hba,
> > > REG_INTERRUPT_ENABLE);
> > > +ÂÂÂÂÂÂÂ if (intr_status)
> > > +ÂÂÂÂÂÂÂÂÂÂÂ ufshcd_writel(hba, intr_status,
> > > REG_INTERRUPT_STATUS);
> > > +ÂÂÂÂÂÂÂ if (enabled_intr_status) {
> > > +ÂÂÂÂÂÂÂÂÂÂÂ ufshcd_sl_intr(hba, enabled_intr_status);
> > > +ÂÂÂÂÂÂÂÂÂÂÂ retval = IRQ_HANDLED;
> > > +ÂÂÂÂÂÂÂ }
> > > +
> > > +ÂÂÂÂÂÂÂ intr_status = ufshcd_readl(hba, REG_INTERRUPT_STATUS);
> > > +ÂÂÂ } while (intr_status && --retries);
> > >
> > > -ÂÂÂ if (enabled_intr_status) {
> > > -ÂÂÂÂÂÂÂ ufshcd_sl_intr(hba, enabled_intr_status);
> > > -ÂÂÂÂÂÂÂ retval = IRQ_HANDLED;
> > > -ÂÂÂ }
> > > ÂÂÂÂÂ spin_unlock(hba->host->host_lock);
> > > ÂÂÂÂÂ return retval;
> > > Â }
> > > --
> > > Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center,
> > > Inc.
> > > Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a 
> > > Linux
> > > Foundation Collaborative Project.
>
> Hi
> yes - interrupt aggregation makes sense here. But there were some 
> performance concerns with it; well, I don't have the data to back that 
> up now though.
> However, I can code it up and check it.
> Will post it in some time.
>
> -asd
Hi Avri,
I went through the UFS HCI - v2.1 spec. Specifically, in sec 7.2.3 it 
explicitly mentions that the software should determine if new TRs were 
completed since the interrupt status was last read/cleared. This step is 
independent of aggregation.

So I think the above implementation makes sense. Please let me know if I 
understood your concern correctly.

-asd

--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a 
Linux Foundation Collaborative Project