diff mbox series

[v6,3/3] scsi: ufs: sysfs: Make max_number_of_rtt read-write

Message ID 20240526081636.2064-4-avri.altman@wdc.com
State New
Headers show
Series scsi: ufs: Allow RTT negotiation | expand

Commit Message

Avri Altman May 26, 2024, 8:16 a.m. UTC
Given the importance of the RTT parameter, we want to be able to
configure it via sysfs. This is because UFS users should be discouraged
from change UFS device parameters without the UFSHCI driver being aware
of these changes.

Signed-off-by: Avri Altman <avri.altman@wdc.com>
---
 Documentation/ABI/testing/sysfs-driver-ufs | 14 +++--
 drivers/ufs/core/ufs-sysfs.c               | 68 +++++++++++++++++++++-
 drivers/ufs/core/ufshcd-priv.h             | 24 ++++++++
 3 files changed, 99 insertions(+), 7 deletions(-)

Comments

Bart Van Assche May 29, 2024, 8:02 p.m. UTC | #1
On 5/26/24 01:16, Avri Altman wrote:
> +static inline void ufshcd_freez_hw_queues(struct ufs_hba *hba)
> +{
> +	struct scsi_device *sdev;
> +
> +	shost_for_each_device(sdev, hba->host) {
> +		if (sdev == hba->ufs_device_wlun)
> +			continue;
> +		blk_mq_freeze_queue(sdev->request_queue);
> +		blk_mq_quiesce_queue(sdev->request_queue);
> +	}
> +}
> +
> +static inline void ufshcd_unfreez_hw_queues(struct ufs_hba *hba)
> +{
> +	struct scsi_device *sdev;
> +
> +	shost_for_each_device(sdev, hba->host) {
> +		if (sdev == hba->ufs_device_wlun)
> +			continue;
> +		blk_mq_unquiesce_queue(sdev->request_queue);
> +		blk_mq_unfreeze_queue(sdev->request_queue);
> +	}
> +}

Why have these functions been declared inline? blk_mq_freeze_queue()
may sleep and hence performance is not an argument to inline these
functions. Additionally, the WLUN should not be skipped when freezing
or unfreezing request queues. The blk_mq_quiesce_queue() and
blk_mq_unquiesce_queue() calls are not necessary in the above code.
Please remove these.

Thanks,

Bart.
Avri Altman May 29, 2024, 9:12 p.m. UTC | #2
> On 5/26/24 01:16, Avri Altman wrote:
> > +static inline void ufshcd_freez_hw_queues(struct ufs_hba *hba) {
> > +     struct scsi_device *sdev;
> > +
> > +     shost_for_each_device(sdev, hba->host) {
> > +             if (sdev == hba->ufs_device_wlun)
> > +                     continue;
> > +             blk_mq_freeze_queue(sdev->request_queue);
> > +             blk_mq_quiesce_queue(sdev->request_queue);
> > +     }
> > +}
> > +
> > +static inline void ufshcd_unfreez_hw_queues(struct ufs_hba *hba) {
> > +     struct scsi_device *sdev;
> > +
> > +     shost_for_each_device(sdev, hba->host) {
> > +             if (sdev == hba->ufs_device_wlun)
> > +                     continue;
> > +             blk_mq_unquiesce_queue(sdev->request_queue);
> > +             blk_mq_unfreeze_queue(sdev->request_queue);
> > +     }
> > +}
> 
> Why have these functions been declared inline? blk_mq_freeze_queue() may
> sleep and hence performance is not an argument to inline these functions.
> Additionally, the WLUN should not be skipped when freezing or unfreezing
> request queues. The blk_mq_quiesce_queue() and
> blk_mq_unquiesce_queue() calls are not necessary in the above code.
> Please remove these.
OK.

Thanks,
Avri

> 
> Thanks,
> 
> Bart.
diff mbox series

Patch

diff --git a/Documentation/ABI/testing/sysfs-driver-ufs b/Documentation/ABI/testing/sysfs-driver-ufs
index 5bf7073b4f75..fe943ce76c60 100644
--- a/Documentation/ABI/testing/sysfs-driver-ufs
+++ b/Documentation/ABI/testing/sysfs-driver-ufs
@@ -920,14 +920,16 @@  Description:	This file shows whether the configuration descriptor is locked.
 
 What:		/sys/bus/platform/drivers/ufshcd/*/attributes/max_number_of_rtt
 What:		/sys/bus/platform/devices/*.ufs/attributes/max_number_of_rtt
-Date:		February 2018
-Contact:	Stanislav Nijnikov <stanislav.nijnikov@wdc.com>
+Date:		May 2024
+Contact:	Avri Altman <avri.altman@wdc.com>
 Description:	This file provides the maximum current number of
-		outstanding RTTs in device that is allowed. The full
-		information about the attribute could be found at
-		UFS specifications 2.1.
+		outstanding RTTs in device that is allowed. bMaxNumOfRTT is a
+		read-write persistent attribute and is equal to two after device
+		manufacturing. It shall not be set to a value greater than
+		bDeviceRTTCap value, and it may be set only when the hw queues are
+		empty.
 
-		The file is read only.
+		The file is read write.
 
 What:		/sys/bus/platform/drivers/ufshcd/*/attributes/exception_event_control
 What:		/sys/bus/platform/devices/*.ufs/attributes/exception_event_control
diff --git a/drivers/ufs/core/ufs-sysfs.c b/drivers/ufs/core/ufs-sysfs.c
index 3d049967f6bc..48ac708b8795 100644
--- a/drivers/ufs/core/ufs-sysfs.c
+++ b/drivers/ufs/core/ufs-sysfs.c
@@ -1340,6 +1340,73 @@  static const struct attribute_group ufs_sysfs_flags_group = {
 	.attrs = ufs_sysfs_device_flags,
 };
 
+static ssize_t max_number_of_rtt_show(struct device *dev,
+				      struct device_attribute *attr, char *buf)
+{
+	struct ufs_hba *hba = dev_get_drvdata(dev);
+	u32 rtt;
+	int ret;
+
+	down(&hba->host_sem);
+	if (!ufshcd_is_user_access_allowed(hba)) {
+		up(&hba->host_sem);
+		return -EBUSY;
+	}
+
+	ufshcd_rpm_get_sync(hba);
+	ret = ufshcd_query_attr(hba, UPIU_QUERY_OPCODE_READ_ATTR,
+		QUERY_ATTR_IDN_MAX_NUM_OF_RTT, 0, 0, &rtt);
+	ufshcd_rpm_put_sync(hba);
+
+	if (ret)
+		goto out;
+
+	ret = sysfs_emit(buf, "0x%08X\n", rtt);
+
+out:
+	up(&hba->host_sem);
+	return ret;
+}
+
+static ssize_t max_number_of_rtt_store(struct device *dev,
+				       struct device_attribute *attr,
+				       const char *buf, size_t count)
+{
+	struct ufs_hba *hba = dev_get_drvdata(dev);
+	struct ufs_dev_info *dev_info = &hba->dev_info;
+	unsigned int rtt;
+	int ret;
+
+	if (kstrtouint(buf, 0, &rtt))
+		return -EINVAL;
+
+	if (rtt > dev_info->rtt_cap) {
+		dev_err(dev, "rtt can be at most bDeviceRTTCap\n");
+		return -EINVAL;
+	}
+
+	down(&hba->host_sem);
+	if (!ufshcd_is_user_access_allowed(hba)) {
+		ret = -EBUSY;
+		goto out;
+	}
+
+	ufshcd_rpm_get_sync(hba);
+	ufshcd_freez_hw_queues(hba);
+
+	ret = ufshcd_query_attr(hba, UPIU_QUERY_OPCODE_WRITE_ATTR,
+		QUERY_ATTR_IDN_MAX_NUM_OF_RTT, 0, 0, &rtt);
+
+	ufshcd_unfreez_hw_queues(hba);
+	ufshcd_rpm_put_sync(hba);
+
+out:
+	up(&hba->host_sem);
+	return ret < 0 ? ret : count;
+}
+
+static DEVICE_ATTR_RW(max_number_of_rtt);
+
 static inline bool ufshcd_is_wb_attrs(enum attr_idn idn)
 {
 	return idn >= QUERY_ATTR_IDN_WB_FLUSH_STATUS &&
@@ -1387,7 +1454,6 @@  UFS_ATTRIBUTE(max_data_in_size, _MAX_DATA_IN);
 UFS_ATTRIBUTE(max_data_out_size, _MAX_DATA_OUT);
 UFS_ATTRIBUTE(reference_clock_frequency, _REF_CLK_FREQ);
 UFS_ATTRIBUTE(configuration_descriptor_lock, _CONF_DESC_LOCK);
-UFS_ATTRIBUTE(max_number_of_rtt, _MAX_NUM_OF_RTT);
 UFS_ATTRIBUTE(exception_event_control, _EE_CONTROL);
 UFS_ATTRIBUTE(exception_event_status, _EE_STATUS);
 UFS_ATTRIBUTE(ffu_status, _FFU_STATUS);
diff --git a/drivers/ufs/core/ufshcd-priv.h b/drivers/ufs/core/ufshcd-priv.h
index f42d99ce5bf1..2cdbe6b48d96 100644
--- a/drivers/ufs/core/ufshcd-priv.h
+++ b/drivers/ufs/core/ufshcd-priv.h
@@ -32,6 +32,30 @@  static inline bool ufshcd_is_wb_buf_flush_allowed(struct ufs_hba *hba)
 		!(hba->quirks & UFSHCI_QUIRK_SKIP_MANUAL_WB_FLUSH_CTRL);
 }
 
+static inline void ufshcd_freez_hw_queues(struct ufs_hba *hba)
+{
+	struct scsi_device *sdev;
+
+	shost_for_each_device(sdev, hba->host) {
+		if (sdev == hba->ufs_device_wlun)
+			continue;
+		blk_mq_freeze_queue(sdev->request_queue);
+		blk_mq_quiesce_queue(sdev->request_queue);
+	}
+}
+
+static inline void ufshcd_unfreez_hw_queues(struct ufs_hba *hba)
+{
+	struct scsi_device *sdev;
+
+	shost_for_each_device(sdev, hba->host) {
+		if (sdev == hba->ufs_device_wlun)
+			continue;
+		blk_mq_unquiesce_queue(sdev->request_queue);
+		blk_mq_unfreeze_queue(sdev->request_queue);
+	}
+}
+
 #ifdef CONFIG_SCSI_UFS_HWMON
 void ufs_hwmon_probe(struct ufs_hba *hba, u8 mask);
 void ufs_hwmon_remove(struct ufs_hba *hba);