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.