| Message ID | 20220727070410epcms2p5206785e4d960b32dcbb6729710dab535@epcms2p5 |
|---|---|
| State | New |
| Headers | show |
| Series | scsi: ufs: wb: Add sysfs attribute and cleanup | expand |
On Wed, 2022-07-27 at 16:05 +0900, Jinyoung CHOI wrote: > Changed to improve readability. > As implemented in ufshcd_wb_togle_flush(), the conditional test is > modified in the same way. > > Reviewed-by: Avri Altman <avri.altman@wdc.com> > Signed-off-by: Jinyoung Choi <j-young.choi@samsung.com> Reviewed-by: Bean Huo <beanhuo@micron.com>
On 7/27/22 00:10, Jinyoung CHOI wrote: > diff --git a/include/ufs/ufshcd.h b/include/ufs/ufshcd.h > index 94bcfec98fb8..78adc556444a 100644 > --- a/include/ufs/ufshcd.h > +++ b/include/ufs/ufshcd.h > @@ -1017,6 +1017,12 @@ static inline bool ufshcd_is_wb_allowed(struct ufs_hba *hba) > return hba->caps & UFSHCD_CAP_WB_EN; > } > > +static inline bool ufshcd_is_wb_buf_flush_allowed(struct ufs_hba *hba) > +{ > + return ufshcd_is_wb_allowed(hba) && > + !(hba->quirks & UFSHCI_QUIRK_SKIP_MANUAL_WB_FLUSH_CTRL); > +} Since this function is only used inside the UFS driver core it should be added in drivers/ufs/core/ufshcd-priv.h instead of include/ufs/ufshcd.h. Thanks, Bart.
>> There is the following quirk to bypass "WB Flush" in Write Booster. >> >> - UFSHCI_QUIRK_SKIP_MANUAL_WB_FLUSH_CTRL >> >> If this quirk is not set, there is no knob that can control "WB >> Flush". > >> >> There are three flags that control Write Booster Feature. >> 1. WB ON/OFF >> 2. WB Hibern Flush ON/OFF (implicitly) >> 3. WB Flush ON/OFF (explicit) >> >> The sysfs attribute that controls the WB was implemented. (1) >> >> In the case of "Hibern Flush", it is always good to turn on. >> Control may not be required. (2) >> >> Finally, "Flush" may be necessary because the Auto-Hibern8 is not >> supported in a specific environment. >> So the sysfs attribute that controls this is necessary. (3) >> >> Reviewed-by: Avri Altman <avri.altman@wdc.com> >> Signed-off-by: Jinyoung Choi <j-young.choi@samsung.com> >> --- >... >> >> +static ssize_t wb_buf_flush_en_store(struct device *dev, >> + struct device_attribute *attr, >> + const char *buf, size_t count) >> +{ >> + struct ufs_hba *hba = dev_get_drvdata(dev); >> + unsigned int wb_buf_flush_en; >> + ssize_t res; >> + >> + if (ufshcd_is_wb_allowed(hba) && >> + !(hba->quirks & UFSHCI_QUIRK_SKIP_MANUAL_WB_FLUSH_CTRL)) { >> + dev_warn(dev, "It is not allowed to configure WB buf >> flush!\n"); >> + return -EOPNOTSUPP; >> + } >> + >Hi J-young, > >I don't understand here, if UFSHCI_QUIRK_SKIP_MANUAL_WB_FLUSH_CTRL is >not set (manual flush is not disable), so we cannot manually flush >buffer? or should we check if Auto-Hibern8 is supported? > >Kind regards, >Bean Hi Bean, As the patch was separated, the conditional sentence went wrong. Thank you for checking. I will modify it quickly. Thanks, Jinyoung.
>On 7/27/22 00:10, Jinyoung CHOI wrote: >> diff --git a/include/ufs/ufshcd.h b/include/ufs/ufshcd.h >> index 94bcfec98fb8..78adc556444a 100644 >> --- a/include/ufs/ufshcd.h >> +++ b/include/ufs/ufshcd.h >> @@ -1017,6 +1017,12 @@ static inline bool ufshcd_is_wb_allowed(struct ufs_hba *hba) >> return hba->caps & UFSHCD_CAP_WB_EN; >> } >> >> +static inline bool ufshcd_is_wb_buf_flush_allowed(struct ufs_hba *hba) >> +{ >> + return ufshcd_is_wb_allowed(hba) && >> + !(hba->quirks & UFSHCI_QUIRK_SKIP_MANUAL_WB_FLUSH_CTRL); >> +} > >Since this function is only used inside the UFS driver core it should be >added in drivers/ufs/core/ufshcd-priv.h instead of include/ufs/ufshcd.h. > >Thanks, > >Bart. OK, I will move it. I didn't know the exact purpose of ufshcd-priv.h. Thank you for telling me. Thanks, Jinyoung.
>On Wed, 2022-07-27 at 16:04 +0900, Jinyoung CHOI wrote: >> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c >> index 8f11f118c30e..a3bdf9986511 100644 >> --- a/drivers/ufs/core/ufshcd.c >> +++ b/drivers/ufs/core/ufshcd.c >> @@ -5722,6 +5722,9 @@ static int __ufshcd_wb_toggle(struct ufs_hba >> *hba, bool set, enum flag_idn idn) >> enum query_opcode opcode = set ? UPIU_QUERY_OPCODE_SET_FLAG : >> UPIU_QUERY_OPCODE_CLEAR_FLAG; >> >> + if (!ufshcd_is_wb_allowed(hba)) >> + return -EPERM; >> + >Hi J-young, > >here you should change its return, Otherwise, there will be an fake >error printing: > > dev_err(hba->dev, "%s Write Booster %s failed %d\n", > __func__, enable ? "enable" : "disable", ret); > > >Kind regards, >Bean You are right! Rather than changing the return value, this patch is likely to be excluded because caller can continue unnecessary work. Thanks, Jinyoung.
diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c index 8f11f118c30e..a3bdf9986511 100644 --- a/drivers/ufs/core/ufshcd.c +++ b/drivers/ufs/core/ufshcd.c @@ -5722,6 +5722,9 @@ static int __ufshcd_wb_toggle(struct ufs_hba *hba, bool set, enum flag_idn idn) enum query_opcode opcode = set ? UPIU_QUERY_OPCODE_SET_FLAG : UPIU_QUERY_OPCODE_CLEAR_FLAG; + if (!ufshcd_is_wb_allowed(hba)) + return -EPERM; + index = ufshcd_wb_get_query_index(hba); return ufshcd_query_flag_retry(hba, opcode, idn, index, NULL); } @@ -5730,9 +5733,6 @@ int ufshcd_wb_toggle(struct ufs_hba *hba, bool enable) { int ret; - if (!ufshcd_is_wb_allowed(hba)) - return 0; - if (!(enable ^ hba->dev_info.wb_enabled)) return 0; @@ -5769,8 +5769,7 @@ static inline void ufshcd_wb_toggle_flush(struct ufs_hba *hba, bool enable) { int ret; - if (!ufshcd_is_wb_allowed(hba) || - hba->dev_info.wb_buf_flush_enabled == enable) + if (hba->dev_info.wb_buf_flush_enabled == enable) return; ret = __ufshcd_wb_toggle(hba, enable, QUERY_FLAG_IDN_WB_BUFF_FLUSH_EN);