Messages in this thread |  | | Subject | Re: [PATCH 1/1] scsi: ufs: make sure all interrupts are processed | From | "Asutosh Das (asd)" <> | Date | Fri, 2 Feb 2018 08:53:09 +0530 |
| |
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@vger.kernel.org [mailto:linux-scsi- >> owner@vger.kernel.org] On Behalf Of Asutosh Das >> Sent: Tuesday, January 30, 2018 6:54 AM >> To: subhashj@codeaurora.org; cang@codeaurora.org; >> vivek.gautam@codeaurora.org; rnayak@codeaurora.org; >> vinholikatti@gmail.com; jejb@linux.vnet.ibm.com; >> martin.petersen@oracle.com >> Cc: linux-scsi@vger.kernel.org; Venkat Gopalakrishnan >> <venkatg@codeaurora.org>; Asutosh Das <asutoshd@codeaurora.org>; open >> list <linux-kernel@vger.kernel.org> >> Subject: [PATCH 1/1] scsi: ufs: make sure all interrupts are processed >> >> From: Venkat Gopalakrishnan <venkatg@codeaurora.org> >> >> 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@codeaurora.org> >> Signed-off-by: Asutosh Das <asutoshd@codeaurora.org> >> --- >> 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
-- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project
|  |