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

From: Arthur Simchaev
Date: Tue Mar 25 2025 - 12:39:21 EST

Next message: Lukasz Majewski: "Re: [PATCH 5/5] net: mtip: The L2 switch driver for imx287"
Previous message: Ian Rogers: "[PATCH v2 31/35] perf vendor events: Update snowridgex events"
Next in thread: Bao D. Nguyen: "Re: [PATCH v4 1/1] scsi: ufs: core: add device level exception support"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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 <quic_nguyenb@xxxxxxxxxxx>
> Sent: Friday, March 21, 2025 5:18 AM
> To: quic_cang@xxxxxxxxxxx; quic_nitirawa@xxxxxxxxxxx;
> bvanassche@xxxxxxx; avri.altman@xxxxxxx; peter.wang@xxxxxxxxxxxx;
> manivannan.sadhasivam@xxxxxxxxxx; minwoo.im@xxxxxxxxxxx;
> adrian.hunter@xxxxxxxxx; martin.petersen@xxxxxxxxxx
> Cc: linux-scsi@xxxxxxxxxxxxxxx; Bao D. Nguyen <quic_nguyenb@xxxxxxxxxxx>;
> Alim Akhtar <alim.akhtar@xxxxxxxxxxx>; James E.J. Bottomley
> <James.Bottomley@xxxxxxxxxxxxxxxxxxxxx>; Matthias Brugger
> <matthias.bgg@xxxxxxxxx>; AngeloGioacchino Del Regno
> <angelogioacchino.delregno@xxxxxxxxxxxxx>; Bean Huo
> <beanhuo@xxxxxxxxxx>; Keoseong Park <keosung.park@xxxxxxxxxxx>;
> Ziqi Chen <quic_ziqichen@xxxxxxxxxxx>; Al Viro <viro@xxxxxxxxxxxxxxxxxx>;
> Gwendal Grignou <gwendal@xxxxxxxxxxxx>; Eric Biggers
> <ebiggers@xxxxxxxxxx>; open list <linux-kernel@xxxxxxxxxxxxxxx>; moderated
> list:ARM/Mediatek SoC support:Keyword:mediatek <linux-arm-
> kernel@xxxxxxxxxxxxxxxxxxx>; moderated list:ARM/Mediatek SoC
> support:Keyword:mediatek <linux-mediatek@xxxxxxxxxxxxxxxxxxx>
> 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 <quic_nguyenb@xxxxxxxxxxx>
> Reviewed-by: Peter Wang <peter.wang@xxxxxxxxxxxx>
> ---
> 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 <quic_nguyenb@xxxxxxxxxxx>
> +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 <quic_nguyenb@xxxxxxxxxxx>
> +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
>

Next message: Lukasz Majewski: "Re: [PATCH 5/5] net: mtip: The L2 switch driver for imx287"
Previous message: Ian Rogers: "[PATCH v2 31/35] perf vendor events: Update snowridgex events"
Next in thread: Bao D. Nguyen: "Re: [PATCH v4 1/1] scsi: ufs: core: add device level exception support"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]