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@xxxxxxx>
> ---
> 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@xxxxxxx>
> 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@xxxxxxx>
> +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,
>
