Re: [PATCH 5/5] scsi: ufs: Add a sysfs attribute for triggering the UFS EH

[Adrian Hunter <adrian.hunter@intel.com>]

On 13/10/2021 00:54, Bart Van Assche wrote:
> Make it possible to test the impact of the UFS error handler on software
> that submits SCSI commands to the UFS driver.

Are you sure this isn't better suited to debugfs?

> 
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
>  Documentation/ABI/testing/sysfs-driver-ufs | 10 ++++++
>  drivers/scsi/ufs/ufshcd.c                  | 37 ++++++++++++++++++++++
>  2 files changed, 47 insertions(+)
> 
> diff --git a/Documentation/ABI/testing/sysfs-driver-ufs b/Documentation/ABI/testing/sysfs-driver-ufs
> index ec3a7149ced5..2a46f91d3f1b 100644
> --- a/Documentation/ABI/testing/sysfs-driver-ufs
> +++ b/Documentation/ABI/testing/sysfs-driver-ufs
> @@ -1534,3 +1534,13 @@ Contact:	Avri Altman <avri.altman@wdc.com>
>  Description:	In host control mode the host is the originator of map requests.
>  		To avoid flooding the device with map requests, use a simple throttling
>  		mechanism that limits the number of inflight map requests.
> +
> +What:		/sys/class/scsi_host/*/trigger_eh
> +Date:		October 2021
> +Contact:	Bart Van Assche <bvanassche@acm.org>
> +Description:	Writing into this sysfs attribute triggers the UFS error
> +		handler. This is useful for testing how the UFS error handler
> +		affects SCSI command processing. The supported values are as
> +		follows: "1" triggers the error handler without resetting the
> +		host controller and "2" starts the error handler and makes it
> +		reset the host interface.
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index ecfe1f124f8a..30ff93979840 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -8144,6 +8144,42 @@ static void ufshcd_async_scan(void *data, async_cookie_t cookie)
>  	}
>  }
>  
> +static ssize_t trigger_eh_store(struct device *dev,
> +				struct device_attribute *attr,
> +				const char *buf, size_t count)
> +{
> +	struct Scsi_Host *host = class_to_shost(dev);
> +	struct ufs_hba *hba = shost_priv(host);
> +
> +	/*
> +	 * Using locking would be a better solution. However, this is a debug
> +	 * attribute so ufshcd_eh_in_progress() should be good enough.
> +	 */
> +	if (ufshcd_eh_in_progress(hba))
> +		return -EBUSY;

Does it matter if ufshcd_eh_in_progress()?

> +
> +	if (sysfs_streq(buf, "1")) {
> +		hba->ufshcd_state = UFSHCD_STATE_EH_SCHEDULED_NON_FATAL;

Shouldn't overwrite UFSHCD_STATE_ERROR

> +		hba->saved_err |= UIC_ERROR;

ufshcd_err_handler() still behaves differently depending on
hba->saved_uic_err

> +	} else if (sysfs_streq(buf, "2")) {
> +		hba->ufshcd_state = UFSHCD_STATE_EH_SCHEDULED_FATAL;
> +		hba->saved_err |= UIC_ERROR;

In addition, a fatal error must be set to get fatal error behaviour from
ufshcd_err_handler.

> +	} else {
> +		return -EINVAL;
> +	}
> +
> +	scsi_schedule_eh(hba->host);

Probably should be:

	queue_work(hba->eh_wq, &hba->eh_work);

However, it might be simpler to replace everything with:

	spin_lock(hba->host->host_lock);
	hba->saved_err |= <something>;
	hba->saved_uic_err |= <something else>;
	ufshcd_schedule_eh_work(hba);
	spin_unlock(hba->host->host_lock);

Perhaps letting the user specify values to determine <something>
and <something else>

> +
> +	return count;
> +}
> +
> +static DEVICE_ATTR_WO(trigger_eh);
> +
> +static struct device_attribute *ufshcd_shost_attrs[] = {
> +	&dev_attr_trigger_eh,
> +	NULL
> +};
> +
>  static const struct attribute_group *ufshcd_driver_groups[] = {
>  	&ufs_sysfs_unit_descriptor_group,
>  	&ufs_sysfs_lun_attributes_group,
> @@ -8183,6 +8219,7 @@ static struct scsi_host_template ufshcd_driver_template = {
>  	.max_segment_size	= PRDT_DATA_BYTE_COUNT_MAX,
>  	.max_host_blocked	= 1,
>  	.track_queue_depth	= 1,
> +	.shost_attrs		= ufshcd_shost_attrs,
>  	.sdev_groups		= ufshcd_driver_groups,
>  	.dma_boundary		= PAGE_SIZE - 1,
>  	.rpm_autosuspend_delay	= RPM_AUTOSUSPEND_DELAY_MS,
>