RE: [EXT] [PATCH v1 1/2] scsi: ufs: Fix unbalanced scsi_block_reqs_cnt caused by ufshcd_hold()

[Bean Huo (beanhuo)]

> 
> Signed-off-by: Can Guo 
> Reviewed-by: Hongwu Su 

Reviewed-by: Bean Huo 

RE: [PATCH] scsi: ufs: Fix unexpected values get from ufshcd_read_desc_param()

[Bean Huo (beanhuo)]

Can,

> Since WB feature has been added, WB related sysfs entries can be accessed
> even when an UFS device does not support WB feature. In that case, the
> descriptors which are not supported by the UFS device may be wrongly reported
> when they are accessed from their corrsponding sysfs entries.
> Fix it by adding a sanity check of parameter offset against the actual 
> decriptor
> length.
> 
> Signed-off-by: Can Guo 
> 
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c index
> a2ebcc8..8861ad6 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -3184,13 +3184,19 @@ int ufshcd_read_desc_param(struct ufs_hba *hba,
>   /* Get the length of descriptor */
>   ufshcd_map_desc_id_to_length(hba, desc_id, _len);
>   if (!buff_len) {
> - dev_err(hba->dev, "%s: Failed to get desc length", __func__);
> + dev_err(hba->dev, "%s: Failed to get desc length\n", __func__);
> + return -EINVAL;
> + }
> +
> + if (param_offset >= buff_len)
> + dev_err(hba->dev, "%s: Invalid offset 0x%x in descriptor IDN
> 0x%x, length 0x%x\n",
> + __func__, param_offset, desc_id, buff_len);
>   return -EINVAL;
>   }

A brace missed!  This right brace misses a left brace.

Bean

RE: [EXT] [PATCH v2] scsi: ufs: Disable WriteBooster capability in non-supported UFS device

[Bean Huo (beanhuo)]

> 
> If UFS device is not qualified to enter the detection of WriteBooster probing 
> by
> disallowed UFS version or device quirks, then WriteBooster capability in host
> shall be disabled to prevent any WriteBooster operations in the future.
> 
> Fixes: 3d17b9b5ab11 ("scsi: ufs: Add write booster feature support")
> Signed-off-by: Stanley Chu 
> Tested-by: Steev Klimaszewski 
> Reviewed-by: Avri Altman 

Thanks fix this.

Reviewed-by: Bean Huo 

Bean

> ---
>  drivers/scsi/ufs/ufshcd.c | 35 +++
>  1 file changed, 19 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c index
> f173ad1bd79f..c62bd47beeaa 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -6847,21 +6847,31 @@ static int ufshcd_scsi_add_wlus(struct ufs_hba
> *hba)
> 
>  static void ufshcd_wb_probe(struct ufs_hba *hba, u8 *desc_buf)  {
> + struct ufs_dev_info *dev_info = >dev_info;
>   u8 lun;
>   u32 d_lu_wb_buf_alloc;
> 
>   if (!ufshcd_is_wb_allowed(hba))
>   return;
> + /*
> +  * Probe WB only for UFS-2.2 and UFS-3.1 (and later) devices or
> +  * UFS devices with quirk
> UFS_DEVICE_QUIRK_SUPPORT_EXTENDED_FEATURES
> +  * enabled
> +  */
> + if (!(dev_info->wspecversion >= 0x310 ||
> +   dev_info->wspecversion == 0x220 ||
> +  (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;
> 
> - hba->dev_info.d_ext_ufs_feature_sup =
> + dev_info->d_ext_ufs_feature_sup =
>   get_unaligned_be32(desc_buf +
> 
> DEVICE_DESC_PARAM_EXT_UFS_FEATURE_SUP);
> 
> - if (!(hba->dev_info.d_ext_ufs_feature_sup &
> UFS_DEV_WRITE_BOOSTER_SUP))
> + if (!(dev_info->d_ext_ufs_feature_sup &
> UFS_DEV_WRITE_BOOSTER_SUP))
>   goto wb_disabled;
> 
>   /*
> @@ -6870,17 +6880,17 @@ static void ufshcd_wb_probe(struct ufs_hba *hba,
> u8 *desc_buf)
>* a max of 1 lun would have wb buffer configured.
>* Now only shared buffer mode is supported.
>*/
> - hba->dev_info.b_wb_buffer_type =
> + dev_info->b_wb_buffer_type =
>   desc_buf[DEVICE_DESC_PARAM_WB_TYPE];
> 
> - hba->dev_info.b_presrv_uspc_en =
> + dev_info->b_presrv_uspc_en =
>   desc_buf[DEVICE_DESC_PARAM_WB_PRESRV_USRSPC_EN];
> 
> - if (hba->dev_info.b_wb_buffer_type == WB_BUF_MODE_SHARED) {
> - hba->dev_info.d_wb_alloc_units =
> + if (dev_info->b_wb_buffer_type == WB_BUF_MODE_SHARED) {
> + dev_info->d_wb_alloc_units =
>   get_unaligned_be32(desc_buf +
> 
> DEVICE_DESC_PARAM_WB_SHARED_ALLOC_UNITS);
> - if (!hba->dev_info.d_wb_alloc_units)
> + if (!dev_info->d_wb_alloc_units)
>   goto wb_disabled;
>   } else {
>   for (lun = 0; lun < UFS_UPIU_MAX_WB_LUN_ID; lun++) { @@ -
> 6891,7 +6901,7 @@ static void ufshcd_wb_probe(struct ufs_hba *hba, u8
> *desc_buf)
>   (u8 *)_lu_wb_buf_alloc,
>   sizeof(d_lu_wb_buf_alloc));
>   if (d_lu_wb_buf_alloc) {
> - hba->dev_info.wb_dedicated_lu = lun;
> + dev_info->wb_dedicated_lu = lun;
>   break;
>   }
>   }
> @@ -6977,14 +6987,7 @@ static int ufs_get_device_desc(struct ufs_hba *hba)
> 
>   ufs_fixup_device_setup(hba);
> 
> - /*
> -  * Probe WB only for UFS-3.1 devices or UFS devices with quirk
> -  * UFS_DEVICE_QUIRK_SUPPORT_EXTENDED_FEATURES enabled
> -  */
> - if (dev_info->wspecversion >= 0x310 ||
> - dev_info->wspecversion == 0x220 ||
> - (hba->dev_quirks &
> UFS_DEVICE_QUIRK_SUPPORT_EXTENDED_FEATURES))
> - ufshcd_wb_probe(hba, desc_buf);
> + ufshcd_wb_probe(hba, desc_buf);
> 
>   /*
>* ufshcd_read_string_desc returns size of the string
> --
> 2.18.0

RE: RE: [EXT] [PATCH] scsi: ufs-bsg: Fix runtime PM imbalance on error

[Bean Huo (beanhuo)]

Hi, Dinghao

> Thank you for your advice! Moving original pm_runtime_put_sync() to after
> "out" label will influence an error path branched from
> ups_bsg_verify_query_size(). So I think changing "goto out" to "break" is a 
> good
> idea. But in this case we may execute an extra
> sg_copy_from_buffer() and an extra kfree() compared with unpatched version.
> Does this matter?
> 
What do you mean " unpatched version "? 

I see, below goto will bypass sg_copy_from_buffer() and an extra kfree()
In case ufs_bsg_alloc_desc_buffer() fails. 

Bean

RE: [EXT] [PATCH] scsi: ufs-bsg: Fix runtime PM imbalance on error

[Bean Huo (beanhuo)]

>  1 file changed, 3 insertions(+), 1 deletion(-)
Hi, Dinghao
> 
> diff --git a/drivers/scsi/ufs/ufs_bsg.c b/drivers/scsi/ufs/ufs_bsg.c index
> 53dd87628cbe..516a7f573942 100644
> --- a/drivers/scsi/ufs/ufs_bsg.c
> +++ b/drivers/scsi/ufs/ufs_bsg.c
> @@ -106,8 +106,10 @@ static int ufs_bsg_request(struct bsg_job *job)
>   desc_op = bsg_request->upiu_req.qr.opcode;
>   ret = ufs_bsg_alloc_desc_buffer(hba, job, _buff,
>   _len, desc_op);
> - if (ret)
> + if (ret) {
> + pm_runtime_put_sync(hba->dev);

No  need to add pm_runtime_put_sync() here, you can change "goto out" to 
"break",
Or move original pm_runtime_put_sync() to after goto label.

>   goto out;
> + }
> 
>   /* fall through */
>   case UPIU_TRANSACTION_NOP_OUT:
> --
> 2.17.1

RE: [EXT] [PATCH v6 6/8] scsi: ufs: add LU Dedicated buffer mode support for WriteBooster

[Bean Huo (beanhuo)]

> From: Stanley Chu 
> Sent: Monday, May 4, 2020 4:56 PM
> To: linux-s...@vger.kernel.org; martin.peter...@oracle.com;
> avri.alt...@wdc.com; alim.akh...@samsung.com; j...@linux.ibm.com;
> asuto...@codeaurora.org
> Cc: Bean Huo (beanhuo) ; c...@codeaurora.org;
> matthias@gmail.com; bvanass...@acm.org; linux-
> media...@lists.infradead.org; linux-arm-ker...@lists.infradead.org; linux-
> ker...@vger.kernel.org; kuohong.w...@mediatek.com;
> peter.w...@mediatek.com; chun-hung...@mediatek.com;
> andy.t...@mediatek.com; Stanley Chu 
> Subject: [EXT] [PATCH v6 6/8] scsi: ufs: add LU Dedicated buffer mode support
> for WriteBooster
> 
> According to UFS specification, there are two WriteBooster mode of
> operations: "LU dedicated buffer" mode and "shared buffer" mode.
> In the "LU dedicated buffer" mode, the WriteBooster Buffer is dedicated to a
> logical unit.
> 
> If the device supports the "LU dedicated buffer" mode, this mode is configured
> by setting bWriteBoosterBufferType to 00h. The logical unit WriteBooster 
> Buffer
> size is configured by setting the dLUNumWriteBoosterBufferAllocUnits field of
> the related Unit Descriptor. Only a value greater than zero enables the
> WriteBooster feature in the logical unit.
> 
> Modify ufshcd_wb_probe() as above description to support LU Dedicated buffer
> mode.
> 
> Note that according to UFS 3.1 specification, the valid value of
> bDeviceMaxWriteBoosterLUs parameter in Geometry Descriptor is 1, which
> means at most one LUN can have WriteBooster buffer in "LU dedicated buffer
> mode". Therefore this patch supports only one LUN with WriteBooster enabled.
> All WriteBooster related sysfs nodes are specifically mapped to the LUN with
> WriteBooster enabled in LU Dedicated buffer mode.
> 
> Signed-off-by: Stanley Chu 
> Reviewed-by: Avri Altman 
Reviewed-by: Bean Huo 

RE: [EXT] [PATCH v2 1/5] scsi: ufs: Allow UFS 3.0 as a valid version

[Bean Huo (beanhuo)]

> > > @@ -8441,7 +8441,8 @@ int ufshcd_init(struct ufs_hba *hba, void
> > > __iomem *mmio_base, unsigned int irq)
> > >   if ((hba->ufs_version != UFSHCI_VERSION_10) &&
> > >   (hba->ufs_version != UFSHCI_VERSION_11) &&
> > >   (hba->ufs_version != UFSHCI_VERSION_20) &&
> > > - (hba->ufs_version != UFSHCI_VERSION_21))
> > > + (hba->ufs_version != UFSHCI_VERSION_21) &&
> > > + (hba->ufs_version != UFSHCI_VERSION_30))
> >
> > I don't think these checkups of UFSHCI version is necessary,  does the 
> > UFSHCI
> have other version number except these?
> > Is there somebody still v1.0 and v1.1?
> 
> Probably. I think we can leave them or change the dev_err to a dev_warn.
> This way we have logs in case someone is using a non-supported version.
> 
> What do you think ?
> 
Hi, Jose
Seems after your patch, all of current released UFS control versions will be 
supported except the
version suffix is non-zero. Right?

> ---
> Thanks,
> Jose Miguel Abreu

RE: [EXT] [PATCH v1 3/4] scsi: ufs: add LU Dedicated buffer type support for WriteBooster

[Bean Huo (beanhuo)]

> 
> +/* WriteBooster buffer type */
> +enum {
> + WB_TYPE_LU_DEDICATED= 0x0,
> + WB_TYPE_SINGLE_SHARED   = 0x1
> +};


Hi,  Stanly
WB_TYPE_SINGLE_SHARED might be WB_TYPE_SHARED_BUFFER.  I think,
we should try to make the name definition correspond to Spec


thanks, 

Bean

RE: [EXT] [PATCH v1 4/4] scsi: ufs-mediatek: enable WriteBooster capability

[Bean Huo (beanhuo)]

> -Original Message-
> From: Stanley Chu 
> Sent: Tuesday, April 28, 2020 1:14 PM
> To: linux-s...@vger.kernel.org; martin.peter...@oracle.com;
> avri.alt...@wdc.com; alim.akh...@samsung.com; j...@linux.ibm.com;
> asuto...@codeaurora.org
> Cc: Bean Huo (beanhuo) ; c...@codeaurora.org;
> matthias@gmail.com; bvanass...@acm.org; linux-
> media...@lists.infradead.org; linux-arm-ker...@lists.infradead.org; linux-
> ker...@vger.kernel.org; kuohong.w...@mediatek.com;
> peter.w...@mediatek.com; chun-hung...@mediatek.com;
> andy.t...@mediatek.com; Stanley Chu 
> Subject: [EXT] [PATCH v1 4/4] scsi: ufs-mediatek: enable WriteBooster 
> capability
> 
> Enable WriteBooster capability on MediaTek UFS platforms.
> 
> Signed-off-by: Stanley Chu 
> ---
>  drivers/scsi/ufs/ufs-mediatek.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/scsi/ufs/ufs-mediatek.c b/drivers/scsi/ufs/ufs-mediatek.c
> index 673c16596fb2..15b9c420a3a5 100644
> --- a/drivers/scsi/ufs/ufs-mediatek.c
> +++ b/drivers/scsi/ufs/ufs-mediatek.c
> @@ -263,6 +263,9 @@ static int ufs_mtk_init(struct ufs_hba *hba)
>   /* Enable clock-gating */
>   hba->caps |= UFSHCD_CAP_CLK_GATING;
> 
> + /* Enable WriteBooster */
> + hba->caps |= UFSHCD_CAP_WB_EN;
> +
>   /*
>* ufshcd_vops_init() is invoked after
>* ufshcd_setup_clock(true) in ufshcd_hba_init() thus
> --
> 2.18.0

Reviewed-by: Bean Huo 

RE: [EXT] [PATCH v1 1/4] scsi: ufs: allow legacy UFS devices to enable WriteBooster

[Bean Huo (beanhuo)]

> -Original Message-
> From: Stanley Chu 
> Sent: Tuesday, April 28, 2020 1:14 PM
> To: linux-s...@vger.kernel.org; martin.peter...@oracle.com;
> avri.alt...@wdc.com; alim.akh...@samsung.com; j...@linux.ibm.com;
> asuto...@codeaurora.org
> Cc: Bean Huo (beanhuo) ; c...@codeaurora.org;
> matthias@gmail.com; bvanass...@acm.org; linux-
> media...@lists.infradead.org; linux-arm-ker...@lists.infradead.org; linux-
> ker...@vger.kernel.org; kuohong.w...@mediatek.com;
> peter.w...@mediatek.com; chun-hung...@mediatek.com;
> andy.t...@mediatek.com; Stanley Chu 
> Subject: [EXT] [PATCH v1 1/4] scsi: ufs: allow legacy UFS devices to enable
> WriteBooster
> 
> WriteBooster feature may be supported by some legacy UFS devices (i.e., < 3.1)
> by upgrading firmware.
> 
> To enable WriteBooster feature in such devices, relax the entrance condition 
> of
> ufshcd_wb_probe() to allow host driver to check those devices' WriteBooster
> capability.
> 
> WriteBooster feature can be available if below both conditions are satisfied,
> 
> 1. Device descriptor has dExtendedUFSFeaturesSupport field.
> 2. WriteBooster support is specified in above field.
> 
> Signed-off-by: Stanley Chu 
> ---
>  drivers/scsi/ufs/ufshcd.c | 19 +--
>  1 file changed, 13 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c index
> 915e963398c4..111812c5304a 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -6800,9 +6800,16 @@ static int ufshcd_scsi_add_wlus(struct ufs_hba *hba)
> 
>  static void ufshcd_wb_probe(struct ufs_hba *hba, u8 *desc_buf)  {
> + if (hba->desc_size.dev_desc <=
> DEVICE_DESC_PARAM_EXT_UFS_FEATURE_SUP)
> + goto wb_disabled;
> +
>   hba->dev_info.d_ext_ufs_feature_sup =
>   get_unaligned_be32(desc_buf +
> 
> DEVICE_DESC_PARAM_EXT_UFS_FEATURE_SUP);
> +
> + if (!(hba->dev_info.d_ext_ufs_feature_sup &
> UFS_DEV_WRITE_BOOSTER_SUP))
> + goto wb_disabled;
> +
>   /*
>* WB may be supported but not configured while provisioning.
>* The spec says, in dedicated wb buffer mode, @@ -6818,11 +6825,12
> @@ static void ufshcd_wb_probe(struct ufs_hba *hba, u8 *desc_buf)
>   hba->dev_info.b_presrv_uspc_en =
>   desc_buf[DEVICE_DESC_PARAM_WB_PRESRV_USRSPC_EN];
> 
> - if (!((hba->dev_info.d_ext_ufs_feature_sup &
> -  UFS_DEV_WRITE_BOOSTER_SUP) &&
> - hba->dev_info.b_wb_buffer_type &&
> + if (!(hba->dev_info.b_wb_buffer_type &&
> hba->dev_info.d_wb_alloc_units))
> - hba->caps &= ~UFSHCD_CAP_WB_EN;
> + goto wb_disabled;
> +
> +wb_disabled:
> + hba->caps &= ~UFSHCD_CAP_WB_EN;
>  }
> 
>  static int ufs_get_device_desc(struct ufs_hba *hba) @@ -6862,8 +6870,7 @@
> static int ufs_get_device_desc(struct ufs_hba *hba)
> 
>   model_index = desc_buf[DEVICE_DESC_PARAM_PRDCT_NAME];
> 
> - /* Enable WB only for UFS-3.1 */
> - if (dev_info->wspecversion >= 0x310)
> + if (ufshcd_is_wb_allowed(hba))
>   ufshcd_wb_probe(hba, desc_buf);
> 
>   err = ufshcd_read_string_desc(hba, model_index,
> --
> 2.18.0

Reviewed-by: Bean Huo 

RE: [EXT] [PATCH v1 2/4] scsi: ufs: add "index" in parameter list of ufshcd_query_flag()

[Bean Huo (beanhuo)]

> -Original Message-
> From: Stanley Chu 
> Sent: Tuesday, April 28, 2020 1:14 PM
> To: linux-s...@vger.kernel.org; martin.peter...@oracle.com;
> avri.alt...@wdc.com; alim.akh...@samsung.com; j...@linux.ibm.com;
> asuto...@codeaurora.org
> Cc: Bean Huo (beanhuo) ; c...@codeaurora.org;
> matthias@gmail.com; bvanass...@acm.org; linux-
> media...@lists.infradead.org; linux-arm-ker...@lists.infradead.org; linux-
> ker...@vger.kernel.org; kuohong.w...@mediatek.com;
> peter.w...@mediatek.com; chun-hung...@mediatek.com;
> andy.t...@mediatek.com; Stanley Chu 
> Subject: [EXT] [PATCH v1 2/4] scsi: ufs: add "index" in parameter list of
> ufshcd_query_flag()
> 
> For preparation of Dedicated LU support on WriteBooster feature, "index"
> parameter shall be added and allowed to be specified by callers.
> 
> Signed-off-by: Stanley Chu 
> ---
>  drivers/scsi/ufs/ufs-sysfs.c |  2 +-
>  drivers/scsi/ufs/ufshcd.c| 28 +++-
>  drivers/scsi/ufs/ufshcd.h|  2 +-
>  3 files changed, 17 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/scsi/ufs/ufs-sysfs.c b/drivers/scsi/ufs/ufs-sysfs.c index
> 93484408bc40..b86b6a40d7e6 100644
> --- a/drivers/scsi/ufs/ufs-sysfs.c
> +++ b/drivers/scsi/ufs/ufs-sysfs.c
> @@ -631,7 +631,7 @@ static ssize_t _name##_show(struct device *dev,
>   \
>   struct ufs_hba *hba = dev_get_drvdata(dev); \
>   pm_runtime_get_sync(hba->dev);
>   \
>   ret = ufshcd_query_flag(hba, UPIU_QUERY_OPCODE_READ_FLAG,
>   \
> - QUERY_FLAG_IDN##_uname, );
>   \
> + QUERY_FLAG_IDN##_uname, 0, );  \
>   pm_runtime_put_sync(hba->dev);
>   \
>   if (ret)\
>   return -EINVAL; \
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c index
> 111812c5304a..465ee023ea4b 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -2782,13 +2782,13 @@ static inline void ufshcd_init_query(struct ufs_hba
> *hba,  }
> 
>  static int ufshcd_query_flag_retry(struct ufs_hba *hba,
> - enum query_opcode opcode, enum flag_idn idn, bool *flag_res)
> + enum query_opcode opcode, enum flag_idn idn, u8 index, bool
> *flag_res)
>  {
>   int ret;
>   int retries;
> 
>   for (retries = 0; retries < QUERY_REQ_RETRIES; retries++) {
> - ret = ufshcd_query_flag(hba, opcode, idn, flag_res);
> + ret = ufshcd_query_flag(hba, opcode, idn, index, flag_res);
>   if (ret)
>   dev_dbg(hba->dev,
>   "%s: failed with error %d, retries %d\n", @@ -
> 2809,16 +2809,17 @@ static int ufshcd_query_flag_retry(struct ufs_hba *hba,
>   * @hba: per-adapter instance
>   * @opcode: flag query to perform
>   * @idn: flag idn to access
> + * @index: flag index to access
>   * @flag_res: the flag value after the query request completes
>   *
>   * Returns 0 for success, non-zero in case of failure
>   */
>  int ufshcd_query_flag(struct ufs_hba *hba, enum query_opcode opcode,
> - enum flag_idn idn, bool *flag_res)
> + enum flag_idn idn, u8 index, bool *flag_res)
>  {
>   struct ufs_query_req *request = NULL;
>   struct ufs_query_res *response = NULL;
> - int err, index = 0, selector = 0;
> + int err, selector = 0;
>   int timeout = QUERY_REQ_TIMEOUT;
> 
>   BUG_ON(!hba);
> @@ -4175,7 +4176,7 @@ static int ufshcd_complete_dev_init(struct ufs_hba
> *hba)
>   bool flag_res = true;
> 
>   err = ufshcd_query_flag_retry(hba, UPIU_QUERY_OPCODE_SET_FLAG,
> - QUERY_FLAG_IDN_FDEVICEINIT, NULL);
> + QUERY_FLAG_IDN_FDEVICEINIT, 0, NULL);
>   if (err) {
>   dev_err(hba->dev,
>   "%s setting fDeviceInit flag failed with error %d\n", @@
> -4186,7 +4187,7 @@ static int ufshcd_complete_dev_init(struct ufs_hba *hba)
>   /* poll for max. 1000 iterations for fDeviceInit flag to clear */
>   for (i = 0; i < 1000 && !err && flag_res; i++)
>   err = ufshcd_query_flag_retry(hba,
> UPIU_QUERY_OPCODE_READ_FLAG,
> - QUERY_FLAG_IDN_FDEVICEINIT, _res);
> + QUERY_FLAG_IDN_FDEVICEINIT, 0, _res);
> 
>   if (err)
>   dev_err(hba->dev,
> @@ -5001,7 +5002,7 @@ static int ufshcd_enable_auto_bkops(struct ufs_hba
> *hba)
>   goto out;
> 
>   err = ufshcd_query_flag_retry(hba, UPIU_QUERY_OP

RE: [EXT] [PATCH v1 1/2] scsi: ufs: Introduce a vops for resetting host controller

[Bean Huo (beanhuo)]

Hi, Can Guo
Actually, we already have DME_RESET,  this is not enough for Qualcomm host? 
Thanks,

//Bean

> 
> Some UFS host controllers need their specific implementations of resetting to
> get them into a good state. Provide a new vops to allow the platform driver to
> implement this own reset operation.
> 
> Signed-off-by: Can Guo 
> ---
>  drivers/scsi/ufs/ufshcd.c | 16   drivers/scsi/ufs/ufshcd.h | 
> 10
> ++
>  2 files changed, 26 insertions(+)
> 
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c index
> c28c144..161e3c4 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -3859,6 +3859,14 @@ static int ufshcd_link_recovery(struct ufs_hba *hba)
>   ufshcd_set_eh_in_progress(hba);
>   spin_unlock_irqrestore(hba->host->host_lock, flags);
> 
> + ret = ufshcd_vops_full_reset(hba);
> + if (ret)
> + dev_warn(hba->dev, "%s: full reset returned %d\n",
> +   __func__, ret);
> +
> + /* Reset the attached device */
> + ufshcd_vops_device_reset(hba);
> +
>   ret = ufshcd_host_reset_and_restore(hba);
> 
>   spin_lock_irqsave(hba->host->host_lock, flags); @@ -6241,6 +6249,11
> @@ static int ufshcd_reset_and_restore(struct ufs_hba *hba)
>   int retries = MAX_HOST_RESET_RETRIES;
> 
>   do {
> + err = ufshcd_vops_full_reset(hba);
> + if (err)
> + dev_warn(hba->dev, "%s: full reset returned %d\n",
> + __func__, err);
> +
>   /* Reset the attached device */
>   ufshcd_vops_device_reset(hba);
> 
> @@ -8384,6 +8397,9 @@ int ufshcd_init(struct ufs_hba *hba, void __iomem
> *mmio_base, unsigned int irq)
>   goto exit_gating;
>   }
> 
> + /* Reset controller to power on reset (POR) state */
> + ufshcd_vops_full_reset(hba);
> +
>   /* Reset the attached device */
>   ufshcd_vops_device_reset(hba);
> 
> diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h index
> e0fe247..253b9ea 100644
> --- a/drivers/scsi/ufs/ufshcd.h
> +++ b/drivers/scsi/ufs/ufshcd.h
> @@ -296,6 +296,8 @@ struct ufs_pwr_mode_info {
>   * @apply_dev_quirks: called to apply device specific quirks
>   * @suspend: called during host controller PM callback
>   * @resume: called during host controller PM callback
> + * @full_reset: called for handling variant specific implementations of
> + *  resetting the hci
>   * @dbg_register_dump: used to dump controller debug information
>   * @phy_initialization: used to initialize phys
>   * @device_reset: called to issue a reset pulse on the UFS device @@ -325,6
> +327,7 @@ struct ufs_hba_variant_ops {
>   int (*apply_dev_quirks)(struct ufs_hba *);
>   int (*suspend)(struct ufs_hba *, enum ufs_pm_op);
>   int (*resume)(struct ufs_hba *, enum ufs_pm_op);
> + int (*full_reset)(struct ufs_hba *hba);
>   void(*dbg_register_dump)(struct ufs_hba *hba);
>   int (*phy_initialization)(struct ufs_hba *);
>   void(*device_reset)(struct ufs_hba *hba);
> @@ -1076,6 +1079,13 @@ static inline int ufshcd_vops_resume(struct ufs_hba
> *hba, enum ufs_pm_op op)
>   return 0;
>  }
> 
> +static inline int ufshcd_vops_full_reset(struct ufs_hba *hba) {
> + if (hba->vops && hba->vops->full_reset)
> + return hba->vops->full_reset(hba);
> + return 0;
> +}
> +
>  static inline void ufshcd_vops_dbg_register_dump(struct ufs_hba *hba)  {
>   if (hba->vops && hba->vops->dbg_register_dump)
> --
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project

RE: [EXT] [PATCH v4 1/3] scsi: ufs: Introduce vops for resetting device

[Bean Huo (beanhuo)]

>
>Reviewed-by: Alim Akhtar 
>Signed-off-by: Bjorn Andersson 
Reviewed-by: Bean Huo 

//Bean Huo

[PATCH v1] scsi: ufs: change msleep to usleep_range

[Bean Huo (beanhuo)]

From: Bean Huo 

This patch is to change msleep() to usleep_range() based on
Documentation/timers/timers-howto.txt. It suggests using
usleep_range() for small msec(1ms - 20ms) since msleep()
will often sleep longer than desired value.

After changing, booting time will be 5ms-10ms faster than before.
I tested this change on two different platforms, one has 5ms faster,
another one is about 10ms. I think this is different on different
platform.

Actually, from UFS host side, 1ms-5ms delay is already sufficient for
its initialization of the local UIC layer.

Fixes: 7a3e97b0dc4b ([SCSI] ufshcd: UFS Host controller driver)
Signed-off-by: Bean Huo 
---
 drivers/scsi/ufs/ufshcd.c | 10 ++
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index a208589426b1..21f7b3b8026c 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -4213,12 +4213,6 @@ static int ufshcd_hba_execute_hce(struct ufs_hba *hba)
 {
int retry;
 
-   /*
-* msleep of 1 and 5 used in this function might result in msleep(20),
-* but it was necessary to send the UFS FPGA to reset mode during
-* development and testing of this driver. msleep can be changed to
-* mdelay and retry count can be reduced based on the controller.
-*/
if (!ufshcd_is_hba_active(hba))
/* change controller state to "reset state" */
ufshcd_hba_stop(hba, true);
@@ -4241,7 +4235,7 @@ static int ufshcd_hba_execute_hce(struct ufs_hba *hba)
 * instruction might be read back.
 * This delay can be changed based on the controller.
 */
-   msleep(1);
+   usleep_range(1000, 1100);
 
/* wait for the host controller to complete initialization */
retry = 10;
@@ -4253,7 +4247,7 @@ static int ufshcd_hba_execute_hce(struct ufs_hba *hba)
"Controller enable failed\n");
return -EIO;
}
-   msleep(5);
+   usleep_range(5000, 5100);
}
 
/* enable UIC related interrupts */
-- 
2.7.4

[PATCH v2 3/3] scsi: ufs-bsg: complete ufs-bsg job only if no error

[Bean Huo (beanhuo)]

From: Bean Huo 

In the case of UPIU/DME request execution failed in UFS device,
ufs_bsg_request() will complete this failed bsg job by calling
bsg_job_done(). Meanwhile, it returns this error status to blk-mq
layer, then triggers blk-mq completing this request again, this will
cause below panic.

Call trace:
ll_sc___cmpxchg_case_acq_32+0x4/0x20
complete+0x28/0x70
blk_end_sync_rq+0x24/0x30
blk_mq_end_request+0xb8/0x118
bsg_job_put+0x4c/0x58
bsg_complete+0x20/0x30
blk_done_softirq+0xb4/0xe8
do_softirq+0x154/0x3f0
run_ksoftirqd+0x4c/0x68
smpboot_thread_fn+0x22c/0x268
kthread+0x130/0x138
ret_from_fork+0x10/0x1c
Code: f84107fe d65f03c0 d503201f f9800011 (885ffc10)
---[ end trace d92825bff6326e66 ]---
Kernel panic - not syncing: Fatal exception in interrupt

This patch is to fix this issue. The solution is we complete
the ufs-bsg job only if no error happened.

Fixes: df032bf27a41 (scsi: ufs: Add a bsg endpoint that supports UPIUs)
Signed-off-by: Bean Huo 
---
 drivers/scsi/ufs/ufs_bsg.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/ufs/ufs_bsg.c b/drivers/scsi/ufs/ufs_bsg.c
index f420d6f8d84c..a9344eb4e047 100644
--- a/drivers/scsi/ufs/ufs_bsg.c
+++ b/drivers/scsi/ufs/ufs_bsg.c
@@ -149,7 +149,9 @@ static int ufs_bsg_request(struct bsg_job *job)
 out:
bsg_reply->result = ret;
job->reply_len = sizeof(struct ufs_bsg_reply);
-   bsg_job_done(job, ret, bsg_reply->reply_payload_rcv_len);
+   /* complete the job here only if no error */
+   if (ret == 0)
+   bsg_job_done(job, ret, bsg_reply->reply_payload_rcv_len);
 
return ret;
 }
-- 
2.7.4

[PATCH v2 2/3] scsi: ufs-bsg: fix typo in ufs_bsg_request

[Bean Huo (beanhuo)]

From: Bean Huo 

Correct dev_dbg to dev_err, so as to print out the error
information in case of DME command failed.

Signed-off-by: Bean Huo 
---
 drivers/scsi/ufs/ufs_bsg.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/scsi/ufs/ufs_bsg.c b/drivers/scsi/ufs/ufs_bsg.c
index 869e71f861d6..f420d6f8d84c 100644
--- a/drivers/scsi/ufs/ufs_bsg.c
+++ b/drivers/scsi/ufs/ufs_bsg.c
@@ -122,7 +122,7 @@ static int ufs_bsg_request(struct bsg_job *job)
memcpy(, _request->upiu_req.uc, UIC_CMD_SIZE);
ret = ufshcd_send_uic_cmd(hba, );
if (ret)
-   dev_dbg(hba->dev,
+   dev_err(hba->dev,
"send uic cmd: error code %d\n", ret);
 
memcpy(_reply->upiu_rsp.uc, , UIC_CMD_SIZE);
-- 
2.7.4

[PATCH v2 0/3] scsi: ufs: typo fixes and improvement

[Bean Huo (beanhuo)]

From: Bean Huo 

This series patch is to fix several typos and fix one issue of twice
completing ufs-bsg job in case of UPIU/DME command failed.

Changed since v1:
- split v1 patch
- add fixes tag
- delete needless blank line

Bean Huo (3):
  scsi: ufs: fix typos in comment of ufshcd_uic_change_pwr_mode
  scsi: ufs-bsg: fix typo in ufs_bsg_request
  scsi: ufs-bsg: complete ufs-bsg job only if no error

 drivers/scsi/ufs/ufs_bsg.c | 6 --
 drivers/scsi/ufs/ufshcd.c  | 4 ++--
 2 files changed, 6 insertions(+), 4 deletions(-)

-- 
2.7.4

[PATCH V1] scsi: ufs-bsg: complete ufs-bsg job only if no error

[Bean Huo (beanhuo)]

From: Bean Huo 

In the case of UPIU/DME request execution failed in UFS device, 
ufs_bsg_request() will complete this failed bsg job by calling
bsg_job_done(). Meanwhile, it returns this error status to blk-mq
layer, then trigger blk-mq complete this request again, this will
cause below panic.

[   68.673050] Call trace:
[   68.675491]  __ll_sc___cmpxchg_case_acq_32+0x4/0x20
[   68.680369]  complete+0x28/0x70
[   68.683510]  blk_end_sync_rq+0x24/0x30
[   68.687255]  blk_mq_end_request+0xb8/0x118
[   68.691350]  bsg_job_put+0x4c/0x58
[   68.694747]  bsg_complete+0x20/0x30
[   68.698231]  blk_done_softirq+0xb4/0xe8
[   68.702066]  __do_softirq+0x154/0x3f0
[   68.705726]  run_ksoftirqd+0x4c/0x68
[   68.709298]  smpboot_thread_fn+0x22c/0x268
[   68.713394]  kthread+0x130/0x138
[   68.716619]  ret_from_fork+0x10/0x1c
[   68.720193] Code: f84107fe d65f03c0 d503201f f9800011 (885ffc10) 
[   68.726298] ---[ end trace d92825bff6326e66 ]---
[   68.730913] Kernel panic - not syncing: Fatal exception in interrupt

This patch is to fix this issue. The solution is we complete
the ufs-bsg job only if no error happened.

Signed-off-by: Bean Huo 
---
 drivers/scsi/ufs/ufs_bsg.c | 7 ---
 drivers/scsi/ufs/ufshcd.c  | 2 +-
 2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/scsi/ufs/ufs_bsg.c b/drivers/scsi/ufs/ufs_bsg.c
index 869e71f..d5516dc 100644
--- a/drivers/scsi/ufs/ufs_bsg.c
+++ b/drivers/scsi/ufs/ufs_bsg.c
@@ -122,7 +122,7 @@ static int ufs_bsg_request(struct bsg_job *job)
memcpy(, _request->upiu_req.uc, UIC_CMD_SIZE);
ret = ufshcd_send_uic_cmd(hba, );
if (ret)
-   dev_dbg(hba->dev,
+   dev_err(hba->dev,
"send uic cmd: error code %d\n", ret);
 
memcpy(_reply->upiu_rsp.uc, , UIC_CMD_SIZE);
@@ -143,13 +143,14 @@ static int ufs_bsg_request(struct bsg_job *job)
sg_copy_from_buffer(job->request_payload.sg_list,
job->request_payload.sg_cnt,
desc_buff, desc_len);
-
kfree(desc_buff);
 
 out:
bsg_reply->result = ret;
job->reply_len = sizeof(struct ufs_bsg_reply);
-   bsg_job_done(job, ret, bsg_reply->reply_payload_rcv_len);
+   /* complete the job here only if no error */
+   if (ret == 0)
+   bsg_job_done(job, ret, bsg_reply->reply_payload_rcv_len);
 
return ret;
 }
diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 04d3686..4718041 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -3776,7 +3776,7 @@ static int ufshcd_uic_pwr_ctrl(struct ufs_hba *hba, 
struct uic_command *cmd)
 }
 
 /**
- * ufshcd_uic_change_pwr_mode - Perform the UIC power mode chage
+ * ufshcd_uic_change_pwr_mode - Perform the UIC power mode change
  * using DME_SET primitives.
  * @hba: per adapter instance
  * @mode: powr mode value
-- 
2.7.4

RE: [EXT] [PATCH v3 2/3] scsi: ufs-qcom: Implement device_reset vops

[Bean Huo (beanhuo)]

Hi, Bjorn
Sorry just saw your message.

You can use UIC command，through function ufshcd_send_uic_cmd( ) with 
UIC_CMD_DME_END_PT_RST command.

DME_ENDPOINTRESET: It is used when UFS host wants the UFS device to perform a 
reset.

//bean

>On Tue 11 Jun 09:08 PDT 2019, Bean Huo (beanhuo) wrote:
>
>> Hi, Bjorn
>> This HW reset is dedicated to QUALCOMM based platform case.
>> how about adding a SW reset as to be default reset routine if platform
>doesn't support HW reset?
>>
>
>Can you please advice how I perform such software reset?
>
>Regards,
>Bjorn
>
>> >-Original Message-
>> >From: linux-scsi-ow...@vger.kernel.org
>> >
>> >On Behalf Of Bjorn Andersson
>> >Sent: Saturday, June 8, 2019 7:05 AM
>> >To: Rob Herring ; Mark Rutland
>> >; Alim Akhtar ; Avri
>> >Altman ; Pedro Sousa
>> >; James E.J. Bottomley
>> >; Martin K. Petersen 
>> >Cc: Andy Gross ; devicet...@vger.kernel.org;
>> >linux- ker...@vger.kernel.org; linux-arm-...@vger.kernel.org; linux-
>> >s...@vger.kernel.org
>> >Subject: [EXT] [PATCH v3 2/3] scsi: ufs-qcom: Implement device_reset
>> >vops
>> >
>> >The UFS_RESET pin on Qualcomm SoCs are controlled by TLMM and
>exposed
>> >through the GPIO framework. Acquire the device-reset GPIO and use
>> >this to implement the device_reset vops, to allow resetting the attached
>memory.
>> >
>> >Based on downstream support implemented by Subhash Jadavani
>> >.
>> >
>> >Signed-off-by: Bjorn Andersson 
>> >---
>> >
>> >Changes since v2:
>> >- Moved implementation to Qualcomm driver
>> >
>> > .../devicetree/bindings/ufs/ufshcd-pltfrm.txt |  2 ++
>> > drivers/scsi/ufs/ufs-qcom.c   | 32 +++
>> > drivers/scsi/ufs/ufs-qcom.h   |  4 +++
>> > 3 files changed, 38 insertions(+)
>> >
>> >diff --git a/Documentation/devicetree/bindings/ufs/ufshcd-pltfrm.txt
>> >b/Documentation/devicetree/bindings/ufs/ufshcd-pltfrm.txt
>> >index a74720486ee2..d562d8b4919c 100644
>> >--- a/Documentation/devicetree/bindings/ufs/ufshcd-pltfrm.txt
>> >+++ b/Documentation/devicetree/bindings/ufs/ufshcd-pltfrm.txt
>> >@@ -54,6 +54,8 @@ Optional properties:
>> >  PHY reset from the UFS controller.
>> > - resets: reset node register
>> > - reset-names   : describe reset node register, the "rst" corresponds 
>> > to
>> >reset the whole UFS IP.
>> >+- device-reset-gpios   : A phandle and gpio specifier denoting the
>GPIO
>> >connected
>> >+ to the RESET pin of the UFS memory device.
>> >
>> > Note: If above properties are not defined it can be assumed that the
>> >supply regulators or clocks are always on.
>> >diff --git a/drivers/scsi/ufs/ufs-qcom.c
>> >b/drivers/scsi/ufs/ufs-qcom.c index ea7219407309..efaf57ba618a 100644
>> >--- a/drivers/scsi/ufs/ufs-qcom.c
>> >+++ b/drivers/scsi/ufs/ufs-qcom.c
>> >@@ -16,6 +16,7 @@
>> > #include 
>> > #include 
>> > #include 
>> >+#include 
>> > #include 
>> >
>> > #include "ufshcd.h"
>> >@@ -1141,6 +1142,15 @@ static int ufs_qcom_init(struct ufs_hba *hba)
>> >goto out_variant_clear;
>> >}
>> >
>> >+   host->device_reset = devm_gpiod_get_optional(dev, "device-reset",
>> >+GPIOD_OUT_HIGH);
>> >+   if (IS_ERR(host->device_reset)) {
>> >+   err = PTR_ERR(host->device_reset);
>> >+   if (err != -EPROBE_DEFER)
>> >+   dev_err(dev, "failed to acquire reset gpio: %d\n", err);
>> >+   goto out_variant_clear;
>> >+   }
>> >+
>> >err = ufs_qcom_bus_register(host);
>> >if (err)
>> >goto out_variant_clear;
>> >@@ -1546,6 +1556,27 @@ static void ufs_qcom_dump_dbg_regs(struct
>> >ufs_hba *hba)
>> >usleep_range(1000, 1100);
>> > }
>> >
>> >+/**
>> >+ * ufs_qcom_device_reset() - toggle the (optional) device reset line
>> >+ * @hba: per-adapter instance
>> >+ *
>> >+ * Toggles the (optional) reset line to reset the attached device.
>> >+ */
>> >+static void ufs_qcom_device_reset(struct ufs_hba *hba) {
>> >+   struct ufs_qcom_host *host = ufshcd_get_variant(hba);
&

RE: [EXT] [PATCH v3 2/3] scsi: ufs-qcom: Implement device_reset vops

[Bean Huo (beanhuo)]

Hi, Bjorn
This HW reset is dedicated to QUALCOMM based platform case.
how about adding a SW reset as to be default reset routine if platform doesn't 
support HW reset?

>-Original Message-
>From: linux-scsi-ow...@vger.kernel.org 
>On Behalf Of Bjorn Andersson
>Sent: Saturday, June 8, 2019 7:05 AM
>To: Rob Herring ; Mark Rutland
>; Alim Akhtar ; Avri
>Altman ; Pedro Sousa
>; James E.J. Bottomley ;
>Martin K. Petersen 
>Cc: Andy Gross ; devicet...@vger.kernel.org; linux-
>ker...@vger.kernel.org; linux-arm-...@vger.kernel.org; linux-
>s...@vger.kernel.org
>Subject: [EXT] [PATCH v3 2/3] scsi: ufs-qcom: Implement device_reset vops
>
>The UFS_RESET pin on Qualcomm SoCs are controlled by TLMM and exposed
>through the GPIO framework. Acquire the device-reset GPIO and use this to
>implement the device_reset vops, to allow resetting the attached memory.
>
>Based on downstream support implemented by Subhash Jadavani
>.
>
>Signed-off-by: Bjorn Andersson 
>---
>
>Changes since v2:
>- Moved implementation to Qualcomm driver
>
> .../devicetree/bindings/ufs/ufshcd-pltfrm.txt |  2 ++
> drivers/scsi/ufs/ufs-qcom.c   | 32 +++
> drivers/scsi/ufs/ufs-qcom.h   |  4 +++
> 3 files changed, 38 insertions(+)
>
>diff --git a/Documentation/devicetree/bindings/ufs/ufshcd-pltfrm.txt
>b/Documentation/devicetree/bindings/ufs/ufshcd-pltfrm.txt
>index a74720486ee2..d562d8b4919c 100644
>--- a/Documentation/devicetree/bindings/ufs/ufshcd-pltfrm.txt
>+++ b/Documentation/devicetree/bindings/ufs/ufshcd-pltfrm.txt
>@@ -54,6 +54,8 @@ Optional properties:
> PHY reset from the UFS controller.
> - resets: reset node register
> - reset-names   : describe reset node register, the "rst" corresponds to
>reset the whole UFS IP.
>+- device-reset-gpios  : A phandle and gpio specifier denoting the GPIO
>connected
>+to the RESET pin of the UFS memory device.
>
> Note: If above properties are not defined it can be assumed that the supply
>regulators or clocks are always on.
>diff --git a/drivers/scsi/ufs/ufs-qcom.c b/drivers/scsi/ufs/ufs-qcom.c index
>ea7219407309..efaf57ba618a 100644
>--- a/drivers/scsi/ufs/ufs-qcom.c
>+++ b/drivers/scsi/ufs/ufs-qcom.c
>@@ -16,6 +16,7 @@
> #include 
> #include 
> #include 
>+#include 
> #include 
>
> #include "ufshcd.h"
>@@ -1141,6 +1142,15 @@ static int ufs_qcom_init(struct ufs_hba *hba)
>   goto out_variant_clear;
>   }
>
>+  host->device_reset = devm_gpiod_get_optional(dev, "device-reset",
>+   GPIOD_OUT_HIGH);
>+  if (IS_ERR(host->device_reset)) {
>+  err = PTR_ERR(host->device_reset);
>+  if (err != -EPROBE_DEFER)
>+  dev_err(dev, "failed to acquire reset gpio: %d\n", err);
>+  goto out_variant_clear;
>+  }
>+
>   err = ufs_qcom_bus_register(host);
>   if (err)
>   goto out_variant_clear;
>@@ -1546,6 +1556,27 @@ static void ufs_qcom_dump_dbg_regs(struct
>ufs_hba *hba)
>   usleep_range(1000, 1100);
> }
>
>+/**
>+ * ufs_qcom_device_reset() - toggle the (optional) device reset line
>+ * @hba: per-adapter instance
>+ *
>+ * Toggles the (optional) reset line to reset the attached device.
>+ */
>+static void ufs_qcom_device_reset(struct ufs_hba *hba) {
>+  struct ufs_qcom_host *host = ufshcd_get_variant(hba);
>+
>+  /*
>+   * The UFS device shall detect reset pulses of 1us, sleep for 10us to
>+   * be on the safe side.
>+   */
>+  gpiod_set_value_cansleep(host->device_reset, 1);
>+  usleep_range(10, 15);
>+
>+  gpiod_set_value_cansleep(host->device_reset, 0);
>+  usleep_range(10, 15);
>+}
>+
> /**
>  * struct ufs_hba_qcom_vops - UFS QCOM specific variant operations
>  *
>@@ -1566,6 +1597,7 @@ static struct ufs_hba_variant_ops
>ufs_hba_qcom_vops = {
>   .suspend= ufs_qcom_suspend,
>   .resume = ufs_qcom_resume,
>   .dbg_register_dump  = ufs_qcom_dump_dbg_regs,
>+  .device_reset   = ufs_qcom_device_reset,
> };
>
> /**
>diff --git a/drivers/scsi/ufs/ufs-qcom.h b/drivers/scsi/ufs/ufs-qcom.h index
>68a880185752..b96ffb6804e4 100644
>--- a/drivers/scsi/ufs/ufs-qcom.h
>+++ b/drivers/scsi/ufs/ufs-qcom.h
>@@ -204,6 +204,8 @@ struct ufs_qcom_testbus {
>   u8 select_minor;
> };
>
>+struct gpio_desc;
>+
> struct ufs_qcom_host {
>   /*
>* Set this capability if host controller supports the QUniPro mode
>@@ -241,6 +243,8 @@ struct ufs_qcom_host {
>   struct ufs_qcom_testbus testbus;
>
>   struct reset_controller_dev rcdev;
>+
>+  struct gpio_desc *device_reset;
> };
>
> static inline u32
>--
>2.18.0

RE: [EXT] [PATCH 2/3] scsi: ufs: Allow resetting the UFS device

[Bean Huo (beanhuo)]

Hi, Bjorn
I think I should give up my preview suggestion DME_ENDPOINTRESET.req software 
reset,  go back to your HW reset.
Although SW reset can cover most of the requirement, and compatible with 
different vendor UFS device, for some device
fatal error cases, UFS internal stuck and would not accept SW Reset. An HW 
reset is expected.  
Please go on your reset way.

Thanks,
//Bean

>Andy Gross ; Linus Walleij ;
>Rob Herring ; Mark Rutland ;
>linux-arm-...@vger.kernel.org; linux-g...@vger.kernel.org;
>devicet...@vger.kernel.org; linux-kernel@vger.kernel.org; linux-
>s...@vger.kernel.org
>Subject: Re: [EXT] [PATCH 2/3] scsi: ufs: Allow resetting the UFS device
>
>On Tue 04 Jun 01:13 PDT 2019, Bean Huo (beanhuo) wrote:
>> >@@ -6159,6 +6179,9 @@ static int ufshcd_reset_and_restore(struct
>> >ufs_hba
>> >*hba)
>> >int retries = MAX_HOST_RESET_RETRIES;
>> >
>> >do {
>> >+   /* Reset the attached device */
>> >+   ufshcd_device_reset(hba);
>> >+
>>
>> what's problem you met, and you should reset UFS device here? could you
>give more info?
>>
>> It is true that we don't reset UFS device in case of device fatal
>> error. According to UFS host spec, Host should be device reset except
>> that in addition to resetting UIC. But as so far, We didn't experience any
>problems result from this missing reset.
>>
>> We have three UFS device reset ways.  Comparing to this hardware
>> reset, I prefer to use DME_ENDPOINTRESET.req software reset.
>>
>
>Hi Bean,
>
>Thanks for your questions. With some memories we see issues establishing
>the link during bootup, so that's the purpose of issuing this reset.
>
>Unfortunately the downstream Qualcomm patch [1] (which I should have
>remembered to attribute), does not mention why the reset during host
>controller reset is needed - but I'm fairly certain that this scenario would be
>similar to the handover from bootloader to kernel that we do see an issue
>with.
>
>
>[1]
>https://nam01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fsource
>.codeaurora.org%2Fquic%2Fla%2Fkernel%2Fmsm-
>4.4%2Fcommit%2F%3Fh%3Dmsm-
>4.4%26id%3D0c82737188e2d63a08196e078e411032dbbc3b89data=02%
>7C01%7Cbeanhuo%40micron.com%7Cf72404eb906440e1f9c408d6e93c401d%7
>Cf38a5ecd28134862b11bac1d563c806f%7C0%7C0%7C636952842324926509
>mp;sdata=I%2FH7km6b34jyoUa1RVEPApfYt5uSFtHqL3%2BvV1bvryM%3D
>;reserved=0
>
>Regards,
>Bjorn

RE: [EXT] [PATCH 2/3] scsi: ufs: Allow resetting the UFS device

[Bean Huo (beanhuo)]

Hi, Bjorn

>Acquire the device-reset GPIO and toggle this to reset the UFS device during
>initialization and host reset.
>
>+/**
>+ ufshcd_device_reset() - toggle the (optional) device reset line
>+ * @hba: per-adapter instance
>+ *
>+ * Toggles the (optional) reset line to reset the attached device.
>+ */
>+static void ufshcd_device_reset(struct ufs_hba *hba) {
>+  /*
>+   * The USB device shall detect reset pulses of 1us, sleep for 10us to
>+   * be on the safe side.
>+   */
>+  gpiod_set_value_cansleep(hba->device_reset, 1);
>+  usleep_range(10, 15);
>+
>+  gpiod_set_value_cansleep(hba->device_reset, 0);
>+  usleep_range(10, 15);
>+}
>+
> /**
>  * ufshcd_host_reset_and_restore - reset and restore host controller
>  * @hba: per-adapter instance
>@@ -6159,6 +6179,9 @@ static int ufshcd_reset_and_restore(struct ufs_hba
>*hba)
>   int retries = MAX_HOST_RESET_RETRIES;
>
>   do {
>+  /* Reset the attached device */
>+  ufshcd_device_reset(hba);
>+

what's problem you met, and you should reset UFS device here? could you give 
more info?

It is true that we don't reset UFS device in case of device fatal error. 
According to UFS host spec,
Host should be device reset except that in addition to resetting UIC. But as so 
far,
We didn't experience any problems result from this missing reset.

We have three UFS device reset ways.  Comparing to this hardware reset, 
I prefer to use DME_ENDPOINTRESET.req software reset.


>   err = ufshcd_host_reset_and_restore(hba);
>   } while (err && --retries);
>
>@@ -7355,6 +7378,18 @@ static void ufshcd_variant_hba_exit(struct ufs_hba
>*hba)
>   ufshcd_vops_exit(hba);
> }
>
>+static int ufshcd_init_device_reset(struct ufs_hba *hba) {
>+  hba->device_reset = devm_gpiod_get_optional(hba->dev, "device-
>reset",
>+  GPIOD_OUT_HIGH);
>+  if (IS_ERR(hba->device_reset)) {
>+  dev_err(hba->dev, "failed to acquire reset gpio: %ld\n",
>+  PTR_ERR(hba->device_reset));
>+  }
>+
>+  return PTR_ERR_OR_ZERO(hba->device_reset);
>+}
>+
> static int ufshcd_hba_init(struct ufs_hba *hba)  {
>   int err;
>@@ -7394,9 +7429,15 @@ static int ufshcd_hba_init(struct ufs_hba *hba)
>   if (err)
>   goto out_disable_vreg;
>
>+  err = ufshcd_init_device_reset(hba);
>+  if (err)
>+  goto out_disable_variant;
>+
>   hba->is_powered = true;
>   goto out;
>
>+out_disable_variant:
>+  ufshcd_vops_setup_regulators(hba, false);
> out_disable_vreg:
>   ufshcd_setup_vreg(hba, false);
> out_disable_clks:
>@@ -8290,6 +8331,9 @@ int ufshcd_init(struct ufs_hba *hba, void __iomem
>*mmio_base, unsigned int irq)
>   goto exit_gating;
>   }
>
>+  /* Reset the attached device */
>+  ufshcd_device_reset(hba);
>+
>   /* Host controller enable */
>   err = ufshcd_hba_enable(hba);
>   if (err) {
>diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h index
>ecfa898b9ccc..d8be67742168 100644
>--- a/drivers/scsi/ufs/ufshcd.h
>+++ b/drivers/scsi/ufs/ufshcd.h
>@@ -72,6 +72,8 @@
> #define UFSHCD "ufshcd"
> #define UFSHCD_DRIVER_VERSION "0.2"
>
>+struct gpio_desc;
>+
> struct ufs_hba;
>
> enum dev_cmd_type {
>@@ -706,6 +708,8 @@ struct ufs_hba {
>
>   struct device   bsg_dev;
>   struct request_queue*bsg_queue;
>+
>+  struct gpio_desc *device_reset;
> };
>
> /* Returns true if clocks can be gated. Otherwise false */
>--
>2.18.0

RE: [EXT] [PATCH] scsi: ufs: Check that space was properly alloced in copy_query_response

[Bean Huo (beanhuo)]

Hi, Avri
>
>Signed-off-by: Avri Altman 
Acked-by: Bean Huo 

Thanks,
//Bean

RE: [EXT] [PATCH v6 2/2] mtd: spi-nor: cadence-quadspi: Add support for Octal SPI controller

[Bean Huo (beanhuo)]

Hi, Vignesh

>
>Cadence OSPI controller IP supports Octal IO (x8 IO lines), It also has an
>integrated PHY. IP register layout is very similar to existing QSPI IP except 
>for
>additional bits to support Octal and Octal DDR mode. Therefore, extend
>current driver to support Octal mode. Only Octal SDR read (1-1-8)mode is
>supported for now.

Does this your Cadence OSPI controller support 8-8-8 IO mode, if yes,
Why not directly enable 8-8-8 mode? 

>Tested with mt35xu512aba Octal flash on TI's AM654 EVM.
>
>Signed-off-by: Vignesh R 
>__
>Linux MTD discussion mailing list
>http://lists.infradead.org/mailman/listinfo/linux-mtd/

RE: [EXT] Re: [PATCH v2] mtd: spi-nor: Fix wrong abbreviation HWCPAS

[Bean Huo (beanhuo)]

>> git send-email 0001-mtd-spi-nor-Fix-wrong-abbreviation-HWCPAS.patch
>> 0001-mtd-spi-nor-Fix-wrong-abbreviation-HWCPAS.patch
>
>git send-email --annotate --to=... --cc=... --cc=... 000*patch
>
>This will likely make your life easier, rather than having to paste various 
>email
>addresses to git send-email queries.
>
Yes, thanks.

>[...]
>
>--
>Best regards,
>Marek Vasut
>
>__
>Linux MTD discussion mailing list
>http://lists.infradead.org/mailman/listinfo/linux-mtd/

RE: [EXT] Re: [PATCH v2] mtd: spi-nor: Fix wrong abbreviation HWCPAS

[Bean Huo (beanhuo)]

Hi,Boris 
I sent three times, seems last time is successful. would you check that is 
correct?

git send-email 0001-mtd-spi-nor-Fix-wrong-abbreviation-HWCPAS.patch
0001-mtd-spi-nor-Fix-wrong-abbreviation-HWCPAS.patch
To whom should the emails be sent (if anyone)? 
linux-...@lists.infradead.org,tudor.amba...@microchip.com,bbrezil...@kernel.org,linux-kernel@vger.kernel.org
Message-ID to be used as In-Reply-To for the first email (if any)? 
(mbox) Adding cc: Bean Huo  from line 'From: Bean Huo 
'
(body) Adding cc: Bean Huo  from line 'Signed-off-by: Bean 
Huo '

From: Bean Huo 
To: linux-...@lists.infradead.org,
tudor.amba...@microchip.com,
bbrezil...@kernel.org,
linux-kernel@vger.kernel.org
Cc: Bean Huo 
Subject: [PATCH] mtd: spi-nor: Fix wrong abbreviation HWCPAS
Date: Fri,  8 Feb 2019 18:00:12 +
Message-Id: <20190208180012.25852-1-bean...@micron.com>
X-Mailer: git-send-email 2.17.1

The Cc list above has been expanded by additional
addresses found in the patch commit message. By default
send-email prompts before sending whenever this occurs.
This behavior is controlled by the sendemail.confirm
configuration setting.

For additional information, run 'git send-email --help'.
To retain the current behavior, but squelch this message,
run 'git config --global sendemail.confirm auto'.

Send this email? ([y]es|[n]o|[q]uit|[a]ll): y
OK. Log says:
Sendmail: /usr/sbin/sendmail -i linux-...@lists.infradead.org 
tudor.amba...@microchip.com bbrezil...@kernel.org linux-kernel@vger.kernel.org 
bean...@micron.com
From: Bean Huo 
To: linux-...@lists.infradead.org,
tudor.amba...@microchip.com,
bbrezil...@kernel.org,
linux-kernel@vger.kernel.org
Cc: Bean Huo 
Subject: [RESEND PATCH v2] mtd: spi-nor: Fix wrong abbreviation HWCPAS
Date: Fri,  8 Feb 2019 18:00:12 +
Message-Id: <20190208180012.25852-1-bean...@micron.com>
X-Mailer: git-send-email 2.17.1

Result: OK

>
>Hi Bean,
>
>On Fri, 8 Feb 2019 17:13:52 +
>"Bean Huo (beanhuo)"  wrote:
>
>> Hi, Tutor
>> Thanks. unfortunately, it doesn't work on my side. Problem is on our email
>server side, not my local setting.
>> I followed your configuration, then git-email failed.
>
>Can you paste the output of git send-email and the [sendemail] section of
>your .gitconfig so we can check.
>It shouldn't fail, really, because the server shouldn't check the body section 
>of
>your email, and this is where the extra
>
>       From: First Last 
>
>line is placed.
>
>> Please just change my S-o-b to 'Bean Huo (beanhuo) '
>in my patch to please checkpacth.pl.  It is not huge change.
>
>It's not a problem when an drive-by contributor posts a patch, but if you don't
>patch it that means we'll have to do it manually every time we apply one of 
>your
>patch, so please find a solution.
>
>Thanks,
>
>Boris

[RESEND PATCH v2] mtd: spi-nor: Fix wrong abbreviation HWCPAS

[Bean Huo (beanhuo)]

From: Bean Huo 

Change SNOR_HWCPAS_READ_OCTAL to SNOR_HWCAPS_READ_OCTAL.

Signed-off-by: Bean Huo 
---
 include/linux/mtd/spi-nor.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h index 
2353af8..b3d360b 100644
--- a/include/linux/mtd/spi-nor.h
+++ b/include/linux/mtd/spi-nor.h
@@ -487,7 +487,7 @@ struct spi_nor_hwcaps {
 #define SNOR_HWCAPS_READ_4_4_4 BIT(9)
 #define SNOR_HWCAPS_READ_1_4_4_DTR BIT(10)
 
-#define SNOR_HWCPAS_READ_OCTAL GENMASK(14, 11)
+#define SNOR_HWCAPS_READ_OCTAL GENMASK(14, 11)
 #define SNOR_HWCAPS_READ_1_1_8 BIT(11)
 #define SNOR_HWCAPS_READ_1_8_8 BIT(12)
 #define SNOR_HWCAPS_READ_8_8_8 BIT(13)
--
2.7.4

RE: [EXT] Re: [PATCH v2] mtd: spi-nor: Fix wrong abbreviation HWCPAS

[Bean Huo (beanhuo)]

Hi, Tutor
Thanks. unfortunately, it doesn't work on my side. Problem is on our email 
server side, not my local setting.
I followed your configuration, then git-email failed.
Please just change my S-o-b to 'Bean Huo (beanhuo) ' in my 
patch to please checkpacth.pl.  It is not huge change. 
Thanks,

>
>On 02/08/2019 05:27 PM, Bean Huo (beanhuo) wrote:
>> Hi, Tutor
>>
>>> $ ./scripts/checkpatch.pl --strict
>>> v2-mtd-spi-nor-Fix-wrong-abbreviation-HWCPAS.patch
>>> WARNING: Missing Signed-off-by: line by nominal patch author 'Bean
>>> Huo
>>> (beanhuo) '
>>>
>> I think, this is because of our email system, it always adds '(xxx)'
>> between user name and email address.
>> If you are convenient, please help fix since it is not such hug change.
>
>I'm on a funny email server too, it keeps overwriting my email header "from 
>field".
>I have a workaround that might work for you too. Here's part of my
>.gitconfig:
>
>[user]
>name = Tudor Ambarus
>email = tudor.amba...@microchip.com [sendemail]
>from = Tudor X Ambarus 
>
>Because my [sendemail] from field is different than my [user] name and email, 
>git
>send-email will add in the body of the patch the [user] name and email:
>
>From: Tudor Ambarus 
>
>When the patch is applied, it takes the author name from the patch body. Even 
>if
>the server overwrites the from field from the header, it will be ignored.
>
>Let me know if this works for you.
>ta
>__
>Linux MTD discussion mailing list
>http://lists.infradead.org/mailman/listinfo/linux-mtd/

RE: [EXT] Re: [PATCH v2] mtd: spi-nor: Fix wrong abbreviation HWCPAS

[Bean Huo (beanhuo)]

Hi, Tutor

>$ ./scripts/checkpatch.pl --strict
>v2-mtd-spi-nor-Fix-wrong-abbreviation-HWCPAS.patch
>WARNING: Missing Signed-off-by: line by nominal patch author 'Bean Huo
>(beanhuo) '
>
I think, this is because of our email system, it always adds '(xxx)' between 
user name 
and email address.
If you are convenient, please help fix since it is not such hug change.
Thanks,
//Bean

RE: [EXT] Re: [PATCH v2] mtd: spi-nor: Fix wrong abbreviation HWCPAS

[Bean Huo (beanhuo)]

Hi, Tudor

./scripts/checkpatch.pl --strict 
0001-mtd-spi-nor-Fix-wrong-abbreviation-HWCPAS.patch 
total: 0 errors, 0 warnings, 0 checks, 8 lines checked

0001-mtd-spi-nor-Fix-wrong-abbreviation-HWCPAS.patch has no obvious style 
problems and is ready for submission.

>
>On 02/08/2019 04:23 PM, Bean Huo (beanhuo) wrote:
>> Change SNOR_HWCPAS_READ_OCTAL to SNOR_HWCAPS_READ_OCTAL.
>>
>> Signed-off-by: Bean Huo 
>
>$ ./scripts/checkpatch.pl --strict
>v2-mtd-spi-nor-Fix-wrong-abbreviation-HWCPAS.patch
>WARNING: Missing Signed-off-by: line by nominal patch author 'Bean Huo
>(beanhuo) '
> 
'Bean Huo (beanhuo) ' is wrong.
 If you prefer to 'Bean Huo (beanhuo) , would you please 
change that.

//Bean

[PATCH v2] mtd: spi-nor: Fix wrong abbreviation HWCPAS

[Bean Huo (beanhuo)]

Change SNOR_HWCPAS_READ_OCTAL to SNOR_HWCAPS_READ_OCTAL.

Signed-off-by: Bean Huo 
---
 include/linux/mtd/spi-nor.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
index 2353af8..b3d360b 100644
--- a/include/linux/mtd/spi-nor.h
+++ b/include/linux/mtd/spi-nor.h
@@ -487,7 +487,7 @@ struct spi_nor_hwcaps {
 #define SNOR_HWCAPS_READ_4_4_4 BIT(9)
 #define SNOR_HWCAPS_READ_1_4_4_DTR BIT(10)
 
-#define SNOR_HWCPAS_READ_OCTAL GENMASK(14, 11)
+#define SNOR_HWCAPS_READ_OCTAL GENMASK(14, 11)
 #define SNOR_HWCAPS_READ_1_1_8 BIT(11)
 #define SNOR_HWCAPS_READ_1_8_8 BIT(12)
 #define SNOR_HWCAPS_READ_8_8_8 BIT(13)
-- 
2.7.4

RE: [EXT] Re: [PATCH] include: mtd: spi-nor: change HWCPAS to HWCAPS

[Bean Huo (beanhuo)]

Hi, Tudor

>SNOR_HWCPAS_READ_OCTO was renamed to SNOR_HWCPAS_READ_OCTAL
>please update your remote and send a new patch.
>
Which branch do you mean? I checked linux-mtd, not renamed yet.

>The patch prefix is wrong, it should be prefixed with "mtd: spi-nor:". When in
>doubt, you can check how were prefixed previous patches on the same file by
>running something like:
>
>git log --oneline include/linux/mtd/spi-nor.h
>
>On 02/07/2019 03:46 PM, Bean Huo (beanhuo) wrote:
>>
>
>This looks like a blank line, it's not needed, remove it please.
>> Maybe this is wrong abbreviation, change from HWCPAS to HWCAPS.
>
>Indeed, it is, use imperative, something like: "Fix wrong abbreviation,
>s/HWCPAS/HWCAPS"
>>
>> Signed-off-by: Bean Huo 
>
>Your S-o-b tag doesn't match the author name, please fix this as well.

It is automatically added by git, what is correct way? 
Thanks,
//Bean

[PATCH] include: mtd: spi-nor: change HWCPAS to HWCAPS

[Bean Huo (beanhuo)]

Maybe this is wrong abbreviation, change from HWCPAS to HWCAPS.

Signed-off-by: Bean Huo 
---
 include/linux/mtd/spi-nor.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
index fa2d89e..5e12bf9 100644
--- a/include/linux/mtd/spi-nor.h
+++ b/include/linux/mtd/spi-nor.h
@@ -479,7 +479,7 @@ struct spi_nor_hwcaps {
 #define SNOR_HWCAPS_READ_4_4_4 BIT(9)
 #define SNOR_HWCAPS_READ_1_4_4_DTR BIT(10)
 
-#define SNOR_HWCPAS_READ_OCTO  GENMASK(14, 11)
+#define SNOR_HWCAPS_READ_OCTO  GENMASK(14, 11)
 #define SNOR_HWCAPS_READ_1_1_8 BIT(11)
 #define SNOR_HWCAPS_READ_1_8_8 BIT(12)
 #define SNOR_HWCAPS_READ_8_8_8 BIT(13)
-- 
2.7.4

RE: [EXT] [PATCH v4 3/3] scsi: ufs-bsg: Allow reading descriptors

[Bean Huo (beanhuo)]

>-Original Message-
>From: linux-scsi-ow...@vger.kernel.org [mailto:linux-scsi-
>ow...@vger.kernel.org] On Behalf Of Avri Altman
>Sent: Sunday, January 27, 2019 8:08 AM
>To: James E.J. Bottomley ; Martin K. Petersen
>; linux-s...@vger.kernel.org; linux-
>ker...@vger.kernel.org; Evan Green 
>Cc: Avi Shchislowski ; Alex Lemberg
>; Avri Altman 
>Subject: [EXT] [PATCH v4 3/3] scsi: ufs-bsg: Allow reading descriptors
>
>Add this functionality, placing the descriptor being read in the actual data 
>buffer
>in the bio.
>
>That is, for both read and write descriptors query upiu, we are using the job's
>request_payload.  This in turn, is mapped back in user land to the applicable
>sg_io_v4 xferp: dout_xferp for write descriptor, and din_xferp for read 
>descriptor.
>
>Signed-off-by: Avri Altman 
Reviewed-by: Bean Huo 

RE: [EXT] [PATCH v4 2/3] scsi: ufs: Allow reading descriptor via raw upiu

[Bean Huo (beanhuo)]

>
>Signed-off-by: Avri Altman 
>Reviewed-by: Evan Green 
Reviewed-by: Bean Huo 

RE: [PATCH v4 1/3] scsi: ufs-bsg: Change the calling convention for write descriptor

[Bean Huo (beanhuo)]

Hi, Avri

>
>When we had a write descriptor query upiu, we appended the descriptor right
>after the bsg request.  This was fine as the bsg driver allows to allocate 
>whatever
>buffer we needed in its job request.
>
>Still, the proper way to deliver payload, however small (we only write config
>descriptors of 144 bytes), is by using the job request payload data buffer.
>
>So change this ABI now, while ufs-bsg is still new, and nobody is actually 
>using it.
>
>Signed-off-by: Avri Altman 
>Reviewed-by: Evan Green 
>---
> Documentation/scsi/ufs.txt |  6 ++
> drivers/scsi/ufs/ufs_bsg.c | 47 +-
> 2 files changed, 32 insertions(+), 21 deletions(-)
>
>diff --git a/Documentation/scsi/ufs.txt b/Documentation/scsi/ufs.txt index
>520b5b0..78fe7cb 100644
>--- a/Documentation/scsi/ufs.txt
>+++ b/Documentation/scsi/ufs.txt
>@@ -147,6 +147,12 @@ send SG_IO with the applicable sg_io_v4:
>   io_hdr_v4.max_response_len = reply_len;
>   io_hdr_v4.request_len = request_len;
>   io_hdr_v4.request = (__u64)request_upiu;
>+  if (dir == SG_DXFER_TO_DEV) {
>+  io_hdr_v4.dout_xfer_len = (uint32_t)byte_cnt;
>+  io_hdr_v4.dout_xferp = (uintptr_t)(__u64)buff;
>+  }
>+
>+If you wish to write a descriptor, use the dout_xferp sg_io_v4.
>
> UFS Specifications can be found at,
> UFS - http://www.jedec.org/sites/default/files/docs/JESD220.pdf
>diff --git a/drivers/scsi/ufs/ufs_bsg.c b/drivers/scsi/ufs/ufs_bsg.c index
>775bb4e..2fd0769 100644
>--- a/drivers/scsi/ufs/ufs_bsg.c
>+++ b/drivers/scsi/ufs/ufs_bsg.c
>@@ -27,15 +27,11 @@ static int ufs_bsg_get_query_desc_size(struct ufs_hba
>*hba, int *desc_len,
>
> static int ufs_bsg_verify_query_size(struct ufs_hba *hba,
>unsigned int request_len,
>-   unsigned int reply_len,
>-   int desc_len, enum query_opcode desc_op)
>+   unsigned int reply_len)
> {
>   int min_req_len = sizeof(struct ufs_bsg_request);
>   int min_rsp_len = sizeof(struct ufs_bsg_reply);
>
>-  if (desc_op == UPIU_QUERY_OPCODE_WRITE_DESC)
>-  min_req_len += desc_len;
>-
>   if (min_req_len > request_len || min_rsp_len > reply_len) {
>   dev_err(hba->dev, "not enough space assigned\n");
>   return -EINVAL;
>@@ -44,14 +40,13 @@ static int ufs_bsg_verify_query_size(struct ufs_hba *hba,
>   return 0;
> }
>
>-static int ufs_bsg_verify_query_params(struct ufs_hba *hba,
>- struct ufs_bsg_request *bsg_request,
>- unsigned int request_len,
>- unsigned int reply_len,
>- uint8_t *desc_buff, int *desc_len,
>- enum query_opcode desc_op)
>+static int ufs_bsg_alloc_desc_buffer(struct ufs_hba *hba, struct bsg_job *job,
>+   uint8_t **desc_buff, int *desc_len,

Maybe here also we should use 'u8'.

Reviewed-by: Bean Huo 

//Beanhuo

Re: [PATCH v6 0/6] mtd: rawnand: support MT29F1G08ABAFAWP-ITE:F

[Bean Huo (beanhuo)]

Hi, Boris
>>
>
>Okay. Can you have a look at the patches I sent an let me know if I do the
>right thing?

I am doing today. And will back to you I could give some advice.


I encounter one question as below when I reply the email of linux-mtd, but If I 
reply other mail list 
Such as scsi mail list, mmc mail list, no any problem. Do you know why? 


Your mail to 'linux-mtd' with the subject

Re: [PATCH v6 0/6] mtd: rawnand: support MT29F1G08ABAFAWP-ITE:F

Is being held until the list moderator can review it for approval.

The reason it is being held:

Message has a suspicious header

Either the message will get posted to the list, or you will receive 
notification of the moderator's decision.  If you would like to cancel this 
posting, please visit the following URL:


http://lists.infradead.org/mailman/confirm/linux-mtd/1a28f9a4069a0bbcadfb6b2a170c01dd8b962d0a


//beanhuo

Re: [PATCH v6 0/6] mtd: rawnand: support MT29F1G08ABAFAWP-ITE:F

[Bean Huo (beanhuo)]

Hi, Boris
>>
>
>Okay. Can you have a look at the patches I sent an let me know if I do the
>right thing?

I am doing today. And will back to you I could give some advice.


I encounter one question as below when I reply the email of linux-mtd, but If I 
reply other mail list 
Such as scsi mail list, mmc mail list, no any problem. Do you know why? 


Your mail to 'linux-mtd' with the subject

Re: [PATCH v6 0/6] mtd: rawnand: support MT29F1G08ABAFAWP-ITE:F

Is being held until the list moderator can review it for approval.

The reason it is being held:

Message has a suspicious header

Either the message will get posted to the list, or you will receive 
notification of the moderator's decision.  If you would like to cancel this 
posting, please visit the following URL:


http://lists.infradead.org/mailman/confirm/linux-mtd/1a28f9a4069a0bbcadfb6b2a170c01dd8b962d0a


//beanhuo

Re: [PATCH v6 0/6] mtd: rawnand: support MT29F1G08ABAFAWP-ITE:F

[Bean Huo (beanhuo)]

Hi,  Boris
>>
>> Okay, I think we already had this discussion, but I'm asking it again.
>> What are the possible values for that field and what do they mean?
>
>Still, it's not clear to me what "Internal ECC level" means. It seems that NAND
>chips having on-die ECC have this field set to 10b (0x2), 00b seems to be
>reserved for "no on-die ECC", but what are 01b and 11b reserved for?
>

That position identifies the part as having Internal ECC capability.  
The 01b and 11b are reserved for future definition.  Bit 7 of READ ID Byte 4 
identifies the
device as an ECC OFF 0x0 or ECC ON 0x1.

>> Also, is it even used to encode the fact that the NAND has on-die ECC
>> on all your NANDs? We already had the problem of incompatible ID
>> schemes, so I wouldn't be surprised if that was the case here, hence
>> my initial suggestion to base the detection on the model name.
>
>I'd really need to have an answer on that one to take a decision. Also, I
>couldn't find a datasheet for an IT (without E) version of the
>MT29F1G08ABAFAWP part. Does it exist, or can we assume
>MT29F1G08ABAFAWP chips always come with forcibly enabled on-die ECC?

MT29F1G08ABAFAWP comes in ECC ON only.  We didn’t develop a
MT29F1G08ABAFAWP ECC OFF version.  Bit 7 of READ ID Byte 4 identifies the
device as an ECC OFF 0x0 or ECC ON 0x1.

Re: [PATCH v6 0/6] mtd: rawnand: support MT29F1G08ABAFAWP-ITE:F

[Bean Huo (beanhuo)]

Hi,  Boris
>>
>> Okay, I think we already had this discussion, but I'm asking it again.
>> What are the possible values for that field and what do they mean?
>
>Still, it's not clear to me what "Internal ECC level" means. It seems that NAND
>chips having on-die ECC have this field set to 10b (0x2), 00b seems to be
>reserved for "no on-die ECC", but what are 01b and 11b reserved for?
>

That position identifies the part as having Internal ECC capability.  
The 01b and 11b are reserved for future definition.  Bit 7 of READ ID Byte 4 
identifies the
device as an ECC OFF 0x0 or ECC ON 0x1.

>> Also, is it even used to encode the fact that the NAND has on-die ECC
>> on all your NANDs? We already had the problem of incompatible ID
>> schemes, so I wouldn't be surprised if that was the case here, hence
>> my initial suggestion to base the detection on the model name.
>
>I'd really need to have an answer on that one to take a decision. Also, I
>couldn't find a datasheet for an IT (without E) version of the
>MT29F1G08ABAFAWP part. Does it exist, or can we assume
>MT29F1G08ABAFAWP chips always come with forcibly enabled on-die ECC?

MT29F1G08ABAFAWP comes in ECC ON only.  We didn’t develop a
MT29F1G08ABAFAWP ECC OFF version.  Bit 7 of READ ID Byte 4 identifies the
device as an ECC OFF 0x0 or ECC ON 0x1.

Re: [PATCH v6 0/6] mtd: rawnand: support MT29F1G08ABAFAWP-ITE:F

[Bean Huo (beanhuo)]

Hi, Boris and Chris

>>
>> I see 2 solutions to this problem:
>> 1/ Bean provides us a solution to reliably detect when ECC can be
>>de-actived and when it can't
>> 2/ We only ever expose 64 bytes of OOB to the user and consider that
>>ECC can be disabled, even if it can't in reality
>>
>
>After reading the doc again, I forgot one thing you can try before deciding to
>go for option #2.
>
>8th bit in byte 5 of READID's result encodes whether the on-die ECC state
>(enabled or not). I remember we had a discussion with Bean where he told us
>this was a runtime status reflecting the on-die ECC state, which is crazy, 
>since
>READID might return different values depending on the NAND state, and most
>of the code in the core assumes READID provides a fixed ID that encodes the
>chip characteristics/capabilities, not its state.
>
>Anyway, if this bit is actually reflecting the on-die ECC state and on-die
>cannot be disabled on your chip, it should stay at 1 even after you have sent
>the SET_FEATURES(DISABLE_ECC) command. Let's hope this works as I expect,
>otherwise we're back to option #2 until Bean suggest something else.

MT29F1G08ABAFAWP-ITE:F is Micron 70s SLC NAND with on-die ECC.
If the bit7 in the byte 5 of READID default is 1(ECC Enabled), it is true 
on-die ECC
Cannot be disabled by SET Feature command.
In case sending Set Feature command, you can check previous command is success 
or not by
Get Feature or check this bit7 in byte 5 of READID later.

To check if this device supports on-die ECC or not, you can use inter ECC level 
(bit 0 and bit1 in byte 5 of READID)  
http://lists.infradead.org/pipermail/linux-mtd/2017-April/073370.html

//beanhuo

Re: [PATCH v6 0/6] mtd: rawnand: support MT29F1G08ABAFAWP-ITE:F

[Bean Huo (beanhuo)]

Hi, Boris and Chris

>>
>> I see 2 solutions to this problem:
>> 1/ Bean provides us a solution to reliably detect when ECC can be
>>de-actived and when it can't
>> 2/ We only ever expose 64 bytes of OOB to the user and consider that
>>ECC can be disabled, even if it can't in reality
>>
>
>After reading the doc again, I forgot one thing you can try before deciding to
>go for option #2.
>
>8th bit in byte 5 of READID's result encodes whether the on-die ECC state
>(enabled or not). I remember we had a discussion with Bean where he told us
>this was a runtime status reflecting the on-die ECC state, which is crazy, 
>since
>READID might return different values depending on the NAND state, and most
>of the code in the core assumes READID provides a fixed ID that encodes the
>chip characteristics/capabilities, not its state.
>
>Anyway, if this bit is actually reflecting the on-die ECC state and on-die
>cannot be disabled on your chip, it should stay at 1 even after you have sent
>the SET_FEATURES(DISABLE_ECC) command. Let's hope this works as I expect,
>otherwise we're back to option #2 until Bean suggest something else.

MT29F1G08ABAFAWP-ITE:F is Micron 70s SLC NAND with on-die ECC.
If the bit7 in the byte 5 of READID default is 1(ECC Enabled), it is true 
on-die ECC
Cannot be disabled by SET Feature command.
In case sending Set Feature command, you can check previous command is success 
or not by
Get Feature or check this bit7 in byte 5 of READID later.

To check if this device supports on-die ECC or not, you can use inter ECC level 
(bit 0 and bit1 in byte 5 of READID)  
http://lists.infradead.org/pipermail/linux-mtd/2017-April/073370.html

//beanhuo

Re: [RESEND PATCH v1 1/2] trace: events: scsi: Add tag in SCSI trace events

[Bean Huo (beanhuo)]

>On Mon, 2018-04-16 at 20:27 +0000, Bean Huo (beanhuo) wrote:
>> By the way, these patches are not to add new feature, they are just to
>> add print tag along with the other exist printed request parameters.
>
>Are you aware that there are two tag fields in struct request, namely "tag"
>and "internal_tag"? Are you aware that how these fields are used depends on
>whether or not a scheduler is attached to a request queue? Have you verified
>that the "tag" field contains a meaningful value for blk-mq for every blk-mq
>tracepoint?
>
>Thanks,
>
>Bart.
>
>
Yes, I noticed and was aware, there are tag and internal_tag.
One thing I was not aware is that the patches would make such a big impact on 
blk-mq based on
Your comments.

I am not expert on block layer, that is why I added block maintainer here.
Please point out which line and what is problem? If there is some wrong with 
the patch.
Which one is correct "tag" to track request? 

Thanks,
//Bean Huo

Re: [RESEND PATCH v1 1/2] trace: events: scsi: Add tag in SCSI trace events

[Bean Huo (beanhuo)]

>On Mon, 2018-04-16 at 20:27 +0000, Bean Huo (beanhuo) wrote:
>> By the way, these patches are not to add new feature, they are just to
>> add print tag along with the other exist printed request parameters.
>
>Are you aware that there are two tag fields in struct request, namely "tag"
>and "internal_tag"? Are you aware that how these fields are used depends on
>whether or not a scheduler is attached to a request queue? Have you verified
>that the "tag" field contains a meaningful value for blk-mq for every blk-mq
>tracepoint?
>
>Thanks,
>
>Bart.
>
>
Yes, I noticed and was aware, there are tag and internal_tag.
One thing I was not aware is that the patches would make such a big impact on 
blk-mq based on
Your comments.

I am not expert on block layer, that is why I added block maintainer here.
Please point out which line and what is problem? If there is some wrong with 
the patch.
Which one is correct "tag" to track request? 

Thanks,
//Bean Huo

Re: [RESEND PATCH v1 1/2] trace: events: scsi: Add tag in SCSI trace events

[Bean Huo (beanhuo)]

Hi, Steve 
Right, Please see below portion log from ftrace and blktrace,
There is no any impact on blktrace. 
>
>Looking at the code from
>git://git.kernel.org/pub/scm/linux/kernel/git/axboe/blktrace.git
>
>It appears that it does not rely on the ftrace ring buffers.
>
>So I'm guessing blktrace is not affected by this patch.
>
>-- Steve


#/blktrace -d /dev/block/sdd14 -o - | ./blkparse -i -  
  8,62   3 1162 0.213039583  4116  P   N [iozone]
  8,62   3 1163 0.213041146  4116  U   N [iozone] 1
  8,62   3 1164 0.213042708  4116  I  WS 39573632 + 128 [iozone]
  8,62   3 1165 0.213044791  4116  D  WS 39573632 + 128 [iozone]
  8,48   3 1166 0.213585416  4116  A  WS 39573760 + 128 <- (8,62) 342272
  8,62   3 1167 0.213590104  4116  Q  WS 39573760 + 128 [iozone]
  8,62   3 1168 0.213592187  4116  G  WS 39573760 + 128 [iozone]
  8,62   3 1169 0.213594271  4116  P   N [iozone]
  8,62   3 1170 0.213595833  4116  U   N [iozone] 1
  8,62   3 1171 0.213597396  4116  I  WS 39573760 + 128 [iozone]
  8,62   3 1172 0.21360  4116  D  WS 39573760 + 128 [iozone]
  8,48   3 1173 0.214092187  4116  A  WS 39573888 + 128 <- (8,62) 342400
  8,62   3 1174 0.214094791  4116  Q  WS 39573888 + 128 [iozone]
  8,62   3 1175 0.214097916  4116  G  WS 39573888 + 128 [iozone]
  8,62   3 1176 0.214102604  4116  P   N [iozone]
  8,62   3 1177 0.214105208  4116  U   N [iozone] 1
  8,62   3 1178 0.214106771  4116  I  WS 39573888 + 128 [iozone]
  8,62   3 1179 0.214111458  4116  D  WS 39573888 + 128 [iozone]
  8,48   3 1180 0.216531771  4115  A  WS 39546496 + 128 <- (8,62) 315008
  8,62   3 1181 0.216534896  4115  Q  WS 39546496 + 128 [iozone]
  8,62   3 1182 0.216538021  4115  G  WS 39546496 + 128 [iozone]
  8,62   3 1183 0.216540625  4115  P   N [iozone]
  8,62   3 1184 0.216543229  4115  U   N [iozone] 1
  8,62   3 1185 0.216544791  4115  I  WS 39546496 + 128 [iozone]
  8,62   3 1186 0.216547396  4115  D  WS 39546496 + 128 [iozone]
  8,48   3 1187 0.217146354  4115  A  WS 39546624 + 128 <- (8,62) 315136
  8,62   3 1188 0.217148437  4115  Q  WS 39546624 + 128 [iozone]
  8,62   3 1189 0.217151041  4115  G  WS 39546624 + 128 [iozone]
  8,62   3 1190 0.217153646  4115  P   N [iozone]
  8,62   3 1191 0.217155208  4115  U   N [iozone] 1
  8,62   3 1192 0.217156771  4115  I  WS 39546624 + 128 [iozone]
  8,62   3 1193 0.217159896  4115  D  WS 39546624 + 128 [iozone]
  8,48   3 1194 0.217712500  4115  A  WS 39546752 + 128 <- (8,62) 315264
  8,62   3 1195 0.217714583  4115  Q  WS 39546752 + 128 [iozone]
  8,62   3 1196 0.217717708  4115  G  WS 39546752 + 128 [iozone]
  8,62   3 1197 0.217722396  4115  P   N [iozone]
  8,62   3 1198 0.217724479  4115  U   N [iozone] 1
  8,62   3 1199 0.217726041  4115  I  WS 39546752 + 128 [iozone]
  8,62   3 1200 0.217728646  4115  D  WS 39546752 + 128 [iozone]
  8,62   5 1150 0.198047916 0  C  WS 39569920 + 128 [0]
  8,62   4 1104 0.198104687 0  C  WS 39576448 + 128 [0]
  8,48   5 1151 0.198182291  4116  A  WS 39570048 + 128 <- (8,62) 338560
  8,62   5 1152 0.19818  4116  Q  WS 39570048 + 128 [iozone]
  8,62   5 1153 0.198184375  4116  G  WS 39570048 + 128 [iozone]
  8,62   5 1154 0.198185937  4116  P   N [iozone]
  8,62   5 1155 0.198186458  4116  U   N [iozone] 1
  8,62   5 1156 0.198186979  4116  I  WS 39570048 + 128 [iozone]
  8,62   5 1157 0.198188541  4116  D  WS 39570048 + 128 [iozone]
  8,48   4 1105 0.198218229  4114  A  WS 39576576 + 128 <- (8,62) 345088
  8,62   4 1106 0.198219271  4114  Q  WS 39576576 + 128 [iozone]
  8,62   4 1107 0.198220312  4114  G  WS 39576576 + 128 [iozone]
  8,62   4 1108 0.198221354  4114  P   N [iozone]
  8,62   4 1109 0.198221875  4114  U   N [iozone] 1
  8,62   4 1110 0.198222396  4114  I  WS 39576576 + 128 [iozone]
  8,62   4  0.198223437  4114  D  WS 39576576 + 128 [iozone]
  8,62   4 1112 0.198245312  4114  C  WS 39594368 + 128 [0]
  8,62   5 1158 0.198336979 0  C  WS 39570048 + 128 [0]
  8,62   4 1113 0.198409375 0  C  WS 39576576 + 128 [0]
  8,48   5 1159 0.199219791  4113  A  WS 39594624 + 128 <- (8,62) 363136
  8,62   5 1160 0.199220833  4113  Q  WS 39594624 + 128 [iozone]
  8,62   5 1161 0.199222396  4113  G  WS 39594624 + 128 [iozone]
  8,62   5 1162 0.199223437  4113  P   N [iozone]
  8,62   5 1163 0.199224479  4113  U   N [iozone] 1
  8,62   5 1164 0.199225000  4113  I  WS 39594624 + 128 [iozone]
  8,62   5 1165 0.199226562  4113  D  WS 39594624 + 128 [iozone]
  8,48   4 1114 0.199235937  4116  A  WS 39570176 + 128 <- 

Re: [RESEND PATCH v1 1/2] trace: events: scsi: Add tag in SCSI trace events

[Bean Huo (beanhuo)]

Hi, Steve 
Right, Please see below portion log from ftrace and blktrace,
There is no any impact on blktrace. 
>
>Looking at the code from
>git://git.kernel.org/pub/scm/linux/kernel/git/axboe/blktrace.git
>
>It appears that it does not rely on the ftrace ring buffers.
>
>So I'm guessing blktrace is not affected by this patch.
>
>-- Steve


#/blktrace -d /dev/block/sdd14 -o - | ./blkparse -i -  
  8,62   3 1162 0.213039583  4116  P   N [iozone]
  8,62   3 1163 0.213041146  4116  U   N [iozone] 1
  8,62   3 1164 0.213042708  4116  I  WS 39573632 + 128 [iozone]
  8,62   3 1165 0.213044791  4116  D  WS 39573632 + 128 [iozone]
  8,48   3 1166 0.213585416  4116  A  WS 39573760 + 128 <- (8,62) 342272
  8,62   3 1167 0.213590104  4116  Q  WS 39573760 + 128 [iozone]
  8,62   3 1168 0.213592187  4116  G  WS 39573760 + 128 [iozone]
  8,62   3 1169 0.213594271  4116  P   N [iozone]
  8,62   3 1170 0.213595833  4116  U   N [iozone] 1
  8,62   3 1171 0.213597396  4116  I  WS 39573760 + 128 [iozone]
  8,62   3 1172 0.21360  4116  D  WS 39573760 + 128 [iozone]
  8,48   3 1173 0.214092187  4116  A  WS 39573888 + 128 <- (8,62) 342400
  8,62   3 1174 0.214094791  4116  Q  WS 39573888 + 128 [iozone]
  8,62   3 1175 0.214097916  4116  G  WS 39573888 + 128 [iozone]
  8,62   3 1176 0.214102604  4116  P   N [iozone]
  8,62   3 1177 0.214105208  4116  U   N [iozone] 1
  8,62   3 1178 0.214106771  4116  I  WS 39573888 + 128 [iozone]
  8,62   3 1179 0.214111458  4116  D  WS 39573888 + 128 [iozone]
  8,48   3 1180 0.216531771  4115  A  WS 39546496 + 128 <- (8,62) 315008
  8,62   3 1181 0.216534896  4115  Q  WS 39546496 + 128 [iozone]
  8,62   3 1182 0.216538021  4115  G  WS 39546496 + 128 [iozone]
  8,62   3 1183 0.216540625  4115  P   N [iozone]
  8,62   3 1184 0.216543229  4115  U   N [iozone] 1
  8,62   3 1185 0.216544791  4115  I  WS 39546496 + 128 [iozone]
  8,62   3 1186 0.216547396  4115  D  WS 39546496 + 128 [iozone]
  8,48   3 1187 0.217146354  4115  A  WS 39546624 + 128 <- (8,62) 315136
  8,62   3 1188 0.217148437  4115  Q  WS 39546624 + 128 [iozone]
  8,62   3 1189 0.217151041  4115  G  WS 39546624 + 128 [iozone]
  8,62   3 1190 0.217153646  4115  P   N [iozone]
  8,62   3 1191 0.217155208  4115  U   N [iozone] 1
  8,62   3 1192 0.217156771  4115  I  WS 39546624 + 128 [iozone]
  8,62   3 1193 0.217159896  4115  D  WS 39546624 + 128 [iozone]
  8,48   3 1194 0.217712500  4115  A  WS 39546752 + 128 <- (8,62) 315264
  8,62   3 1195 0.217714583  4115  Q  WS 39546752 + 128 [iozone]
  8,62   3 1196 0.217717708  4115  G  WS 39546752 + 128 [iozone]
  8,62   3 1197 0.217722396  4115  P   N [iozone]
  8,62   3 1198 0.217724479  4115  U   N [iozone] 1
  8,62   3 1199 0.217726041  4115  I  WS 39546752 + 128 [iozone]
  8,62   3 1200 0.217728646  4115  D  WS 39546752 + 128 [iozone]
  8,62   5 1150 0.198047916 0  C  WS 39569920 + 128 [0]
  8,62   4 1104 0.198104687 0  C  WS 39576448 + 128 [0]
  8,48   5 1151 0.198182291  4116  A  WS 39570048 + 128 <- (8,62) 338560
  8,62   5 1152 0.19818  4116  Q  WS 39570048 + 128 [iozone]
  8,62   5 1153 0.198184375  4116  G  WS 39570048 + 128 [iozone]
  8,62   5 1154 0.198185937  4116  P   N [iozone]
  8,62   5 1155 0.198186458  4116  U   N [iozone] 1
  8,62   5 1156 0.198186979  4116  I  WS 39570048 + 128 [iozone]
  8,62   5 1157 0.198188541  4116  D  WS 39570048 + 128 [iozone]
  8,48   4 1105 0.198218229  4114  A  WS 39576576 + 128 <- (8,62) 345088
  8,62   4 1106 0.198219271  4114  Q  WS 39576576 + 128 [iozone]
  8,62   4 1107 0.198220312  4114  G  WS 39576576 + 128 [iozone]
  8,62   4 1108 0.198221354  4114  P   N [iozone]
  8,62   4 1109 0.198221875  4114  U   N [iozone] 1
  8,62   4 1110 0.198222396  4114  I  WS 39576576 + 128 [iozone]
  8,62   4  0.198223437  4114  D  WS 39576576 + 128 [iozone]
  8,62   4 1112 0.198245312  4114  C  WS 39594368 + 128 [0]
  8,62   5 1158 0.198336979 0  C  WS 39570048 + 128 [0]
  8,62   4 1113 0.198409375 0  C  WS 39576576 + 128 [0]
  8,48   5 1159 0.199219791  4113  A  WS 39594624 + 128 <- (8,62) 363136
  8,62   5 1160 0.199220833  4113  Q  WS 39594624 + 128 [iozone]
  8,62   5 1161 0.199222396  4113  G  WS 39594624 + 128 [iozone]
  8,62   5 1162 0.199223437  4113  P   N [iozone]
  8,62   5 1163 0.199224479  4113  U   N [iozone] 1
  8,62   5 1164 0.199225000  4113  I  WS 39594624 + 128 [iozone]
  8,62   5 1165 0.199226562  4113  D  WS 39594624 + 128 [iozone]
  8,48   4 1114 0.199235937  4116  A  WS 39570176 + 128 <- 

Re: [RESEND PATCH v1 1/2] trace: events: scsi: Add tag in SCSI trace events

[Bean Huo (beanhuo)]

Hi, Bart

>mi...@redhad.com; linux-bl...@vger.kernel.org; raja...@google.com
>Subject: [EXT] Re: [RESEND PATCH v1 1/2] trace: events: scsi: Add tag in SCSI
>trace events
>
>On Mon, 2018-04-16 at 14:31 +0000, Bean Huo (beanhuo) wrote:
>>  TP_printk("host_no=%u channel=%u id=%u lun=%u data_sgl=%u
>prot_sgl=%u" \
>> -  " prot_op=%s cmnd=(%s %s raw=%s)",
>> +  " prot_op=%s tag=%d cmnd=(%s %s raw=%s)",
>>
>> [ ... ]
>>  TP_printk("host_no=%u channel=%u id=%u lun=%u data_sgl=%u
>prot_sgl=%u" \
>> -  " prot_op=%s cmnd=(%s %s raw=%s) rtn=%d",
>> +  " prot_op=%s tag=%d cmnd=(%s %s raw=%s) rtn=%d",
>> [ ... ]
>>  TP_printk("host_no=%u channel=%u id=%u lun=%u data_sgl=%u " \
>> -  "prot_sgl=%u prot_op=%s cmnd=(%s %s raw=%s)
>result=(driver=" \
>> -  "%s host=%s message=%s status=%s)",
>> +  "prot_sgl=%u prot_op=%s tag=%d cmnd=(%s %s raw=%s) " \
>> +  "result=(driver=%s host=%s message=%s status=%s)",
>
>Which tools process these strings? Has it been verified whether or not the
>tools that process these strings still work fine with this patch applied?
> 

I don't use one certain special tool to analyze this string, I am using ftrace 
with event.
With tag information, I can see how many tasks in storage device and easily to 
trace each request
under multiple thread workload. 
Event there is someone who using certain tool to analyze this string, after 
adding additional
tag printout, in my opinion, they are happy to see it there. 

Again, you said if we add new feature in legacy block, we will also add new 
feature in blk-mq.
I still don't understand here.  "include/trace/event/block.h ... scsi.h" will 
be changed?
If yes, how? Because blk-mq is still using the events defined in 
include/trace/event/block.h.

Bean Huo

>Thanks,
>
>Bart.
>

Re: [RESEND PATCH v1 1/2] trace: events: scsi: Add tag in SCSI trace events

[Bean Huo (beanhuo)]

Hi, Bart

>mi...@redhad.com; linux-bl...@vger.kernel.org; raja...@google.com
>Subject: [EXT] Re: [RESEND PATCH v1 1/2] trace: events: scsi: Add tag in SCSI
>trace events
>
>On Mon, 2018-04-16 at 14:31 +0000, Bean Huo (beanhuo) wrote:
>>  TP_printk("host_no=%u channel=%u id=%u lun=%u data_sgl=%u
>prot_sgl=%u" \
>> -  " prot_op=%s cmnd=(%s %s raw=%s)",
>> +  " prot_op=%s tag=%d cmnd=(%s %s raw=%s)",
>>
>> [ ... ]
>>  TP_printk("host_no=%u channel=%u id=%u lun=%u data_sgl=%u
>prot_sgl=%u" \
>> -  " prot_op=%s cmnd=(%s %s raw=%s) rtn=%d",
>> +  " prot_op=%s tag=%d cmnd=(%s %s raw=%s) rtn=%d",
>> [ ... ]
>>  TP_printk("host_no=%u channel=%u id=%u lun=%u data_sgl=%u " \
>> -  "prot_sgl=%u prot_op=%s cmnd=(%s %s raw=%s)
>result=(driver=" \
>> -  "%s host=%s message=%s status=%s)",
>> +  "prot_sgl=%u prot_op=%s tag=%d cmnd=(%s %s raw=%s) " \
>> +  "result=(driver=%s host=%s message=%s status=%s)",
>
>Which tools process these strings? Has it been verified whether or not the
>tools that process these strings still work fine with this patch applied?
> 

I don't use one certain special tool to analyze this string, I am using ftrace 
with event.
With tag information, I can see how many tasks in storage device and easily to 
trace each request
under multiple thread workload. 
Event there is someone who using certain tool to analyze this string, after 
adding additional
tag printout, in my opinion, they are happy to see it there. 

Again, you said if we add new feature in legacy block, we will also add new 
feature in blk-mq.
I still don't understand here.  "include/trace/event/block.h ... scsi.h" will 
be changed?
If yes, how? Because blk-mq is still using the events defined in 
include/trace/event/block.h.

Bean Huo

>Thanks,
>
>Bart.
>

Re: [RESEND PATCH v1 1/2] trace: events: scsi: Add tag in SCSI trace events

[Bean Huo (beanhuo)]

>>> This patch is not acceptable because it adds support for tag tracing
>>> to the legacy block layer only. Any patch that adds a new feature to
>>> the legacy block layer must also add it to blk-mq.
>>>
>> To be honest, I don't understand your point, can you give me more
>explanation?
>
>The legacy block layer will be removed as soon as blk-mq provides all the
>functionality of the legacy block layer and as soon as it performs at least as
>well as the legacy block layer for all use cases. If new features are added to
>the legacy block layer but not to blk-mq that prevents removal of the legacy
>block layer. Hence the requirement I explained in my previous e-mail.
>
>Bart.

Thanks for your information.
I have several questions again.
When the legacy block layer will be replaced by blk-mq? 
And "include/trece/event/block.h .. scsi.h" will also be changed? 
Do you have the related git rep or mail list about this topic?
Maybe this is great big change, I am very interested in that. And want to have 
a look at.

By the way, these patches are not to add new feature, they are just to add 
print tag along with the other exist
Printed request parameters.  The blk-mq is now still using 
"include/trace/evet/block.h" defined trace events.

For example: 
void blk_mq_start_request(struct request *rq)  
{
...
...
trace_block_rq_issue(q, rq);
...
...
}
Do you mean that this will also be removed/replaced by someone else?

Bean Huo

Re: [RESEND PATCH v1 1/2] trace: events: scsi: Add tag in SCSI trace events

[Bean Huo (beanhuo)]

>>> This patch is not acceptable because it adds support for tag tracing
>>> to the legacy block layer only. Any patch that adds a new feature to
>>> the legacy block layer must also add it to blk-mq.
>>>
>> To be honest, I don't understand your point, can you give me more
>explanation?
>
>The legacy block layer will be removed as soon as blk-mq provides all the
>functionality of the legacy block layer and as soon as it performs at least as
>well as the legacy block layer for all use cases. If new features are added to
>the legacy block layer but not to blk-mq that prevents removal of the legacy
>block layer. Hence the requirement I explained in my previous e-mail.
>
>Bart.

Thanks for your information.
I have several questions again.
When the legacy block layer will be replaced by blk-mq? 
And "include/trece/event/block.h .. scsi.h" will also be changed? 
Do you have the related git rep or mail list about this topic?
Maybe this is great big change, I am very interested in that. And want to have 
a look at.

By the way, these patches are not to add new feature, they are just to add 
print tag along with the other exist
Printed request parameters.  The blk-mq is now still using 
"include/trace/evet/block.h" defined trace events.

For example: 
void blk_mq_start_request(struct request *rq)  
{
...
...
trace_block_rq_issue(q, rq);
...
...
}
Do you mean that this will also be removed/replaced by someone else?

Bean Huo

Re: [RESEND PATCH v1 1/2] trace: events: scsi: Add tag in SCSI trace events

[Bean Huo (beanhuo)]

Hi, Bart

>On Mon, 2018-04-16 at 09:41 -0700, Rajat Jain wrote:
>> On Mon, Apr 16, 2018 at 8:28 AM, Steven Rostedt <rost...@goodmis.org>
>wrote:
>> > On Mon, 16 Apr 2018 14:31:49 +0000
>> > "Bean Huo (beanhuo)" <bean...@micron.com> wrote:
>> >
>> > > Print the request tag along with other information while tracing a
>> > > command.
>> > >
>> > > Signed-off-by: Bean Huo <bean...@micron.com>
>>
>> Acked-by: Rajat Jain <raja...@google.com>
>
>This patch is not acceptable because it adds support for tag tracing to the
>legacy block layer only. Any patch that adds a new feature to the legacy block
>layer must also add it to blk-mq.
>
To be honest, I don't understand your point, can you give me more explanation?

>Bart.
>
>

Re: [RESEND PATCH v1 1/2] trace: events: scsi: Add tag in SCSI trace events

[Bean Huo (beanhuo)]

Hi, Bart

>On Mon, 2018-04-16 at 09:41 -0700, Rajat Jain wrote:
>> On Mon, Apr 16, 2018 at 8:28 AM, Steven Rostedt 
>wrote:
>> > On Mon, 16 Apr 2018 14:31:49 +
>> > "Bean Huo (beanhuo)"  wrote:
>> >
>> > > Print the request tag along with other information while tracing a
>> > > command.
>> > >
>> > > Signed-off-by: Bean Huo 
>>
>> Acked-by: Rajat Jain 
>
>This patch is not acceptable because it adds support for tag tracing to the
>legacy block layer only. Any patch that adds a new feature to the legacy block
>layer must also add it to blk-mq.
>
To be honest, I don't understand your point, can you give me more explanation?

>Bart.
>
>

[RESEND PATCH v1 1/2] trace: events: scsi: Add tag in SCSI trace events

[Bean Huo (beanhuo)]

Print the request tag along with other information
while tracing a command.

Signed-off-by: Bean Huo 
---
 include/trace/events/scsi.h | 20 +---
 1 file changed, 13 insertions(+), 7 deletions(-)

diff --git a/include/trace/events/scsi.h b/include/trace/events/scsi.h
index f624969..a4ada90 100644
--- a/include/trace/events/scsi.h
+++ b/include/trace/events/scsi.h
@@ -210,6 +210,7 @@ TRACE_EVENT(scsi_dispatch_cmd_start,
__field( unsigned int,  lun )
__field( unsigned int,  opcode  )
__field( unsigned int,  cmd_len )
+   __field( int,   tag )
__field( unsigned int,  data_sglen )
__field( unsigned int,  prot_sglen )
__field( unsigned char, prot_op )
@@ -223,6 +224,7 @@ TRACE_EVENT(scsi_dispatch_cmd_start,
__entry->lun= cmd->device->lun;
__entry->opcode = cmd->cmnd[0];
__entry->cmd_len= cmd->cmd_len;
+   __entry->tag= cmd->request->tag;
__entry->data_sglen = scsi_sg_count(cmd);
__entry->prot_sglen = scsi_prot_sg_count(cmd);
__entry->prot_op= scsi_get_prot_op(cmd);
@@ -230,10 +232,10 @@ TRACE_EVENT(scsi_dispatch_cmd_start,
),
 
TP_printk("host_no=%u channel=%u id=%u lun=%u data_sgl=%u prot_sgl=%u" \
- " prot_op=%s cmnd=(%s %s raw=%s)",
+ " prot_op=%s tag=%d cmnd=(%s %s raw=%s)",
  __entry->host_no, __entry->channel, __entry->id,
  __entry->lun, __entry->data_sglen, __entry->prot_sglen,
- show_prot_op_name(__entry->prot_op),
+ show_prot_op_name(__entry->prot_op), __entry->tag,
  show_opcode_name(__entry->opcode),
  __parse_cdb(__get_dynamic_array(cmnd), __entry->cmd_len),
  __print_hex(__get_dynamic_array(cmnd), __entry->cmd_len))
@@ -253,6 +255,7 @@ TRACE_EVENT(scsi_dispatch_cmd_error,
__field( int,   rtn )
__field( unsigned int,  opcode  )
__field( unsigned int,  cmd_len )
+   __field( int,   tag )
__field( unsigned int,  data_sglen )
__field( unsigned int,  prot_sglen )
__field( unsigned char, prot_op )
@@ -267,6 +270,7 @@ TRACE_EVENT(scsi_dispatch_cmd_error,
__entry->rtn= rtn;
__entry->opcode = cmd->cmnd[0];
__entry->cmd_len= cmd->cmd_len;
+   __entry->tag= cmd->request->tag;
__entry->data_sglen = scsi_sg_count(cmd);
__entry->prot_sglen = scsi_prot_sg_count(cmd);
__entry->prot_op= scsi_get_prot_op(cmd);
@@ -274,10 +278,10 @@ TRACE_EVENT(scsi_dispatch_cmd_error,
),
 
TP_printk("host_no=%u channel=%u id=%u lun=%u data_sgl=%u prot_sgl=%u" \
- " prot_op=%s cmnd=(%s %s raw=%s) rtn=%d",
+ " prot_op=%s tag=%d cmnd=(%s %s raw=%s) rtn=%d",
  __entry->host_no, __entry->channel, __entry->id,
  __entry->lun, __entry->data_sglen, __entry->prot_sglen,
- show_prot_op_name(__entry->prot_op),
+ show_prot_op_name(__entry->prot_op), __entry->tag,
  show_opcode_name(__entry->opcode),
  __parse_cdb(__get_dynamic_array(cmnd), __entry->cmd_len),
  __print_hex(__get_dynamic_array(cmnd), __entry->cmd_len),
@@ -298,6 +302,7 @@ DECLARE_EVENT_CLASS(scsi_cmd_done_timeout_template,
__field( int,   result  )
__field( unsigned int,  opcode  )
__field( unsigned int,  cmd_len )
+   __field( int,   tag )
__field( unsigned int,  data_sglen )
__field( unsigned int,  prot_sglen )
__field( unsigned char, prot_op )
@@ -312,6 +317,7 @@ DECLARE_EVENT_CLASS(scsi_cmd_done_timeout_template,
__entry->result = cmd->result;
__entry->opcode = cmd->cmnd[0];
__entry->cmd_len= cmd->cmd_len;
+   __entry->tag= cmd->request->tag;
__entry->data_sglen = scsi_sg_count(cmd);
__entry->prot_sglen = scsi_prot_sg_count(cmd);
__entry->prot_op= scsi_get_prot_op(cmd);
@@ -319,11 +325,11 @@ DECLARE_EVENT_CLASS(scsi_cmd_done_timeout_template,
),
 
TP_printk("host_no=%u channel=%u id=%u lun=%u data_sgl=%u " \
- "prot_sgl=%u prot_op=%s cmnd=(%s %s raw=%s) result=(driver=" \
- "%s host=%s message=%s status=%s)",
+ "prot_sgl=%u prot_op=%s tag=%d cmnd=(%s %s raw=%s) " \
+ "result=(driver=%s host=%s message=%s 

[RESEND PATCH v1 1/2] trace: events: scsi: Add tag in SCSI trace events

[Bean Huo (beanhuo)]

Print the request tag along with other information
while tracing a command.

Signed-off-by: Bean Huo 
---
 include/trace/events/scsi.h | 20 +---
 1 file changed, 13 insertions(+), 7 deletions(-)

diff --git a/include/trace/events/scsi.h b/include/trace/events/scsi.h
index f624969..a4ada90 100644
--- a/include/trace/events/scsi.h
+++ b/include/trace/events/scsi.h
@@ -210,6 +210,7 @@ TRACE_EVENT(scsi_dispatch_cmd_start,
__field( unsigned int,  lun )
__field( unsigned int,  opcode  )
__field( unsigned int,  cmd_len )
+   __field( int,   tag )
__field( unsigned int,  data_sglen )
__field( unsigned int,  prot_sglen )
__field( unsigned char, prot_op )
@@ -223,6 +224,7 @@ TRACE_EVENT(scsi_dispatch_cmd_start,
__entry->lun= cmd->device->lun;
__entry->opcode = cmd->cmnd[0];
__entry->cmd_len= cmd->cmd_len;
+   __entry->tag= cmd->request->tag;
__entry->data_sglen = scsi_sg_count(cmd);
__entry->prot_sglen = scsi_prot_sg_count(cmd);
__entry->prot_op= scsi_get_prot_op(cmd);
@@ -230,10 +232,10 @@ TRACE_EVENT(scsi_dispatch_cmd_start,
),
 
TP_printk("host_no=%u channel=%u id=%u lun=%u data_sgl=%u prot_sgl=%u" \
- " prot_op=%s cmnd=(%s %s raw=%s)",
+ " prot_op=%s tag=%d cmnd=(%s %s raw=%s)",
  __entry->host_no, __entry->channel, __entry->id,
  __entry->lun, __entry->data_sglen, __entry->prot_sglen,
- show_prot_op_name(__entry->prot_op),
+ show_prot_op_name(__entry->prot_op), __entry->tag,
  show_opcode_name(__entry->opcode),
  __parse_cdb(__get_dynamic_array(cmnd), __entry->cmd_len),
  __print_hex(__get_dynamic_array(cmnd), __entry->cmd_len))
@@ -253,6 +255,7 @@ TRACE_EVENT(scsi_dispatch_cmd_error,
__field( int,   rtn )
__field( unsigned int,  opcode  )
__field( unsigned int,  cmd_len )
+   __field( int,   tag )
__field( unsigned int,  data_sglen )
__field( unsigned int,  prot_sglen )
__field( unsigned char, prot_op )
@@ -267,6 +270,7 @@ TRACE_EVENT(scsi_dispatch_cmd_error,
__entry->rtn= rtn;
__entry->opcode = cmd->cmnd[0];
__entry->cmd_len= cmd->cmd_len;
+   __entry->tag= cmd->request->tag;
__entry->data_sglen = scsi_sg_count(cmd);
__entry->prot_sglen = scsi_prot_sg_count(cmd);
__entry->prot_op= scsi_get_prot_op(cmd);
@@ -274,10 +278,10 @@ TRACE_EVENT(scsi_dispatch_cmd_error,
),
 
TP_printk("host_no=%u channel=%u id=%u lun=%u data_sgl=%u prot_sgl=%u" \
- " prot_op=%s cmnd=(%s %s raw=%s) rtn=%d",
+ " prot_op=%s tag=%d cmnd=(%s %s raw=%s) rtn=%d",
  __entry->host_no, __entry->channel, __entry->id,
  __entry->lun, __entry->data_sglen, __entry->prot_sglen,
- show_prot_op_name(__entry->prot_op),
+ show_prot_op_name(__entry->prot_op), __entry->tag,
  show_opcode_name(__entry->opcode),
  __parse_cdb(__get_dynamic_array(cmnd), __entry->cmd_len),
  __print_hex(__get_dynamic_array(cmnd), __entry->cmd_len),
@@ -298,6 +302,7 @@ DECLARE_EVENT_CLASS(scsi_cmd_done_timeout_template,
__field( int,   result  )
__field( unsigned int,  opcode  )
__field( unsigned int,  cmd_len )
+   __field( int,   tag )
__field( unsigned int,  data_sglen )
__field( unsigned int,  prot_sglen )
__field( unsigned char, prot_op )
@@ -312,6 +317,7 @@ DECLARE_EVENT_CLASS(scsi_cmd_done_timeout_template,
__entry->result = cmd->result;
__entry->opcode = cmd->cmnd[0];
__entry->cmd_len= cmd->cmd_len;
+   __entry->tag= cmd->request->tag;
__entry->data_sglen = scsi_sg_count(cmd);
__entry->prot_sglen = scsi_prot_sg_count(cmd);
__entry->prot_op= scsi_get_prot_op(cmd);
@@ -319,11 +325,11 @@ DECLARE_EVENT_CLASS(scsi_cmd_done_timeout_template,
),
 
TP_printk("host_no=%u channel=%u id=%u lun=%u data_sgl=%u " \
- "prot_sgl=%u prot_op=%s cmnd=(%s %s raw=%s) result=(driver=" \
- "%s host=%s message=%s status=%s)",
+ "prot_sgl=%u prot_op=%s tag=%d cmnd=(%s %s raw=%s) " \
+ "result=(driver=%s host=%s message=%s status=%s)",
   

[RESEND PATCH v1 2/2] trace: events: block: Add tag in block trace events

[Bean Huo (beanhuo)]

Print the request tag along with other information in block trace events
when tracing request , and unplug type (Sync / Async).

Signed-off-by: Bean Huo 
---
 include/trace/events/block.h | 36 +---
 1 file changed, 25 insertions(+), 11 deletions(-)

diff --git a/include/trace/events/block.h b/include/trace/events/block.h
index 81b43f5..f8c0b9e 100644
--- a/include/trace/events/block.h
+++ b/include/trace/events/block.h
@@ -81,6 +81,7 @@ TRACE_EVENT(block_rq_requeue,
__field(  dev_t,dev )
__field(  sector_t, sector  )
__field(  unsigned int, nr_sector   )
+   __field(  int,  tag )
__array(  char, rwbs,   RWBS_LEN)
__dynamic_array( char,  cmd,1   )
),
@@ -89,16 +90,17 @@ TRACE_EVENT(block_rq_requeue,
__entry->dev   = rq->rq_disk ? disk_devt(rq->rq_disk) : 0;
__entry->sector= blk_rq_trace_sector(rq);
__entry->nr_sector = blk_rq_trace_nr_sectors(rq);
+   __entry->tag   = rq->tag;
 
blk_fill_rwbs(__entry->rwbs, rq->cmd_flags, blk_rq_bytes(rq));
__get_str(cmd)[0] = '\0';
),
 
-   TP_printk("%d,%d %s (%s) %llu + %u [%d]",
+   TP_printk("%d,%d %s (%s) %llu + %u tag=%d [%d]",
  MAJOR(__entry->dev), MINOR(__entry->dev),
  __entry->rwbs, __get_str(cmd),
  (unsigned long long)__entry->sector,
- __entry->nr_sector, 0)
+ __entry->nr_sector, __entry->tag, 0)
 );
 
 /**
@@ -124,6 +126,7 @@ TRACE_EVENT(block_rq_complete,
__field(  sector_t, sector  )
__field(  unsigned int, nr_sector   )
__field(  int,  error   )
+   __field(  int,  tag )
__array(  char, rwbs,   RWBS_LEN)
__dynamic_array( char,  cmd,1   )
),
@@ -133,16 +136,17 @@ TRACE_EVENT(block_rq_complete,
__entry->sector= blk_rq_pos(rq);
__entry->nr_sector = nr_bytes >> 9;
__entry->error = error;
+   __entry->tag   = rq->tag;
 
blk_fill_rwbs(__entry->rwbs, rq->cmd_flags, nr_bytes);
__get_str(cmd)[0] = '\0';
),
 
-   TP_printk("%d,%d %s (%s) %llu + %u [%d]",
+   TP_printk("%d,%d %s (%s) %llu + %u tag=%d [%d]",
  MAJOR(__entry->dev), MINOR(__entry->dev),
  __entry->rwbs, __get_str(cmd),
  (unsigned long long)__entry->sector,
- __entry->nr_sector, __entry->error)
+ __entry->nr_sector, __entry->tag, __entry->error)
 );
 
 DECLARE_EVENT_CLASS(block_rq,
@@ -156,6 +160,7 @@ DECLARE_EVENT_CLASS(block_rq,
__field(  sector_t, sector  )
__field(  unsigned int, nr_sector   )
__field(  unsigned int, bytes   )
+   __field(  int,  tag )
__array(  char, rwbs,   RWBS_LEN)
__array(  char, comm,   TASK_COMM_LEN   )
__dynamic_array( char,  cmd,1   )
@@ -166,17 +171,18 @@ DECLARE_EVENT_CLASS(block_rq,
__entry->sector= blk_rq_trace_sector(rq);
__entry->nr_sector = blk_rq_trace_nr_sectors(rq);
__entry->bytes = blk_rq_bytes(rq);
+   __entry->tag   = rq->tag;
 
blk_fill_rwbs(__entry->rwbs, rq->cmd_flags, blk_rq_bytes(rq));
__get_str(cmd)[0] = '\0';
memcpy(__entry->comm, current->comm, TASK_COMM_LEN);
),
 
-   TP_printk("%d,%d %s %u (%s) %llu + %u [%s]",
+   TP_printk("%d,%d %s %u (%s) %llu + %u tag=%d [%s]",
  MAJOR(__entry->dev), MINOR(__entry->dev),
  __entry->rwbs, __entry->bytes, __get_str(cmd),
  (unsigned long long)__entry->sector,
- __entry->nr_sector, __entry->comm)
+ __entry->nr_sector, __entry->tag, __entry->comm)
 );
 
 /**
@@ -297,6 +303,7 @@ DECLARE_EVENT_CLASS(block_bio_merge,
__field( dev_t, dev )
__field( sector_t,  sector  )
__field( unsigned int,  nr_sector   )
+   __field( int,   tag )
__array( char,  rwbs,   RWBS_LEN)
__array( char,  comm,   TASK_COMM_LEN   )
),
@@ -305,14 +312,15 @@ DECLARE_EVENT_CLASS(block_bio_merge,
__entry->dev= 

[RESEND PATCH v1 2/2] trace: events: block: Add tag in block trace events

[Bean Huo (beanhuo)]

Print the request tag along with other information in block trace events
when tracing request , and unplug type (Sync / Async).

Signed-off-by: Bean Huo 
---
 include/trace/events/block.h | 36 +---
 1 file changed, 25 insertions(+), 11 deletions(-)

diff --git a/include/trace/events/block.h b/include/trace/events/block.h
index 81b43f5..f8c0b9e 100644
--- a/include/trace/events/block.h
+++ b/include/trace/events/block.h
@@ -81,6 +81,7 @@ TRACE_EVENT(block_rq_requeue,
__field(  dev_t,dev )
__field(  sector_t, sector  )
__field(  unsigned int, nr_sector   )
+   __field(  int,  tag )
__array(  char, rwbs,   RWBS_LEN)
__dynamic_array( char,  cmd,1   )
),
@@ -89,16 +90,17 @@ TRACE_EVENT(block_rq_requeue,
__entry->dev   = rq->rq_disk ? disk_devt(rq->rq_disk) : 0;
__entry->sector= blk_rq_trace_sector(rq);
__entry->nr_sector = blk_rq_trace_nr_sectors(rq);
+   __entry->tag   = rq->tag;
 
blk_fill_rwbs(__entry->rwbs, rq->cmd_flags, blk_rq_bytes(rq));
__get_str(cmd)[0] = '\0';
),
 
-   TP_printk("%d,%d %s (%s) %llu + %u [%d]",
+   TP_printk("%d,%d %s (%s) %llu + %u tag=%d [%d]",
  MAJOR(__entry->dev), MINOR(__entry->dev),
  __entry->rwbs, __get_str(cmd),
  (unsigned long long)__entry->sector,
- __entry->nr_sector, 0)
+ __entry->nr_sector, __entry->tag, 0)
 );
 
 /**
@@ -124,6 +126,7 @@ TRACE_EVENT(block_rq_complete,
__field(  sector_t, sector  )
__field(  unsigned int, nr_sector   )
__field(  int,  error   )
+   __field(  int,  tag )
__array(  char, rwbs,   RWBS_LEN)
__dynamic_array( char,  cmd,1   )
),
@@ -133,16 +136,17 @@ TRACE_EVENT(block_rq_complete,
__entry->sector= blk_rq_pos(rq);
__entry->nr_sector = nr_bytes >> 9;
__entry->error = error;
+   __entry->tag   = rq->tag;
 
blk_fill_rwbs(__entry->rwbs, rq->cmd_flags, nr_bytes);
__get_str(cmd)[0] = '\0';
),
 
-   TP_printk("%d,%d %s (%s) %llu + %u [%d]",
+   TP_printk("%d,%d %s (%s) %llu + %u tag=%d [%d]",
  MAJOR(__entry->dev), MINOR(__entry->dev),
  __entry->rwbs, __get_str(cmd),
  (unsigned long long)__entry->sector,
- __entry->nr_sector, __entry->error)
+ __entry->nr_sector, __entry->tag, __entry->error)
 );
 
 DECLARE_EVENT_CLASS(block_rq,
@@ -156,6 +160,7 @@ DECLARE_EVENT_CLASS(block_rq,
__field(  sector_t, sector  )
__field(  unsigned int, nr_sector   )
__field(  unsigned int, bytes   )
+   __field(  int,  tag )
__array(  char, rwbs,   RWBS_LEN)
__array(  char, comm,   TASK_COMM_LEN   )
__dynamic_array( char,  cmd,1   )
@@ -166,17 +171,18 @@ DECLARE_EVENT_CLASS(block_rq,
__entry->sector= blk_rq_trace_sector(rq);
__entry->nr_sector = blk_rq_trace_nr_sectors(rq);
__entry->bytes = blk_rq_bytes(rq);
+   __entry->tag   = rq->tag;
 
blk_fill_rwbs(__entry->rwbs, rq->cmd_flags, blk_rq_bytes(rq));
__get_str(cmd)[0] = '\0';
memcpy(__entry->comm, current->comm, TASK_COMM_LEN);
),
 
-   TP_printk("%d,%d %s %u (%s) %llu + %u [%s]",
+   TP_printk("%d,%d %s %u (%s) %llu + %u tag=%d [%s]",
  MAJOR(__entry->dev), MINOR(__entry->dev),
  __entry->rwbs, __entry->bytes, __get_str(cmd),
  (unsigned long long)__entry->sector,
- __entry->nr_sector, __entry->comm)
+ __entry->nr_sector, __entry->tag, __entry->comm)
 );
 
 /**
@@ -297,6 +303,7 @@ DECLARE_EVENT_CLASS(block_bio_merge,
__field( dev_t, dev )
__field( sector_t,  sector  )
__field( unsigned int,  nr_sector   )
+   __field( int,   tag )
__array( char,  rwbs,   RWBS_LEN)
__array( char,  comm,   TASK_COMM_LEN   )
),
@@ -305,14 +312,15 @@ DECLARE_EVENT_CLASS(block_bio_merge,
__entry->dev= bio_dev(bio);
   

[RESEND PATCH v1 0/2] Print the request tag in Block/SCSI trace events

[Bean Huo (beanhuo)]

These patches are to add the printout of the request tag in Block/SCSI trace
events when tracing one request or command, this is very useful for tracing
the task running status in the storage device which supports multiple command
queue.

As for the first patch " Add tag in SCSI trace events", copied from
Rajat Jain [1]. I am just re-sending here.
 
[1]https://patchwork.kernel.org/patch/6399661/

-- Resend since patches need to go through the block and SCSI subsystem, and 
need related
maintainer's confirmation.

Bean Huo (2):
  trace: events: scsi: Add tag in SCSI trace events
  trace: events: block: Add tag in block trace events

 include/trace/events/block.h | 36 +---
 include/trace/events/scsi.h  | 20 +---
 2 files changed, 38 insertions(+), 18 deletions(-)

-- 
2.7.4

[RESEND PATCH v1 0/2] Print the request tag in Block/SCSI trace events

[Bean Huo (beanhuo)]

These patches are to add the printout of the request tag in Block/SCSI trace
events when tracing one request or command, this is very useful for tracing
the task running status in the storage device which supports multiple command
queue.

As for the first patch " Add tag in SCSI trace events", copied from
Rajat Jain [1]. I am just re-sending here.
 
[1]https://patchwork.kernel.org/patch/6399661/

-- Resend since patches need to go through the block and SCSI subsystem, and 
need related
maintainer's confirmation.

Bean Huo (2):
  trace: events: scsi: Add tag in SCSI trace events
  trace: events: block: Add tag in block trace events

 include/trace/events/block.h | 36 +---
 include/trace/events/scsi.h  | 20 +---
 2 files changed, 38 insertions(+), 18 deletions(-)

-- 
2.7.4

Re: UFS writing request failure handling

[Bean Huo (beanhuo)]

Hi Bart
Thanks for your answer.
I looked at SCSI core source codes these days, UFS also follows SCSI core error 
handling.
There is already re-issue behavior, and assign 5 retries for each UFS request 
if there is error.

>
>On 02/12/18 04:21, Bean Huo (beanhuo) wrote
>> I am looking at UFS error handling, but I didn't notice re-issues
>> requests with UTP error to the host controller. According UFS host
>> spec, "host software either completes the request that had the error
>> and requests still outstanding with error to higher level software, or
>> re-issues these requests to the host controller". For the latest Linux
>> UFS driver, it belongs to former one? If I am wrong, please correct
>> me.
>
>Hello Bean,
>
>I'm afraid that reissuing a failed write forever could cause an infinite loop 
>that
>makes e.g. user space processes unkillable or that could cause an orderly
>system shutdown to hang. I'm not sure that we want such behavior.
>
>If you want to learn more about how the SCSI core handles failed writes,
>please have a look at scsi_decide_disposition(). A possible approach to
>modify the error handling behavior is to stack a dm driver on top of the SCSI
>core that implements the desired behavior.
>
>Bart.

Re: UFS writing request failure handling

[Bean Huo (beanhuo)]

Hi Bart
Thanks for your answer.
I looked at SCSI core source codes these days, UFS also follows SCSI core error 
handling.
There is already re-issue behavior, and assign 5 retries for each UFS request 
if there is error.

>
>On 02/12/18 04:21, Bean Huo (beanhuo) wrote
>> I am looking at UFS error handling, but I didn't notice re-issues
>> requests with UTP error to the host controller. According UFS host
>> spec, "host software either completes the request that had the error
>> and requests still outstanding with error to higher level software, or
>> re-issues these requests to the host controller". For the latest Linux
>> UFS driver, it belongs to former one? If I am wrong, please correct
>> me.
>
>Hello Bean,
>
>I'm afraid that reissuing a failed write forever could cause an infinite loop 
>that
>makes e.g. user space processes unkillable or that could cause an orderly
>system shutdown to hang. I'm not sure that we want such behavior.
>
>If you want to learn more about how the SCSI core handles failed writes,
>please have a look at scsi_decide_disposition(). A possible approach to
>modify the error handling behavior is to stack a dm driver on top of the SCSI
>core that implements the desired behavior.
>
>Bart.

UFS writing request failure handling

[Bean Huo (beanhuo)]

Hi, 
I am looking at UFS error handling, but I didn't notice re-issues requests with 
UTP error to the host controller.
According UFS host spec, "host software either completes the request that had 
the error and requests still outstanding 
with error to higher level software, or re-issues these requests to the host 
controller".
For the latest Linux UFS driver, it belongs to former one? 
If I am wrong, please correct me.
Thanks!

UFS writing request failure handling

[Bean Huo (beanhuo)]

Hi, 
I am looking at UFS error handling, but I didn't notice re-issues requests with 
UTP error to the host controller.
According UFS host spec, "host software either completes the request that had 
the error and requests still outstanding 
with error to higher level software, or re-issues these requests to the host 
controller".
For the latest Linux UFS driver, it belongs to former one? 
If I am wrong, please correct me.
Thanks!

Re: UFS utilities

[Bean Huo (beanhuo)]

Hi, greg k-h

>
>So what UFS commands are you missing that you need to see implemented?
>
>And again, have you checked the different forks of the driver?
>

Seems there is something misunderstood, I want to use UPIU, rather than CDB.
Maybe it is not possible based on current UFS stacks. Of course, exactly, 
there is no missing SCSI command listed in UFS 2.1.

>> >> And also it doesn't support several UFS special command.
>> >
>> >Are you referring to SCSI commands or rather to UFS commands that
>> >fall outside the SCSI spec? Anyway, an approach that is used by many
>> >SCSI drivers to export information to user space that falls outside
>> >the SCSI spec is to create additional sysfs attributes. See also the
>> >sdev_attrs and shost_attrs members of struct scsi_host_template.
>> >
>> Yes, for the UFS information, I can use these interface/approach to easily
>get.
>> I am thinking how about some testing case and configuration operation.
>
>Which ones exactly?
>
>> Also, is it possible bypass SCSI stacks and go into directly UFS stack?
>
>Look at the different sysfs files for the UFS device, it does that for some
>commands.
>
To be honest, I don't know which interface, it can pass UPIU to UFS driver,
And bypass SCSI stacks.

Thanks.
Bean Huo

Re: UFS utilities

[Bean Huo (beanhuo)]

Hi, greg k-h

>
>So what UFS commands are you missing that you need to see implemented?
>
>And again, have you checked the different forks of the driver?
>

Seems there is something misunderstood, I want to use UPIU, rather than CDB.
Maybe it is not possible based on current UFS stacks. Of course, exactly, 
there is no missing SCSI command listed in UFS 2.1.

>> >> And also it doesn't support several UFS special command.
>> >
>> >Are you referring to SCSI commands or rather to UFS commands that
>> >fall outside the SCSI spec? Anyway, an approach that is used by many
>> >SCSI drivers to export information to user space that falls outside
>> >the SCSI spec is to create additional sysfs attributes. See also the
>> >sdev_attrs and shost_attrs members of struct scsi_host_template.
>> >
>> Yes, for the UFS information, I can use these interface/approach to easily
>get.
>> I am thinking how about some testing case and configuration operation.
>
>Which ones exactly?
>
>> Also, is it possible bypass SCSI stacks and go into directly UFS stack?
>
>Look at the different sysfs files for the UFS device, it does that for some
>commands.
>
To be honest, I don't know which interface, it can pass UPIU to UFS driver,
And bypass SCSI stacks.

Thanks.
Bean Huo

RE: [EXT] Re: UFS utilities

[Bean Huo (beanhuo)]

Hi, Bart
Sorry for later!
>
>Hello Bean,
>
>Please be more specific. What is inconvenient about sg3_utils on embedded
>ARM systems?
>
Exactly, I don't know how to compile sg3_utils with static library, instead of 
sharing library. I used following configuration
Parameter:
./configure --enable-static=yes --build=x86_64-unknown-linux-gnu 
--host=aarch64-linux-gnu  --prefix=$PWD/out/  CC=aarch64-linux-gnu-g
cc --target=aarch64-linux-gnu LD=aarch64-linux-gnu-ld 
AS=aarch64-linux-gnu-as CFLAGS=-static LDFLAGS=-static

But the object files are still dynamically linked.

ELF 64-bit LSB executable, ARM aarch64, version 1 (SYSV), dynamically linked, 
interpreter /lib/ld-linux-aarch64.so.1, 
for GNU/Linux 3.7.0, BuildID[sha1]=4f01b4c9f1ff47bc00aef93950c02734b4cc8e57, 
not stripped.

I want it to be statically linked. Otherwise, I should copy its library to my 
lib folder, and sometimes for the embedded, 
Need to re-build rootfs. Meanwhile, for the UFS, there are totally 27 scsi 
commands being used based on UFS2.1.
For the most case, we just use several sg3_utils object files, don't need to 
copy all object files to the ending product. 

>> And also it doesn't support several UFS special command.
>
>Are you referring to SCSI commands or rather to UFS commands that fall
>outside the SCSI spec? Anyway, an approach that is used by many SCSI drivers
>to export information to user space that falls outside the SCSI spec is to
>create additional sysfs attributes. See also the sdev_attrs and shost_attrs
>members of struct scsi_host_template.
>
Yes, for the UFS information, I can use these interface/approach to easily get.
I am thinking how about some testing case and configuration operation.
Also, is it possible bypass SCSI stacks and go into directly UFS stack?
Thanks for your inputs.
>Bart.

//Bean Huo

RE: [EXT] Re: UFS utilities

[Bean Huo (beanhuo)]

Hi, Bart
Sorry for later!
>
>Hello Bean,
>
>Please be more specific. What is inconvenient about sg3_utils on embedded
>ARM systems?
>
Exactly, I don't know how to compile sg3_utils with static library, instead of 
sharing library. I used following configuration
Parameter:
./configure --enable-static=yes --build=x86_64-unknown-linux-gnu 
--host=aarch64-linux-gnu  --prefix=$PWD/out/  CC=aarch64-linux-gnu-g
cc --target=aarch64-linux-gnu LD=aarch64-linux-gnu-ld 
AS=aarch64-linux-gnu-as CFLAGS=-static LDFLAGS=-static

But the object files are still dynamically linked.

ELF 64-bit LSB executable, ARM aarch64, version 1 (SYSV), dynamically linked, 
interpreter /lib/ld-linux-aarch64.so.1, 
for GNU/Linux 3.7.0, BuildID[sha1]=4f01b4c9f1ff47bc00aef93950c02734b4cc8e57, 
not stripped.

I want it to be statically linked. Otherwise, I should copy its library to my 
lib folder, and sometimes for the embedded, 
Need to re-build rootfs. Meanwhile, for the UFS, there are totally 27 scsi 
commands being used based on UFS2.1.
For the most case, we just use several sg3_utils object files, don't need to 
copy all object files to the ending product. 

>> And also it doesn't support several UFS special command.
>
>Are you referring to SCSI commands or rather to UFS commands that fall
>outside the SCSI spec? Anyway, an approach that is used by many SCSI drivers
>to export information to user space that falls outside the SCSI spec is to
>create additional sysfs attributes. See also the sdev_attrs and shost_attrs
>members of struct scsi_host_template.
>
Yes, for the UFS information, I can use these interface/approach to easily get.
I am thinking how about some testing case and configuration operation.
Also, is it possible bypass SCSI stacks and go into directly UFS stack?
Thanks for your inputs.
>Bart.

//Bean Huo

RE: [PATCH v2] drivers:mtd:spi-nor:checkup FSR error bits

[Bean Huo (beanhuo)]

Hi, Cyrille

>Hi Bean,
>
>Le 04/12/2017 à 13:34, Bean Huo (beanhuo) a écrit :
>> For Micron spi nor device, when erase/program operation fails,
>> especially the failure results from intending to modify protected
>> space, spi-nor upper layers still get the return which shows the
>> operation succeeds. This is because current spi_nor_fsr_ready() only
>> uses FSR bit.7 (flag status register) to check device whether ready.
>> This patch fixs this issue by checking relevant error
>
>s/fixs/fixes/
>
>> bits in FSR.
>> The FSR is a powerful tool to investigate the staus of
>
>s/staus/status/
>
>> device, checking information regarding what is actually doing the
>> memory and detecting possible error conditions.
>>
>> Signed-off-by: beanhuo <bean...@micron.com>
>
>Otherwise,
>Acked-by: Cyrille Pitchen <cyrille.pitc...@wedev4u.fr>
>
>No need to resend, I'll fix these tiny issues myself :)
>

Thanks!

// Bean Huo

RE: [PATCH v2] drivers:mtd:spi-nor:checkup FSR error bits

[Bean Huo (beanhuo)]

Hi, Cyrille

>Hi Bean,
>
>Le 04/12/2017 à 13:34, Bean Huo (beanhuo) a écrit :
>> For Micron spi nor device, when erase/program operation fails,
>> especially the failure results from intending to modify protected
>> space, spi-nor upper layers still get the return which shows the
>> operation succeeds. This is because current spi_nor_fsr_ready() only
>> uses FSR bit.7 (flag status register) to check device whether ready.
>> This patch fixs this issue by checking relevant error
>
>s/fixs/fixes/
>
>> bits in FSR.
>> The FSR is a powerful tool to investigate the staus of
>
>s/staus/status/
>
>> device, checking information regarding what is actually doing the
>> memory and detecting possible error conditions.
>>
>> Signed-off-by: beanhuo 
>
>Otherwise,
>Acked-by: Cyrille Pitchen 
>
>No need to resend, I'll fix these tiny issues myself :)
>

Thanks!

// Bean Huo

[PATCH v2] drivers:mtd:spi-nor:checkup FSR error bits

[Bean Huo (beanhuo)]

For Micron spi nor device, when erase/program operation
fails, especially the failure results from intending to
modify protected space, spi-nor upper layers still get
the return which shows the operation succeeds. This is
because current spi_nor_fsr_ready() only uses FSR bit.7
(flag status register) to check device whether ready.
This patch fixs this issue by checking relevant error
bits in FSR.
The FSR is a powerful tool to investigate the staus of
device, checking information regarding what is actually
doing the memory and detecting possible error conditions.

Signed-off-by: beanhuo 
---
 v1 - v2:
- Changed some error comments based on Cyrille's reviews.

 drivers/mtd/spi-nor/spi-nor.c | 18 --
 include/linux/mtd/spi-nor.h   |  6 +-
 2 files changed, 21 insertions(+), 3 deletions(-)

diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index 19c00072..4423605 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -328,8 +328,22 @@ static inline int spi_nor_fsr_ready(struct spi_nor *nor)
int fsr = read_fsr(nor);
if (fsr < 0)
return fsr;
-   else
-   return fsr & FSR_READY;
+
+   if (fsr & (FSR_E_ERR | FSR_P_ERR)) {
+   if (fsr & FSR_E_ERR)
+   dev_err(nor->dev, "Erase operation failed.\n");
+   else
+   dev_err(nor->dev, "Program operation failed.\n");
+
+   if (fsr & FSR_PT_ERR)
+   dev_err(nor->dev,
+   "Attempted to modify a protected sector.\n");
+
+   nor->write_reg(nor, SPINOR_OP_CLFSR, NULL, 0);
+   return -EIO;
+   }
+
+   return fsr & FSR_READY;
 }
 
 static int spi_nor_ready(struct spi_nor *nor)
diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
index 1f0a7fc..da8e0d5 100644
--- a/include/linux/mtd/spi-nor.h
+++ b/include/linux/mtd/spi-nor.h
@@ -61,6 +61,7 @@
 #define SPINOR_OP_RDSFDP   0x5a/* Read SFDP */
 #define SPINOR_OP_RDCR 0x35/* Read configuration register */
 #define SPINOR_OP_RDFSR0x70/* Read flag status register */
+#define SPINOR_OP_CLFSR0x50/* Clear flag status register */
 
 /* 4-byte address opcodes - used on Spansion and some Macronix flashes. */
 #define SPINOR_OP_READ_4B  0x13/* Read data bytes (low frequency) */
@@ -130,7 +131,10 @@
 #define EVCR_QUAD_EN_MICRONBIT(7)  /* Micron Quad I/O */
 
 /* Flag Status Register bits */
-#define FSR_READY  BIT(7)
+#define FSR_READY  BIT(7)  /* Device status, 0 = Busy, 1 = Ready */
+#define FSR_E_ERR  BIT(5)  /* Erase operation status */
+#define FSR_P_ERR  BIT(4)  /* Program operation status */
+#define FSR_PT_ERR BIT(1)  /* Protection error bit */
 
 /* Configuration Register bits. */
 #define CR_QUAD_EN_SPANBIT(1)  /* Spansion Quad I/O */
-- 
2.7.4

[PATCH v2] drivers:mtd:spi-nor:checkup FSR error bits

[Bean Huo (beanhuo)]

For Micron spi nor device, when erase/program operation
fails, especially the failure results from intending to
modify protected space, spi-nor upper layers still get
the return which shows the operation succeeds. This is
because current spi_nor_fsr_ready() only uses FSR bit.7
(flag status register) to check device whether ready.
This patch fixs this issue by checking relevant error
bits in FSR.
The FSR is a powerful tool to investigate the staus of
device, checking information regarding what is actually
doing the memory and detecting possible error conditions.

Signed-off-by: beanhuo 
---
 v1 - v2:
- Changed some error comments based on Cyrille's reviews.

 drivers/mtd/spi-nor/spi-nor.c | 18 --
 include/linux/mtd/spi-nor.h   |  6 +-
 2 files changed, 21 insertions(+), 3 deletions(-)

diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index 19c00072..4423605 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -328,8 +328,22 @@ static inline int spi_nor_fsr_ready(struct spi_nor *nor)
int fsr = read_fsr(nor);
if (fsr < 0)
return fsr;
-   else
-   return fsr & FSR_READY;
+
+   if (fsr & (FSR_E_ERR | FSR_P_ERR)) {
+   if (fsr & FSR_E_ERR)
+   dev_err(nor->dev, "Erase operation failed.\n");
+   else
+   dev_err(nor->dev, "Program operation failed.\n");
+
+   if (fsr & FSR_PT_ERR)
+   dev_err(nor->dev,
+   "Attempted to modify a protected sector.\n");
+
+   nor->write_reg(nor, SPINOR_OP_CLFSR, NULL, 0);
+   return -EIO;
+   }
+
+   return fsr & FSR_READY;
 }
 
 static int spi_nor_ready(struct spi_nor *nor)
diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
index 1f0a7fc..da8e0d5 100644
--- a/include/linux/mtd/spi-nor.h
+++ b/include/linux/mtd/spi-nor.h
@@ -61,6 +61,7 @@
 #define SPINOR_OP_RDSFDP   0x5a/* Read SFDP */
 #define SPINOR_OP_RDCR 0x35/* Read configuration register */
 #define SPINOR_OP_RDFSR0x70/* Read flag status register */
+#define SPINOR_OP_CLFSR0x50/* Clear flag status register */
 
 /* 4-byte address opcodes - used on Spansion and some Macronix flashes. */
 #define SPINOR_OP_READ_4B  0x13/* Read data bytes (low frequency) */
@@ -130,7 +131,10 @@
 #define EVCR_QUAD_EN_MICRONBIT(7)  /* Micron Quad I/O */
 
 /* Flag Status Register bits */
-#define FSR_READY  BIT(7)
+#define FSR_READY  BIT(7)  /* Device status, 0 = Busy, 1 = Ready */
+#define FSR_E_ERR  BIT(5)  /* Erase operation status */
+#define FSR_P_ERR  BIT(4)  /* Program operation status */
+#define FSR_PT_ERR BIT(1)  /* Protection error bit */
 
 /* Configuration Register bits. */
 #define CR_QUAD_EN_SPANBIT(1)  /* Spansion Quad I/O */
-- 
2.7.4

Re: [PATCH V1] drivers:mtd:spi-nor:checkup FSR error bits

[Bean Huo (beanhuo)]

Hi, Cyrille

Finally, I get your comments, thanks.

>Hi Bean,
>
>Le 11/11/2017 à 21:49, Bean Huo (beanhuo) a écrit :
>> For the Micron SPI NOR, when the erase/program operation fails,
>> especially,
>
>To be verified but I think you'd rather remove "the" words in this case:
>
>"For Micron SPI NOR memories, when erase/program operation fails,
>especially ..."
>
>Maybe no comma after "especially".
>
>> for the failure results from intending to modify protected space,
>> spi-nor upper layers still get the return which shows the operation succeeds.
>> this because spi_nor_fsr_ready() only uses bit.7 to device whether ready.
>
>"This": missing capital letter and maybe a verb too.
>
>> For the most cases, even the error of erase/program occurs, SPI NOR
>> device is still ready. The device ready and the error are two different 
>> cases.
>
When the program/erase failed, or there is the error during
Erasing/programming, user space always gets the status of the
previous operation is successful.  Because current spi_nor_fsr_ready()
only checks FSR ready bit. Even if failure/error happened, spi nor
device still can go into the ready stage. This is what I want to say.
So, the device ready and the operation failure are two different cases.

>I don't really understand what you mean here.
>
>> This patch is to fixup this issue and adding FSR (flag status
>> register)
>
>This patch fixes the issue by checking relevant bits in the FSR.
>
>> error bits checkup.
>> The FSR(flag status register) is a powerful tool to investigate the
>> staus
>
>"The FSR (flag status register)": please insert a space ' '.
>
>s/staus/status/
>
>
>> of device,checking information regarding what is actually doing the
>> memory
>
>"of device, checking": missing space
>> and detecting possible error conditions.
>>
>
>Globally, I think you need to reword your commit message. IMHO, it is not
>clear though I think I've mostly understood what you meant reading the
>actual source code.
>
I am not a native English speaker, hopefully I can make big progress on writing.

>> Signed-off-by: beanhuo <bean...@micron.com>
>> ---
>>  drivers/mtd/spi-nor/spi-nor.c | 19 +--
>>  include/linux/mtd/spi-nor.h   |  6 +-
>>  2 files changed, 22 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/mtd/spi-nor/spi-nor.c
>> b/drivers/mtd/spi-nor/spi-nor.c index bc266f7..200e814 100644
>> --- a/drivers/mtd/spi-nor/spi-nor.c
>> +++ b/drivers/mtd/spi-nor/spi-nor.c
>> @@ -330,8 +330,23 @@ static inline int spi_nor_fsr_ready(struct spi_nor
>*nor)
>>  int fsr = read_fsr(nor);
>>  if (fsr < 0)
>>  return fsr;
>> -else
>> -return fsr & FSR_READY;
>> +
>> +if (fsr & (FSR_E_ERR | FSR_P_ERR)) {
>> +if (fsr & FSR_E_ERR)
>> +dev_err(nor->dev, "Erase operation failed.\n");
>> +else
>> +dev_err(nor->dev, "Program operation failed.\n");
>> +
>> +if (fsr & FSR_PT_ERR)
>> +dev_err(nor->dev,
>> +"The operation has attempted to modify the
>protected"
>
>A space ' ' is missing after "protected".
>Also please verify next version passes the checkpatch test because this
>version doesn't.
>
Yes, I already re-sent this patch which merged these two line into one line.

>I think you should check the verb tense consistency:
>[...] operation failed. The operation attempted [...]
>
>Also maybe you should write "a protected sector" or "some protected sector":
>"the protected sector" sounds like there is only one protected sector.
>
>> +"sector or the locked OPT space.\n");
>
>You meant OTP (One Time Programmable), didn't you ? s/OPT/OTP/
>

Yes, it is one time programmable space.

>> +
>> +nor->write_reg(nor, SPINOR_OP_CLFSR, NULL, 0);
>> +return -EIO;
>> +}
>> +
>> +return fsr & FSR_READY;
>>  }
>>
>>  static int spi_nor_ready(struct spi_nor *nor) diff --git
>> a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h index
>> d0c66a0..46b5608 100644
>> --- a/include/linux/mtd/spi-nor.h
>> +++ b/include/linux/mtd/spi-nor.h
>> @@ -61,6 +61,7 @@
>>  #define SPINOR_OP_RDSFDP0x5a/* Read SFDP */
>>  #define SPINOR_OP_RDCR  0x35/* Read configuration register
>*/
>>  #d

Re: [PATCH V1] drivers:mtd:spi-nor:checkup FSR error bits

[Bean Huo (beanhuo)]

Hi, Cyrille

Finally, I get your comments, thanks.

>Hi Bean,
>
>Le 11/11/2017 à 21:49, Bean Huo (beanhuo) a écrit :
>> For the Micron SPI NOR, when the erase/program operation fails,
>> especially,
>
>To be verified but I think you'd rather remove "the" words in this case:
>
>"For Micron SPI NOR memories, when erase/program operation fails,
>especially ..."
>
>Maybe no comma after "especially".
>
>> for the failure results from intending to modify protected space,
>> spi-nor upper layers still get the return which shows the operation succeeds.
>> this because spi_nor_fsr_ready() only uses bit.7 to device whether ready.
>
>"This": missing capital letter and maybe a verb too.
>
>> For the most cases, even the error of erase/program occurs, SPI NOR
>> device is still ready. The device ready and the error are two different 
>> cases.
>
When the program/erase failed, or there is the error during
Erasing/programming, user space always gets the status of the
previous operation is successful.  Because current spi_nor_fsr_ready()
only checks FSR ready bit. Even if failure/error happened, spi nor
device still can go into the ready stage. This is what I want to say.
So, the device ready and the operation failure are two different cases.

>I don't really understand what you mean here.
>
>> This patch is to fixup this issue and adding FSR (flag status
>> register)
>
>This patch fixes the issue by checking relevant bits in the FSR.
>
>> error bits checkup.
>> The FSR(flag status register) is a powerful tool to investigate the
>> staus
>
>"The FSR (flag status register)": please insert a space ' '.
>
>s/staus/status/
>
>
>> of device,checking information regarding what is actually doing the
>> memory
>
>"of device, checking": missing space
>> and detecting possible error conditions.
>>
>
>Globally, I think you need to reword your commit message. IMHO, it is not
>clear though I think I've mostly understood what you meant reading the
>actual source code.
>
I am not a native English speaker, hopefully I can make big progress on writing.

>> Signed-off-by: beanhuo 
>> ---
>>  drivers/mtd/spi-nor/spi-nor.c | 19 +--
>>  include/linux/mtd/spi-nor.h   |  6 +-
>>  2 files changed, 22 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/mtd/spi-nor/spi-nor.c
>> b/drivers/mtd/spi-nor/spi-nor.c index bc266f7..200e814 100644
>> --- a/drivers/mtd/spi-nor/spi-nor.c
>> +++ b/drivers/mtd/spi-nor/spi-nor.c
>> @@ -330,8 +330,23 @@ static inline int spi_nor_fsr_ready(struct spi_nor
>*nor)
>>  int fsr = read_fsr(nor);
>>  if (fsr < 0)
>>  return fsr;
>> -else
>> -return fsr & FSR_READY;
>> +
>> +if (fsr & (FSR_E_ERR | FSR_P_ERR)) {
>> +if (fsr & FSR_E_ERR)
>> +dev_err(nor->dev, "Erase operation failed.\n");
>> +else
>> +dev_err(nor->dev, "Program operation failed.\n");
>> +
>> +if (fsr & FSR_PT_ERR)
>> +dev_err(nor->dev,
>> +"The operation has attempted to modify the
>protected"
>
>A space ' ' is missing after "protected".
>Also please verify next version passes the checkpatch test because this
>version doesn't.
>
Yes, I already re-sent this patch which merged these two line into one line.

>I think you should check the verb tense consistency:
>[...] operation failed. The operation attempted [...]
>
>Also maybe you should write "a protected sector" or "some protected sector":
>"the protected sector" sounds like there is only one protected sector.
>
>> +"sector or the locked OPT space.\n");
>
>You meant OTP (One Time Programmable), didn't you ? s/OPT/OTP/
>

Yes, it is one time programmable space.

>> +
>> +nor->write_reg(nor, SPINOR_OP_CLFSR, NULL, 0);
>> +return -EIO;
>> +}
>> +
>> +return fsr & FSR_READY;
>>  }
>>
>>  static int spi_nor_ready(struct spi_nor *nor) diff --git
>> a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h index
>> d0c66a0..46b5608 100644
>> --- a/include/linux/mtd/spi-nor.h
>> +++ b/include/linux/mtd/spi-nor.h
>> @@ -61,6 +61,7 @@
>>  #define SPINOR_OP_RDSFDP0x5a/* Read SFDP */
>>  #define SPINOR_OP_RDCR  0x35/* Read configuration register
>*/
>>  #define SPINOR_OP_RDFSR 

Re: UFS utilities

[Bean Huo (beanhuo)]

Hi, Greg

>On Mon, Nov 27, 2017 at 11:25:47AM +0000, Bean Huo (beanhuo) wrote:
>> Hi, all
>> Is there someone knows if exists one utilis dedicated to UFS device, rather
>than SCSI utils?
>> I have tried sg3-utils, but it is not convenient for the embedded ARM-based
>system.
>> And also it doesn't support several UFS special command.
>
>What specific UFS commands do you need to make to the device that the
>current driver does not support?


There are some UFS/vendor native commands. They are not SCSI based.

>And yes, this is a trick question as there are about 4 different major forks 
>that
>I know of of the UFS driver in different vendor trees, all of which support
>different types of UFS commands :(
>
>> If we don't have this kind of tool for UFS, is it necessary for us to 
>> develop a
>>ufs-utils?
>
>I doubt it, what neds to happen is getting all of the functionality that lives 
>in
>these different forks all merged upstream into the in-kernel driver.  Then I 
>bet
>all of the needed functionality you are looking for will be there.
>
Sometimes customers tend to use user space tool to do some configuration.
And especially, for example the UFS FFU.

>good luck!
>
Thanks !
>greg k-h

//Bean Huo

Re: UFS utilities

[Bean Huo (beanhuo)]

Hi, Greg

>On Mon, Nov 27, 2017 at 11:25:47AM +0000, Bean Huo (beanhuo) wrote:
>> Hi, all
>> Is there someone knows if exists one utilis dedicated to UFS device, rather
>than SCSI utils?
>> I have tried sg3-utils, but it is not convenient for the embedded ARM-based
>system.
>> And also it doesn't support several UFS special command.
>
>What specific UFS commands do you need to make to the device that the
>current driver does not support?


There are some UFS/vendor native commands. They are not SCSI based.

>And yes, this is a trick question as there are about 4 different major forks 
>that
>I know of of the UFS driver in different vendor trees, all of which support
>different types of UFS commands :(
>
>> If we don't have this kind of tool for UFS, is it necessary for us to 
>> develop a
>>ufs-utils?
>
>I doubt it, what neds to happen is getting all of the functionality that lives 
>in
>these different forks all merged upstream into the in-kernel driver.  Then I 
>bet
>all of the needed functionality you are looking for will be there.
>
Sometimes customers tend to use user space tool to do some configuration.
And especially, for example the UFS FFU.

>good luck!
>
Thanks !
>greg k-h

//Bean Huo

[PATCH] [RESEND] drivers:mtd:spi-nor:checkup FSR error bits

[Bean Huo (beanhuo)]

For the Micron SPI NOR, when the erase/program operation fails, especially,
for the failure results from intending to modify protected space, spi-nor
upper layers still get the return which shows the operation succeeds.
This because spi_nor_fsr_ready() only uses bit.7 to device whether ready.
For the most cases, even the error of erase/program occurs, SPI NOR device
is still ready. The device ready and the error are two different cases.
This patch is to fixup this issue and adding FSR (flag status register)
error bits checkup.
The FSR(flag status register) is a powerful tool to investigate the staus
of device, checking information regarding what is actually doing the memory
and detecting possible error conditions.

Signed-off-by: beanhuo 
---
 drivers/mtd/spi-nor/spi-nor.c | 18 --
 include/linux/mtd/spi-nor.h   |  6 +-
 2 files changed, 21 insertions(+), 3 deletions(-)

diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index 19c00072..b87c04f 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -328,8 +328,22 @@ static inline int spi_nor_fsr_ready(struct spi_nor *nor)
int fsr = read_fsr(nor);
if (fsr < 0)
return fsr;
-   else
-   return fsr & FSR_READY;
+
+   if (fsr & (FSR_E_ERR | FSR_P_ERR)) {
+   if (fsr & FSR_E_ERR)
+   dev_err(nor->dev, "Erase operation failed.\n");
+   else
+   dev_err(nor->dev, "Program operation failed.\n");
+
+   if (fsr & FSR_PT_ERR)
+   dev_err(nor->dev,
+   "Attempted to modify the protected sectors.\n");
+
+   nor->write_reg(nor, SPINOR_OP_CLFSR, NULL, 0);
+   return -EIO;
+   }
+
+   return fsr & FSR_READY;
 }
 
 static int spi_nor_ready(struct spi_nor *nor)
diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
index 1f0a7fc..b6187e3 100644
--- a/include/linux/mtd/spi-nor.h
+++ b/include/linux/mtd/spi-nor.h
@@ -61,6 +61,7 @@
 #define SPINOR_OP_RDSFDP   0x5a/* Read SFDP */
 #define SPINOR_OP_RDCR 0x35/* Read configuration register */
 #define SPINOR_OP_RDFSR0x70/* Read flag status register */
+#define SPINOR_OP_CLFSR0x50/* Clear flag status register */
 
 /* 4-byte address opcodes - used on Spansion and some Macronix flashes. */
 #define SPINOR_OP_READ_4B  0x13/* Read data bytes (low frequency) */
@@ -130,7 +131,10 @@
 #define EVCR_QUAD_EN_MICRONBIT(7)  /* Micron Quad I/O */
 
 /* Flag Status Register bits */
-#define FSR_READY  BIT(7)
+#define FSR_READY  BIT(7)  /* Device status, 0 = Busy,1 = Ready */
+#define FSR_E_ERR  BIT(5)  /* Erase operation status */
+#define FSR_P_ERR  BIT(4)  /* Program operation status */
+#define FSR_PT_ERR BIT(1)  /* Protection error bit */
 
 /* Configuration Register bits. */
 #define CR_QUAD_EN_SPANBIT(1)  /* Spansion Quad I/O */
-- 
2.7.4

[PATCH] [RESEND] drivers:mtd:spi-nor:checkup FSR error bits

[Bean Huo (beanhuo)]

For the Micron SPI NOR, when the erase/program operation fails, especially,
for the failure results from intending to modify protected space, spi-nor
upper layers still get the return which shows the operation succeeds.
This because spi_nor_fsr_ready() only uses bit.7 to device whether ready.
For the most cases, even the error of erase/program occurs, SPI NOR device
is still ready. The device ready and the error are two different cases.
This patch is to fixup this issue and adding FSR (flag status register)
error bits checkup.
The FSR(flag status register) is a powerful tool to investigate the staus
of device, checking information regarding what is actually doing the memory
and detecting possible error conditions.

Signed-off-by: beanhuo 
---
 drivers/mtd/spi-nor/spi-nor.c | 18 --
 include/linux/mtd/spi-nor.h   |  6 +-
 2 files changed, 21 insertions(+), 3 deletions(-)

diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index 19c00072..b87c04f 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -328,8 +328,22 @@ static inline int spi_nor_fsr_ready(struct spi_nor *nor)
int fsr = read_fsr(nor);
if (fsr < 0)
return fsr;
-   else
-   return fsr & FSR_READY;
+
+   if (fsr & (FSR_E_ERR | FSR_P_ERR)) {
+   if (fsr & FSR_E_ERR)
+   dev_err(nor->dev, "Erase operation failed.\n");
+   else
+   dev_err(nor->dev, "Program operation failed.\n");
+
+   if (fsr & FSR_PT_ERR)
+   dev_err(nor->dev,
+   "Attempted to modify the protected sectors.\n");
+
+   nor->write_reg(nor, SPINOR_OP_CLFSR, NULL, 0);
+   return -EIO;
+   }
+
+   return fsr & FSR_READY;
 }
 
 static int spi_nor_ready(struct spi_nor *nor)
diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
index 1f0a7fc..b6187e3 100644
--- a/include/linux/mtd/spi-nor.h
+++ b/include/linux/mtd/spi-nor.h
@@ -61,6 +61,7 @@
 #define SPINOR_OP_RDSFDP   0x5a/* Read SFDP */
 #define SPINOR_OP_RDCR 0x35/* Read configuration register */
 #define SPINOR_OP_RDFSR0x70/* Read flag status register */
+#define SPINOR_OP_CLFSR0x50/* Clear flag status register */
 
 /* 4-byte address opcodes - used on Spansion and some Macronix flashes. */
 #define SPINOR_OP_READ_4B  0x13/* Read data bytes (low frequency) */
@@ -130,7 +131,10 @@
 #define EVCR_QUAD_EN_MICRONBIT(7)  /* Micron Quad I/O */
 
 /* Flag Status Register bits */
-#define FSR_READY  BIT(7)
+#define FSR_READY  BIT(7)  /* Device status, 0 = Busy,1 = Ready */
+#define FSR_E_ERR  BIT(5)  /* Erase operation status */
+#define FSR_P_ERR  BIT(4)  /* Program operation status */
+#define FSR_PT_ERR BIT(1)  /* Protection error bit */
 
 /* Configuration Register bits. */
 #define CR_QUAD_EN_SPANBIT(1)  /* Spansion Quad I/O */
-- 
2.7.4

RE: [PATCH V1] drivers:mtd:spi-nor:checkup FSR error bits

[Bean Huo (beanhuo)]

Ping SPI-NOR maintainers again


>
>Ping SPI-NOR maintainers
>
>>-Original Message-----
>>From: Bean Huo (beanhuo)
>>Sent: Samstag, 11. November 2017 21:49
>>To: 'cyrille.pitc...@wedev4u.fr' <cyrille.pitc...@wedev4u.fr>;
>>marek.va...@gmail.com
>>Cc: 'dw...@infradead.org' <dw...@infradead.org>;
>>computersforpe...@gmail.com; 'linux-...@lists.infradead.org' >m...@lists.infradead.org>; linux-kernel@vger.kernel.org
>>Subject: [PATCH V1] drivers:mtd:spi-nor:checkup FSR error bits
>>
>>For the Micron SPI NOR, when the erase/program operation fails,
>>especially, for the failure results from intending to modify protected
>>space, spi-nor upper layers still get the return which shows the operation
>succeeds.
>>this because spi_nor_fsr_ready() only uses bit.7 to device whether ready.
>>For the most cases, even the error of erase/program occurs, SPI NOR
>>device is still ready. The device ready and the error are two different cases.
>>This patch is to fixup this issue and adding FSR (flag status register)
>>error bits checkup.
>>The FSR(flag status register) is a powerful tool to investigate the
>>staus of device,checking information regarding what is actually doing
>>the memory and detecting possible error conditions.
>>
>>Signed-off-by: beanhuo <bean...@micron.com>
>>---
>> drivers/mtd/spi-nor/spi-nor.c | 19 +--
>> include/linux/mtd/spi-nor.h   |  6 +-
>> 2 files changed, 22 insertions(+), 3 deletions(-)
>>
>>diff --git a/drivers/mtd/spi-nor/spi-nor.c
>>b/drivers/mtd/spi-nor/spi-nor.c index bc266f7..200e814 100644
>>--- a/drivers/mtd/spi-nor/spi-nor.c
>>+++ b/drivers/mtd/spi-nor/spi-nor.c
>>@@ -330,8 +330,23 @@ static inline int spi_nor_fsr_ready(struct spi_nor
>*nor)
>>  int fsr = read_fsr(nor);
>>  if (fsr < 0)
>>  return fsr;
>>- else
>>- return fsr & FSR_READY;
>>+
>>+ if (fsr & (FSR_E_ERR | FSR_P_ERR)) {
>>+ if (fsr & FSR_E_ERR)
>>+ dev_err(nor->dev, "Erase operation failed.\n");
>>+ else
>>+ dev_err(nor->dev, "Program operation failed.\n");
>>+
>>+ if (fsr & FSR_PT_ERR)
>>+ dev_err(nor->dev,
>>+ "The operation has attempted to modify the
>>protected"
>>+ "sector or the locked OPT space.\n");
>>+
>>+ nor->write_reg(nor, SPINOR_OP_CLFSR, NULL, 0);
>>+ return -EIO;
>>+ }
>>+
>>+ return fsr & FSR_READY;
>> }
>>
>> static int spi_nor_ready(struct spi_nor *nor) diff --git
>>a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h index
>>d0c66a0..46b5608 100644
>>--- a/include/linux/mtd/spi-nor.h
>>+++ b/include/linux/mtd/spi-nor.h
>>@@ -61,6 +61,7 @@
>> #define SPINOR_OP_RDSFDP 0x5a/* Read SFDP */
>> #define SPINOR_OP_RDCR   0x35/* Read configuration register
>>*/
>> #define SPINOR_OP_RDFSR  0x70/* Read flag status register */
>>+#define SPINOR_OP_CLFSR  0x50/* Clear flag status register */
>>
>> /* 4-byte address opcodes - used on Spansion and some Macronix flashes.
>*/
>> #define SPINOR_OP_READ_4B0x13/* Read data bytes (low
>frequency) */
>>@@ -130,7 +131,10 @@
>> #define EVCR_QUAD_EN_MICRON  BIT(7)  /* Micron Quad I/O */
>>
>> /* Flag Status Register bits */
>>-#define FSR_READYBIT(7)
>>+#define FSR_READYBIT(7)  /* Device status, 0 = Busy,1 = Ready */
>>+#define FSR_E_ERRBIT(5)  /* Erase operation status */
>>+#define FSR_P_ERRBIT(4)  /* Program operation status */
>>+#define FSR_PT_ERR   BIT(1)  /* Protection error bit */
>>
>> /* Configuration Register bits. */
>> #define CR_QUAD_EN_SPAN  BIT(1)  /* Spansion Quad I/O */
>>--
>>2.7.4
>>

RE: [PATCH V1] drivers:mtd:spi-nor:checkup FSR error bits

[Bean Huo (beanhuo)]

Ping SPI-NOR maintainers again


>
>Ping SPI-NOR maintainers
>
>>-Original Message-----
>>From: Bean Huo (beanhuo)
>>Sent: Samstag, 11. November 2017 21:49
>>To: 'cyrille.pitc...@wedev4u.fr' ;
>>marek.va...@gmail.com
>>Cc: 'dw...@infradead.org' ;
>>computersforpe...@gmail.com; 'linux-...@lists.infradead.org' >m...@lists.infradead.org>; linux-kernel@vger.kernel.org
>>Subject: [PATCH V1] drivers:mtd:spi-nor:checkup FSR error bits
>>
>>For the Micron SPI NOR, when the erase/program operation fails,
>>especially, for the failure results from intending to modify protected
>>space, spi-nor upper layers still get the return which shows the operation
>succeeds.
>>this because spi_nor_fsr_ready() only uses bit.7 to device whether ready.
>>For the most cases, even the error of erase/program occurs, SPI NOR
>>device is still ready. The device ready and the error are two different cases.
>>This patch is to fixup this issue and adding FSR (flag status register)
>>error bits checkup.
>>The FSR(flag status register) is a powerful tool to investigate the
>>staus of device,checking information regarding what is actually doing
>>the memory and detecting possible error conditions.
>>
>>Signed-off-by: beanhuo 
>>---
>> drivers/mtd/spi-nor/spi-nor.c | 19 +--
>> include/linux/mtd/spi-nor.h   |  6 +-
>> 2 files changed, 22 insertions(+), 3 deletions(-)
>>
>>diff --git a/drivers/mtd/spi-nor/spi-nor.c
>>b/drivers/mtd/spi-nor/spi-nor.c index bc266f7..200e814 100644
>>--- a/drivers/mtd/spi-nor/spi-nor.c
>>+++ b/drivers/mtd/spi-nor/spi-nor.c
>>@@ -330,8 +330,23 @@ static inline int spi_nor_fsr_ready(struct spi_nor
>*nor)
>>  int fsr = read_fsr(nor);
>>  if (fsr < 0)
>>  return fsr;
>>- else
>>- return fsr & FSR_READY;
>>+
>>+ if (fsr & (FSR_E_ERR | FSR_P_ERR)) {
>>+ if (fsr & FSR_E_ERR)
>>+ dev_err(nor->dev, "Erase operation failed.\n");
>>+ else
>>+ dev_err(nor->dev, "Program operation failed.\n");
>>+
>>+ if (fsr & FSR_PT_ERR)
>>+ dev_err(nor->dev,
>>+ "The operation has attempted to modify the
>>protected"
>>+ "sector or the locked OPT space.\n");
>>+
>>+ nor->write_reg(nor, SPINOR_OP_CLFSR, NULL, 0);
>>+ return -EIO;
>>+ }
>>+
>>+ return fsr & FSR_READY;
>> }
>>
>> static int spi_nor_ready(struct spi_nor *nor) diff --git
>>a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h index
>>d0c66a0..46b5608 100644
>>--- a/include/linux/mtd/spi-nor.h
>>+++ b/include/linux/mtd/spi-nor.h
>>@@ -61,6 +61,7 @@
>> #define SPINOR_OP_RDSFDP 0x5a/* Read SFDP */
>> #define SPINOR_OP_RDCR   0x35/* Read configuration register
>>*/
>> #define SPINOR_OP_RDFSR  0x70/* Read flag status register */
>>+#define SPINOR_OP_CLFSR  0x50/* Clear flag status register */
>>
>> /* 4-byte address opcodes - used on Spansion and some Macronix flashes.
>*/
>> #define SPINOR_OP_READ_4B0x13/* Read data bytes (low
>frequency) */
>>@@ -130,7 +131,10 @@
>> #define EVCR_QUAD_EN_MICRON  BIT(7)  /* Micron Quad I/O */
>>
>> /* Flag Status Register bits */
>>-#define FSR_READYBIT(7)
>>+#define FSR_READYBIT(7)  /* Device status, 0 = Busy,1 = Ready */
>>+#define FSR_E_ERRBIT(5)  /* Erase operation status */
>>+#define FSR_P_ERRBIT(4)  /* Program operation status */
>>+#define FSR_PT_ERR   BIT(1)  /* Protection error bit */
>>
>> /* Configuration Register bits. */
>> #define CR_QUAD_EN_SPAN  BIT(1)  /* Spansion Quad I/O */
>>--
>>2.7.4
>>

UFS utilities

[Bean Huo (beanhuo)]

Hi, all 
Is there someone knows if exists one utilis dedicated to UFS device, rather 
than SCSI utils?
I have tried sg3-utils, but it is not convenient for the embedded ARM-based 
system.
And also it doesn't support several UFS special command. 
If we don't have this kind of tool for UFS, is it necessary for us to develop a 
ufs-utils?
I need your every expert's inputs and suggestions.
Thanks in advance.

//Bean Huo

UFS utilities

[Bean Huo (beanhuo)]

Hi, all 
Is there someone knows if exists one utilis dedicated to UFS device, rather 
than SCSI utils?
I have tried sg3-utils, but it is not convenient for the embedded ARM-based 
system.
And also it doesn't support several UFS special command. 
If we don't have this kind of tool for UFS, is it necessary for us to develop a 
ufs-utils?
I need your every expert's inputs and suggestions.
Thanks in advance.

//Bean Huo

RE: [PATCH V1] drivers:mtd:spi-nor:checkup FSR error bits

[Bean Huo (beanhuo)]

Ping SPI-NOR maintainers

>-Original Message-
>From: Bean Huo (beanhuo)
>Sent: Samstag, 11. November 2017 21:49
>To: 'cyrille.pitc...@wedev4u.fr' <cyrille.pitc...@wedev4u.fr>;
>marek.va...@gmail.com
>Cc: 'dw...@infradead.org' <dw...@infradead.org>;
>computersforpe...@gmail.com; 'linux-...@lists.infradead.org' m...@lists.infradead.org>; linux-kernel@vger.kernel.org
>Subject: [PATCH V1] drivers:mtd:spi-nor:checkup FSR error bits
>
>For the Micron SPI NOR, when the erase/program operation fails, especially,
>for the failure results from intending to modify protected space, spi-nor
>upper layers still get the return which shows the operation succeeds.
>this because spi_nor_fsr_ready() only uses bit.7 to device whether ready.
>For the most cases, even the error of erase/program occurs, SPI NOR device is
>still ready. The device ready and the error are two different cases.
>This patch is to fixup this issue and adding FSR (flag status register) error 
>bits
>checkup.
>The FSR(flag status register) is a powerful tool to investigate the staus of
>device,checking information regarding what is actually doing the memory and
>detecting possible error conditions.
>
>Signed-off-by: beanhuo <bean...@micron.com>
>---
> drivers/mtd/spi-nor/spi-nor.c | 19 +--
> include/linux/mtd/spi-nor.h   |  6 +-
> 2 files changed, 22 insertions(+), 3 deletions(-)
>
>diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
>index bc266f7..200e814 100644
>--- a/drivers/mtd/spi-nor/spi-nor.c
>+++ b/drivers/mtd/spi-nor/spi-nor.c
>@@ -330,8 +330,23 @@ static inline int spi_nor_fsr_ready(struct spi_nor *nor)
>   int fsr = read_fsr(nor);
>   if (fsr < 0)
>   return fsr;
>-  else
>-  return fsr & FSR_READY;
>+
>+  if (fsr & (FSR_E_ERR | FSR_P_ERR)) {
>+  if (fsr & FSR_E_ERR)
>+  dev_err(nor->dev, "Erase operation failed.\n");
>+  else
>+  dev_err(nor->dev, "Program operation failed.\n");
>+
>+  if (fsr & FSR_PT_ERR)
>+  dev_err(nor->dev,
>+  "The operation has attempted to modify the
>protected"
>+  "sector or the locked OPT space.\n");
>+
>+  nor->write_reg(nor, SPINOR_OP_CLFSR, NULL, 0);
>+  return -EIO;
>+  }
>+
>+  return fsr & FSR_READY;
> }
>
> static int spi_nor_ready(struct spi_nor *nor) diff --git
>a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h index
>d0c66a0..46b5608 100644
>--- a/include/linux/mtd/spi-nor.h
>+++ b/include/linux/mtd/spi-nor.h
>@@ -61,6 +61,7 @@
> #define SPINOR_OP_RDSFDP  0x5a/* Read SFDP */
> #define SPINOR_OP_RDCR0x35/* Read configuration register
>*/
> #define SPINOR_OP_RDFSR   0x70/* Read flag status register */
>+#define SPINOR_OP_CLFSR   0x50/* Clear flag status register */
>
> /* 4-byte address opcodes - used on Spansion and some Macronix flashes. */
> #define SPINOR_OP_READ_4B 0x13/* Read data bytes (low frequency) */
>@@ -130,7 +131,10 @@
> #define EVCR_QUAD_EN_MICRON   BIT(7)  /* Micron Quad I/O */
>
> /* Flag Status Register bits */
>-#define FSR_READY BIT(7)
>+#define FSR_READY BIT(7)  /* Device status, 0 = Busy,1 = Ready */
>+#define FSR_E_ERR BIT(5)  /* Erase operation status */
>+#define FSR_P_ERR BIT(4)  /* Program operation status */
>+#define FSR_PT_ERRBIT(1)  /* Protection error bit */
>
> /* Configuration Register bits. */
> #define CR_QUAD_EN_SPAN   BIT(1)  /* Spansion Quad I/O */
>--
>2.7.4
>

RE: [PATCH V1] drivers:mtd:spi-nor:checkup FSR error bits

[Bean Huo (beanhuo)]

Ping SPI-NOR maintainers

>-Original Message-
>From: Bean Huo (beanhuo)
>Sent: Samstag, 11. November 2017 21:49
>To: 'cyrille.pitc...@wedev4u.fr' ;
>marek.va...@gmail.com
>Cc: 'dw...@infradead.org' ;
>computersforpe...@gmail.com; 'linux-...@lists.infradead.org' m...@lists.infradead.org>; linux-kernel@vger.kernel.org
>Subject: [PATCH V1] drivers:mtd:spi-nor:checkup FSR error bits
>
>For the Micron SPI NOR, when the erase/program operation fails, especially,
>for the failure results from intending to modify protected space, spi-nor
>upper layers still get the return which shows the operation succeeds.
>this because spi_nor_fsr_ready() only uses bit.7 to device whether ready.
>For the most cases, even the error of erase/program occurs, SPI NOR device is
>still ready. The device ready and the error are two different cases.
>This patch is to fixup this issue and adding FSR (flag status register) error 
>bits
>checkup.
>The FSR(flag status register) is a powerful tool to investigate the staus of
>device,checking information regarding what is actually doing the memory and
>detecting possible error conditions.
>
>Signed-off-by: beanhuo 
>---
> drivers/mtd/spi-nor/spi-nor.c | 19 +--
> include/linux/mtd/spi-nor.h   |  6 +-
> 2 files changed, 22 insertions(+), 3 deletions(-)
>
>diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
>index bc266f7..200e814 100644
>--- a/drivers/mtd/spi-nor/spi-nor.c
>+++ b/drivers/mtd/spi-nor/spi-nor.c
>@@ -330,8 +330,23 @@ static inline int spi_nor_fsr_ready(struct spi_nor *nor)
>   int fsr = read_fsr(nor);
>   if (fsr < 0)
>   return fsr;
>-  else
>-  return fsr & FSR_READY;
>+
>+  if (fsr & (FSR_E_ERR | FSR_P_ERR)) {
>+  if (fsr & FSR_E_ERR)
>+  dev_err(nor->dev, "Erase operation failed.\n");
>+  else
>+  dev_err(nor->dev, "Program operation failed.\n");
>+
>+  if (fsr & FSR_PT_ERR)
>+  dev_err(nor->dev,
>+  "The operation has attempted to modify the
>protected"
>+  "sector or the locked OPT space.\n");
>+
>+  nor->write_reg(nor, SPINOR_OP_CLFSR, NULL, 0);
>+  return -EIO;
>+  }
>+
>+  return fsr & FSR_READY;
> }
>
> static int spi_nor_ready(struct spi_nor *nor) diff --git
>a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h index
>d0c66a0..46b5608 100644
>--- a/include/linux/mtd/spi-nor.h
>+++ b/include/linux/mtd/spi-nor.h
>@@ -61,6 +61,7 @@
> #define SPINOR_OP_RDSFDP  0x5a/* Read SFDP */
> #define SPINOR_OP_RDCR0x35/* Read configuration register
>*/
> #define SPINOR_OP_RDFSR   0x70/* Read flag status register */
>+#define SPINOR_OP_CLFSR   0x50/* Clear flag status register */
>
> /* 4-byte address opcodes - used on Spansion and some Macronix flashes. */
> #define SPINOR_OP_READ_4B 0x13/* Read data bytes (low frequency) */
>@@ -130,7 +131,10 @@
> #define EVCR_QUAD_EN_MICRON   BIT(7)  /* Micron Quad I/O */
>
> /* Flag Status Register bits */
>-#define FSR_READY BIT(7)
>+#define FSR_READY BIT(7)  /* Device status, 0 = Busy,1 = Ready */
>+#define FSR_E_ERR BIT(5)  /* Erase operation status */
>+#define FSR_P_ERR BIT(4)  /* Program operation status */
>+#define FSR_PT_ERRBIT(1)  /* Protection error bit */
>
> /* Configuration Register bits. */
> #define CR_QUAD_EN_SPAN   BIT(1)  /* Spansion Quad I/O */
>--
>2.7.4
>

[PATCH V1] drivers:mtd:spi-nor:checkup FSR error bits

[Bean Huo (beanhuo)]

For the Micron SPI NOR, when the erase/program operation fails, especially,
for the failure results from intending to modify protected space, spi-nor
upper layers still get the return which shows the operation succeeds.
this because spi_nor_fsr_ready() only uses bit.7 to device whether ready.
For the most cases, even the error of erase/program occurs, SPI NOR device
is still ready. The device ready and the error are two different cases.
This patch is to fixup this issue and adding FSR (flag status register)
error bits checkup.
The FSR(flag status register) is a powerful tool to investigate the staus
of device,checking information regarding what is actually doing the memory
and detecting possible error conditions.

Signed-off-by: beanhuo 
---
 drivers/mtd/spi-nor/spi-nor.c | 19 +--
 include/linux/mtd/spi-nor.h   |  6 +-
 2 files changed, 22 insertions(+), 3 deletions(-)

diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index bc266f7..200e814 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -330,8 +330,23 @@ static inline int spi_nor_fsr_ready(struct spi_nor *nor)
int fsr = read_fsr(nor);
if (fsr < 0)
return fsr;
-   else
-   return fsr & FSR_READY;
+
+   if (fsr & (FSR_E_ERR | FSR_P_ERR)) {
+   if (fsr & FSR_E_ERR)
+   dev_err(nor->dev, "Erase operation failed.\n");
+   else
+   dev_err(nor->dev, "Program operation failed.\n");
+
+   if (fsr & FSR_PT_ERR)
+   dev_err(nor->dev,
+   "The operation has attempted to modify the protected"
+   "sector or the locked OPT space.\n");
+
+   nor->write_reg(nor, SPINOR_OP_CLFSR, NULL, 0);
+   return -EIO;
+   }
+
+   return fsr & FSR_READY;
 }
 
 static int spi_nor_ready(struct spi_nor *nor)
diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
index d0c66a0..46b5608 100644
--- a/include/linux/mtd/spi-nor.h
+++ b/include/linux/mtd/spi-nor.h
@@ -61,6 +61,7 @@
 #define SPINOR_OP_RDSFDP   0x5a/* Read SFDP */
 #define SPINOR_OP_RDCR 0x35/* Read configuration register */
 #define SPINOR_OP_RDFSR0x70/* Read flag status register */
+#define SPINOR_OP_CLFSR0x50/* Clear flag status register */
 
 /* 4-byte address opcodes - used on Spansion and some Macronix flashes. */
 #define SPINOR_OP_READ_4B  0x13/* Read data bytes (low frequency) */
@@ -130,7 +131,10 @@
 #define EVCR_QUAD_EN_MICRONBIT(7)  /* Micron Quad I/O */
 
 /* Flag Status Register bits */
-#define FSR_READY  BIT(7)
+#define FSR_READY  BIT(7)  /* Device status, 0 = Busy,1 = Ready */
+#define FSR_E_ERR  BIT(5)  /* Erase operation status */
+#define FSR_P_ERR  BIT(4)  /* Program operation status */
+#define FSR_PT_ERR BIT(1)  /* Protection error bit */
 
 /* Configuration Register bits. */
 #define CR_QUAD_EN_SPANBIT(1)  /* Spansion Quad I/O */
-- 
2.7.4

[PATCH V1] drivers:mtd:spi-nor:checkup FSR error bits

[Bean Huo (beanhuo)]

For the Micron SPI NOR, when the erase/program operation fails, especially,
for the failure results from intending to modify protected space, spi-nor
upper layers still get the return which shows the operation succeeds.
this because spi_nor_fsr_ready() only uses bit.7 to device whether ready.
For the most cases, even the error of erase/program occurs, SPI NOR device
is still ready. The device ready and the error are two different cases.
This patch is to fixup this issue and adding FSR (flag status register)
error bits checkup.
The FSR(flag status register) is a powerful tool to investigate the staus
of device,checking information regarding what is actually doing the memory
and detecting possible error conditions.

Signed-off-by: beanhuo 
---
 drivers/mtd/spi-nor/spi-nor.c | 19 +--
 include/linux/mtd/spi-nor.h   |  6 +-
 2 files changed, 22 insertions(+), 3 deletions(-)

diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index bc266f7..200e814 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -330,8 +330,23 @@ static inline int spi_nor_fsr_ready(struct spi_nor *nor)
int fsr = read_fsr(nor);
if (fsr < 0)
return fsr;
-   else
-   return fsr & FSR_READY;
+
+   if (fsr & (FSR_E_ERR | FSR_P_ERR)) {
+   if (fsr & FSR_E_ERR)
+   dev_err(nor->dev, "Erase operation failed.\n");
+   else
+   dev_err(nor->dev, "Program operation failed.\n");
+
+   if (fsr & FSR_PT_ERR)
+   dev_err(nor->dev,
+   "The operation has attempted to modify the protected"
+   "sector or the locked OPT space.\n");
+
+   nor->write_reg(nor, SPINOR_OP_CLFSR, NULL, 0);
+   return -EIO;
+   }
+
+   return fsr & FSR_READY;
 }
 
 static int spi_nor_ready(struct spi_nor *nor)
diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
index d0c66a0..46b5608 100644
--- a/include/linux/mtd/spi-nor.h
+++ b/include/linux/mtd/spi-nor.h
@@ -61,6 +61,7 @@
 #define SPINOR_OP_RDSFDP   0x5a/* Read SFDP */
 #define SPINOR_OP_RDCR 0x35/* Read configuration register */
 #define SPINOR_OP_RDFSR0x70/* Read flag status register */
+#define SPINOR_OP_CLFSR0x50/* Clear flag status register */
 
 /* 4-byte address opcodes - used on Spansion and some Macronix flashes. */
 #define SPINOR_OP_READ_4B  0x13/* Read data bytes (low frequency) */
@@ -130,7 +131,10 @@
 #define EVCR_QUAD_EN_MICRONBIT(7)  /* Micron Quad I/O */
 
 /* Flag Status Register bits */
-#define FSR_READY  BIT(7)
+#define FSR_READY  BIT(7)  /* Device status, 0 = Busy,1 = Ready */
+#define FSR_E_ERR  BIT(5)  /* Erase operation status */
+#define FSR_P_ERR  BIT(4)  /* Program operation status */
+#define FSR_PT_ERR BIT(1)  /* Protection error bit */
 
 /* Configuration Register bits. */
 #define CR_QUAD_EN_SPANBIT(1)  /* Spansion Quad I/O */
-- 
2.7.4

RE: [PATCH V1] mmc: core: fix still flush cache when eMMC cache off

[Bean Huo (beanhuo)]

Hi, Uffe

>>>On 19 March 2017 at 01:45, Bean Huo (beanhuo) <bean...@micron.com>
>wrote:
>>>> This patch fixes the issue that mmc_blk_issue_rq still flushes cache
>>>> when eMMC cache has already been off through user space tool, such
>>>> as mmc-utils.
>>>> The reason is that card->ext_csd.cache_ctrl isn't reset.
>>>
>>>First, why do you want to turn of the cache ctrl? Is the eMMC device
>>>having issues with it? Then we should invent a card quirk instead.
>>
>>
>> Why I turn off it? because I did power loss testing and validation, I should
>switch it off and on.
>> When I do some performance and power loss case validation on several
>> Linux release versions, I need to switch off or on cache through user space 
>> tool.
>> I can't confirm every user that likes me, But I think at least it is
>> not reasonable to flush eMMC cache, when internal eMMC cache is off.
>
>Ah, I see. Your use-case seems reasonable while validating robustness of the
>eMMC!
>
>>
>>>Second, what errors do you encounter when the mmc core tries to flush
>>>the cache when it has been turned off? Can you please elaborate on this?
>>
>>
>> No error found, but firstly, please think about overhead introduced by
>> useless flush cache, Unless you Don't care this tiny time. second,
>> under the condition of cache off, additional flush cache request still has 
>> impact
>>on Internal eMMC logic. I don't know What and how impact, but at least it is
>>really exist.
>
>Got it!
>
>However I still don't like the mmc ioctls API to compensate and deal with all
>"crazy-ness" that user-space may cause. Cache-ctrl is only one case out of 
>many.

Question is that not every Linux-mmc user really understand that shouldn't use 
"mmc ioctls API 
to compensate and deal with all crazy-ness that user-space may cause ".
no matter we like or dislike, Issue is already there; I think that most of eMMC 
user is now 
configuring eMMC through mmc-utils and mmc ioctls. This is a real condition.
Some automotive applications, Cache would not be enabled. 

Maybe I am shallow, mmc ioctls likes backdoor that let user access and 
configure eMMC some feature,
Even not too often. I still suggest to fix this in mmc ioctls, unless mmc 
iotcls being deleted, and mmc-utils
doesn't support Cache off/on.
Because eMMC internal cache has been disabled, but Linux-mmc still issue flush 
cache request, this is indeed
Not reasonable. It definitely increases system level overhead.

>I see two viable options to solve your problem.
>1) Extend mmc_test with a new test(s) for cache ctrl and perhaps 
>suspend/resume.
>Isn't this actually exactly what you want?

>2) Extend debugfs to be able to turn cache ctrl on/off.

I can try, but this patch is just to fix mmc ctrls backdoor on eMMC flush cache.

>[...]
>
>Kind regards
>Uffe
>--
>To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the 
>body of
>a message to majord...@vger.kernel.org More majordomo info at
>http://vger.kernel.org/majordomo-info.html

RE: [PATCH V1] mmc: core: fix still flush cache when eMMC cache off

[Bean Huo (beanhuo)]

Hi, Uffe

>>>On 19 March 2017 at 01:45, Bean Huo (beanhuo) 
>wrote:
>>>> This patch fixes the issue that mmc_blk_issue_rq still flushes cache
>>>> when eMMC cache has already been off through user space tool, such
>>>> as mmc-utils.
>>>> The reason is that card->ext_csd.cache_ctrl isn't reset.
>>>
>>>First, why do you want to turn of the cache ctrl? Is the eMMC device
>>>having issues with it? Then we should invent a card quirk instead.
>>
>>
>> Why I turn off it? because I did power loss testing and validation, I should
>switch it off and on.
>> When I do some performance and power loss case validation on several
>> Linux release versions, I need to switch off or on cache through user space 
>> tool.
>> I can't confirm every user that likes me, But I think at least it is
>> not reasonable to flush eMMC cache, when internal eMMC cache is off.
>
>Ah, I see. Your use-case seems reasonable while validating robustness of the
>eMMC!
>
>>
>>>Second, what errors do you encounter when the mmc core tries to flush
>>>the cache when it has been turned off? Can you please elaborate on this?
>>
>>
>> No error found, but firstly, please think about overhead introduced by
>> useless flush cache, Unless you Don't care this tiny time. second,
>> under the condition of cache off, additional flush cache request still has 
>> impact
>>on Internal eMMC logic. I don't know What and how impact, but at least it is
>>really exist.
>
>Got it!
>
>However I still don't like the mmc ioctls API to compensate and deal with all
>"crazy-ness" that user-space may cause. Cache-ctrl is only one case out of 
>many.

Question is that not every Linux-mmc user really understand that shouldn't use 
"mmc ioctls API 
to compensate and deal with all crazy-ness that user-space may cause ".
no matter we like or dislike, Issue is already there; I think that most of eMMC 
user is now 
configuring eMMC through mmc-utils and mmc ioctls. This is a real condition.
Some automotive applications, Cache would not be enabled. 

Maybe I am shallow, mmc ioctls likes backdoor that let user access and 
configure eMMC some feature,
Even not too often. I still suggest to fix this in mmc ioctls, unless mmc 
iotcls being deleted, and mmc-utils
doesn't support Cache off/on.
Because eMMC internal cache has been disabled, but Linux-mmc still issue flush 
cache request, this is indeed
Not reasonable. It definitely increases system level overhead.

>I see two viable options to solve your problem.
>1) Extend mmc_test with a new test(s) for cache ctrl and perhaps 
>suspend/resume.
>Isn't this actually exactly what you want?

>2) Extend debugfs to be able to turn cache ctrl on/off.

I can try, but this patch is just to fix mmc ctrls backdoor on eMMC flush cache.

>[...]
>
>Kind regards
>Uffe
>--
>To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the 
>body of
>a message to majord...@vger.kernel.org More majordomo info at
>http://vger.kernel.org/majordomo-info.html

RE: [PATCH V1] mmc: core: fix still flush cache when eMMC cache off

[Bean Huo (beanhuo)]

Hi, 

>On 19 March 2017 at 01:45, Bean Huo (beanhuo) <bean...@micron.com> wrote:
>> This patch fixes the issue that mmc_blk_issue_rq still flushes cache
>> when eMMC cache has already been off through user space tool, such as
>> mmc-utils.
>> The reason is that card->ext_csd.cache_ctrl isn't reset.
>
>First, why do you want to turn of the cache ctrl? Is the eMMC device having
>issues with it? Then we should invent a card quirk instead.


Why I turn off it? because I did power loss testing and validation, I should 
switch it off and on.
When I do some performance and power loss case validation on several Linux 
release versions,
I need to switch off or on cache through user space tool.
I can't confirm every user that likes me, But I think at least it is not 
reasonable to 
flush eMMC cache, when internal eMMC cache is off.

>Second, what errors do you encounter when the mmc core tries to flush the
>cache when it has been turned off? Can you please elaborate on this?


No error found, but firstly, please think about overhead introduced by useless 
flush cache, Unless you
Don't care this tiny time. second, under the condition of cache off, additional 
flush cache request still has impact on
Internal eMMC logic. I don't know What and how impact, but at least it is 
really exist.


>>
>> Signed-off-by: beanhuo <bean...@micron.com>
>> ---
>>  drivers/mmc/core/block.c | 9 +
>>  1 file changed, 9 insertions(+)
>>
>> diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c index
>> 1621fa0..fb3635ac 100644
>> --- a/drivers/mmc/core/block.c
>> +++ b/drivers/mmc/core/block.c
>> @@ -64,6 +64,7 @@ MODULE_ALIAS("mmc:block");
>>  #define MMC_BLK_TIMEOUT_MS  (10 * 60 * 1000)/* 10 minute timeout */
>>  #define MMC_SANITIZE_REQ_TIMEOUT 24  #define
>> MMC_EXTRACT_INDEX_FROM_ARG(x) ((x & 0x00FF) >> 16)
>> +#define MMC_EXTRACT_VALUE_FROM_ARG(x) ((x & 0xFF00) >> 8)
>>
>>  #define mmc_req_rel_wr(req)((req->cmd_flags & REQ_FUA) && \
>>   (rq_data_dir(req) == WRITE)) @@
>> -535,6 +536,14 @@ static int __mmc_blk_ioctl_cmd(struct mmc_card *card,
>struct mmc_blk_data *md,
>> return data.error;
>> }
>>
>> +   if ((MMC_EXTRACT_INDEX_FROM_ARG(cmd.arg) ==
>EXT_CSD_CACHE_CTRL) &&
>> +   (cmd.opcode == MMC_SWITCH) && (card->ext_csd.cache_size > 0)) {
>> +   if (MMC_EXTRACT_VALUE_FROM_ARG(cmd.arg) & 1)
>> +   card->ext_csd.cache_ctrl = 1;
>> +   else
>> +   card->ext_csd.cache_ctrl = 0;
>> +   }
>> +
>
>I am sure "cache ctrl" isn't the only thing user space via mmc ioctl can cause
>problems for. The mmc core keep tracks of other ext csd states, etc, as well. I
>don't think it's worth to compensate and try to act accordingly to cover cases
>when user space has messed up.
>
>To be clear, it would have been entirely different if the something was changed
>via a mmc sysfs interface. Then we really should act accordingly, however for
>mmc ioctls it just becomes unmanageable due to its flexibility.

I will check this later. But flush cache seams now there is only one entry, and
It checks "cache_ctrl".

>
>> /*
>>  * According to the SD specs, some commands require a delay after
>>  * issuing the command.
>> --
>> 2.7.4
>
>Kind regards
>Uffe
>--
>To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the 
>body of
>a message to majord...@vger.kernel.org More majordomo info at
>http://vger.kernel.org/majordomo-info.html

RE: [PATCH V1] mmc: core: fix still flush cache when eMMC cache off

[Bean Huo (beanhuo)]

Hi, 

>On 19 March 2017 at 01:45, Bean Huo (beanhuo)  wrote:
>> This patch fixes the issue that mmc_blk_issue_rq still flushes cache
>> when eMMC cache has already been off through user space tool, such as
>> mmc-utils.
>> The reason is that card->ext_csd.cache_ctrl isn't reset.
>
>First, why do you want to turn of the cache ctrl? Is the eMMC device having
>issues with it? Then we should invent a card quirk instead.


Why I turn off it? because I did power loss testing and validation, I should 
switch it off and on.
When I do some performance and power loss case validation on several Linux 
release versions,
I need to switch off or on cache through user space tool.
I can't confirm every user that likes me, But I think at least it is not 
reasonable to 
flush eMMC cache, when internal eMMC cache is off.

>Second, what errors do you encounter when the mmc core tries to flush the
>cache when it has been turned off? Can you please elaborate on this?


No error found, but firstly, please think about overhead introduced by useless 
flush cache, Unless you
Don't care this tiny time. second, under the condition of cache off, additional 
flush cache request still has impact on
Internal eMMC logic. I don't know What and how impact, but at least it is 
really exist.


>>
>> Signed-off-by: beanhuo 
>> ---
>>  drivers/mmc/core/block.c | 9 +
>>  1 file changed, 9 insertions(+)
>>
>> diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c index
>> 1621fa0..fb3635ac 100644
>> --- a/drivers/mmc/core/block.c
>> +++ b/drivers/mmc/core/block.c
>> @@ -64,6 +64,7 @@ MODULE_ALIAS("mmc:block");
>>  #define MMC_BLK_TIMEOUT_MS  (10 * 60 * 1000)/* 10 minute timeout */
>>  #define MMC_SANITIZE_REQ_TIMEOUT 24  #define
>> MMC_EXTRACT_INDEX_FROM_ARG(x) ((x & 0x00FF) >> 16)
>> +#define MMC_EXTRACT_VALUE_FROM_ARG(x) ((x & 0xFF00) >> 8)
>>
>>  #define mmc_req_rel_wr(req)((req->cmd_flags & REQ_FUA) && \
>>   (rq_data_dir(req) == WRITE)) @@
>> -535,6 +536,14 @@ static int __mmc_blk_ioctl_cmd(struct mmc_card *card,
>struct mmc_blk_data *md,
>> return data.error;
>> }
>>
>> +   if ((MMC_EXTRACT_INDEX_FROM_ARG(cmd.arg) ==
>EXT_CSD_CACHE_CTRL) &&
>> +   (cmd.opcode == MMC_SWITCH) && (card->ext_csd.cache_size > 0)) {
>> +   if (MMC_EXTRACT_VALUE_FROM_ARG(cmd.arg) & 1)
>> +   card->ext_csd.cache_ctrl = 1;
>> +   else
>> +   card->ext_csd.cache_ctrl = 0;
>> +   }
>> +
>
>I am sure "cache ctrl" isn't the only thing user space via mmc ioctl can cause
>problems for. The mmc core keep tracks of other ext csd states, etc, as well. I
>don't think it's worth to compensate and try to act accordingly to cover cases
>when user space has messed up.
>
>To be clear, it would have been entirely different if the something was changed
>via a mmc sysfs interface. Then we really should act accordingly, however for
>mmc ioctls it just becomes unmanageable due to its flexibility.

I will check this later. But flush cache seams now there is only one entry, and
It checks "cache_ctrl".

>
>> /*
>>  * According to the SD specs, some commands require a delay after
>>  * issuing the command.
>> --
>> 2.7.4
>
>Kind regards
>Uffe
>--
>To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the 
>body of
>a message to majord...@vger.kernel.org More majordomo info at
>http://vger.kernel.org/majordomo-info.html

RE: [PATCH V1] mmc: core: fix still flush cache when eMMC cache off

[Bean Huo (beanhuo)]

Ping Linux-mmc maintainer...

Tested-by: Shawn Lin 
Reviewed-by: Bartlomiej Zolnierkiewicz 
Reviewed-by: Shawn Lin 

>On Sunday, March 19, 2017 12:45:40 AM Bean Huo wrote:
>> This patch fixes the issue that mmc_blk_issue_rq still flushes cache
>> when eMMC cache has already been off through user space tool, such as
>> mmc-utils.
>> The reason is that card->ext_csd.cache_ctrl isn't reset.
>>
>> Signed-off-by: beanhuo 
>
>
>Best regards,
>--
>Bartlomiej Zolnierkiewicz
>Samsung R Institute Poland
>Samsung Electronics
>
>--
>To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the 
>body of
>a message to majord...@vger.kernel.org More majordomo info at
>http://vger.kernel.org/majordomo-info.html

//beanhuo

RE: [PATCH V1] mmc: core: fix still flush cache when eMMC cache off

[Bean Huo (beanhuo)]

Ping Linux-mmc maintainer...

Tested-by: Shawn Lin 
Reviewed-by: Bartlomiej Zolnierkiewicz 
Reviewed-by: Shawn Lin 

>On Sunday, March 19, 2017 12:45:40 AM Bean Huo wrote:
>> This patch fixes the issue that mmc_blk_issue_rq still flushes cache
>> when eMMC cache has already been off through user space tool, such as
>> mmc-utils.
>> The reason is that card->ext_csd.cache_ctrl isn't reset.
>>
>> Signed-off-by: beanhuo 
>
>
>Best regards,
>--
>Bartlomiej Zolnierkiewicz
>Samsung R Institute Poland
>Samsung Electronics
>
>--
>To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the 
>body of
>a message to majord...@vger.kernel.org More majordomo info at
>http://vger.kernel.org/majordomo-info.html

//beanhuo

[PATCH V1] mmc: core: fix still flush cache when eMMC cache off

[Bean Huo (beanhuo)]

This patch fixes the issue that mmc_blk_issue_rq still
flushes cache when eMMC cache has already been off
through user space tool, such as mmc-utils.
The reason is that card->ext_csd.cache_ctrl isn't reset.

Signed-off-by: beanhuo 
---
 drivers/mmc/core/block.c | 9 +
 1 file changed, 9 insertions(+)

diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c
index 1621fa0..fb3635ac 100644
--- a/drivers/mmc/core/block.c
+++ b/drivers/mmc/core/block.c
@@ -64,6 +64,7 @@ MODULE_ALIAS("mmc:block");
 #define MMC_BLK_TIMEOUT_MS  (10 * 60 * 1000)/* 10 minute timeout */
 #define MMC_SANITIZE_REQ_TIMEOUT 24
 #define MMC_EXTRACT_INDEX_FROM_ARG(x) ((x & 0x00FF) >> 16)
+#define MMC_EXTRACT_VALUE_FROM_ARG(x) ((x & 0xFF00) >> 8)
 
 #define mmc_req_rel_wr(req)((req->cmd_flags & REQ_FUA) && \
  (rq_data_dir(req) == WRITE))
@@ -535,6 +536,14 @@ static int __mmc_blk_ioctl_cmd(struct mmc_card *card, 
struct mmc_blk_data *md,
return data.error;
}
 
+   if ((MMC_EXTRACT_INDEX_FROM_ARG(cmd.arg) == EXT_CSD_CACHE_CTRL) &&
+   (cmd.opcode == MMC_SWITCH) && (card->ext_csd.cache_size > 0)) {
+   if (MMC_EXTRACT_VALUE_FROM_ARG(cmd.arg) & 1)
+   card->ext_csd.cache_ctrl = 1;
+   else
+   card->ext_csd.cache_ctrl = 0;
+   }
+
/*
 * According to the SD specs, some commands require a delay after
 * issuing the command.
-- 
2.7.4

[PATCH V1] mmc: core: fix still flush cache when eMMC cache off

[Bean Huo (beanhuo)]

This patch fixes the issue that mmc_blk_issue_rq still
flushes cache when eMMC cache has already been off
through user space tool, such as mmc-utils.
The reason is that card->ext_csd.cache_ctrl isn't reset.

Signed-off-by: beanhuo 
---
 drivers/mmc/core/block.c | 9 +
 1 file changed, 9 insertions(+)

diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c
index 1621fa0..fb3635ac 100644
--- a/drivers/mmc/core/block.c
+++ b/drivers/mmc/core/block.c
@@ -64,6 +64,7 @@ MODULE_ALIAS("mmc:block");
 #define MMC_BLK_TIMEOUT_MS  (10 * 60 * 1000)/* 10 minute timeout */
 #define MMC_SANITIZE_REQ_TIMEOUT 24
 #define MMC_EXTRACT_INDEX_FROM_ARG(x) ((x & 0x00FF) >> 16)
+#define MMC_EXTRACT_VALUE_FROM_ARG(x) ((x & 0xFF00) >> 8)
 
 #define mmc_req_rel_wr(req)((req->cmd_flags & REQ_FUA) && \
  (rq_data_dir(req) == WRITE))
@@ -535,6 +536,14 @@ static int __mmc_blk_ioctl_cmd(struct mmc_card *card, 
struct mmc_blk_data *md,
return data.error;
}
 
+   if ((MMC_EXTRACT_INDEX_FROM_ARG(cmd.arg) == EXT_CSD_CACHE_CTRL) &&
+   (cmd.opcode == MMC_SWITCH) && (card->ext_csd.cache_size > 0)) {
+   if (MMC_EXTRACT_VALUE_FROM_ARG(cmd.arg) & 1)
+   card->ext_csd.cache_ctrl = 1;
+   else
+   card->ext_csd.cache_ctrl = 0;
+   }
+
/*
 * According to the SD specs, some commands require a delay after
 * issuing the command.
-- 
2.7.4

RE: ftrace function_graph causes system crash

[Bean Huo (beanhuo)]

> -Original Message-
> From: Steven Rostedt [mailto:rost...@goodmis.org]
> Sent: Mittwoch, 21. September 2016 20:17
> To: Jisheng Zhang <jszh...@marvell.com>
> Cc: Bean Huo (beanhuo) <bean...@micron.com>; Zoltan Szubbocsev
> (zszubbocsev) <zszubboc...@micron.com>; catalin.mari...@arm.com;
> will.dea...@arm.com; r...@lists.rocketboards.org; linux-
> ker...@vger.kernel.org; mi...@redhat.com; linux-arm-
> ker...@lists.infradead.org
> Subject: Re: ftrace function_graph causes system crash
> 
> On Wed, 21 Sep 2016 17:13:07 +0800
> Jisheng Zhang <jszh...@marvell.com> wrote:
> 
> > I'm not sure whether the commit d6df3576e6b4
> ("clocksource/drivers/arm_global_timer
> > : Prevent ftrace recursion") can fix this issue.
> >
> > this commit is merged since v4.3, I noticed your kernel version is v4.0
> 
> BTW, yes, that would be the fix.
> 
> -- Steve
> 
> >
> > Thanks,
> > Jisheng
> >
> > > Do you know now how to deeply debug and trace which line is wrong
> through Ftrace?
> > >

Hi, Steven and Jisheng
Thanks to both warm-hearted guys. I merged  d6df3576e6b4 patch into my kernel 
4.0.
Then it is true, no cash appears again.

I have one more question that current ftrace can trace DMA latency, include 
mapping and unmapping?
Means I want to know when one BIO request be completed. Just like blktrace.
But blktrace can not tell me the function calling sequence.

--Bean 

RE: ftrace function_graph causes system crash

[Bean Huo (beanhuo)]

> -Original Message-
> From: Steven Rostedt [mailto:rost...@goodmis.org]
> Sent: Mittwoch, 21. September 2016 20:17
> To: Jisheng Zhang 
> Cc: Bean Huo (beanhuo) ; Zoltan Szubbocsev
> (zszubbocsev) ; catalin.mari...@arm.com;
> will.dea...@arm.com; r...@lists.rocketboards.org; linux-
> ker...@vger.kernel.org; mi...@redhat.com; linux-arm-
> ker...@lists.infradead.org
> Subject: Re: ftrace function_graph causes system crash
> 
> On Wed, 21 Sep 2016 17:13:07 +0800
> Jisheng Zhang  wrote:
> 
> > I'm not sure whether the commit d6df3576e6b4
> ("clocksource/drivers/arm_global_timer
> > : Prevent ftrace recursion") can fix this issue.
> >
> > this commit is merged since v4.3, I noticed your kernel version is v4.0
> 
> BTW, yes, that would be the fix.
> 
> -- Steve
> 
> >
> > Thanks,
> > Jisheng
> >
> > > Do you know now how to deeply debug and trace which line is wrong
> through Ftrace?
> > >

Hi, Steven and Jisheng
Thanks to both warm-hearted guys. I merged  d6df3576e6b4 patch into my kernel 
4.0.
Then it is true, no cash appears again.

I have one more question that current ftrace can trace DMA latency, include 
mapping and unmapping?
Means I want to know when one BIO request be completed. Just like blktrace.
But blktrace can not tell me the function calling sequence.

--Bean 

RE: ftrace function_graph causes system crash

[Bean Huo (beanhuo)]

> From: linux-arm-kernel [mailto:linux-arm-kernel-boun...@lists.infradead.org]
> On Behalf Of Steven Rostedt
> Sent: Dienstag, 20. September 2016 16:07
> To: Bean Huo (beanhuo) <bean...@micron.com>
> Cc: Zoltan Szubbocsev (zszubbocsev) <zszubboc...@micron.com>;
> catalin.mari...@arm.com; will.dea...@arm.com; r...@lists.rocketboards.org;
> linux-kernel@vger.kernel.org; mi...@redhat.com; linux-arm-
> ker...@lists.infradead.org
> Subject: Re: ftrace function_graph causes system crash
> 
> On Tue, 20 Sep 2016 13:10:39 +
> "Bean Huo (beanhuo)" <bean...@micron.com> wrote:
> 
> > Hi, all
> > I just use ftrace to do some latency study, found that function_graph
> > can not Work, as long as enable it, will cause kernel panic. I searched this
> online.
> > Found that there are also some cause the same as mine. I am a newer of
> ftrace.
> > I want to know who know what root cause? Here is some partial log:
> >
> >
> 
> Can you do a function bisect to find what function this is.
> 
> This script is used to help find functions that are being traced by function 
> tracer
> or function graph tracing that causes the machine to reboot, hang, or crash.
> Here's the steps to take.
> 
> First, determine if function graph is working with a single function:
> 
> # cd /sys/kernel/debug/tracing
> # echo schedule > set_ftrace_filter
> # echo function_graph > current_tracer
> 
> If this works, then we know that something is being traced that shouldn't be.
> 
> # echo nop > current_tracer
> 
> # cat available_filter_functions > ~/full-file # ftrace-bisect ~/full-file 
> ~/test-file
> ~/non-test-file # cat ~/test-file > set_ftrace_filter
> 
> *** Note *** this will take several minutes. Setting multiple functions is an
> O(n^2) operation, and we are dealing with thousands of functions.
> So go have  coffee, talk with your coworkers, read facebook. And eventually,
> this operation will end.
> 
> # echo function_graph > current_tracer
> 
> If it crashes, we know that ~/test-file has a bad function.
> 
>Reboot back to test kernel.
> 
># cd /sys/kernel/debug/tracing
># mv ~/test-file ~/full-file
> 
> If it didn't crash.
> 
># echo nop > current_tracer
># mv ~/non-test-file ~/full-file
> 
> Get rid of the other test file from previous run (or save them off somewhere.
> # rm -f ~/test-file ~/non-test-file
> 
> And start again:
> 
> # ftrace-bisect ~/full-file ~/test-file ~/non-test-file
> 
> The good thing is, because this cuts the number of functions in ~/test-file 
> by half,
> the cat of it into set_ftrace_filter takes half as long each iteration, so 
> don't talk
> so much at the water cooler the second time.
> 
> Eventually, if you did this correctly, you will get down to the problem 
> function,
> and all we need to do is to notrace it.
> 
> The way to figure out if the problem function is bad, just do:
> 
> # echo  > set_ftrace_notrace # echo > set_ftrace_filter #
> echo function_graph > current_tracer
> 
> And if it doesn't crash, we are done.
> 
> -- Steve


Hi, Steve
Thanks very much! This is a very useful trace tool, I now know the problem 
function,
It is gt_counter_read, if not trace this function, ftrace function_graph work 
well.
Do you know now how to deeply debug and trace which line is wrong through 
Ftrace?

--Bean

RE: ftrace function_graph causes system crash

[Bean Huo (beanhuo)]

> From: linux-arm-kernel [mailto:linux-arm-kernel-boun...@lists.infradead.org]
> On Behalf Of Steven Rostedt
> Sent: Dienstag, 20. September 2016 16:07
> To: Bean Huo (beanhuo) 
> Cc: Zoltan Szubbocsev (zszubbocsev) ;
> catalin.mari...@arm.com; will.dea...@arm.com; r...@lists.rocketboards.org;
> linux-kernel@vger.kernel.org; mi...@redhat.com; linux-arm-
> ker...@lists.infradead.org
> Subject: Re: ftrace function_graph causes system crash
> 
> On Tue, 20 Sep 2016 13:10:39 +
> "Bean Huo (beanhuo)"  wrote:
> 
> > Hi, all
> > I just use ftrace to do some latency study, found that function_graph
> > can not Work, as long as enable it, will cause kernel panic. I searched this
> online.
> > Found that there are also some cause the same as mine. I am a newer of
> ftrace.
> > I want to know who know what root cause? Here is some partial log:
> >
> >
> 
> Can you do a function bisect to find what function this is.
> 
> This script is used to help find functions that are being traced by function 
> tracer
> or function graph tracing that causes the machine to reboot, hang, or crash.
> Here's the steps to take.
> 
> First, determine if function graph is working with a single function:
> 
> # cd /sys/kernel/debug/tracing
> # echo schedule > set_ftrace_filter
> # echo function_graph > current_tracer
> 
> If this works, then we know that something is being traced that shouldn't be.
> 
> # echo nop > current_tracer
> 
> # cat available_filter_functions > ~/full-file # ftrace-bisect ~/full-file 
> ~/test-file
> ~/non-test-file # cat ~/test-file > set_ftrace_filter
> 
> *** Note *** this will take several minutes. Setting multiple functions is an
> O(n^2) operation, and we are dealing with thousands of functions.
> So go have  coffee, talk with your coworkers, read facebook. And eventually,
> this operation will end.
> 
> # echo function_graph > current_tracer
> 
> If it crashes, we know that ~/test-file has a bad function.
> 
>Reboot back to test kernel.
> 
># cd /sys/kernel/debug/tracing
># mv ~/test-file ~/full-file
> 
> If it didn't crash.
> 
># echo nop > current_tracer
># mv ~/non-test-file ~/full-file
> 
> Get rid of the other test file from previous run (or save them off somewhere.
> # rm -f ~/test-file ~/non-test-file
> 
> And start again:
> 
> # ftrace-bisect ~/full-file ~/test-file ~/non-test-file
> 
> The good thing is, because this cuts the number of functions in ~/test-file 
> by half,
> the cat of it into set_ftrace_filter takes half as long each iteration, so 
> don't talk
> so much at the water cooler the second time.
> 
> Eventually, if you did this correctly, you will get down to the problem 
> function,
> and all we need to do is to notrace it.
> 
> The way to figure out if the problem function is bad, just do:
> 
> # echo  > set_ftrace_notrace # echo > set_ftrace_filter #
> echo function_graph > current_tracer
> 
> And if it doesn't crash, we are done.
> 
> -- Steve


Hi, Steve
Thanks very much! This is a very useful trace tool, I now know the problem 
function,
It is gt_counter_read, if not trace this function, ftrace function_graph work 
well.
Do you know now how to deeply debug and trace which line is wrong through 
Ftrace?

--Bean

ftrace function_graph causes system crash

[Bean Huo (beanhuo)]

Hi, all 
I just use ftrace to do some latency study, found that function_graph can not
Work, as long as enable it, will cause kernel panic. I searched this online.
Found that there are also some cause the same as mine. I am a newer of ftrace. 
I want to know who know what root cause? Here is some partial log:


echo function_graph > current_tracer 
[9.583813] Unable to handle kernel paging request at virtual address 
b0200083
[9.590997] pgd = c0004000
[9.593683] [b0200083] *pgd=
[9.597253] Internal error: Oops: 5 [#1] PREEMPT SMP ARM
[9.602542] Modules linked in:
[9.605586] CPU: 1 PID: 15 Comm: kworker/1:0 Not tainted 
4.0.0-xilinx-00043-gc701690-dirty #515
[9.614256] Hardware name: Xilinx Zynq Platform
[9.618793] Workqueue:  0xe3a1 ( “å)
[9.622765] task: df517500 ti: df518000 task.ti: df518000
[9.628162] PC is at rb_update_write_stamp+0x18/0xa0
[9.633100] LR is at rb_commit+0x34/0xdc
[9.637005] pc : []lr : []psr: 6093
[9.637005] sp : df51a0f0  ip : df40b00c  fp : df51a104
[9.648456] r10:   r9 :   r8 : 00bbff80
[9.653666] r7 : c0edcf18  r6 : df51a190  r5 : df404100  r4 : df484680
[9.660175] r3 : b020006b  r2 : df40b000  r1 : df40b2bc  r0 : df484680
[9.87] Flags: nZCv  IRQs off  FIQs on  Mode SVC_32  ISA ARM  Segment 
kernel
[9.674065] Control: 18c5387d  Table: 1ee7004a  DAC: 0015
[9.679792] Process kworker/1:0 (pid: 15, stack limit = 0xdf518210)
[9.686041] Stack: (0xdf51a0f0 to 0xdf51a000)
[9.690411] [] (rb_update_write_stamp) from [] 
(rb_commit+0x34/0xdc)
[9.698478] [] (rb_commit) from [] 
(ring_buffer_unlock_commit+0x30/0x18c)
[9.706987] [] (ring_buffer_unlock_commit) from [] 
(__buffer_unlock_commit+0x20/0x28)
[9.716534] [] (__buffer_unlock_commit) from [] 
(__trace_graph_entry+0x94/0xa8)
[9.725557] [] (__trace_graph_entry) from [] 
(trace_graph_entry+0x1b8/0x224)
[9.734323] [] (trace_graph_entry) from [] 
(prepare_ftrace_return+0x70/0xac)
[9.743089] [] (prepare_ftrace_return) from [] 
(ftrace_graph_caller+0x18/0x20)
[9.752020] Code: e24cb004 e5903048 e3c1ceff e3cc200f (e5933018) 
[9.758104] ---[ end trace 5781781938261a54 ]---
[9.762689] Kernel panic - not syncing: Fatal exception
[   10.933397] SMP: failed to stop secondary CPUs
[   10.937805] ---[ end Kernel panic - not syncing: Fatal exception
[   10.943779] CPU1: stopping
[   10.946471] CPU: 1 PID: 15 Comm: kworker/1:0 Tainted: G  D 
4.0.0-xilinx-00043-gc701690-dirty #515
[   10.956358] Hardware name: Xilinx Zynq Platform
[   10.960889] Workqueue:  0xe3a1 ( “å)
[   10.964896] [] (unwind_backtrace) from [] 
(show_stack+0x20/0x24)
[   10.972620] [] (show_stack) from [] 
(dump_stack+0x80/0xd0)
[   10.979822] [] (dump_stack) from [] 
(ipi_cpu_stop+0x4c/0x80)
[   10.987197] [] (ipi_cpu_stop) from [] 
(handle_IPI+0x74/0xbc)
[   10.994573] [] (handle_IPI) from [] 
(gic_handle_irq+0x68/0x70)
[   11.002123] [] (gic_handle_irq) from [] 
(__irq_svc+0x44/0x7c)
[   11.009569] Exception stack(0xdf519e70 to 0xdf519eb8)
[   11.014608] 9e60: 0034 df518000 
 c08d9524
[   11.022770] 9e80:   c00317c8  c00b6674  
0008 df519edc
[   11.030929] 9ea0: df519eb8 df519eb8 c058f64c c058f650 6113 
[   11.037541] [] (__irq_svc) from [] (panic+0x188/0x20c)
[   11.044399] [] (panic) from [] (die+0x380/0x3e0)
[   11.050739] [] (die) from [] 
(__do_kernel_fault+0x74/0x94)
[   11.057941] [] (__do_kernel_fault) from [] 
(do_page_fault+0x2fc/0x31c)
[   11.066185] [] (do_page_fault) from [] 
(do_translation_fault+0x2c/0xc0)

ftrace function_graph causes system crash

[Bean Huo (beanhuo)]

Hi, all 
I just use ftrace to do some latency study, found that function_graph can not
Work, as long as enable it, will cause kernel panic. I searched this online.
Found that there are also some cause the same as mine. I am a newer of ftrace. 
I want to know who know what root cause? Here is some partial log:


echo function_graph > current_tracer 
[9.583813] Unable to handle kernel paging request at virtual address 
b0200083
[9.590997] pgd = c0004000
[9.593683] [b0200083] *pgd=
[9.597253] Internal error: Oops: 5 [#1] PREEMPT SMP ARM
[9.602542] Modules linked in:
[9.605586] CPU: 1 PID: 15 Comm: kworker/1:0 Not tainted 
4.0.0-xilinx-00043-gc701690-dirty #515
[9.614256] Hardware name: Xilinx Zynq Platform
[9.618793] Workqueue:  0xe3a1 ( “å)
[9.622765] task: df517500 ti: df518000 task.ti: df518000
[9.628162] PC is at rb_update_write_stamp+0x18/0xa0
[9.633100] LR is at rb_commit+0x34/0xdc
[9.637005] pc : []lr : []psr: 6093
[9.637005] sp : df51a0f0  ip : df40b00c  fp : df51a104
[9.648456] r10:   r9 :   r8 : 00bbff80
[9.653666] r7 : c0edcf18  r6 : df51a190  r5 : df404100  r4 : df484680
[9.660175] r3 : b020006b  r2 : df40b000  r1 : df40b2bc  r0 : df484680
[9.87] Flags: nZCv  IRQs off  FIQs on  Mode SVC_32  ISA ARM  Segment 
kernel
[9.674065] Control: 18c5387d  Table: 1ee7004a  DAC: 0015
[9.679792] Process kworker/1:0 (pid: 15, stack limit = 0xdf518210)
[9.686041] Stack: (0xdf51a0f0 to 0xdf51a000)
[9.690411] [] (rb_update_write_stamp) from [] 
(rb_commit+0x34/0xdc)
[9.698478] [] (rb_commit) from [] 
(ring_buffer_unlock_commit+0x30/0x18c)
[9.706987] [] (ring_buffer_unlock_commit) from [] 
(__buffer_unlock_commit+0x20/0x28)
[9.716534] [] (__buffer_unlock_commit) from [] 
(__trace_graph_entry+0x94/0xa8)
[9.725557] [] (__trace_graph_entry) from [] 
(trace_graph_entry+0x1b8/0x224)
[9.734323] [] (trace_graph_entry) from [] 
(prepare_ftrace_return+0x70/0xac)
[9.743089] [] (prepare_ftrace_return) from [] 
(ftrace_graph_caller+0x18/0x20)
[9.752020] Code: e24cb004 e5903048 e3c1ceff e3cc200f (e5933018) 
[9.758104] ---[ end trace 5781781938261a54 ]---
[9.762689] Kernel panic - not syncing: Fatal exception
[   10.933397] SMP: failed to stop secondary CPUs
[   10.937805] ---[ end Kernel panic - not syncing: Fatal exception
[   10.943779] CPU1: stopping
[   10.946471] CPU: 1 PID: 15 Comm: kworker/1:0 Tainted: G  D 
4.0.0-xilinx-00043-gc701690-dirty #515
[   10.956358] Hardware name: Xilinx Zynq Platform
[   10.960889] Workqueue:  0xe3a1 ( “å)
[   10.964896] [] (unwind_backtrace) from [] 
(show_stack+0x20/0x24)
[   10.972620] [] (show_stack) from [] 
(dump_stack+0x80/0xd0)
[   10.979822] [] (dump_stack) from [] 
(ipi_cpu_stop+0x4c/0x80)
[   10.987197] [] (ipi_cpu_stop) from [] 
(handle_IPI+0x74/0xbc)
[   10.994573] [] (handle_IPI) from [] 
(gic_handle_irq+0x68/0x70)
[   11.002123] [] (gic_handle_irq) from [] 
(__irq_svc+0x44/0x7c)
[   11.009569] Exception stack(0xdf519e70 to 0xdf519eb8)
[   11.014608] 9e60: 0034 df518000 
 c08d9524
[   11.022770] 9e80:   c00317c8  c00b6674  
0008 df519edc
[   11.030929] 9ea0: df519eb8 df519eb8 c058f64c c058f650 6113 
[   11.037541] [] (__irq_svc) from [] (panic+0x188/0x20c)
[   11.044399] [] (panic) from [] (die+0x380/0x3e0)
[   11.050739] [] (die) from [] 
(__do_kernel_fault+0x74/0x94)
[   11.057941] [] (__do_kernel_fault) from [] 
(do_page_fault+0x2fc/0x31c)
[   11.066185] [] (do_page_fault) from [] 
(do_translation_fault+0x2c/0xc0)