On 8/14/2023 9:23 AM, Bart Van Assche wrote:
On 8/12/23 10:09, Bao D. Nguyen wrote:
I am not reviewing other patches in this series, so I don't know the
whole context. Here is my comment on this patch alone.
Looks like you rely on ufshcd_auto_hibern8_update() being invoked so
that you can update the driver_preserves_write_order. However, the
hba->ahit value can be updated by the vendor's driver, and
ufshcd_auto_hibern8_enable() can be invoked without
ufshcd_auto_hibern8_update(). Therefore, you may have a situation
where the driver_preserves_write_order is true by default, but
Auto-hibernation is enabled by default.
Hi Bao,
Other than setting a default value for auto-hibernation, vendor drivers
must not modify the auto-hibernation settings.
IMO, it is not a good solution to prohibit changing auto-hibernation
settings during runtime. I can think of a few situations where changing
this parameter may help the system such as:
- Reduce heat. The device may be hot, so hibernate often helps.
- Battery is low, so hibernate quicker to save battery.
- Allows the vendor to make decision whether to trade performance for
power, or vice versa. Sometimes, the system can afford trading
performance for power saving.
How about this?
- Make ufshcd_auto_hibern8_enable() a static function. It should not be
called from the vendor drivers.
- Mandate that vendor drivers must only update auto-hibernation settings
via the ufshcd_auto_hibern8_update() api. This function is already
exported, so only need to update the function comment to make it clear
(updating the hba->ahit alone may result in abnormal behavior).
Thanks,
Bao
ufshcd_auto_hibern8_enable() calls from outside
ufshcd_auto_hibern8_update() are used to reapply auto-hibernation
settings and not to modify auto-hibernation settings.
So I think that integrating the following change on top of this patch is
sufficient:
@@ -5172,7 +5172,9 @@ static int ufshcd_slave_configure(struct
scsi_device *sdev)
ufshcd_hpb_configure(hba, sdev);
- q->limits.driver_preserves_write_order = true;
+ q->limits.driver_preserves_write_order =
+ !ufshcd_is_auto_hibern8_supported(hba) ||
+ FIELD_GET(UFSHCI_AHIBERN8_TIMER_MASK, hba->ahit) == 0;
Yes, this should help.
blk_queue_update_dma_pad(q, PRDT_DATA_BYTE_COUNT_PAD - 1);
if (hba->quirks & UFSHCD_QUIRK_4KB_DMA_ALIGNMENT)
blk_queue_update_dma_alignment(q, SZ_4K - 1);
Thanks,
Bart.
