linux-kernel - Re: [PATCH v1 1/1] scsi: ufs: core: add device level exception support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <41678800-470f-4ea2-802c-f9f4d21817f6@acm.org>
Date: Thu, 13 Mar 2025 11:14:37 -0700
From: Bart Van Assche <bvanassche@....org>
To: "Bao D. Nguyen" <quic_nguyenb@...cinc.com>, quic_cang@...cinc.com,
 quic_nitirawa@...cinc.com, avri.altman@....com, peter.wang@...iatek.com,
 manivannan.sadhasivam@...aro.org, minwoo.im@...sung.com,
 adrian.hunter@...el.com, martin.petersen@...cle.com
Cc: linux-scsi@...r.kernel.org, Alim Akhtar <alim.akhtar@...sung.com>,
 "James E.J. Bottomley" <James.Bottomley@...senPartnership.com>,
 Bean Huo <beanhuo@...ron.com>, Ziqi Chen <quic_ziqichen@...cinc.com>,
 Keoseong Park <keosung.park@...sung.com>,
 Gwendal Grignou <gwendal@...omium.org>, Al Viro <viro@...iv.linux.org.uk>,
 Eric Biggers <ebiggers@...gle.com>, open list <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v1 1/1] scsi: ufs: core: add device level exception
 support

On 3/6/25 9:31 AM, Bao D. Nguyen wrote:
> +What:		/sys/bus/platform/drivers/ufshcd/*/device_lvl_exception
> +What:		/sys/bus/platform/devices/*.ufs/device_lvl_exception
> +Date:		March 2025
> +Contact:	Bao D. Nguyen <quic_nguyenb@...cinc.com>
> +Description:
> +		The device_lvl_exception is a counter indicating the number
> +		of times the device level exceptions have occurred since the
> +		last time this variable is reset. Read the device_lvl_exception_id
> +		sysfs node to know more information about the exception.
> +		The file is read only.

Shouldn't this sysfs attribute have a "_count" suffix to make it clear
that it represents a count?

Additionally, here and below, please change "file" into "attribute".

> +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@...cinc.com>
> +Description:
> +		Reading the device_lvl_exception_id returns the device JEDEC
> +		standard's qDeviceLevelExceptionID attribute. The definition of the
> +		qDeviceLevelExceptionID is the ufs device vendor specific design.
> +		Refer to the device manufacturer datasheet for more information
> +		on the meaning of the qDeviceLevelExceptionID attribute value.
> +		The file is read only.

I'm not sure it is useful to export vendor-specific information to
sysfs.

> diff --git a/drivers/ufs/core/ufs-sysfs.c b/drivers/ufs/core/ufs-sysfs.c
> index 90b5ab6..0248288a 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_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", hba->dev_lvl_exception_count);
> +}

The preferred approach for sysfs attributes that are not supported is to 
make these invisible rather than returning an error code.

> +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);
> +}

Just like device_lvl_exception, this attribute shouldn't be visible if
device level exceptions are not supported.

> +	if (status & hba->ee_drv_mask & MASK_EE_DEV_LVL_EXCEPTION) {
> +		hba->dev_lvl_exception_count++;
> +		sysfs_notify(&hba->dev->kobj, NULL, "device_lvl_exception");
> +	}

sysfs_notify() may sleep and hence must not be called from an interrupt
handler.

Thanks,

Bart.