| Message ID | 20230816195447.3703954-17-bvanassche@acm.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Improve performance for zoned UFS devices | expand |
On 8/16/2023 12:53 PM, Bart Van Assche wrote: > UFSHCI 3.0 controllers do not preserve the write order if auto-hibernation > is enabled. If the write order is not preserved, an I/O scheduler is > required to serialize zoned writes. Hence do not allow auto-hibernation > to be enabled without I/O scheduler if a zoned logical unit is present > and if the controller is operating in legacy mode. This patch has been > tested with the following shell script: > > show_ah8() { > echo -n "auto_hibern8: " > adb shell "cat /sys/devices/platform/13200000.ufs/auto_hibern8" > } > > set_ah8() { > local rc > adb shell "echo $1 > /sys/devices/platform/13200000.ufs/auto_hibern8" > rc=$? > show_ah8 > return $rc > } > > set_iosched() { > adb shell "echo $1 >/sys/class/block/$zoned_bdev/queue/scheduler && > echo -n 'I/O scheduler: ' && > cat /sys/class/block/sde/queue/scheduler" > } > > adb root > zoned_bdev=$(adb shell grep -lvw 0 /sys/class/block/sd*/queue/chunk_sectors |& > sed 's|/sys/class/block/||g;s|/queue/chunk_sectors||g') > [ -n "$zoned_bdev" ] > show_ah8 > set_ah8 0 > set_iosched none > if set_ah8 150000; then > echo "Error: enabled AH8 without I/O scheduler" > fi > set_iosched mq-deadline > set_ah8 150000 > > Cc: Martin K. Petersen <martin.petersen@oracle.com> > Cc: Can Guo <quic_cang@quicinc.com> > Cc: Avri Altman <avri.altman@wdc.com> > Cc: Christoph Hellwig <hch@lst.de> > Cc: Damien Le Moal <dlemoal@kernel.org> > Cc: Ming Lei <ming.lei@redhat.com> > Signed-off-by: Bart Van Assche <bvanassche@acm.org> > --- > drivers/ufs/core/ufs-sysfs.c | 2 +- > drivers/ufs/core/ufshcd-priv.h | 1 - > drivers/ufs/core/ufshcd.c | 60 ++++++++++++++++++++++++++++++++-- > include/ufs/ufshcd.h | 2 +- > 4 files changed, 60 insertions(+), 5 deletions(-) > > diff --git a/drivers/ufs/core/ufs-sysfs.c b/drivers/ufs/core/ufs-sysfs.c > index 6c72075750dd..a693dea1bd18 100644 > --- a/drivers/ufs/core/ufs-sysfs.c > +++ b/drivers/ufs/core/ufs-sysfs.c > @@ -203,7 +203,7 @@ static ssize_t auto_hibern8_store(struct device *dev, > goto out; > } > > - ufshcd_auto_hibern8_update(hba, ufshcd_us_to_ahit(timer)); > + ret = ufshcd_auto_hibern8_update(hba, ufshcd_us_to_ahit(timer)); > > out: > up(&hba->host_sem); > diff --git a/drivers/ufs/core/ufshcd-priv.h b/drivers/ufs/core/ufshcd-priv.h > index 0f3bd943b58b..a2b74fbc2056 100644 > --- a/drivers/ufs/core/ufshcd-priv.h > +++ b/drivers/ufs/core/ufshcd-priv.h > @@ -60,7 +60,6 @@ int ufshcd_query_attr(struct ufs_hba *hba, enum query_opcode opcode, > enum attr_idn idn, u8 index, u8 selector, u32 *attr_val); > int ufshcd_query_flag(struct ufs_hba *hba, enum query_opcode opcode, > enum flag_idn idn, u8 index, bool *flag_res); > -void ufshcd_auto_hibern8_update(struct ufs_hba *hba, u32 ahit); > void ufshcd_compl_one_cqe(struct ufs_hba *hba, int task_tag, > struct cq_entry *cqe); > int ufshcd_mcq_init(struct ufs_hba *hba); > diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c > index 39000c018d8b..37d430d20939 100644 > --- a/drivers/ufs/core/ufshcd.c > +++ b/drivers/ufs/core/ufshcd.c > @@ -4337,6 +4337,29 @@ int ufshcd_uic_hibern8_exit(struct ufs_hba *hba) > } > EXPORT_SYMBOL_GPL(ufshcd_uic_hibern8_exit); > > +static int ufshcd_update_preserves_write_order(struct ufs_hba *hba, > + bool preserves_write_order) > +{ > + struct scsi_device *sdev; > + > + if (!preserves_write_order) { > + shost_for_each_device(sdev, hba->host) { > + struct request_queue *q = sdev->request_queue; > + > + /* > + * This code does not check whether the attached I/O > + * scheduler serializes zoned writes > + * (ELEVATOR_F_ZBD_SEQ_WRITE) because this cannot be > + * checked from outside the block layer core. > + */ > + if (blk_queue_is_zoned(q) && !q->elevator) > + return -EPERM; > + } > + } > + > + return 0; > +} > + > static void ufshcd_configure_auto_hibern8(struct ufs_hba *hba) > { > if (!ufshcd_is_auto_hibern8_supported(hba)) > @@ -4345,13 +4368,37 @@ static void ufshcd_configure_auto_hibern8(struct ufs_hba *hba) > ufshcd_writel(hba, hba->ahit, REG_AUTO_HIBERNATE_IDLE_TIMER); > } > > -void ufshcd_auto_hibern8_update(struct ufs_hba *hba, u32 ahit) > +/** > + * ufshcd_auto_hibern8_update() - Modify the auto-hibernation control register > + * @hba: per-adapter instance > + * @ahit: New auto-hibernate settings. Includes the scale and the value of the > + * auto-hibernation timer. See also the UFSHCI_AHIBERN8_TIMER_MASK and > + * UFSHCI_AHIBERN8_SCALE_MASK constants. > + * > + * Note: enabling auto-hibernation if a zoned logical unit is present without > + * attaching the mq-deadline scheduler first to the zoned logical unit may cause > + * unaligned write errors for the zoned logical unit. > + */ Please improve this "Note:". It seems like a run-on sentence and not very clear. > +int ufshcd_auto_hibern8_update(struct ufs_hba *hba, u32 ahit) > { > const u32 cur_ahit = READ_ONCE(hba->ahit); > + bool prev_state, new_state; > + int ret; > > if (!ufshcd_is_auto_hibern8_supported(hba) || cur_ahit == ahit) > - return; > + return 0; > > + prev_state = FIELD_GET(UFSHCI_AHIBERN8_TIMER_MASK, cur_ahit); > + new_state = FIELD_GET(UFSHCI_AHIBERN8_TIMER_MASK, ahit); > + > + if (!is_mcq_enabled(hba) && !prev_state && new_state) { > + /* > + * Auto-hibernation will be enabled for legacy UFSHCI mode. > + */ > + ret = ufshcd_update_preserves_write_order(hba, false); > + if (ret) > + return ret; > + } > WRITE_ONCE(hba->ahit, ahit); > if (!pm_runtime_suspended(&hba->ufs_device_wlun->sdev_gendev)) { > ufshcd_rpm_get_sync(hba); > @@ -4360,6 +4407,15 @@ void ufshcd_auto_hibern8_update(struct ufs_hba *hba, u32 ahit) > ufshcd_release(hba); > ufshcd_rpm_put_sync(hba); > } > + if (!is_mcq_enabled(hba) && prev_state && !new_state) { > + /* > + * Auto-hibernation has been disabled. > + */ > + ret = ufshcd_update_preserves_write_order(hba, true); > + WARN_ON_ONCE(ret); > + } > + > + return 0; > } > EXPORT_SYMBOL_GPL(ufshcd_auto_hibern8_update); > > diff --git a/include/ufs/ufshcd.h b/include/ufs/ufshcd.h > index 040d66d99912..7ae071a6811c 100644 > --- a/include/ufs/ufshcd.h > +++ b/include/ufs/ufshcd.h > @@ -1363,7 +1363,7 @@ static inline int ufshcd_disable_host_tx_lcc(struct ufs_hba *hba) > return ufshcd_dme_set(hba, UIC_ARG_MIB(PA_LOCAL_TX_LCC_ENABLE), 0); > } > > -void ufshcd_auto_hibern8_update(struct ufs_hba *hba, u32 ahit); > +int ufshcd_auto_hibern8_update(struct ufs_hba *hba, u32 ahit); > void ufshcd_fixup_dev_quirks(struct ufs_hba *hba, > const struct ufs_dev_quirk *fixups); > #define SD_ASCII_STD true
On 8/17/23 11:50, Bao D. Nguyen wrote: > On 8/16/2023 12:53 PM, Bart Van Assche wrote: >> +/** >> + * ufshcd_auto_hibern8_update() - Modify the auto-hibernation control register >> + * @hba: per-adapter instance >> + * @ahit: New auto-hibernate settings. Includes the scale and the value of the >> + * auto-hibernation timer. See also the UFSHCI_AHIBERN8_TIMER_MASK and >> + * UFSHCI_AHIBERN8_SCALE_MASK constants. >> + * >> + * Note: enabling auto-hibernation if a zoned logical unit is present without >> + * attaching the mq-deadline scheduler first to the zoned logical unit may cause >> + * unaligned write errors for the zoned logical unit. >> + */ > > Please improve this "Note:". It seems like a run-on sentence and not very clear. There are no grammatical errors in that note. Anyway, I will make the note more clear. Thanks, Bart.
diff --git a/drivers/ufs/core/ufs-sysfs.c b/drivers/ufs/core/ufs-sysfs.c index 6c72075750dd..a693dea1bd18 100644 --- a/drivers/ufs/core/ufs-sysfs.c +++ b/drivers/ufs/core/ufs-sysfs.c @@ -203,7 +203,7 @@ static ssize_t auto_hibern8_store(struct device *dev, goto out; } - ufshcd_auto_hibern8_update(hba, ufshcd_us_to_ahit(timer)); + ret = ufshcd_auto_hibern8_update(hba, ufshcd_us_to_ahit(timer)); out: up(&hba->host_sem); diff --git a/drivers/ufs/core/ufshcd-priv.h b/drivers/ufs/core/ufshcd-priv.h index 0f3bd943b58b..a2b74fbc2056 100644 --- a/drivers/ufs/core/ufshcd-priv.h +++ b/drivers/ufs/core/ufshcd-priv.h @@ -60,7 +60,6 @@ int ufshcd_query_attr(struct ufs_hba *hba, enum query_opcode opcode, enum attr_idn idn, u8 index, u8 selector, u32 *attr_val); int ufshcd_query_flag(struct ufs_hba *hba, enum query_opcode opcode, enum flag_idn idn, u8 index, bool *flag_res); -void ufshcd_auto_hibern8_update(struct ufs_hba *hba, u32 ahit); void ufshcd_compl_one_cqe(struct ufs_hba *hba, int task_tag, struct cq_entry *cqe); int ufshcd_mcq_init(struct ufs_hba *hba); diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c index 39000c018d8b..37d430d20939 100644 --- a/drivers/ufs/core/ufshcd.c +++ b/drivers/ufs/core/ufshcd.c @@ -4337,6 +4337,29 @@ int ufshcd_uic_hibern8_exit(struct ufs_hba *hba) } EXPORT_SYMBOL_GPL(ufshcd_uic_hibern8_exit); +static int ufshcd_update_preserves_write_order(struct ufs_hba *hba, + bool preserves_write_order) +{ + struct scsi_device *sdev; + + if (!preserves_write_order) { + shost_for_each_device(sdev, hba->host) { + struct request_queue *q = sdev->request_queue; + + /* + * This code does not check whether the attached I/O + * scheduler serializes zoned writes + * (ELEVATOR_F_ZBD_SEQ_WRITE) because this cannot be + * checked from outside the block layer core. + */ + if (blk_queue_is_zoned(q) && !q->elevator) + return -EPERM; + } + } + + return 0; +} + static void ufshcd_configure_auto_hibern8(struct ufs_hba *hba) { if (!ufshcd_is_auto_hibern8_supported(hba)) @@ -4345,13 +4368,37 @@ static void ufshcd_configure_auto_hibern8(struct ufs_hba *hba) ufshcd_writel(hba, hba->ahit, REG_AUTO_HIBERNATE_IDLE_TIMER); } -void ufshcd_auto_hibern8_update(struct ufs_hba *hba, u32 ahit) +/** + * ufshcd_auto_hibern8_update() - Modify the auto-hibernation control register + * @hba: per-adapter instance + * @ahit: New auto-hibernate settings. Includes the scale and the value of the + * auto-hibernation timer. See also the UFSHCI_AHIBERN8_TIMER_MASK and + * UFSHCI_AHIBERN8_SCALE_MASK constants. + * + * Note: enabling auto-hibernation if a zoned logical unit is present without + * attaching the mq-deadline scheduler first to the zoned logical unit may cause + * unaligned write errors for the zoned logical unit. + */ +int ufshcd_auto_hibern8_update(struct ufs_hba *hba, u32 ahit) { const u32 cur_ahit = READ_ONCE(hba->ahit); + bool prev_state, new_state; + int ret; if (!ufshcd_is_auto_hibern8_supported(hba) || cur_ahit == ahit) - return; + return 0; + prev_state = FIELD_GET(UFSHCI_AHIBERN8_TIMER_MASK, cur_ahit); + new_state = FIELD_GET(UFSHCI_AHIBERN8_TIMER_MASK, ahit); + + if (!is_mcq_enabled(hba) && !prev_state && new_state) { + /* + * Auto-hibernation will be enabled for legacy UFSHCI mode. + */ + ret = ufshcd_update_preserves_write_order(hba, false); + if (ret) + return ret; + } WRITE_ONCE(hba->ahit, ahit); if (!pm_runtime_suspended(&hba->ufs_device_wlun->sdev_gendev)) { ufshcd_rpm_get_sync(hba); @@ -4360,6 +4407,15 @@ void ufshcd_auto_hibern8_update(struct ufs_hba *hba, u32 ahit) ufshcd_release(hba); ufshcd_rpm_put_sync(hba); } + if (!is_mcq_enabled(hba) && prev_state && !new_state) { + /* + * Auto-hibernation has been disabled. + */ + ret = ufshcd_update_preserves_write_order(hba, true); + WARN_ON_ONCE(ret); + } + + return 0; } EXPORT_SYMBOL_GPL(ufshcd_auto_hibern8_update); diff --git a/include/ufs/ufshcd.h b/include/ufs/ufshcd.h index 040d66d99912..7ae071a6811c 100644 --- a/include/ufs/ufshcd.h +++ b/include/ufs/ufshcd.h @@ -1363,7 +1363,7 @@ static inline int ufshcd_disable_host_tx_lcc(struct ufs_hba *hba) return ufshcd_dme_set(hba, UIC_ARG_MIB(PA_LOCAL_TX_LCC_ENABLE), 0); } -void ufshcd_auto_hibern8_update(struct ufs_hba *hba, u32 ahit); +int ufshcd_auto_hibern8_update(struct ufs_hba *hba, u32 ahit); void ufshcd_fixup_dev_quirks(struct ufs_hba *hba, const struct ufs_dev_quirk *fixups); #define SD_ASCII_STD true
UFSHCI 3.0 controllers do not preserve the write order if auto-hibernation is enabled. If the write order is not preserved, an I/O scheduler is required to serialize zoned writes. Hence do not allow auto-hibernation to be enabled without I/O scheduler if a zoned logical unit is present and if the controller is operating in legacy mode. This patch has been tested with the following shell script: show_ah8() { echo -n "auto_hibern8: " adb shell "cat /sys/devices/platform/13200000.ufs/auto_hibern8" } set_ah8() { local rc adb shell "echo $1 > /sys/devices/platform/13200000.ufs/auto_hibern8" rc=$? show_ah8 return $rc } set_iosched() { adb shell "echo $1 >/sys/class/block/$zoned_bdev/queue/scheduler && echo -n 'I/O scheduler: ' && cat /sys/class/block/sde/queue/scheduler" } adb root zoned_bdev=$(adb shell grep -lvw 0 /sys/class/block/sd*/queue/chunk_sectors |& sed 's|/sys/class/block/||g;s|/queue/chunk_sectors||g') [ -n "$zoned_bdev" ] show_ah8 set_ah8 0 set_iosched none if set_ah8 150000; then echo "Error: enabled AH8 without I/O scheduler" fi set_iosched mq-deadline set_ah8 150000 Cc: Martin K. Petersen <martin.petersen@oracle.com> Cc: Can Guo <quic_cang@quicinc.com> Cc: Avri Altman <avri.altman@wdc.com> Cc: Christoph Hellwig <hch@lst.de> Cc: Damien Le Moal <dlemoal@kernel.org> Cc: Ming Lei <ming.lei@redhat.com> Signed-off-by: Bart Van Assche <bvanassche@acm.org> --- drivers/ufs/core/ufs-sysfs.c | 2 +- drivers/ufs/core/ufshcd-priv.h | 1 - drivers/ufs/core/ufshcd.c | 60 ++++++++++++++++++++++++++++++++-- include/ufs/ufshcd.h | 2 +- 4 files changed, 60 insertions(+), 5 deletions(-)