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

[Bart Van Assche <bvanassche@xxxxxxx>]

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@xxxxxxxxxxx>
> +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@xxxxxxxxxxx>
> +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.