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.
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;
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.
