Re: [PATCH v8 9/9] scsi: ufs: Inform the block layer about write ordering

[Bart Van Assche <bvanassche@acm.org>]

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.