Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755231AbbHYMpc (ORCPT ); Tue, 25 Aug 2015 08:45:32 -0400 Received: from smtp.codeaurora.org ([198.145.29.96]:48531 "EHLO smtp.codeaurora.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752039AbbHYMpa (ORCPT ); Tue, 25 Aug 2015 08:45:30 -0400 Message-ID: <4117ea4cc6f5a3dbfc673553e2f94d6c.squirrel@www.codeaurora.org> In-Reply-To: References: <1440194989-28835-1-git-send-email-ygardi@codeaurora.org> <1440194989-28835-16-git-send-email-ygardi@codeaurora.org> Date: Tue, 25 Aug 2015 12:45:29 -0000 Subject: Re: [PATCH v1 15/15] scsi: ufs: add wrapper for retrying sending query attribute From: ygardi@codeaurora.org To: "Rob Herring" Cc: "Yaniv Gardi" , "linux-arm-msm" , linux-scsi@vger.kernel.org, "Dolev Raviv" , "Gilad Broner" , linux-scsi-owner@vger.kernel.org, "Jej B" , "Santosh Y" , "Subhash Jadavani" , hch@infradead.org, "Paul Bolle" , "James E.J. Bottomley" , "Vinayak Holikatti" , "linux-kernel@vger.kernel.org" User-Agent: SquirrelMail/1.4.22-4.el6 MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7BIT X-Priority: 3 (Normal) Importance: Normal Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5402 Lines: 153 > On Aug 21, 2015 3:10 PM, "Yaniv Gardi" wrote: >> >> Sometimes queries from the device might return a failure so it is >> recommended to retry sending the query, before giving up. >> This change adds a wrapper to retry sending a query attribute, >> in cases where we need to wait longer, before we continue, >> or before reporting a failure. > > Why not just always retry? Are there cases where retrying would be a > problem? There is no problem to retry whenever we encounter a query that returns with and error. In the code, it's recommended to replace any call to > > >> >> Signed-off-by: Yaniv Gardi >> >> --- >> drivers/scsi/ufs/ufshcd.c | 51 >> ++++++++++++++++++++++++++++++++++++++++------- >> 1 file changed, 44 insertions(+), 7 deletions(-) >> >> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c >> index 876148b..bfef67d 100644 >> --- a/drivers/scsi/ufs/ufshcd.c >> +++ b/drivers/scsi/ufs/ufshcd.c >> @@ -1827,6 +1827,43 @@ out: >> } >> >> /** >> + * ufshcd_query_attr_retry() - API function for sending query >> + * attribute with retries >> + * @hba: per-adapter instance >> + * @opcode: attribute opcode >> + * @idn: attribute idn to access >> + * @index: index field >> + * @selector: selector field >> + * @attr_val: the attribute value after the query request >> + * completes >> + * >> + * Returns 0 for success, non-zero in case of failure >> +*/ >> +static int ufshcd_query_attr_retry(struct ufs_hba *hba, >> + enum query_opcode opcode, enum attr_idn idn, u8 index, u8 >> selector, >> + u32 *attr_val) >> +{ >> + int ret = 0; >> + u32 retries; >> + >> + for (retries = QUERY_REQ_RETRIES; retries > 0; retries--) { >> + ret = ufshcd_query_attr(hba, opcode, idn, index, >> + selector, attr_val); >> + if (ret) >> + dev_dbg(hba->dev, "%s: failed with error %d, >> retries %d\n", >> + __func__, ret, retries); >> + else >> + break; >> + } >> + >> + if (ret) >> + dev_err(hba->dev, >> + "%s: query attribute, idn %d, failed with error >> %d after %d retires\n", >> + __func__, idn, ret, retries); > > The retry count will be wrong here. you are correct. will be fixed in V2 > >> + return ret; >> +} >> + >> +/** >> * ufshcd_query_descriptor - API function for sending descriptor >> requests >> * hba: per-adapter instance >> * opcode: attribute opcode >> @@ -3407,7 +3444,7 @@ static int ufshcd_disable_ee(struct ufs_hba *hba, >> u16 mask) >> >> val = hba->ee_ctrl_mask & ~mask; >> val &= 0xFFFF; /* 2 bytes */ >> - err = ufshcd_query_attr(hba, UPIU_QUERY_OPCODE_WRITE_ATTR, >> + err = ufshcd_query_attr_retry(hba, UPIU_QUERY_OPCODE_WRITE_ATTR, >> QUERY_ATTR_IDN_EE_CONTROL, 0, 0, &val); >> if (!err) >> hba->ee_ctrl_mask &= ~mask; >> @@ -3435,7 +3472,7 @@ static int ufshcd_enable_ee(struct ufs_hba *hba, >> u16 mask) >> >> val = hba->ee_ctrl_mask | mask; >> val &= 0xFFFF; /* 2 bytes */ >> - err = ufshcd_query_attr(hba, UPIU_QUERY_OPCODE_WRITE_ATTR, >> + err = ufshcd_query_attr_retry(hba, UPIU_QUERY_OPCODE_WRITE_ATTR, >> QUERY_ATTR_IDN_EE_CONTROL, 0, 0, &val); >> if (!err) >> hba->ee_ctrl_mask |= mask; >> @@ -3541,7 +3578,7 @@ static void ufshcd_force_reset_auto_bkops(struct >> ufs_hba *hba) >> >> static inline int ufshcd_get_bkops_status(struct ufs_hba *hba, u32 >> *status) >> { >> - return ufshcd_query_attr(hba, UPIU_QUERY_OPCODE_READ_ATTR, >> + return ufshcd_query_attr_retry(hba, UPIU_QUERY_OPCODE_READ_ATTR, >> QUERY_ATTR_IDN_BKOPS_STATUS, 0, 0, status); >> } >> >> @@ -3604,7 +3641,7 @@ static int ufshcd_urgent_bkops(struct ufs_hba >> *hba) >> >> static inline int ufshcd_get_ee_status(struct ufs_hba *hba, u32 >> *status) >> { >> - return ufshcd_query_attr(hba, UPIU_QUERY_OPCODE_READ_ATTR, >> + return ufshcd_query_attr_retry(hba, UPIU_QUERY_OPCODE_READ_ATTR, >> QUERY_ATTR_IDN_EE_STATUS, 0, 0, status); >> } >> >> @@ -4360,9 +4397,9 @@ static void ufshcd_init_icc_levels(struct ufs_hba >> *hba) >> dev_dbg(hba->dev, "%s: setting icc_level 0x%x", >> __func__, hba->init_prefetch_data.icc_level); >> >> - ret = ufshcd_query_attr(hba, UPIU_QUERY_OPCODE_WRITE_ATTR, >> - QUERY_ATTR_IDN_ACTIVE_ICC_LVL, 0, 0, >> - &hba->init_prefetch_data.icc_level); >> + ret = ufshcd_query_attr_retry(hba, UPIU_QUERY_OPCODE_WRITE_ATTR, >> + QUERY_ATTR_IDN_ACTIVE_ICC_LVL, 0, 0, >> + &hba->init_prefetch_data.icc_level); >> >> if (ret) >> dev_err(hba->dev, >> -- >> 1.8.5.2 >> >> -- >> QUALCOMM ISRAEL, on behalf of Qualcomm Innovation Center, Inc. is a >> member of Code Aurora Forum, hosted by The Linux Foundation > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/