| 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
| ||
|
Date: Wed, 22 Feb 2017 15:23:03 -0800
From: Subhash Jadavani <subhashj@...eaurora.org>
To: "Potomski, MichalX" <michalx.potomski@...el.com>
Cc: "'vinholikatti@...il.com'" <vinholikatti@...il.com>,
jejb@...ux.vnet.ibm.com, martin.petersen@...cle.com,
linux-scsi@...r.kernel.org, linux-kernel@...r.kernel.org,
"Janca, Grzegorz" <grzegorz.janca@...el.com>,
"Mielczarek, SzymonX" <szymonx.mielczarek@...el.com>,
linux-scsi-owner@...r.kernel.org
Subject: Re: [PATCH] scsi: ufs: Factor out ufshcd_read_desc_param
On 2017-02-20 23:36, Potomski, MichalX wrote:
> Since in UFS 2.1 specification some of the descriptor
> lengths differs from 2.0 specification and some devices,
> which are reporting spec version 2.0 have different
> descriptor lengths we can not rely on hardcoded values
> taken from 2.0 specification. This patch introduces
> reading these lengths per each device from descriptor
> headers at probe time to ensure their correctness.
>
> Signed-off-by: Michal' Potomski <michalx.potomski@...el.com>
> ---
> drivers/scsi/ufs/ufs.h | 22 ++---
> drivers/scsi/ufs/ufshcd.c | 230
> ++++++++++++++++++++++++++++++++++------------
> drivers/scsi/ufs/ufshcd.h | 15 +++
> 3 files changed, 195 insertions(+), 72 deletions(-)
>
> diff --git a/drivers/scsi/ufs/ufs.h b/drivers/scsi/ufs/ufs.h
> index 8e6709a..9e1b1a8 100644
> --- a/drivers/scsi/ufs/ufs.h
> +++ b/drivers/scsi/ufs/ufs.h
> @@ -146,7 +146,7 @@ enum attr_idn {
> /* Descriptor idn for Query requests */
> enum desc_idn {
> QUERY_DESC_IDN_DEVICE = 0x0,
> - QUERY_DESC_IDN_CONFIGURAION = 0x1,
> + QUERY_DESC_IDN_CONFIGURATION = 0x1,
> QUERY_DESC_IDN_UNIT = 0x2,
> QUERY_DESC_IDN_RFU_0 = 0x3,
> QUERY_DESC_IDN_INTERCONNECT = 0x4,
> @@ -162,19 +162,13 @@ enum desc_header_offset {
> QUERY_DESC_DESC_TYPE_OFFSET = 0x01,
> };
>
> -enum ufs_desc_max_size {
> - QUERY_DESC_DEVICE_MAX_SIZE = 0x40,
> - QUERY_DESC_CONFIGURAION_MAX_SIZE = 0x90,
> - QUERY_DESC_UNIT_MAX_SIZE = 0x23,
> - QUERY_DESC_INTERCONNECT_MAX_SIZE = 0x06,
> - /*
> - * Max. 126 UNICODE characters (2 bytes per character) plus 2 bytes
> - * of descriptor header.
> - */
> - QUERY_DESC_STRING_MAX_SIZE = 0xFE,
> - QUERY_DESC_GEOMETRY_MAX_SIZE = 0x44,
> - QUERY_DESC_POWER_MAX_SIZE = 0x62,
> - QUERY_DESC_RFU_MAX_SIZE = 0x00,
> +enum ufs_desc_def_size {
> + QUERY_DESC_DEVICE_DEF_SIZE = 0x40,
> + QUERY_DESC_CONFIGURATION_DEF_SIZE = 0x90,
> + QUERY_DESC_UNIT_DEF_SIZE = 0x23,
> + QUERY_DESC_INTERCONNECT_DEF_SIZE = 0x06,
> + QUERY_DESC_GEOMETRY_DEF_SIZE = 0x44,
> + QUERY_DESC_POWER_DEF_SIZE = 0x62,
> };
>
> /* Unit descriptor parameters offsets in bytes*/
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index 20e5e5f..79d5055 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -94,19 +94,6 @@
> _ret; \
> })
>
> -static u32 ufs_query_desc_max_size[] = {
> - QUERY_DESC_DEVICE_MAX_SIZE,
> - QUERY_DESC_CONFIGURAION_MAX_SIZE,
> - QUERY_DESC_UNIT_MAX_SIZE,
> - QUERY_DESC_RFU_MAX_SIZE,
> - QUERY_DESC_INTERCONNECT_MAX_SIZE,
> - QUERY_DESC_STRING_MAX_SIZE,
> - QUERY_DESC_RFU_MAX_SIZE,
> - QUERY_DESC_GEOMETRY_MAX_SIZE,
> - QUERY_DESC_POWER_MAX_SIZE,
> - QUERY_DESC_RFU_MAX_SIZE,
> -};
> -
> enum {
> UFSHCD_MAX_CHANNEL = 0,
> UFSHCD_MAX_ID = 1,
> @@ -2012,7 +1999,7 @@ static int __ufshcd_query_descriptor(struct
> ufs_hba *hba,
> goto out;
> }
>
> - if (*buf_len <= QUERY_DESC_MIN_SIZE || *buf_len >
> QUERY_DESC_MAX_SIZE) {
> + if (*buf_len < QUERY_DESC_MIN_SIZE || *buf_len > QUERY_DESC_MAX_SIZE)
> {
> dev_err(hba->dev, "%s: descriptor buffer size (%d) is out of
> range\n",
> __func__, *buf_len);
> err = -EINVAL;
> @@ -2092,6 +2079,92 @@ int ufshcd_query_descriptor_retry(struct ufs_hba
> *hba,
> EXPORT_SYMBOL(ufshcd_query_descriptor_retry);
>
> /**
> + * ufshcd_read_desc_length - read the specified descriptor length from
> header
> + * @hba: Pointer to adapter instance
> + * @desc_id: descriptor idn value
> + * @desc_index: descriptor index
> + * @desc_length: pointer to variable to read the length of descriptor
> + *
> + * Return 0 in case of success, non-zero otherwise
> + */
> +static int ufshcd_read_desc_length(struct ufs_hba *hba,
> + enum desc_idn desc_id,
> + int desc_index,
> + int *desc_length)
> +{
> + int ret;
> + u8 header[QUERY_DESC_HDR_SIZE];
> + int header_len = QUERY_DESC_HDR_SIZE;
> +
> + if (desc_id >= QUERY_DESC_IDN_MAX)
> + return -EINVAL;
> +
> + ret = ufshcd_query_descriptor_retry(hba, UPIU_QUERY_OPCODE_READ_DESC,
> + desc_id, desc_index, 0, header,
> + &header_len);
> +
> + if (ret) {
> + dev_err(hba->dev, "%s: Failed to get descriptor header id %d",
> + __func__, desc_id);
> + return ret;
> + } else if (desc_id != header[QUERY_DESC_DESC_TYPE_OFFSET]) {
> + dev_warn(hba->dev, "%s: descriptor header id %d and desc_id %d
> mismatch",
> + __func__, header[QUERY_DESC_DESC_TYPE_OFFSET],
> + desc_id);
> + ret = -EINVAL;
> + }
> +
> + *desc_length = header[QUERY_DESC_LENGTH_OFFSET];
> + return ret;
> +
> +}
> +
> +/**
> + * ufshcd_map_desc_id_to_length - map descriptor IDN to its length
> + * @hba: Pointer to adapter instance
> + * @desc_id: descriptor idn value
> + * @desc_len: mapped desc length (out)
> + *
> + * Return 0 in case of success, non-zero otherwise
> + */
> +int ufshcd_map_desc_id_to_length(struct ufs_hba *hba,
> + enum desc_idn desc_id, int *desc_len)
> +{
> + switch (desc_id) {
> + case QUERY_DESC_IDN_DEVICE:
> + *desc_len = hba->desc_size.dev_desc;
> + break;
> + case QUERY_DESC_IDN_POWER:
> + *desc_len = hba->desc_size.pwr_desc;
> + break;
> + case QUERY_DESC_IDN_GEOMETRY:
> + *desc_len = hba->desc_size.geom_desc;
> + break;
> + case QUERY_DESC_IDN_CONFIGURATION:
> + *desc_len = hba->desc_size.conf_desc;
> + break;
> + case QUERY_DESC_IDN_UNIT:
> + *desc_len = hba->desc_size.unit_desc;
> + break;
> + case QUERY_DESC_IDN_INTERCONNECT:
> + *desc_len = hba->desc_size.interc_desc;
> + break;
> + case QUERY_DESC_IDN_STRING:
> + *desc_len = QUERY_DESC_MAX_SIZE;
> + break;
> + case QUERY_DESC_IDN_RFU_0:
> + case QUERY_DESC_IDN_RFU_1:
> + *desc_len = 0;
> + break;
> + default:
> + *desc_len = 0;
> + return -EINVAL;
> + }
> + return 0;
> +}
> +EXPORT_SYMBOL(ufshcd_map_desc_id_to_length);
> +
> +/**
> * ufshcd_read_desc_param - read the specified descriptor parameter
> * @hba: Pointer to adapter instance
> * @desc_id: descriptor idn value
> @@ -2105,42 +2178,49 @@ int ufshcd_query_descriptor_retry(struct
> ufs_hba *hba,
> static int ufshcd_read_desc_param(struct ufs_hba *hba,
> enum desc_idn desc_id,
> int desc_index,
> - u32 param_offset,
> + u8 param_offset,
> u8 *param_read_buf,
> - u32 param_size)
> + u8 param_size)
> {
> int ret;
> u8 *desc_buf;
> - u32 buff_len;
> + int buff_len;
> bool is_kmalloc = true;
>
> - /* safety checks */
> - if (desc_id >= QUERY_DESC_IDN_MAX)
> + /* Safety check */
> + if (desc_id >= QUERY_DESC_IDN_MAX || !param_size)
> return -EINVAL;
>
> - buff_len = ufs_query_desc_max_size[desc_id];
> - if ((param_offset + param_size) > buff_len)
> - return -EINVAL;
> + /* Get the max length of descriptor from structure filled up at probe
> + * time.
> + */
> + ret = ufshcd_map_desc_id_to_length(hba, desc_id, &buff_len);
>
> - if (!param_offset && (param_size == buff_len)) {
> - /* memory space already available to hold full descriptor */
> - desc_buf = param_read_buf;
> - is_kmalloc = false;
> - } else {
> - /* allocate memory to hold full descriptor */
> + /* Sanity checks */
> + if (ret || !buff_len) {
> + dev_err(hba->dev, "%s: Failed to get full descriptor length",
> + __func__);
> + return ret;
> + }
> +
> + /* Check whether we need temp memory */
> + if (param_offset != 0 || param_size < buff_len) {
> desc_buf = kmalloc(buff_len, GFP_KERNEL);
> if (!desc_buf)
> return -ENOMEM;
> + } else {
> + desc_buf = param_read_buf;
> + is_kmalloc = false;
> }
>
> + /* Request for full descriptor */
> ret = ufshcd_query_descriptor_retry(hba, UPIU_QUERY_OPCODE_READ_DESC,
> - desc_id, desc_index, 0, desc_buf,
> - &buff_len);
> + desc_id, desc_index, 0,
> + desc_buf, &buff_len);
>
> if (ret) {
> dev_err(hba->dev, "%s: Failed reading descriptor. desc_id %d,
> desc_index %d, param_offset %d, ret %d",
> __func__, desc_id, desc_index, param_offset, ret);
> -
> goto out;
> }
>
> @@ -2152,25 +2232,9 @@ static int ufshcd_read_desc_param(struct ufs_hba
> *hba,
> goto out;
> }
>
> - /*
> - * While reading variable size descriptors (like string descriptor),
> - * some UFS devices may report the "LENGTH" (field in "Transaction
> - * Specific fields" of Query Response UPIU) same as what was
> requested
> - * in Query Request UPIU instead of reporting the actual size of the
> - * variable size descriptor.
> - * Although it's safe to ignore the "LENGTH" field for variable size
> - * descriptors as we can always derive the length of the descriptor
> from
> - * the descriptor header fields. Hence this change impose the length
> - * match check only for fixed size descriptors (for which we always
> - * request the correct size as part of Query Request UPIU).
> - */
> - if ((desc_id != QUERY_DESC_IDN_STRING) &&
> - (buff_len != desc_buf[QUERY_DESC_LENGTH_OFFSET])) {
> - dev_err(hba->dev, "%s: desc_buf length mismatch: buff_len %d,
> buff_len(desc_header) %d",
> - __func__, buff_len, desc_buf[QUERY_DESC_LENGTH_OFFSET]);
> - ret = -EINVAL;
> - goto out;
> - }
> + /* Check wherher we will not copy more data, than available */
> + if (is_kmalloc && param_size > buff_len)
> + param_size = buff_len;
>
> if (is_kmalloc)
> memcpy(param_read_buf, &desc_buf[param_offset], param_size);
> @@ -4915,8 +4979,8 @@ static int ufshcd_set_icc_levels_attr(struct
> ufs_hba *hba, u32 icc_level)
> static void ufshcd_init_icc_levels(struct ufs_hba *hba)
> {
> int ret;
> - int buff_len = QUERY_DESC_POWER_MAX_SIZE;
> - u8 desc_buf[QUERY_DESC_POWER_MAX_SIZE];
> + int buff_len = hba->desc_size.pwr_desc;
> + u8 desc_buf[buff_len];
>
> ret = ufshcd_read_power_desc(hba, desc_buf, buff_len);
> if (ret) {
> @@ -5013,11 +5077,10 @@ static int ufs_get_device_info(struct ufs_hba
> *hba,
> {
> int err;
> u8 model_index;
> - u8 str_desc_buf[QUERY_DESC_STRING_MAX_SIZE + 1] = {0};
> - u8 desc_buf[QUERY_DESC_DEVICE_MAX_SIZE];
> + u8 str_desc_buf[QUERY_DESC_MAX_SIZE + 1] = {0};
> + u8 desc_buf[hba->desc_size.dev_desc];
>
> - err = ufshcd_read_device_desc(hba, desc_buf,
> - QUERY_DESC_DEVICE_MAX_SIZE);
> + err = ufshcd_read_device_desc(hba, desc_buf,
> hba->desc_size.dev_desc);
> if (err) {
> dev_err(hba->dev, "%s: Failed reading Device Desc. err = %d\n",
> __func__, err);
> @@ -5034,14 +5097,14 @@ static int ufs_get_device_info(struct ufs_hba
> *hba,
> model_index = desc_buf[DEVICE_DESC_PARAM_PRDCT_NAME];
>
> err = ufshcd_read_string_desc(hba, model_index, str_desc_buf,
> - QUERY_DESC_STRING_MAX_SIZE, ASCII_STD);
> + QUERY_DESC_MAX_SIZE, ASCII_STD);
> if (err) {
> dev_err(hba->dev, "%s: Failed reading Product Name. err = %d\n",
> __func__, err);
> goto out;
> }
>
> - str_desc_buf[QUERY_DESC_STRING_MAX_SIZE] = '\0';
> + str_desc_buf[QUERY_DESC_MAX_SIZE] = '\0';
> strlcpy(card_data->model, (str_desc_buf + QUERY_DESC_HDR_SIZE),
> min_t(u8, str_desc_buf[QUERY_DESC_LENGTH_OFFSET],
> MAX_MODEL_LEN));
> @@ -5241,6 +5304,50 @@ static void ufshcd_tune_unipro_params(struct
> ufs_hba *hba)
> ufshcd_vops_apply_dev_quirks(hba);
> }
>
> +static void ufshcd_init_desc_sizes(struct ufs_hba *hba)
> +{
> + int err;
> +
> + err = ufshcd_read_desc_length(hba, QUERY_DESC_IDN_DEVICE, 0,
> + &hba->desc_size.dev_desc);
> + if (err)
> + hba->desc_size.dev_desc = QUERY_DESC_DEVICE_DEF_SIZE;
> +
> + err = ufshcd_read_desc_length(hba, QUERY_DESC_IDN_POWER, 0,
> + &hba->desc_size.pwr_desc);
> + if (err)
> + hba->desc_size.pwr_desc = QUERY_DESC_POWER_DEF_SIZE;
> +
> + err = ufshcd_read_desc_length(hba, QUERY_DESC_IDN_INTERCONNECT, 0,
> + &hba->desc_size.interc_desc);
> + if (err)
> + hba->desc_size.interc_desc = QUERY_DESC_INTERCONNECT_DEF_SIZE;
> +
> + err = ufshcd_read_desc_length(hba, QUERY_DESC_IDN_CONFIGURATION, 0,
> + &hba->desc_size.conf_desc);
> + if (err)
> + hba->desc_size.conf_desc = QUERY_DESC_CONFIGURATION_DEF_SIZE;
> +
> + err = ufshcd_read_desc_length(hba, QUERY_DESC_IDN_UNIT, 0,
> + &hba->desc_size.unit_desc);
> + if (err)
> + hba->desc_size.unit_desc = QUERY_DESC_UNIT_DEF_SIZE;
> +
> + err = ufshcd_read_desc_length(hba, QUERY_DESC_IDN_GEOMETRY, 0,
> + &hba->desc_size.geom_desc);
> + if (err)
> + hba->desc_size.geom_desc = QUERY_DESC_GEOMETRY_DEF_SIZE;
> +}
> +
> +static void ufshcd_def_desc_sizes(struct ufs_hba *hba)
> +{
> + hba->desc_size.dev_desc = QUERY_DESC_DEVICE_DEF_SIZE;
> + hba->desc_size.pwr_desc = QUERY_DESC_POWER_DEF_SIZE;
> + hba->desc_size.interc_desc = QUERY_DESC_INTERCONNECT_DEF_SIZE;
> + hba->desc_size.conf_desc = QUERY_DESC_CONFIGURATION_DEF_SIZE;
> + hba->desc_size.unit_desc = QUERY_DESC_UNIT_DEF_SIZE;
> + hba->desc_size.geom_desc = QUERY_DESC_GEOMETRY_DEF_SIZE;
> +}
> /**
> * ufshcd_probe_hba - probe hba to detect device and initialize
> * @hba: per-adapter instance
> @@ -5272,6 +5379,9 @@ static int ufshcd_probe_hba(struct ufs_hba *hba)
> if (ret)
> goto out;
>
> + /* Init check for device descriptor sizes */
> + ufshcd_init_desc_sizes(hba);
> +
> ufs_advertise_fixup_device(hba);
> ufshcd_tune_unipro_params(hba);
>
> @@ -5300,6 +5410,7 @@ static int ufshcd_probe_hba(struct ufs_hba *hba)
>
> /* set the state as operational after switching to desired gear */
> hba->ufshcd_state = UFSHCD_STATE_OPERATIONAL;
> +
> /*
> * If we are in error handling context or in power management
> callbacks
> * context, no need to scan the host
> @@ -6697,6 +6808,9 @@ int ufshcd_init(struct ufs_hba *hba, void
> __iomem *mmio_base, unsigned int irq)
> hba->mmio_base = mmio_base;
> hba->irq = irq;
>
> + /* Set descriptor lengths to specification defaults */
> + ufshcd_def_desc_sizes(hba);
> +
> err = ufshcd_hba_init(hba);
> if (err)
> goto out_error;
> diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
> index 08cd26e..308675a 100644
> --- a/drivers/scsi/ufs/ufshcd.h
> +++ b/drivers/scsi/ufs/ufshcd.h
> @@ -205,6 +205,15 @@ struct ufs_dev_cmd {
> struct ufs_query query;
> };
>
> +struct ufs_desc_size {
> + int dev_desc;
> + int pwr_desc;
> + int geom_desc;
> + int interc_desc;
> + int unit_desc;
> + int conf_desc;
> +};
> +
> /**
> * struct ufs_clk_info - UFS clock related info
> * @list: list headed by hba->clk_list_head
> @@ -398,6 +407,7 @@ struct ufs_init_prefetch {
> * @clk_list_head: UFS host controller clocks list node head
> * @pwr_info: holds current power mode
> * @max_pwr_info: keeps the device max valid pwm
> + * @desc_size: descriptor sizes reported by device
> * @urgent_bkops_lvl: keeps track of urgent bkops level for device
> * @is_urgent_bkops_lvl_checked: keeps track if the urgent bkops level
> for
> * device is known or not.
> @@ -565,6 +575,7 @@ struct ufs_hba {
>
> enum bkops_status urgent_bkops_lvl;
> bool is_urgent_bkops_lvl_checked;
> + struct ufs_desc_size desc_size;
> };
>
> /* Returns true if clocks can be gated. Otherwise false */
> @@ -733,6 +744,10 @@ int ufshcd_query_flag(struct ufs_hba *hba, enum
> query_opcode opcode,
> enum flag_idn idn, bool *flag_res);
> int ufshcd_hold(struct ufs_hba *hba, bool async);
> void ufshcd_release(struct ufs_hba *hba);
> +
> +int ufshcd_map_desc_id_to_length(struct ufs_hba *hba, enum desc_idn
> desc_id,
> + int *desc_length);
> +
> u32 ufshcd_get_local_unipro_ver(struct ufs_hba *hba);
>
> /* Wrapper functions for safely calling variant operations */
Looks good to me.
Reviewed-by: Subhash Jadavani <subhashj@...eaurora.org>
--
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
Powered by blists - more mailing lists