Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
IronPort-SDR: 7KLujGP4NUyf/AlpRjGxahOXsOdPe5h24LI8+IdyJDsrNzTFMQJ5DJWEuGKWTLBRFTp1H87WYv
 mgPwq3a60Fi5GTcBTQ1gRurP0obaRmDwkJMaI9Y2BfFxrM2gj2Xi13IbOhGb+vMzIhlXLSWOnp
 uANaRWeTzOS+FscGK1uF37XpKd/V95BiHFoIEffeGRUYaJNuRCL+eM/0v+Xl9Fk24jlH8qA7/S
 XsMCC8al9/3TbsUmvosm4E6bZQMcS5vMOJuztk6tPNT2NCfMsobyqLN9wlozD8dFYgdB24DLmT
 yAE=
From:   Avri Altman <Avri.Altman@wdc.com>
To:     Can Guo <cang@codeaurora.org>,
        "asutoshd@codeaurora.org" <asutoshd@codeaurora.org>,
        "nguyenb@codeaurora.org" <nguyenb@codeaurora.org>,
        "rnayak@codeaurora.org" <rnayak@codeaurora.org>,
        "linux-scsi@vger.kernel.org" <linux-scsi@vger.kernel.org>,
        "kernel-team@android.com" <kernel-team@android.com>,
        "saravanak@google.com" <saravanak@google.com>,
        "salyzyn@google.com" <salyzyn@google.com>
CC:     Venkat Gopalakrishnan <venkatg@codeaurora.org>,
        Alim Akhtar <alim.akhtar@samsung.com>,
        Pedro Sousa <pedrom.sousa@synopsys.com>,
        "James E.J. Bottomley" <jejb@linux.ibm.com>,
        "Martin K. Petersen" <martin.petersen@oracle.com>,
        Stanley Chu <stanley.chu@mediatek.com>,
        Bean Huo <beanhuo@micron.com>,
        Tomas Winkler <tomas.winkler@intel.com>,
        open list <linux-kernel@vger.kernel.org>
Subject: RE: [PATCH v4 5/7] scsi: ufs: Fix irq return code
Thread-Topic: [PATCH v4 5/7] scsi: ufs: Fix irq return code
Thread-Index: AQHVme4tLWcA454rdEyTvCpbgBLcVKeKfCcA
Date:   Thu, 14 Nov 2019 10:43:38 +0000
Message-ID: <MN2PR04MB69913709436481CD96F5C3C0FC710@MN2PR04MB6991.namprd04.prod.outlook.com>
References: <1573627552-12615-1-git-send-email-cang@codeaurora.org>
 <1573627552-12615-6-git-send-email-cang@codeaurora.org>
In-Reply-To: <1573627552-12615-6-git-send-email-cang@codeaurora.org>
Accept-Language: en-US
Content-Language: en-US
wdcipoutbound: EOP-TRUE
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: 7007e1e4-68e1-4e11-c0f7-08d768ef8260
X-MS-Exchange-CrossTenant-originalarrivaltime: 14 Nov 2019 10:43:38.8642
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: b61c8803-16f3-4c35-9b17-6f65f441df86
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: bKkrDRoRXAICtistI0TMJrSCiowGXaFbbFr6P24EGAoKWP6k3ipns+3YNpXueMwGj/JSNf0RHXCCStLWSvat3Q==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MN2PR04MB6912
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

>=20
> From: Venkat Gopalakrishnan <venkatg@codeaurora.org>
>=20
> Return IRQ_HANDLED only if the irq is really handled, this will help in c=
atching
> spurious interrupts that go unhandled.
>=20
> Signed-off-by: Can Guo <cang@codeaurora.org>
Reviewed-by: Avri Altman <avri.altman@wdc.com>

Interesting enough - did your patch exposed unhandled interrupts?
If it does - maybe you can share some info in your commit log.

Thanks,
Avri

> ---
>  drivers/scsi/ufs/ufshcd.c | 134 ++++++++++++++++++++++++++++++++++-----
> -------
>  drivers/scsi/ufs/ufshci.h |   2 +-
>  2 files changed, 100 insertions(+), 36 deletions(-)
>=20
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c index
> 671ea2a..8d1b04f 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -240,7 +240,7 @@ struct ufs_pm_lvl_states ufs_pm_lvl_states[] =3D {
>         END_FIX
>  };
>=20
> -static void ufshcd_tmc_handler(struct ufs_hba *hba);
> +static irqreturn_t ufshcd_tmc_handler(struct ufs_hba *hba);
>  static void ufshcd_async_scan(void *data, async_cookie_t cookie);  stati=
c int
> ufshcd_reset_and_restore(struct ufs_hba *hba);  static int
> ufshcd_eh_host_reset_handler(struct scsi_cmnd *cmd); @@ -4799,19
> +4799,29 @@ static void ufshcd_slave_destroy(struct scsi_device *sdev)
>   * ufshcd_uic_cmd_compl - handle completion of uic command
>   * @hba: per adapter instance
>   * @intr_status: interrupt status generated by the controller
> + *
> + * Returns
> + *  IRQ_HANDLED - If interrupt is valid
> + *  IRQ_NONE    - If invalid interrupt
>   */
> -static void ufshcd_uic_cmd_compl(struct ufs_hba *hba, u32 intr_status)
> +static irqreturn_t ufshcd_uic_cmd_compl(struct ufs_hba *hba, u32
> +intr_status)
>  {
> +       irqreturn_t retval =3D IRQ_NONE;
> +
>         if ((intr_status & UIC_COMMAND_COMPL) && hba->active_uic_cmd) {
>                 hba->active_uic_cmd->argument2 |=3D
>                         ufshcd_get_uic_cmd_result(hba);
>                 hba->active_uic_cmd->argument3 =3D
>                         ufshcd_get_dme_attr_val(hba);
>                 complete(&hba->active_uic_cmd->done);
> +               retval =3D IRQ_HANDLED;
>         }
>=20
> -       if ((intr_status & UFSHCD_UIC_PWR_MASK) && hba->uic_async_done)
> +       if ((intr_status & UFSHCD_UIC_PWR_MASK) && hba->uic_async_done)
> + {
>                 complete(hba->uic_async_done);
> +               retval =3D IRQ_HANDLED;
> +       }
> +       return retval;
>  }
>=20
>  /**
> @@ -4867,8 +4877,12 @@ static void __ufshcd_transfer_req_compl(struct
> ufs_hba *hba,
>  /**
>   * ufshcd_transfer_req_compl - handle SCSI and query command completion
>   * @hba: per adapter instance
> + *
> + * Returns
> + *  IRQ_HANDLED - If interrupt is valid
> + *  IRQ_NONE    - If invalid interrupt
>   */
> -static void ufshcd_transfer_req_compl(struct ufs_hba *hba)
> +static irqreturn_t ufshcd_transfer_req_compl(struct ufs_hba *hba)
>  {
>         unsigned long completed_reqs;
>         u32 tr_doorbell;
> @@ -4887,7 +4901,12 @@ static void ufshcd_transfer_req_compl(struct
> ufs_hba *hba)
>         tr_doorbell =3D ufshcd_readl(hba, REG_UTP_TRANSFER_REQ_DOOR_BELL)=
;
>         completed_reqs =3D tr_doorbell ^ hba->outstanding_reqs;
>=20
> -       __ufshcd_transfer_req_compl(hba, completed_reqs);
> +       if (completed_reqs) {
> +               __ufshcd_transfer_req_compl(hba, completed_reqs);
> +               return IRQ_HANDLED;
> +       } else {
> +               return IRQ_NONE;
> +       }
>  }
>=20
>  /**
> @@ -5406,61 +5425,77 @@ static void ufshcd_err_handler(struct work_struct
> *work)
>  /**
>   * ufshcd_update_uic_error - check and set fatal UIC error flags.
>   * @hba: per-adapter instance
> + *
> + * Returns
> + *  IRQ_HANDLED - If interrupt is valid
> + *  IRQ_NONE    - If invalid interrupt
>   */
> -static void ufshcd_update_uic_error(struct ufs_hba *hba)
> +static irqreturn_t ufshcd_update_uic_error(struct ufs_hba *hba)
>  {
>         u32 reg;
> +       irqreturn_t retval =3D IRQ_NONE;
>=20
>         /* PHY layer lane error */
>         reg =3D ufshcd_readl(hba, REG_UIC_ERROR_CODE_PHY_ADAPTER_LAYER);
>         /* Ignore LINERESET indication, as this is not an error */
>         if ((reg & UIC_PHY_ADAPTER_LAYER_ERROR) &&
> -                       (reg & UIC_PHY_ADAPTER_LAYER_LANE_ERR_MASK)) {
> +           (reg & UIC_PHY_ADAPTER_LAYER_LANE_ERR_MASK)) {
>                 /*
>                  * To know whether this error is fatal or not, DB timeout
>                  * must be checked but this error is handled separately.
>                  */
>                 dev_dbg(hba->dev, "%s: UIC Lane error reported\n", __func=
__);
>                 ufshcd_update_reg_hist(&hba->ufs_stats.pa_err, reg);
> +               retval |=3D IRQ_HANDLED;
>         }
>=20
>         /* PA_INIT_ERROR is fatal and needs UIC reset */
>         reg =3D ufshcd_readl(hba, REG_UIC_ERROR_CODE_DATA_LINK_LAYER);
> -       if (reg)
> +       if ((reg & UIC_DATA_LINK_LAYER_ERROR) &&
> +           (reg & UIC_DATA_LINK_LAYER_ERROR_CODE_MASK)) {
>                 ufshcd_update_reg_hist(&hba->ufs_stats.dl_err, reg);
>=20
> -       if (reg & UIC_DATA_LINK_LAYER_ERROR_PA_INIT)
> -               hba->uic_error |=3D UFSHCD_UIC_DL_PA_INIT_ERROR;
> -       else if (hba->dev_quirks &
> -                  UFS_DEVICE_QUIRK_RECOVERY_FROM_DL_NAC_ERRORS) {
> -               if (reg & UIC_DATA_LINK_LAYER_ERROR_NAC_RECEIVED)
> -                       hba->uic_error |=3D
> -                               UFSHCD_UIC_DL_NAC_RECEIVED_ERROR;
> -               else if (reg & UIC_DATA_LINK_LAYER_ERROR_TCx_REPLAY_TIMEO=
UT)
> -                       hba->uic_error |=3D UFSHCD_UIC_DL_TCx_REPLAY_ERRO=
R;
> +               if (reg & UIC_DATA_LINK_LAYER_ERROR_PA_INIT)
> +                       hba->uic_error |=3D UFSHCD_UIC_DL_PA_INIT_ERROR;
> +               else if (hba->dev_quirks &
> +                               UFS_DEVICE_QUIRK_RECOVERY_FROM_DL_NAC_ERR=
ORS) {
> +                       if (reg & UIC_DATA_LINK_LAYER_ERROR_NAC_RECEIVED)
> +                               hba->uic_error |=3D
> +                                       UFSHCD_UIC_DL_NAC_RECEIVED_ERROR;
> +                       else if (reg &
> UIC_DATA_LINK_LAYER_ERROR_TCx_REPLAY_TIMEOUT)
> +                               hba->uic_error |=3D UFSHCD_UIC_DL_TCx_REP=
LAY_ERROR;
> +               }
> +               retval |=3D IRQ_HANDLED;
>         }
>=20
>         /* UIC NL/TL/DME errors needs software retry */
>         reg =3D ufshcd_readl(hba, REG_UIC_ERROR_CODE_NETWORK_LAYER);
> -       if (reg) {
> +       if ((reg & UIC_NETWORK_LAYER_ERROR) &&
> +           (reg & UIC_NETWORK_LAYER_ERROR_CODE_MASK)) {
>                 ufshcd_update_reg_hist(&hba->ufs_stats.nl_err, reg);
>                 hba->uic_error |=3D UFSHCD_UIC_NL_ERROR;
> +               retval |=3D IRQ_HANDLED;
>         }
>=20
>         reg =3D ufshcd_readl(hba, REG_UIC_ERROR_CODE_TRANSPORT_LAYER);
> -       if (reg) {
> +       if ((reg & UIC_TRANSPORT_LAYER_ERROR) &&
> +           (reg & UIC_TRANSPORT_LAYER_ERROR_CODE_MASK)) {
>                 ufshcd_update_reg_hist(&hba->ufs_stats.tl_err, reg);
>                 hba->uic_error |=3D UFSHCD_UIC_TL_ERROR;
> +               retval |=3D IRQ_HANDLED;
>         }
>=20
>         reg =3D ufshcd_readl(hba, REG_UIC_ERROR_CODE_DME);
> -       if (reg) {
> +       if ((reg & UIC_DME_ERROR) &&
> +           (reg & UIC_DME_ERROR_CODE_MASK)) {
>                 ufshcd_update_reg_hist(&hba->ufs_stats.dme_err, reg);
>                 hba->uic_error |=3D UFSHCD_UIC_DME_ERROR;
> +               retval |=3D IRQ_HANDLED;
>         }
>=20
>         dev_dbg(hba->dev, "%s: UIC error flags =3D 0x%08x\n",
>                         __func__, hba->uic_error);
> +       return retval;
>  }
>=20
>  static bool ufshcd_is_auto_hibern8_error(struct ufs_hba *hba, @@ -5483,1=
0
> +5518,15 @@ static bool ufshcd_is_auto_hibern8_error(struct ufs_hba *hba,
>  /**
>   * ufshcd_check_errors - Check for errors that need s/w attention
>   * @hba: per-adapter instance
> + *
> + * Returns
> + *  IRQ_HANDLED - If interrupt is valid
> + *  IRQ_NONE    - If invalid interrupt
>   */
> -static void ufshcd_check_errors(struct ufs_hba *hba)
> +static irqreturn_t ufshcd_check_errors(struct ufs_hba *hba)
>  {
>         bool queue_eh_work =3D false;
> +       irqreturn_t retval =3D IRQ_NONE;
>=20
>         if (hba->errors & INT_FATAL_ERRORS) {
>                 ufshcd_update_reg_hist(&hba->ufs_stats.fatal_err, hba->er=
rors); @@
> -5495,7 +5535,7 @@ static void ufshcd_check_errors(struct ufs_hba *hba)
>=20
>         if (hba->errors & UIC_ERROR) {
>                 hba->uic_error =3D 0;
> -               ufshcd_update_uic_error(hba);
> +               retval =3D ufshcd_update_uic_error(hba);
>                 if (hba->uic_error)
>                         queue_eh_work =3D true;
>         }
> @@ -5543,6 +5583,7 @@ static void ufshcd_check_errors(struct ufs_hba
> *hba)
>                         }
>                         schedule_work(&hba->eh_work);
>                 }
> +               retval |=3D IRQ_HANDLED;
>         }
>         /*
>          * if (!queue_eh_work) -
> @@ -5550,44 +5591,62 @@ static void ufshcd_check_errors(struct ufs_hba
> *hba)
>          * itself without s/w intervention or errors that will be
>          * handled by the SCSI core layer.
>          */
> +       return retval;
>  }
>=20
>  /**
>   * ufshcd_tmc_handler - handle task management function completion
>   * @hba: per adapter instance
> + *
> + * Returns
> + *  IRQ_HANDLED - If interrupt is valid
> + *  IRQ_NONE    - If invalid interrupt
>   */
> -static void ufshcd_tmc_handler(struct ufs_hba *hba)
> +static irqreturn_t ufshcd_tmc_handler(struct ufs_hba *hba)
>  {
>         u32 tm_doorbell;
>=20
>         tm_doorbell =3D ufshcd_readl(hba, REG_UTP_TASK_REQ_DOOR_BELL);
>         hba->tm_condition =3D tm_doorbell ^ hba->outstanding_tasks;
> -       wake_up(&hba->tm_wq);
> +       if (hba->tm_condition) {
> +               wake_up(&hba->tm_wq);
> +               return IRQ_HANDLED;
> +       } else {
> +               return IRQ_NONE;
> +       }
>  }
>=20
>  /**
>   * ufshcd_sl_intr - Interrupt service routine
>   * @hba: per adapter instance
>   * @intr_status: contains interrupts generated by the controller
> + *
> + * Returns
> + *  IRQ_HANDLED - If interrupt is valid
> + *  IRQ_NONE    - If invalid interrupt
>   */
> -static void ufshcd_sl_intr(struct ufs_hba *hba, u32 intr_status)
> +static irqreturn_t ufshcd_sl_intr(struct ufs_hba *hba, u32 intr_status)
>  {
> +       irqreturn_t retval =3D IRQ_NONE;
> +
>         hba->errors =3D UFSHCD_ERROR_MASK & intr_status;
>=20
>         if (ufshcd_is_auto_hibern8_error(hba, intr_status))
>                 hba->errors |=3D (UFSHCD_UIC_HIBERN8_MASK & intr_status);
>=20
>         if (hba->errors)
> -               ufshcd_check_errors(hba);
> +               retval |=3D ufshcd_check_errors(hba);
>=20
>         if (intr_status & UFSHCD_UIC_MASK)
> -               ufshcd_uic_cmd_compl(hba, intr_status);
> +               retval |=3D ufshcd_uic_cmd_compl(hba, intr_status);
>=20
>         if (intr_status & UTP_TASK_REQ_COMPL)
> -               ufshcd_tmc_handler(hba);
> +               retval |=3D ufshcd_tmc_handler(hba);
>=20
>         if (intr_status & UTP_TRANSFER_REQ_COMPL)
> -               ufshcd_transfer_req_compl(hba);
> +               retval |=3D ufshcd_transfer_req_compl(hba);
> +
> +       return retval;
>  }
>=20
>  /**
> @@ -5595,8 +5654,9 @@ static void ufshcd_sl_intr(struct ufs_hba *hba, u32
> intr_status)
>   * @irq: irq number
>   * @__hba: pointer to adapter instance
>   *
> - * Returns IRQ_HANDLED - If interrupt is valid
> - *             IRQ_NONE - If invalid interrupt
> + * Returns
> + *  IRQ_HANDLED - If interrupt is valid
> + *  IRQ_NONE    - If invalid interrupt
>   */
>  static irqreturn_t ufshcd_intr(int irq, void *__hba)  { @@ -5619,14 +567=
9,18
> @@ static irqreturn_t ufshcd_intr(int irq, void *__hba)
>                         intr_status & ufshcd_readl(hba, REG_INTERRUPT_ENA=
BLE);
>                 if (intr_status)
>                         ufshcd_writel(hba, intr_status, REG_INTERRUPT_STA=
TUS);
> -               if (enabled_intr_status) {
> -                       ufshcd_sl_intr(hba, enabled_intr_status);
> -                       retval =3D IRQ_HANDLED;
> -               }
> +               if (enabled_intr_status)
> +                       retval |=3D ufshcd_sl_intr(hba,
> + enabled_intr_status);
>=20
>                 intr_status =3D ufshcd_readl(hba, REG_INTERRUPT_STATUS);
>         } while (intr_status && --retries);
>=20
> +       if (retval =3D=3D IRQ_NONE) {
> +               dev_err(hba->dev, "%s: Unhandled interrupt 0x%08x\n",
> +                                       __func__, intr_status);
> +               ufshcd_dump_regs(hba, 0, UFSHCI_REG_SPACE_SIZE, "host_reg=
s: ");
> +       }
> +
>         spin_unlock(hba->host->host_lock);
>         return retval;
>  }
> diff --git a/drivers/scsi/ufs/ufshci.h b/drivers/scsi/ufs/ufshci.h index
> dbb75cd..c2961d3 100644
> --- a/drivers/scsi/ufs/ufshci.h
> +++ b/drivers/scsi/ufs/ufshci.h
> @@ -195,7 +195,7 @@ enum {
>=20
>  /* UECDL - Host UIC Error Code Data Link Layer 3Ch */
>  #define UIC_DATA_LINK_LAYER_ERROR              0x80000000
> -#define UIC_DATA_LINK_LAYER_ERROR_CODE_MASK    0x7FFF
> +#define UIC_DATA_LINK_LAYER_ERROR_CODE_MASK    0xFFFF
>  #define UIC_DATA_LINK_LAYER_ERROR_TCX_REP_TIMER_EXP    0x2
>  #define UIC_DATA_LINK_LAYER_ERROR_AFCX_REQ_TIMER_EXP   0x4
>  #define UIC_DATA_LINK_LAYER_ERROR_FCX_PRO_TIMER_EXP    0x8
> --
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum=
,
> a Linux Foundation Collaborative Project