> > 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, > >