| Message ID | 1440194989-28835-16-git-send-email-ygardi@codeaurora.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Aug 21, 2015 3:10 PM, "Yaniv Gardi" <ygardi@codeaurora.org> 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? > > Signed-off-by: Yaniv Gardi <ygardi@codeaurora.org> > > --- > 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. > + 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-scsi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> On Aug 21, 2015 3:10 PM, "Yaniv Gardi" <ygardi@codeaurora.org> 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 <ygardi@codeaurora.org> >> >> --- >> 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-scsi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> On Aug 21, 2015 3:10 PM, "Yaniv Gardi" <ygardi@codeaurora.org> 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. > > >> >> Signed-off-by: Yaniv Gardi <ygardi@codeaurora.org> >> >> --- >> 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-scsi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Delete & Prev | Delete & Next Move to: -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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); + 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,
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. Signed-off-by: Yaniv Gardi <ygardi@codeaurora.org> --- drivers/scsi/ufs/ufshcd.c | 51 ++++++++++++++++++++++++++++++++++++++++------- 1 file changed, 44 insertions(+), 7 deletions(-)