| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <SN6PR04MB46400873245235EA56838A19FC8C0@SN6PR04MB4640.namprd04.prod.outlook.com>
Date: Sat, 30 May 2020 20:37:57 +0000
From: Avri Altman <Avri.Altman@....com>
To: Stanley Chu <stanley.chu@...iatek.com>,
"linux-scsi@...r.kernel.org" <linux-scsi@...r.kernel.org>,
"martin.petersen@...cle.com" <martin.petersen@...cle.com>,
"alim.akhtar@...sung.com" <alim.akhtar@...sung.com>,
"jejb@...ux.ibm.com" <jejb@...ux.ibm.com>
CC: "beanhuo@...ron.com" <beanhuo@...ron.com>,
"asutoshd@...eaurora.org" <asutoshd@...eaurora.org>,
"cang@...eaurora.org" <cang@...eaurora.org>,
"matthias.bgg@...il.com" <matthias.bgg@...il.com>,
"bvanassche@....org" <bvanassche@....org>,
"linux-mediatek@...ts.infradead.org"
<linux-mediatek@...ts.infradead.org>,
"linux-arm-kernel@...ts.infradead.org"
<linux-arm-kernel@...ts.infradead.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"kuohong.wang@...iatek.com" <kuohong.wang@...iatek.com>,
"peter.wang@...iatek.com" <peter.wang@...iatek.com>,
"chun-hung.wu@...iatek.com" <chun-hung.wu@...iatek.com>,
"andy.teng@...iatek.com" <andy.teng@...iatek.com>,
"cc.chou@...iatek.com" <cc.chou@...iatek.com>,
"chaotian.jing@...iatek.com" <chaotian.jing@...iatek.com>
Subject: RE: [PATCH v1 1/2] scsi: ufs: Support WriteBooster on Samsung UFS
devices
> @@ -2801,11 +2801,17 @@ int ufshcd_query_flag(struct ufs_hba *hba, enum
> query_opcode opcode,
> {
> struct ufs_query_req *request = NULL;
> struct ufs_query_res *response = NULL;
> - int err, selector = 0;
> + int err;
> int timeout = QUERY_REQ_TIMEOUT;
> + u8 selector = 0;
>
> BUG_ON(!hba);
>
> + if (hba->dev_quirks & UFS_DEVICE_QUIRK_WB_SPECIAL_SELECTOR) {
> + if (ufshcd_is_wb_flags(idn))
> + selector = 1;
> + }
> +
Why not make the caller set the applicable selector,
Instead of checking this for every flag?
> ufshcd_hold(hba, false);
> mutex_lock(&hba->dev_cmd.lock);
> ufshcd_init_query(hba, &request, &response, opcode, idn, index,
> @@ -2882,6 +2888,11 @@ int ufshcd_query_attr(struct ufs_hba *hba, enum
> query_opcode opcode,
> goto out;
> }
>
> + if (hba->dev_quirks & UFS_DEVICE_QUIRK_WB_SPECIAL_SELECTOR) {
> + if (ufshcd_is_wb_attrs(idn))
> + selector = 1;
> + }
> +
Same here
> mutex_lock(&hba->dev_cmd.lock);
> ufshcd_init_query(hba, &request, &response, opcode, idn, index,
> selector);
> @@ -3042,6 +3053,11 @@ int ufshcd_query_descriptor_retry(struct ufs_hba
> *hba,
> int err;
> int retries;
>
> + if (hba->dev_quirks & UFS_DEVICE_QUIRK_WB_SPECIAL_SELECTOR) {
> + if (ufshcd_is_wb_desc(idn, index))
> + selector = 1;
> + }
> +
And here.
But this can't be true -
Are you setting the selector = 1 for reading any field for those descriptors?
Shouldn't it be for the wb specific fields?
> for (retries = QUERY_REQ_RETRIES; retries > 0; retries--) {
> err = __ufshcd_query_descriptor(hba, opcode, idn, index,
> selector, desc_buf, buf_len);
> @@ -6907,8 +6923,10 @@ static int ufs_get_device_desc(struct ufs_hba *hba)
> size_t buff_len;
> u8 model_index;
> u8 *desc_buf;
> + u8 retry_cnt = 0;
> struct ufs_dev_info *dev_info = &hba->dev_info;
>
> +retry:
> buff_len = max_t(size_t, hba->desc_size.dev_desc,
> QUERY_DESC_MAX_SIZE + 1);
> desc_buf = kmalloc(buff_len, GFP_KERNEL);
> @@ -6948,6 +6966,29 @@ static int ufs_get_device_desc(struct ufs_hba *hba)
>
> ufs_fixup_device_setup(hba);
>
> + if (!retry_cnt && (hba->dev_quirks &
> + UFS_DEVICE_QUIRK_WB_SPECIAL_SELECTOR)) {
If you only want to enter this clause once - you should use something other than retry_cnt,
Which the reader expects to performs retries....
Also, this is becoming too wired -
>From your commit log I get that for specific Samsung devices,
You need to query wb descriptor fields/attributes/flags using selectore = 1.
But what it has to do with descriptor sizes?
> + /*
> + * Update WriteBooster related descriptor length with specific
> + * seletor used.
> + */
> + ufshcd_read_desc_length(hba, QUERY_DESC_IDN_DEVICE, 0,
> + &hba->desc_size.dev_desc);
> + ufshcd_read_desc_length(hba, QUERY_DESC_IDN_CONFIGURATION,
> 0,
> + &hba->desc_size.conf_desc);
> + ufshcd_read_desc_length(hba, QUERY_DESC_IDN_UNIT, 0,
> + &hba->desc_size.unit_desc);
> + ufshcd_read_desc_length(hba, QUERY_DESC_IDN_GEOMETRY, 0,
> + &hba->desc_size.geom_desc);
> + /*
> + * Read device descriptor again with specific selector used to
> + * get WriteBooster related fileds.
> + */
> + kfree(desc_buf);
> + retry_cnt++;
> + goto retry;
> + }
> +
> /*
> * Probe WB only for UFS-3.1 devices or UFS devices with quirk
> * UFS_DEVICE_QUIRK_SUPPORT_EXTENDED_FEATURES enabled
> diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
> index bf97d616e597..d850c47e8ae0 100644
> --- a/drivers/scsi/ufs/ufshcd.h
> +++ b/drivers/scsi/ufs/ufshcd.h
> @@ -876,6 +876,26 @@ static inline u8 ufshcd_wb_get_query_index(struct
> ufs_hba *hba)
> return 0;
> }
>
> +static inline bool ufshcd_is_wb_attrs(enum attr_idn idn)
> +{
> + return ((idn >= QUERY_ATTR_IDN_WB_FLUSH_STATUS) &&
> + (idn <= QUERY_ATTR_IDN_CURR_WB_BUFF_SIZE));
> +}
> +
> +static inline bool ufshcd_is_wb_desc(enum desc_idn idn, u8 index)
> +{
> + return (idn <= QUERY_DESC_IDN_CONFIGURATION) ||
> + ((idn == QUERY_DESC_IDN_UNIT) &&
> + (index != UFS_UPIU_RPMB_QUERY_INDEX)) ||
> + (idn == QUERY_DESC_IDN_GEOMETRY);
> +}
> +
> +static inline bool ufshcd_is_wb_flags(enum flag_idn idn)
> +{
> + return ((idn >= QUERY_FLAG_IDN_WB_EN) &&
> + (idn <= QUERY_FLAG_IDN_WB_BUFF_FLUSH_DURING_HIBERN8));
> +}
> +
> extern int ufshcd_runtime_suspend(struct ufs_hba *hba);
> extern int ufshcd_runtime_resume(struct ufs_hba *hba);
> extern int ufshcd_runtime_idle(struct ufs_hba *hba);
> --
> 2.18.0
Powered by blists - more mailing lists