[v4,1/1] scsi: ufs: core: add device level exception support

Commit Message

Bao D. Nguyen March 21, 2025, 3:18 a.m. UTC

The ufs device JEDEC specification version 4.1 adds support for the
device level exception events. To support this new device level
exception feature, expose two new sysfs nodes below to provide
the user space access to the device level exception information.
/sys/bus/platform/drivers/ufshcd/*/device_lvl_exception_count
/sys/bus/platform/drivers/ufshcd/*/device_lvl_exception_id

The device_lvl_exception_count sysfs node reports the number of
device level exceptions that have occurred since the last time
this variable is reset. Writing a value of 0 will reset it.
The device_lvl_exception_id reports the exception ID which is the
qDeviceLevelExceptionID attribute of the device JEDEC specifications
version 4.1 and later. The user space application can query these
sysfs nodes to get more information about the device level exception.

Signed-off-by: Bao D. Nguyen <[e-mail]>
Reviewed-by: Peter Wang <[e-mail]>
---
Changes in v4:
1. Changed the hba->dev_lvl_exception_count to atomic_t type. Removed
the spinlock() around hba->dev_lvl_exception_count accesses
(Peter's and Bart's comments).

Changes in v3:
1. Add protection for hba->dev_lvl_exception_count accesses in different
contexts (Bart's comment).

Changes in v2:
1. Addressed Mani's comments:
   - Update the documentation of dev_lvl_exception_count to read/write.
   - Rephrase the description of the Documentation and commit text.
   - Remove the export of ufshcd_read_device_lvl_exception_id().
2. Addressed Bart's comments:
   - Rename dev_lvl_exception sysfs node to dev_lvl_exception_count.
   - Update the documentation of the sysfs nodes.
   - Skip comment about sysfs_notify() being used in interrupt
     context because Avri already addressed it.
---
 Documentation/ABI/testing/sysfs-driver-ufs | 27 ++++++++++++++
 drivers/ufs/core/ufs-sysfs.c               | 54 +++++++++++++++++++++++++++
 drivers/ufs/core/ufshcd-priv.h             |  1 +
 drivers/ufs/core/ufshcd.c                  | 60 ++++++++++++++++++++++++++++++
 include/uapi/scsi/scsi_bsg_ufs.h           |  9 +++++
 include/ufs/ufs.h                          |  5 ++-
 include/ufs/ufshcd.h                       |  5 +++
 7 files changed, 160 insertions(+), 1 deletion(-)

Comments

Bart Van Assche March 21, 2025, 4:31 p.m. UTC | #1

On 3/20/25 8:18 PM, Bao D. Nguyen wrote:
> The ufs device JEDEC specification version 4.1 adds support for the
> device level exception events. To support this new device level
> exception feature, expose two new sysfs nodes below to provide
> the user space access to the device level exception information.
> /sys/bus/platform/drivers/ufshcd/*/device_lvl_exception_count
> /sys/bus/platform/drivers/ufshcd/*/device_lvl_exception_id
> 
> The device_lvl_exception_count sysfs node reports the number of
> device level exceptions that have occurred since the last time
> this variable is reset. Writing a value of 0 will reset it.
> The device_lvl_exception_id reports the exception ID which is the
> qDeviceLevelExceptionID attribute of the device JEDEC specifications
> version 4.1 and later. The user space application can query these
> sysfs nodes to get more information about the device level exception.

Reviewed-by: Bart Van Assche <[e-mail]>

Arthur Simchaev March 25, 2025, 4:33 p.m. UTC | #2

Hi Bao

I think adding "struct utp_upiu_query_response_v4_0" is redundant and not correct for flags upiu response . 
You can use "struct utp_upiu_query_v4_0" 

Regards
Arthur

> -----Original Message-----
> From: Bao D. Nguyen <[e-mail]>
> Sent: Friday, March 21, 2025 5:18 AM
> To: [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]; [e-mail]
> Cc: [e-mail]; Bao D. Nguyen <[e-mail]>;
> Alim Akhtar <[e-mail]>; James E.J. Bottomley
> <[e-mail]>; Matthias Brugger
> <[e-mail]>; AngeloGioacchino Del Regno
> <[e-mail]>; Bean Huo
> <[e-mail]>; Keoseong Park <[e-mail]>;
> Ziqi Chen <[e-mail]>; Al Viro <[e-mail]>;
> Gwendal Grignou <[e-mail]>; Eric Biggers
> <[e-mail]>; open list <[e-mail]>; moderated
> list:ARM/Mediatek SoC support:Keyword:mediatek <linux-arm-
> [e-mail]>; moderated list:ARM/Mediatek SoC
> support:Keyword:mediatek <[e-mail]>
> Subject: [PATCH v4 1/1] scsi: ufs: core: add device level exception support
> 
> The ufs device JEDEC specification version 4.1 adds support for the device level
> exception events. To support this new device level exception feature, expose
> two new sysfs nodes below to provide the user space access to the device level
> exception information.
> /sys/bus/platform/drivers/ufshcd/*/device_lvl_exception_count
> /sys/bus/platform/drivers/ufshcd/*/device_lvl_exception_id
> 
> The device_lvl_exception_count sysfs node reports the number of device level
> exceptions that have occurred since the last time this variable is reset. Writing
> a value of 0 will reset it.
> The device_lvl_exception_id reports the exception ID which is the
> qDeviceLevelExceptionID attribute of the device JEDEC specifications version
> 4.1 and later. The user space application can query these sysfs nodes to get
> more information about the device level exception.
> 
> Signed-off-by: Bao D. Nguyen <[e-mail]>
> Reviewed-by: Peter Wang <[e-mail]>
> ---
> Changes in v4:
> 1. Changed the hba->dev_lvl_exception_count to atomic_t type. Removed the
> spinlock() around hba->dev_lvl_exception_count accesses (Peter's and Bart's
> comments).
> 
> Changes in v3:
> 1. Add protection for hba->dev_lvl_exception_count accesses in different
> contexts (Bart's comment).
> 
> Changes in v2:
> 1. Addressed Mani's comments:
>    - Update the documentation of dev_lvl_exception_count to read/write.
>    - Rephrase the description of the Documentation and commit text.
>    - Remove the export of ufshcd_read_device_lvl_exception_id().
> 2. Addressed Bart's comments:
>    - Rename dev_lvl_exception sysfs node to dev_lvl_exception_count.
>    - Update the documentation of the sysfs nodes.
>    - Skip comment about sysfs_notify() being used in interrupt
>      context because Avri already addressed it.
> ---
>  Documentation/ABI/testing/sysfs-driver-ufs | 27 ++++++++++++++
>  drivers/ufs/core/ufs-sysfs.c               | 54 +++++++++++++++++++++++++++
>  drivers/ufs/core/ufshcd-priv.h             |  1 +
>  drivers/ufs/core/ufshcd.c                  | 60
> ++++++++++++++++++++++++++++++
>  include/uapi/scsi/scsi_bsg_ufs.h           |  9 +++++
>  include/ufs/ufs.h                          |  5 ++-
>  include/ufs/ufshcd.h                       |  5 +++
>  7 files changed, 160 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/ABI/testing/sysfs-driver-ufs
> b/Documentation/ABI/testing/sysfs-driver-ufs
> index ae01912..6a6c35a 100644
> --- a/Documentation/ABI/testing/sysfs-driver-ufs
> +++ b/Documentation/ABI/testing/sysfs-driver-ufs
> @@ -1604,3 +1604,30 @@ Description:
>  		prevent the UFS from frequently performing clock
> gating/ungating.
> 
>  		The attribute is read/write.
> +
> +What:
> 	/sys/bus/platform/drivers/ufshcd/*/device_lvl_exception_count
> +What:		/sys/bus/platform/devices/*.ufs/device_lvl_exception_count
> +Date:		March 2025
> +Contact:	Bao D. Nguyen <[e-mail]>
> +Description:
> +		This attribute is applicable to ufs devices compliant to the
> JEDEC
> +		specifications version 4.1 or later. The
> device_lvl_exception_count
> +		is a counter indicating the number of times the device level
> exceptions
> +		have occurred since the last time this variable is reset.
> +		Writing a 0 value to this attribute will reset the
> device_lvl_exception_count.
> +		If the device_lvl_exception_count reads a positive value, the
> user
> +		application should read the device_lvl_exception_id attribute
> to know more
> +		information about the exception.
> +		This attribute is read/write.
> +
> +What:		/sys/bus/platform/drivers/ufshcd/*/device_lvl_exception_id
> +What:		/sys/bus/platform/devices/*.ufs/device_lvl_exception_id
> +Date:		March 2025
> +Contact:	Bao D. Nguyen <[e-mail]>
> +Description:
> +		Reading the device_lvl_exception_id returns the
> qDeviceLevelExceptionID
> +		attribute of the ufs device JEDEC specification version 4.1. The
> definition
> +		of the qDeviceLevelExceptionID is the ufs device vendor
> specific implementation.
> +		Refer to the device manufacturer datasheet for more
> information
> +		on the meaning of the qDeviceLevelExceptionID attribute
> value.
> +		The attribute is read only.
> diff --git a/drivers/ufs/core/ufs-sysfs.c b/drivers/ufs/core/ufs-sysfs.c index
> 90b5ab6..634cf16 100644
> --- a/drivers/ufs/core/ufs-sysfs.c
> +++ b/drivers/ufs/core/ufs-sysfs.c
> @@ -466,6 +466,56 @@ static ssize_t critical_health_show(struct device
> *dev,
>  	return sysfs_emit(buf, "%d\n", hba->critical_health_count);  }
> 
> +static ssize_t device_lvl_exception_count_show(struct device *dev,
> +					       struct device_attribute *attr,
> +					       char *buf)
> +{
> +	struct ufs_hba *hba = dev_get_drvdata(dev);
> +
> +	if (hba->dev_info.wspecversion < 0x410)
> +		return -EOPNOTSUPP;
> +
> +	return sysfs_emit(buf, "%u\n",
> +atomic_read(&hba->dev_lvl_exception_count));
> +}
> +
> +static ssize_t device_lvl_exception_count_store(struct device *dev,
> +						struct device_attribute *attr,
> +						const char *buf, size_t count)
> +{
> +	struct ufs_hba *hba = dev_get_drvdata(dev);
> +	unsigned int value;
> +
> +	if (kstrtouint(buf, 0, &value))
> +		return -EINVAL;
> +
> +	/* the only supported usecase is to reset the dev_lvl_exception_count
> */
> +	if (value)
> +		return -EINVAL;
> +
> +	atomic_set(&hba->dev_lvl_exception_count, 0);
> +
> +	return count;
> +}
> +
> +static ssize_t device_lvl_exception_id_show(struct device *dev,
> +					    struct device_attribute *attr,
> +					    char *buf)
> +{
> +	struct ufs_hba *hba = dev_get_drvdata(dev);
> +	u64 exception_id;
> +	int err;
> +
> +	ufshcd_rpm_get_sync(hba);
> +	err = ufshcd_read_device_lvl_exception_id(hba, &exception_id);
> +	ufshcd_rpm_put_sync(hba);
> +
> +	if (err)
> +		return err;
> +
> +	hba->dev_lvl_exception_id = exception_id;
> +	return sysfs_emit(buf, "%llu\n", exception_id); }
> +
>  static DEVICE_ATTR_RW(rpm_lvl);
>  static DEVICE_ATTR_RO(rpm_target_dev_state);
>  static DEVICE_ATTR_RO(rpm_target_link_state);
> @@ -479,6 +529,8 @@ static DEVICE_ATTR_RW(wb_flush_threshold);
>  static DEVICE_ATTR_RW(rtc_update_ms);
>  static DEVICE_ATTR_RW(pm_qos_enable);
>  static DEVICE_ATTR_RO(critical_health);
> +static DEVICE_ATTR_RW(device_lvl_exception_count);
> +static DEVICE_ATTR_RO(device_lvl_exception_id);
> 
>  static struct attribute *ufs_sysfs_ufshcd_attrs[] = {
>  	&dev_attr_rpm_lvl.attr,
> @@ -494,6 +546,8 @@ static struct attribute *ufs_sysfs_ufshcd_attrs[] = {
>  	&dev_attr_rtc_update_ms.attr,
>  	&dev_attr_pm_qos_enable.attr,
>  	&dev_attr_critical_health.attr,
> +	&dev_attr_device_lvl_exception_count.attr,
> +	&dev_attr_device_lvl_exception_id.attr,
>  	NULL
>  };
> 
> diff --git a/drivers/ufs/core/ufshcd-priv.h b/drivers/ufs/core/ufshcd-priv.h
> index 10b4a19..d0a2c96 100644
> --- a/drivers/ufs/core/ufshcd-priv.h
> +++ b/drivers/ufs/core/ufshcd-priv.h
> @@ -94,6 +94,7 @@ int ufshcd_exec_raw_upiu_cmd(struct ufs_hba *hba,
>  			     enum query_opcode desc_op);
> 
>  int ufshcd_wb_toggle(struct ufs_hba *hba, bool enable);
> +int ufshcd_read_device_lvl_exception_id(struct ufs_hba *hba, u64
> +*exception_id);
> 
>  /* Wrapper functions for safely calling variant operations */  static inline const
> char *ufshcd_get_var_name(struct ufs_hba *hba) diff --git
> a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c index
> 4e1e214..86ef716 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c
> @@ -6013,6 +6013,43 @@ static void
> ufshcd_bkops_exception_event_handler(struct ufs_hba *hba)
>  				__func__, err);
>  }
> 
> +int ufshcd_read_device_lvl_exception_id(struct ufs_hba *hba, u64
> +*exception_id) {
> +	struct utp_upiu_query_response_v4_0 *upiu_resp;
> +	struct ufs_query_req *request = NULL;
> +	struct ufs_query_res *response = NULL;
> +	int err;
> +
> +	if (hba->dev_info.wspecversion < 0x410)
> +		return -EOPNOTSUPP;
> +
> +	ufshcd_hold(hba);
> +	mutex_lock(&hba->dev_cmd.lock);
> +
> +	ufshcd_init_query(hba, &request, &response,
> +			  UPIU_QUERY_OPCODE_READ_ATTR,
> +			  QUERY_ATTR_IDN_DEV_LVL_EXCEPTION_ID, 0, 0);
> +
> +	request->query_func =
> UPIU_QUERY_FUNC_STANDARD_READ_REQUEST;
> +
> +	err = ufshcd_exec_dev_cmd(hba, DEV_CMD_TYPE_QUERY,
> QUERY_REQ_TIMEOUT);
> +
> +	if (err) {
> +		dev_err(hba->dev, "%s: failed to read device level exception
> %d\n",
> +			__func__, err);
> +		goto out;
> +	}
> +
> +	upiu_resp = (struct utp_upiu_query_response_v4_0 *)response;
> +	*exception_id = get_unaligned_be64(&upiu_resp->value);
> +
> +out:
> +	mutex_unlock(&hba->dev_cmd.lock);
> +	ufshcd_release(hba);
> +
> +	return err;
> +}
> +
>  static int __ufshcd_wb_toggle(struct ufs_hba *hba, bool set, enum flag_idn
> idn)  {
>  	u8 index;
> @@ -6240,6 +6277,11 @@ static void
> ufshcd_exception_event_handler(struct work_struct *work)
>  		sysfs_notify(&hba->dev->kobj, NULL, "critical_health");
>  	}
> 
> +	if (status & hba->ee_drv_mask & MASK_EE_DEV_LVL_EXCEPTION) {
> +		atomic_inc(&hba->dev_lvl_exception_count);
> +		sysfs_notify(&hba->dev->kobj, NULL,
> "device_lvl_exception_count");
> +	}
> +
>  	ufs_debugfs_exception_event(hba, status);  }
> 
> @@ -8139,6 +8181,22 @@ static void ufshcd_temp_notif_probe(struct
> ufs_hba *hba, const u8 *desc_buf)
>  	}
>  }
> 
> +static void ufshcd_device_lvl_exception_probe(struct ufs_hba *hba, u8
> +*desc_buf) {
> +	u32 ext_ufs_feature;
> +
> +	if (hba->dev_info.wspecversion < 0x410)
> +		return;
> +
> +	ext_ufs_feature = get_unaligned_be32(desc_buf +
> +
> 	DEVICE_DESC_PARAM_EXT_UFS_FEATURE_SUP);
> +	if (!(ext_ufs_feature & UFS_DEV_LVL_EXCEPTION_SUP))
> +		return;
> +
> +	atomic_set(&hba->dev_lvl_exception_count, 0);
> +	ufshcd_enable_ee(hba, MASK_EE_DEV_LVL_EXCEPTION); }
> +
>  static void ufshcd_set_rtt(struct ufs_hba *hba)  {
>  	struct ufs_dev_info *dev_info = &hba->dev_info; @@ -8339,6
> +8397,8 @@ static int ufs_get_device_desc(struct ufs_hba *hba)
> 
>  	ufs_init_rtc(hba, desc_buf);
> 
> +	ufshcd_device_lvl_exception_probe(hba, desc_buf);
> +
>  	/*
>  	 * ufshcd_read_string_desc returns size of the string
>  	 * reset the error value
> diff --git a/include/uapi/scsi/scsi_bsg_ufs.h b/include/uapi/scsi/scsi_bsg_ufs.h
> index 8c29e49..8b61dff 100644
> --- a/include/uapi/scsi/scsi_bsg_ufs.h
> +++ b/include/uapi/scsi/scsi_bsg_ufs.h
> @@ -143,6 +143,15 @@ struct utp_upiu_query_v4_0 {
>  	__be32 reserved;
>  };
> 
> +struct utp_upiu_query_response_v4_0 {
> +	__u8 opcode;
> +	__u8 idn;
> +	__u8 index;
> +	__u8 selector;
> +	__be64 value;
> +	__be32 reserved;
> +} __attribute__((__packed__));
> +
>  /**
>   * struct utp_upiu_cmd - Command UPIU structure
>   * @exp_data_transfer_len: Data Transfer Length DW-3 diff --git
> a/include/ufs/ufs.h b/include/ufs/ufs.h index 8a24ed5..1c47136 100644
> --- a/include/ufs/ufs.h
> +++ b/include/ufs/ufs.h
> @@ -180,7 +180,8 @@ enum attr_idn {
>  	QUERY_ATTR_IDN_AVAIL_WB_BUFF_SIZE       = 0x1D,
>  	QUERY_ATTR_IDN_WB_BUFF_LIFE_TIME_EST    = 0x1E,
>  	QUERY_ATTR_IDN_CURR_WB_BUFF_SIZE        = 0x1F,
> -	QUERY_ATTR_IDN_TIMESTAMP		= 0x30
> +	QUERY_ATTR_IDN_TIMESTAMP		= 0x30,
> +	QUERY_ATTR_IDN_DEV_LVL_EXCEPTION_ID     = 0x34,
>  };
> 
>  /* Descriptor idn for Query requests */ @@ -390,6 +391,7 @@ enum {
>  	UFS_DEV_EXT_TEMP_NOTIF		= BIT(6),
>  	UFS_DEV_HPB_SUPPORT		= BIT(7),
>  	UFS_DEV_WRITE_BOOSTER_SUP	= BIT(8),
> +	UFS_DEV_LVL_EXCEPTION_SUP       = BIT(12),
>  };
>  #define UFS_DEV_HPB_SUPPORT_VERSION		0x310
> 
> @@ -419,6 +421,7 @@ enum {
>  	MASK_EE_TOO_LOW_TEMP		= BIT(4),
>  	MASK_EE_WRITEBOOSTER_EVENT	= BIT(5),
>  	MASK_EE_PERFORMANCE_THROTTLING	= BIT(6),
> +	MASK_EE_DEV_LVL_EXCEPTION       = BIT(7),
>  	MASK_EE_HEALTH_CRITICAL		= BIT(9),
>  };
>  #define MASK_EE_URGENT_TEMP (MASK_EE_TOO_HIGH_TEMP |
> MASK_EE_TOO_LOW_TEMP) diff --git a/include/ufs/ufshcd.h
> b/include/ufs/ufshcd.h index e3909cc..5888463 100644
> --- a/include/ufs/ufshcd.h
> +++ b/include/ufs/ufshcd.h
> @@ -968,6 +968,9 @@ enum ufshcd_mcq_opr {
>   * @pm_qos_req: PM QoS request handle
>   * @pm_qos_enabled: flag to check if pm qos is enabled
>   * @critical_health_count: count of critical health exceptions
> + * @dev_lvl_exception_count: count of device level exceptions since
> + last reset
> + * @dev_lvl_exception_id: vendor specific information about the
> + * device level exception event.
>   */
>  struct ufs_hba {
>  	void __iomem *mmio_base;
> @@ -1138,6 +1141,8 @@ struct ufs_hba {
>  	bool pm_qos_enabled;
> 
>  	int critical_health_count;
> +	atomic_t dev_lvl_exception_count;
> +	u64 dev_lvl_exception_id;
>  };
> 
>  /**
> --
> 2.7.4
>

Bao D. Nguyen March 25, 2025, 10:15 p.m. UTC | #3

On 3/25/2025 9:33 AM, Arthur Simchaev wrote:
> Hi Bao
> 
> I think adding "struct utp_upiu_query_response_v4_0" is redundant and not correct for flags upiu response .
> You can use "struct utp_upiu_query_v4_0"
> 
Hi Arthur,
This is not a flags attribute. This is for a Query Read 64-bit Attribute 
data. In the existing code, we do not have a read 64-bit attribute, so 
adding this new code would also allow future re-use.

The new "struct utp_upiu_query_response_v4_0" would improve readability 
because it is formatted exactly as how the jedec standard defines for 
Attribute Read. We won't need to use type cast to get the 64-bit value.
There would be no issue with efficiency because the same machine code 
would be generated.

The existing "struct utp_upiu_query_v4_0" probably has a bug in it. It 
does not use the  __attribute__((__packed__)) attribute. The compiler is 
free to add padding in this structure, resulting in the read attribute 
value being incorrect. I plan to provide a separate patch to fix this issue.

Thanks, Bao

Arthur Simchaev March 26, 2025, 7:30 a.m. UTC | #4

> Hi Arthur,
> This is not a flags attribute. This is for a Query Read 64-bit Attribute data. In
> the existing code, we do not have a read 64-bit attribute, so adding this new
> code would also allow future re-use.
> 
> The new "struct utp_upiu_query_response_v4_0" would improve readability
> because it is formatted exactly as how the jedec standard defines for Attribute
> Read. We won't need to use type cast to get the 64-bit value.
> There would be no issue with efficiency because the same machine code
> would be generated.
> 
> The existing "struct utp_upiu_query_v4_0" probably has a bug in it. It does
> not use the  __attribute__((__packed__)) attribute. The compiler is free to add
> padding in this structure, resulting in the read attribute value being incorrect. I
> plan to provide a separate patch to fix this issue.

Hi Bao 

Upiu_query can be used for all device management command (descriptions, attributes, flags)
See section 10.7.9 UPIU QUERY RESPONSE in the UFS 4.1 specification.
If "struct utp_upiu_query" was properly defined, according to the UFS specification  (by OSF's),
we would not need to add additional "struct utp_upiu_query_v4_0" structures.
If you think the structure should be packaged, you can fix "struct utp_upiu_query" and 
"struct utp_upiu_query_v4_0".

Regards
Arthur

> -----Original Message-----
> From: Bao D. Nguyen <[e-mail]>
> Sent: Wednesday, March 26, 2025 12:16 AM
> To: Arthur Simchaev <[e-mail]>;
> [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]; [e-mail]
> Cc: [e-mail]; Alim Akhtar <[e-mail]>;
> James E.J. Bottomley <[e-mail]>; Matthias
> Brugger <[e-mail]>; AngeloGioacchino Del Regno
> <[e-mail]>; Bean Huo
> <[e-mail]>; Keoseong Park <[e-mail]>;
> Ziqi Chen <[e-mail]>; Al Viro <[e-mail]>;
> Gwendal Grignou <[e-mail]>; Eric Biggers
> <[e-mail]>; open list <[e-mail]>; moderated
> list:ARM/Mediatek SoC support:Keyword:mediatek <linux-arm-
> [e-mail]>; moderated list:ARM/Mediatek SoC
> support:Keyword:mediatek <[e-mail]>
> Subject: Re: [PATCH v4 1/1] scsi: ufs: core: add device level exception support
> 
> On 3/25/2025 9:33 AM, Arthur Simchaev wrote:
> > Hi Bao
> >
> > I think adding "struct utp_upiu_query_response_v4_0" is redundant and not
> correct for flags upiu response .
> > You can use "struct utp_upiu_query_v4_0"
> >
> Hi Arthur,
> This is not a flags attribute. This is for a Query Read 64-bit Attribute data. In
> the existing code, we do not have a read 64-bit attribute, so adding this new
> code would also allow future re-use.
> 
> The new "struct utp_upiu_query_response_v4_0" would improve readability
> because it is formatted exactly as how the jedec standard defines for Attribute
> Read. We won't need to use type cast to get the 64-bit value.
> There would be no issue with efficiency because the same machine code
> would be generated.
> 
> The existing "struct utp_upiu_query_v4_0" probably has a bug in it. It does
> not use the  __attribute__((__packed__)) attribute. The compiler is free to add
> padding in this structure, resulting in the read attribute value being incorrect. I
> plan to provide a separate patch to fix this issue.
> 
> Thanks, Bao

Bart Van Assche March 26, 2025, 10:49 a.m. UTC | #5

On 3/25/25 6:15 PM, Bao D. Nguyen wrote:
> The existing "struct utp_upiu_query_v4_0" probably has a bug in it. It 
> does not use the  __attribute__((__packed__)) attribute. The compiler is 
> free to add padding in this structure, resulting in the read attribute 
> value being incorrect. I plan to provide a separate patch to fix this 
> issue.

Adding __attribute__((__packed__)) or __packed to data structures that
don't need it is not an improvement but is a change that makes
processing slower on architectures that do not support unaligned
accesses. Instead of adding __packed to data structures in their
entirety, only add it to those members that need it and check the
structure size as follows:

static_assert(sizeof(...) == ...);

Thanks,

Bart.

Bao D. Nguyen March 26, 2025, 11:28 p.m. UTC | #6

On 3/26/2025 12:30 AM, Arthur Simchaev wrote:
>> Hi Arthur,
>> This is not a flags attribute. This is for a Query Read 64-bit Attribute data. In
>> the existing code, we do not have a read 64-bit attribute, so adding this new
>> code would also allow future re-use.
>>
>> The new "struct utp_upiu_query_response_v4_0" would improve readability
>> because it is formatted exactly as how the jedec standard defines for Attribute
>> Read. We won't need to use type cast to get the 64-bit value.
>> There would be no issue with efficiency because the same machine code
>> would be generated.
>>
>> The existing "struct utp_upiu_query_v4_0" probably has a bug in it. It does
>> not use the  __attribute__((__packed__)) attribute. The compiler is free to add
>> padding in this structure, resulting in the read attribute value being incorrect. I
>> plan to provide a separate patch to fix this issue.
> 
> Hi Bao
> 
> Upiu_query can be used for all device management command (descriptions, attributes, flags)
> See section 10.7.9 UPIU QUERY RESPONSE in the UFS 4.1 specification.
> If "struct utp_upiu_query" was properly defined, according to the UFS specification  (by OSF's),
> we would not need to add additional "struct utp_upiu_query_v4_0" structures.
Section 10.7.9 of the device spec v4.0 and v4.1 define the QUERY 
RESPONSE mostly as OSFs (Opcode Specific Field). If the driver used OSFs 
as defined by section 10.7.9, it would not be very readable. Instead, 
the driver tries its best to map these OSFs to meaningful names 
depending on the type of the QUERY command. In my opinion, there is a 
good reason to introduce "struct utp_upiu_query_v4_0". For example, in 
the spec v4.0, Write Attribute only supports up to 32-bit attribute 
size. However, v4.1 supports up to  64-bit attribute size. The "struct 
utp_upiu_query_v4_0" renamed the "__be16 reserved_osf" to "__u8 osf3" 
and "__u8 osf4" which would be better for code readability.

> If you think the structure should be packaged, you can fix "struct utp_upiu_query" and
> "struct utp_upiu_query_v4_0".
That's a fair request. I would like to fix it in a separate patch if 
that works.

Thanks, Bao

Bao D. Nguyen March 26, 2025, 11:47 p.m. UTC | #7

On 3/26/2025 3:49 AM, Bart Van Assche wrote:
> On 3/25/25 6:15 PM, Bao D. Nguyen wrote:
>> The existing "struct utp_upiu_query_v4_0" probably has a bug in it. It 
>> does not use the  __attribute__((__packed__)) attribute. The compiler 
>> is free to add padding in this structure, resulting in the read 
>> attribute value being incorrect. I plan to provide a separate patch to 
>> fix this issue.
> 
> Adding __attribute__((__packed__)) or __packed to data structures that
> don't need it is not an improvement but is a change that makes
> processing slower on architectures that do not support unaligned
> accesses. Instead of adding __packed to data structures in their
> entirety, only add it to those members that need it and check the
> structure size as follows:
> 
> static_assert(sizeof(...) == ...);
> 
Thank you for the info on this, Bart.
IMO, this response upiu data should be __packed because the data coming 
from the hardware follows a strict format as defined by the spec. If we 
support __pack each individual field which data may be read by the 
driver (the attribute read commands) and check the validity of their 
sizes, it may add some complexity?

Thanks, Bao

Bart Van Assche March 27, 2025, 11:36 a.m. UTC | #8

On 3/26/25 7:47 PM, Bao D. Nguyen wrote:
> On 3/26/2025 3:49 AM, Bart Van Assche wrote:
>> On 3/25/25 6:15 PM, Bao D. Nguyen wrote:
>>> The existing "struct utp_upiu_query_v4_0" probably has a bug in it. 
>>> It does not use the  __attribute__((__packed__)) attribute. The 
>>> compiler is free to add padding in this structure, resulting in the 
>>> read attribute value being incorrect. I plan to provide a separate 
>>> patch to fix this issue.
>>
>> Adding __attribute__((__packed__)) or __packed to data structures that
>> don't need it is not an improvement but is a change that makes
>> processing slower on architectures that do not support unaligned
>> accesses. Instead of adding __packed to data structures in their
>> entirety, only add it to those members that need it and check the
>> structure size as follows:
>>
>> static_assert(sizeof(...) == ...);
>>
> Thank you for the info on this, Bart.
> IMO, this response upiu data should be __packed because the data coming 
> from the hardware follows a strict format as defined by the spec. If we 
> support __pack each individual field which data may be read by the 
> driver (the attribute read commands) and check the validity of their 
> sizes, it may add some complexity?

Hi Bao,

As explained in my previous email, adding __packed to data structures in
their entirety is a bad practice. Please don't do this.

Regarding your question: I have not yet seen any data structure that
represents an on-the-wire data format where every single data member
has to be annotated with __packed. Only data members that are not
aligned to a natural boundary need this annotation. Examples are
available in this header file: include/scsi/srp.h.

Thanks,

Bart.

Bao D. Nguyen March 27, 2025, 9:45 p.m. UTC | #9

On 3/27/2025 4:36 AM, Bart Van Assche wrote:
> On 3/26/25 7:47 PM, Bao D. Nguyen wrote:
>> On 3/26/2025 3:49 AM, Bart Van Assche wrote:
>>> On 3/25/25 6:15 PM, Bao D. Nguyen wrote:
>>>> The existing "struct utp_upiu_query_v4_0" probably has a bug in it. 
>>>> It does not use the  __attribute__((__packed__)) attribute. The 
>>>> compiler is free to add padding in this structure, resulting in the 
>>>> read attribute value being incorrect. I plan to provide a separate 
>>>> patch to fix this issue.
>>>
>>> Adding __attribute__((__packed__)) or __packed to data structures that
>>> don't need it is not an improvement but is a change that makes
>>> processing slower on architectures that do not support unaligned
>>> accesses. Instead of adding __packed to data structures in their
>>> entirety, only add it to those members that need it and check the
>>> structure size as follows:
>>>
>>> static_assert(sizeof(...) == ...);
>>>
>> Thank you for the info on this, Bart.
>> IMO, this response upiu data should be __packed because the data 
>> coming from the hardware follows a strict format as defined by the 
>> spec. If we support __pack each individual field which data may be 
>> read by the driver (the attribute read commands) and check the 
>> validity of their sizes, it may add some complexity?
> 
> Hi Bao,
> 
> As explained in my previous email, adding __packed to data structures in
> their entirety is a bad practice. Please don't do this.
> 
> Regarding your question: I have not yet seen any data structure that
> represents an on-the-wire data format where every single data member
> has to be annotated with __packed. Only data members that are not
> aligned to a natural boundary need this annotation. Examples are
> available in this header file: include/scsi/srp.h.
> 

Thanks Bart. How about we change the current utp_upiu_query_v4_0 to

struct utp_upiu_query_v4_0 {
         __u8 opcode;
         __u8 idn;
         __u8 index;
         __u8 selector;
         __u8 cmd_specifics[8];
         /* private: */
         __be32 reserved;
};

Depending on the opcode/transaction, the cmd_specifics[] can be type 
casted to access the LENGTH, FLAG_VALUE, VALUE[0:63] fields of the QUERY 
UPIU. The __u8 array[8] would also prevent the compiler padding to the data.

Thanks, Bao

Bart Van Assche March 28, 2025, 2:02 p.m. UTC | #10

On 3/27/25 5:45 PM, Bao D. Nguyen wrote:
> Thanks Bart. How about we change the current utp_upiu_query_v4_0 to
> 
> struct utp_upiu_query_v4_0 {
>          __u8 opcode;
>          __u8 idn;
>          __u8 index;
>          __u8 selector;
>          __u8 cmd_specifics[8];
>          /* private: */
>          __be32 reserved;
> };
> 
> Depending on the opcode/transaction, the cmd_specifics[] can be type 
> casted to access the LENGTH, FLAG_VALUE, VALUE[0:63] fields of the QUERY 
> UPIU. The __u8 array[8] would also prevent the compiler padding to the 
> data.

Are there any user space applications that use the osf3/4/5/6/7 member
names? Has it been considered to preserve these by introducing a union?

Thanks,

Bart.

Patch

diff --git a/Documentation/ABI/testing/sysfs-driver-ufs b/Documentation/ABI/testing/sysfs-driver-ufs
index ae01912..6a6c35a 100644
--- a/Documentation/ABI/testing/sysfs-driver-ufs
+++ b/Documentation/ABI/testing/sysfs-driver-ufs
@@ -1604,3 +1604,30 @@  Description:
 		prevent the UFS from frequently performing clock gating/ungating.
 
 		The attribute is read/write.
+
+What:		/sys/bus/platform/drivers/ufshcd/*/device_lvl_exception_count
+What:		/sys/bus/platform/devices/*.ufs/device_lvl_exception_count
+Date:		March 2025
+Contact:	Bao D. Nguyen <[e-mail]>
+Description:
+		This attribute is applicable to ufs devices compliant to the JEDEC
+		specifications version 4.1 or later. The device_lvl_exception_count
+		is a counter indicating the number of times the device level exceptions
+		have occurred since the last time this variable is reset.
+		Writing a 0 value to this attribute will reset the device_lvl_exception_count.
+		If the device_lvl_exception_count reads a positive value, the user
+		application should read the device_lvl_exception_id attribute to know more
+		information about the exception.
+		This attribute is read/write.
+
+What:		/sys/bus/platform/drivers/ufshcd/*/device_lvl_exception_id
+What:		/sys/bus/platform/devices/*.ufs/device_lvl_exception_id
+Date:		March 2025
+Contact:	Bao D. Nguyen <[e-mail]>
+Description:
+		Reading the device_lvl_exception_id returns the qDeviceLevelExceptionID
+		attribute of the ufs device JEDEC specification version 4.1. The definition
+		of the qDeviceLevelExceptionID is the ufs device vendor specific implementation.
+		Refer to the device manufacturer datasheet for more information
+		on the meaning of the qDeviceLevelExceptionID attribute value.
+		The attribute is read only.
diff --git a/drivers/ufs/core/ufs-sysfs.c b/drivers/ufs/core/ufs-sysfs.c
index 90b5ab6..634cf16 100644
--- a/drivers/ufs/core/ufs-sysfs.c
+++ b/drivers/ufs/core/ufs-sysfs.c
@@ -466,6 +466,56 @@  static ssize_t critical_health_show(struct device *dev,
 	return sysfs_emit(buf, "%d\n", hba->critical_health_count);
 }
 
+static ssize_t device_lvl_exception_count_show(struct device *dev,
+					       struct device_attribute *attr,
+					       char *buf)
+{
+	struct ufs_hba *hba = dev_get_drvdata(dev);
+
+	if (hba->dev_info.wspecversion < 0x410)
+		return -EOPNOTSUPP;
+
+	return sysfs_emit(buf, "%u\n", atomic_read(&hba->dev_lvl_exception_count));
+}
+
+static ssize_t device_lvl_exception_count_store(struct device *dev,
+						struct device_attribute *attr,
+						const char *buf, size_t count)
+{
+	struct ufs_hba *hba = dev_get_drvdata(dev);
+	unsigned int value;
+
+	if (kstrtouint(buf, 0, &value))
+		return -EINVAL;
+
+	/* the only supported usecase is to reset the dev_lvl_exception_count */
+	if (value)
+		return -EINVAL;
+
+	atomic_set(&hba->dev_lvl_exception_count, 0);
+
+	return count;
+}
+
+static ssize_t device_lvl_exception_id_show(struct device *dev,
+					    struct device_attribute *attr,
+					    char *buf)
+{
+	struct ufs_hba *hba = dev_get_drvdata(dev);
+	u64 exception_id;
+	int err;
+
+	ufshcd_rpm_get_sync(hba);
+	err = ufshcd_read_device_lvl_exception_id(hba, &exception_id);
+	ufshcd_rpm_put_sync(hba);
+
+	if (err)
+		return err;
+
+	hba->dev_lvl_exception_id = exception_id;
+	return sysfs_emit(buf, "%llu\n", exception_id);
+}
+
 static DEVICE_ATTR_RW(rpm_lvl);
 static DEVICE_ATTR_RO(rpm_target_dev_state);
 static DEVICE_ATTR_RO(rpm_target_link_state);
@@ -479,6 +529,8 @@  static DEVICE_ATTR_RW(wb_flush_threshold);
 static DEVICE_ATTR_RW(rtc_update_ms);
 static DEVICE_ATTR_RW(pm_qos_enable);
 static DEVICE_ATTR_RO(critical_health);
+static DEVICE_ATTR_RW(device_lvl_exception_count);
+static DEVICE_ATTR_RO(device_lvl_exception_id);
 
 static struct attribute *ufs_sysfs_ufshcd_attrs[] = {
 	&dev_attr_rpm_lvl.attr,
@@ -494,6 +546,8 @@  static struct attribute *ufs_sysfs_ufshcd_attrs[] = {
 	&dev_attr_rtc_update_ms.attr,
 	&dev_attr_pm_qos_enable.attr,
 	&dev_attr_critical_health.attr,
+	&dev_attr_device_lvl_exception_count.attr,
+	&dev_attr_device_lvl_exception_id.attr,
 	NULL
 };
 
diff --git a/drivers/ufs/core/ufshcd-priv.h b/drivers/ufs/core/ufshcd-priv.h
index 10b4a19..d0a2c96 100644
--- a/drivers/ufs/core/ufshcd-priv.h
+++ b/drivers/ufs/core/ufshcd-priv.h
@@ -94,6 +94,7 @@  int ufshcd_exec_raw_upiu_cmd(struct ufs_hba *hba,
 			     enum query_opcode desc_op);
 
 int ufshcd_wb_toggle(struct ufs_hba *hba, bool enable);
+int ufshcd_read_device_lvl_exception_id(struct ufs_hba *hba, u64 *exception_id);
 
 /* Wrapper functions for safely calling variant operations */
 static inline const char *ufshcd_get_var_name(struct ufs_hba *hba)
diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index 4e1e214..86ef716 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -6013,6 +6013,43 @@  static void ufshcd_bkops_exception_event_handler(struct ufs_hba *hba)
 				__func__, err);
 }
 
+int ufshcd_read_device_lvl_exception_id(struct ufs_hba *hba, u64 *exception_id)
+{
+	struct utp_upiu_query_response_v4_0 *upiu_resp;
+	struct ufs_query_req *request = NULL;
+	struct ufs_query_res *response = NULL;
+	int err;
+
+	if (hba->dev_info.wspecversion < 0x410)
+		return -EOPNOTSUPP;
+
+	ufshcd_hold(hba);
+	mutex_lock(&hba->dev_cmd.lock);
+
+	ufshcd_init_query(hba, &request, &response,
+			  UPIU_QUERY_OPCODE_READ_ATTR,
+			  QUERY_ATTR_IDN_DEV_LVL_EXCEPTION_ID, 0, 0);
+
+	request->query_func = UPIU_QUERY_FUNC_STANDARD_READ_REQUEST;
+
+	err = ufshcd_exec_dev_cmd(hba, DEV_CMD_TYPE_QUERY, QUERY_REQ_TIMEOUT);
+
+	if (err) {
+		dev_err(hba->dev, "%s: failed to read device level exception %d\n",
+			__func__, err);
+		goto out;
+	}
+
+	upiu_resp = (struct utp_upiu_query_response_v4_0 *)response;
+	*exception_id = get_unaligned_be64(&upiu_resp->value);
+
+out:
+	mutex_unlock(&hba->dev_cmd.lock);
+	ufshcd_release(hba);
+
+	return err;
+}
+
 static int __ufshcd_wb_toggle(struct ufs_hba *hba, bool set, enum flag_idn idn)
 {
 	u8 index;
@@ -6240,6 +6277,11 @@  static void ufshcd_exception_event_handler(struct work_struct *work)
 		sysfs_notify(&hba->dev->kobj, NULL, "critical_health");
 	}
 
+	if (status & hba->ee_drv_mask & MASK_EE_DEV_LVL_EXCEPTION) {
+		atomic_inc(&hba->dev_lvl_exception_count);
+		sysfs_notify(&hba->dev->kobj, NULL, "device_lvl_exception_count");
+	}
+
 	ufs_debugfs_exception_event(hba, status);
 }
 
@@ -8139,6 +8181,22 @@  static void ufshcd_temp_notif_probe(struct ufs_hba *hba, const u8 *desc_buf)
 	}
 }
 
+static void ufshcd_device_lvl_exception_probe(struct ufs_hba *hba, u8 *desc_buf)
+{
+	u32 ext_ufs_feature;
+
+	if (hba->dev_info.wspecversion < 0x410)
+		return;
+
+	ext_ufs_feature = get_unaligned_be32(desc_buf +
+				DEVICE_DESC_PARAM_EXT_UFS_FEATURE_SUP);
+	if (!(ext_ufs_feature & UFS_DEV_LVL_EXCEPTION_SUP))
+		return;
+
+	atomic_set(&hba->dev_lvl_exception_count, 0);
+	ufshcd_enable_ee(hba, MASK_EE_DEV_LVL_EXCEPTION);
+}
+
 static void ufshcd_set_rtt(struct ufs_hba *hba)
 {
 	struct ufs_dev_info *dev_info = &hba->dev_info;
@@ -8339,6 +8397,8 @@  static int ufs_get_device_desc(struct ufs_hba *hba)
 
 	ufs_init_rtc(hba, desc_buf);
 
+	ufshcd_device_lvl_exception_probe(hba, desc_buf);
+
 	/*
 	 * ufshcd_read_string_desc returns size of the string
 	 * reset the error value
diff --git a/include/uapi/scsi/scsi_bsg_ufs.h b/include/uapi/scsi/scsi_bsg_ufs.h
index 8c29e49..8b61dff 100644
--- a/include/uapi/scsi/scsi_bsg_ufs.h
+++ b/include/uapi/scsi/scsi_bsg_ufs.h
@@ -143,6 +143,15 @@  struct utp_upiu_query_v4_0 {
 	__be32 reserved;
 };
 
+struct utp_upiu_query_response_v4_0 {
+	__u8 opcode;
+	__u8 idn;
+	__u8 index;
+	__u8 selector;
+	__be64 value;
+	__be32 reserved;
+} __attribute__((__packed__));
+
 /**
  * struct utp_upiu_cmd - Command UPIU structure
  * @exp_data_transfer_len: Data Transfer Length DW-3
diff --git a/include/ufs/ufs.h b/include/ufs/ufs.h
index 8a24ed5..1c47136 100644
--- a/include/ufs/ufs.h
+++ b/include/ufs/ufs.h
@@ -180,7 +180,8 @@  enum attr_idn {
 	QUERY_ATTR_IDN_AVAIL_WB_BUFF_SIZE       = 0x1D,
 	QUERY_ATTR_IDN_WB_BUFF_LIFE_TIME_EST    = 0x1E,
 	QUERY_ATTR_IDN_CURR_WB_BUFF_SIZE        = 0x1F,
-	QUERY_ATTR_IDN_TIMESTAMP		= 0x30
+	QUERY_ATTR_IDN_TIMESTAMP		= 0x30,
+	QUERY_ATTR_IDN_DEV_LVL_EXCEPTION_ID     = 0x34,
 };
 
 /* Descriptor idn for Query requests */
@@ -390,6 +391,7 @@  enum {
 	UFS_DEV_EXT_TEMP_NOTIF		= BIT(6),
 	UFS_DEV_HPB_SUPPORT		= BIT(7),
 	UFS_DEV_WRITE_BOOSTER_SUP	= BIT(8),
+	UFS_DEV_LVL_EXCEPTION_SUP       = BIT(12),
 };
 #define UFS_DEV_HPB_SUPPORT_VERSION		0x310
 
@@ -419,6 +421,7 @@  enum {
 	MASK_EE_TOO_LOW_TEMP		= BIT(4),
 	MASK_EE_WRITEBOOSTER_EVENT	= BIT(5),
 	MASK_EE_PERFORMANCE_THROTTLING	= BIT(6),
+	MASK_EE_DEV_LVL_EXCEPTION       = BIT(7),
 	MASK_EE_HEALTH_CRITICAL		= BIT(9),
 };
 #define MASK_EE_URGENT_TEMP (MASK_EE_TOO_HIGH_TEMP | MASK_EE_TOO_LOW_TEMP)
diff --git a/include/ufs/ufshcd.h b/include/ufs/ufshcd.h
index e3909cc..5888463 100644
--- a/include/ufs/ufshcd.h
+++ b/include/ufs/ufshcd.h
@@ -968,6 +968,9 @@  enum ufshcd_mcq_opr {
  * @pm_qos_req: PM QoS request handle
  * @pm_qos_enabled: flag to check if pm qos is enabled
  * @critical_health_count: count of critical health exceptions
+ * @dev_lvl_exception_count: count of device level exceptions since last reset
+ * @dev_lvl_exception_id: vendor specific information about the
+ * device level exception event.
  */
 struct ufs_hba {
 	void __iomem *mmio_base;
@@ -1138,6 +1141,8 @@  struct ufs_hba {
 	bool pm_qos_enabled;
 
 	int critical_health_count;
+	atomic_t dev_lvl_exception_count;
+	u64 dev_lvl_exception_id;
 };
 
 /**