v3--v4:
Add "Reviewed-by" to patch's commits
Use kzalloc instead of kmalloc in drivers/ufs/core/ufshcd.c - patch 2/4
v2--v3:
Based on Bean's comments:
1)Use kzalloc instead of kmalloc in ufshcd_set_active_icc_lvl - patch 2/4
2)Delete UFS_RPMB_UNIT definition - patch 2/4
3)Delete len description - patch 3/4
v1--v2:
Fix argument warning in ufshpb.c
Read any descriptor with a maximum size of QUERY_DESC_MAX_SIZE.
According to the spec the device rerurns the actual size.
Thus can improve code readability and save CPU cycles.
While at it, cleanup few leftovers around the descriptor size parameter.
Suggested-by: Bean Huo <[email protected]>
Arthur Simchaev (4):
ufs:core: Remove redundant wb check
ufs:core: Remove redundant desc_size variable from hba
ufs: core: Remove len parameter from ufshcd_set_active_icc_lvl
ufs: core: Remove ufshcd_map_desc_id_to_length function
drivers/ufs/core/ufs_bsg.c | 7 +--
drivers/ufs/core/ufshcd-priv.h | 3 --
drivers/ufs/core/ufshcd.c | 100 ++++++++++-------------------------------
drivers/ufs/core/ufshpb.c | 5 +--
include/ufs/ufshcd.h | 1 -
5 files changed, 26 insertions(+), 90 deletions(-)
--
2.7.4
We used to use the extended-feature field in the device descriptor,
as an indication that the device supports ufs2.2 or later.
Remove that as this check is specifically done few lines above.
Reviewed-by: Bean Huo <[email protected]>
Signed-off-by: Arthur Simchaev <[email protected]>
---
drivers/ufs/core/ufshcd.c | 4 ----
1 file changed, 4 deletions(-)
diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index 2dbe249..2e47c69 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -7608,10 +7608,6 @@ static void ufshcd_wb_probe(struct ufs_hba *hba, const u8 *desc_buf)
(hba->dev_quirks & UFS_DEVICE_QUIRK_SUPPORT_EXTENDED_FEATURES)))
goto wb_disabled;
- if (hba->desc_size[QUERY_DESC_IDN_DEVICE] <
- DEVICE_DESC_PARAM_EXT_UFS_FEATURE_SUP + 4)
- goto wb_disabled;
-
ext_ufs_feature = get_unaligned_be32(desc_buf +
DEVICE_DESC_PARAM_EXT_UFS_FEATURE_SUP);
--
2.7.4
len argument is not used anymore in ufshcd_set_active_icc_lvl function.
Reviewed-by: Bean Huo <[email protected]>
Signed-off-by: Arthur Simchaev <[email protected]>
---
drivers/ufs/core/ufshcd.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index 70e96b6..617c4e0 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -7415,12 +7415,11 @@ static u32 ufshcd_get_max_icc_level(int sup_curr_uA, u32 start_scan,
* In case regulators are not initialized we'll return 0
* @hba: per-adapter instance
* @desc_buf: power descriptor buffer to extract ICC levels from.
- * @len: length of desc_buff
*
* Returns calculated ICC level
*/
static u32 ufshcd_find_max_sup_active_icc_level(struct ufs_hba *hba,
- const u8 *desc_buf, int len)
+ const u8 *desc_buf)
{
u32 icc_level = 0;
@@ -7478,8 +7477,7 @@ static void ufshcd_set_active_icc_lvl(struct ufs_hba *hba)
goto out;
}
- icc_level = ufshcd_find_max_sup_active_icc_level(hba, desc_buf,
- QUERY_DESC_MAX_SIZE);
+ icc_level = ufshcd_find_max_sup_active_icc_level(hba, desc_buf);
dev_dbg(hba->dev, "%s: setting icc_level 0x%x", __func__, icc_level);
ret = ufshcd_query_attr_retry(hba, UPIU_QUERY_OPCODE_WRITE_ATTR,
--
2.7.4
Hi Martin,
Gentle reminder
Regards
Arthur
> -----Original Message-----
> From: Arthur Simchaev <[email protected]>
> Sent: Sunday, November 27, 2022 2:08 PM
> To: [email protected]
> Cc: [email protected]; [email protected]; linux-
> [email protected]; Arthur Simchaev <[email protected]>
> Subject: [PATCH v4 0/4] ufs: core: Always read the descriptors with max length
>
> v3--v4:
> Add "Reviewed-by" to patch's commits
> Use kzalloc instead of kmalloc in drivers/ufs/core/ufshcd.c - patch 2/4
>
> v2--v3:
> Based on Bean's comments:
> 1)Use kzalloc instead of kmalloc in ufshcd_set_active_icc_lvl - patch 2/4
> 2)Delete UFS_RPMB_UNIT definition - patch 2/4
> 3)Delete len description - patch 3/4
>
> v1--v2:
> Fix argument warning in ufshpb.c
>
> Read any descriptor with a maximum size of QUERY_DESC_MAX_SIZE.
> According to the spec the device rerurns the actual size.
> Thus can improve code readability and save CPU cycles.
> While at it, cleanup few leftovers around the descriptor size parameter.
>
> Suggested-by: Bean Huo <[email protected]>
>
> Arthur Simchaev (4):
> ufs:core: Remove redundant wb check
> ufs:core: Remove redundant desc_size variable from hba
> ufs: core: Remove len parameter from ufshcd_set_active_icc_lvl
> ufs: core: Remove ufshcd_map_desc_id_to_length function
>
> drivers/ufs/core/ufs_bsg.c | 7 +--
> drivers/ufs/core/ufshcd-priv.h | 3 --
> drivers/ufs/core/ufshcd.c | 100 ++++++++++-------------------------------
> drivers/ufs/core/ufshpb.c | 5 +--
> include/ufs/ufshcd.h | 1 -
> 5 files changed, 26 insertions(+), 90 deletions(-)
>
> --
> 2.7.4
Hi Arthur!
> Gentle reminder
We're just a few days away from release, not adding new code this late
in the cycle. I would also like to see an additional review given that
this is a core change.
--
Martin K. Petersen Oracle Linux Engineering
On 11/27/22 04:08, Arthur Simchaev wrote:
> We used to use the extended-feature field in the device descriptor,
> as an indication that the device supports ufs2.2 or later.
> Remove that as this check is specifically done few lines above.
>
> Reviewed-by: Bean Huo <[email protected]>
> Signed-off-by: Arthur Simchaev <[email protected]>
> ---
> drivers/ufs/core/ufshcd.c | 4 ----
> 1 file changed, 4 deletions(-)
>
> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> index 2dbe249..2e47c69 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c
> @@ -7608,10 +7608,6 @@ static void ufshcd_wb_probe(struct ufs_hba *hba, const u8 *desc_buf)
> (hba->dev_quirks & UFS_DEVICE_QUIRK_SUPPORT_EXTENDED_FEATURES)))
> goto wb_disabled;
>
> - if (hba->desc_size[QUERY_DESC_IDN_DEVICE] <
> - DEVICE_DESC_PARAM_EXT_UFS_FEATURE_SUP + 4)
> - goto wb_disabled;
> -
> ext_ufs_feature = get_unaligned_be32(desc_buf +
> DEVICE_DESC_PARAM_EXT_UFS_FEATURE_SUP);
Does this code really have to be removed? I see a check of the
UFS_DEVICE_QUIRK_SUPPORT_EXTENDED_FEATURES flag above the removed
code but no check of the descriptor size?
Thanks,
Bart.
On 11/27/22 04:08, Arthur Simchaev wrote:
> len argument is not used anymore in ufshcd_set_active_icc_lvl function.
Reviewed-by: Bart Van Assche <[email protected]>
On 08.12.22 12:31 AM, Bart Van Assche wrote:
> On 11/27/22 04:08, Arthur Simchaev wrote:
>> We used to use the extended-feature field in the device descriptor,
>> as an indication that the device supports ufs2.2 or later.
>> Remove that as this check is specifically done few lines above.
>>
>> Reviewed-by: Bean Huo <[email protected]>
>> Signed-off-by: Arthur Simchaev <[email protected]>
>> ---
>> drivers/ufs/core/ufshcd.c | 4 ----
>> 1 file changed, 4 deletions(-)
>>
>> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
>> index 2dbe249..2e47c69 100644
>> --- a/drivers/ufs/core/ufshcd.c
>> +++ b/drivers/ufs/core/ufshcd.c
>> @@ -7608,10 +7608,6 @@ static void ufshcd_wb_probe(struct ufs_hba
>> *hba, const u8 *desc_buf)
>> (hba->dev_quirks &
>> UFS_DEVICE_QUIRK_SUPPORT_EXTENDED_FEATURES)))
>> goto wb_disabled;
>> - if (hba->desc_size[QUERY_DESC_IDN_DEVICE] <
>> - DEVICE_DESC_PARAM_EXT_UFS_FEATURE_SUP + 4)
>> - goto wb_disabled;
>> -
>> ext_ufs_feature = get_unaligned_be32(desc_buf +
>> DEVICE_DESC_PARAM_EXT_UFS_FEATURE_SUP);
>
> Does this code really have to be removed? I see a check of the
> UFS_DEVICE_QUIRK_SUPPORT_EXTENDED_FEATURES flag above the removed
> code but no check of the descriptor size?
>
it is not necessary to check this, but if you have concern, we could
change to like this:
if (desc_buf[DEVICE_DESC_PARAM_LEN] <
DEVICE_DESC_PARAM_EXT_UFS_FEATURE_SUP + 4)
goto wb_disabled;
then hba->desc_size could be removed.
Kind regards,
Bean
On 11/27/22 04:08, Arthur Simchaev wrote:
> We used to use the extended-feature field in the device descriptor,
> as an indication that the device supports ufs2.2 or later.
> Remove that as this check is specifically done few lines above.
Reviewed-by: Bart Van Assche <[email protected]>
On 12/8/22 04:22, Bean Huo wrote:
>
> On 08.12.22 12:31 AM, Bart Van Assche wrote:
>> On 11/27/22 04:08, Arthur Simchaev wrote:
>>> We used to use the extended-feature field in the device descriptor,
>>> as an indication that the device supports ufs2.2 or later.
>>> Remove that as this check is specifically done few lines above.
>>>
>>> Reviewed-by: Bean Huo <[email protected]>
>>> Signed-off-by: Arthur Simchaev <[email protected]>
>>> ---
>>> drivers/ufs/core/ufshcd.c | 4 ----
>>> 1 file changed, 4 deletions(-)
>>>
>>> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
>>> index 2dbe249..2e47c69 100644
>>> --- a/drivers/ufs/core/ufshcd.c
>>> +++ b/drivers/ufs/core/ufshcd.c
>>> @@ -7608,10 +7608,6 @@ static void ufshcd_wb_probe(struct ufs_hba
>>> *hba, const u8 *desc_buf)
>>> (hba->dev_quirks &
>>> UFS_DEVICE_QUIRK_SUPPORT_EXTENDED_FEATURES)))
>>> goto wb_disabled;
>>> - if (hba->desc_size[QUERY_DESC_IDN_DEVICE] <
>>> - DEVICE_DESC_PARAM_EXT_UFS_FEATURE_SUP + 4)
>>> - goto wb_disabled;
>>> -
>>> ext_ufs_feature = get_unaligned_be32(desc_buf +
>>> DEVICE_DESC_PARAM_EXT_UFS_FEATURE_SUP);
>>
>> Does this code really have to be removed? I see a check of the
>> UFS_DEVICE_QUIRK_SUPPORT_EXTENDED_FEATURES flag above the removed
>> code but no check of the descriptor size?
>>
> it is not necessary to check this, but if you have concern, we could
> change to like this:
>
>
> if (desc_buf[DEVICE_DESC_PARAM_LEN] <
> DEVICE_DESC_PARAM_EXT_UFS_FEATURE_SUP + 4)
> goto wb_disabled;
>
> then hba->desc_size could be removed.
Hi Bean,
My only concern is that this patch conflicts with the pending MCQ patch
series. Since that conflict is unavoidable, let's keep this patch.
Bart.