>
> 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?
Was just thinking the very same thing.
Thanks,
Avri
>
> >
> > 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,
> >
