[PATCH] scsi: ufs: add a quirk to disable FUA support

[PATCH] scsi: ufs: add a quirk to disable FUA support

[Jaegeuk Kim]

UFS stack shows very low performance of FUA comparing to write and cache_flush.
Let's add a quirk to adjust it.

E.g., average latency according to the chunk size of write

Write(us/KB)	4	64	256	1024	2048
FUA		873.792	754.604	995.624	1011.67	1067.99
CACHE_FLUSH	824.703	712.98	800.307	1019.5	1037.37

Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
---
 drivers/scsi/ufs/ufshcd.c | 3 +++
 drivers/scsi/ufs/ufshcd.h | 5 +++++
 2 files changed, 8 insertions(+)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 3f9caafa91bf..811f3467879c 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -5035,6 +5035,9 @@ static int ufshcd_slave_configure(struct scsi_device *sdev)
 	 */
 	sdev->silence_suspend = 1;
 
+	if (hba->quirks & UFSHCD_QUIRK_BROKEN_FUA)
+		sdev->broken_fua = 1;
+
 	ufshcd_crypto_register(hba, q);
 
 	return 0;
diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
index 94f545be183a..6c480c6741d6 100644
--- a/drivers/scsi/ufs/ufshcd.h
+++ b/drivers/scsi/ufs/ufshcd.h
@@ -602,6 +602,11 @@ enum ufshcd_quirks {
 	 * support physical host configuration.
 	 */
 	UFSHCD_QUIRK_SKIP_PH_CONFIGURATION		= 1 << 16,
+
+	/*
+	 * This quirk disables FUA support.
+	 */
+	UFSHCD_QUIRK_BROKEN_FUA				= 1 << 17,
 };
 
 enum ufshcd_caps {
-- 
2.36.1.255.ge46751e96f-goog

Re: [PATCH] scsi: ufs: add a quirk to disable FUA support

[Eric Biggers]

On Tue, May 31, 2022 at 01:10:53PM -0700, Jaegeuk Kim wrote:
> UFS stack shows very low performance of FUA comparing to write and cache_flush.
> Let's add a quirk to adjust it.
> 
> E.g., average latency according to the chunk size of write
> 
> Write(us/KB)	4	64	256	1024	2048
> FUA		873.792	754.604	995.624	1011.67	1067.99
> CACHE_FLUSH	824.703	712.98	800.307	1019.5	1037.37
> 
> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
> ---
>  drivers/scsi/ufs/ufshcd.c | 3 +++
>  drivers/scsi/ufs/ufshcd.h | 5 +++++
>  2 files changed, 8 insertions(+)
> 
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index 3f9caafa91bf..811f3467879c 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -5035,6 +5035,9 @@ static int ufshcd_slave_configure(struct scsi_device *sdev)
>  	 */
>  	sdev->silence_suspend = 1;
>  
> +	if (hba->quirks & UFSHCD_QUIRK_BROKEN_FUA)
> +		sdev->broken_fua = 1;
> +
>  	ufshcd_crypto_register(hba, q);
>  
>  	return 0;
> diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
> index 94f545be183a..6c480c6741d6 100644
> --- a/drivers/scsi/ufs/ufshcd.h
> +++ b/drivers/scsi/ufs/ufshcd.h
> @@ -602,6 +602,11 @@ enum ufshcd_quirks {
>  	 * support physical host configuration.
>  	 */
>  	UFSHCD_QUIRK_SKIP_PH_CONFIGURATION		= 1 << 16,
> +
> +	/*
> +	 * This quirk disables FUA support.
> +	 */
> +	UFSHCD_QUIRK_BROKEN_FUA				= 1 << 17,
>  };

"Broken" is ambiguous.  IIUC, the issue is that FUA performance is very bad, not
that it doesn't work.  Can you clarify the intent in the comment?

Also, this patch does nothing by itself.  Which UFS host driver(s) need this
quirk bit?  Can you update them to use it?  Or do they all need this, in which
case a quirk bit would be unnecessary?

- Eric

Re: [PATCH] scsi: ufs: add a quirk to disable FUA support

[Jaegeuk Kim]

On 05/31, Eric Biggers wrote:
> On Tue, May 31, 2022 at 01:10:53PM -0700, Jaegeuk Kim wrote:
> > UFS stack shows very low performance of FUA comparing to write and cache_flush.
> > Let's add a quirk to adjust it.
> > 
> > E.g., average latency according to the chunk size of write
> > 
> > Write(us/KB)	4	64	256	1024	2048
> > FUA		873.792	754.604	995.624	1011.67	1067.99
> > CACHE_FLUSH	824.703	712.98	800.307	1019.5	1037.37
> > 
> > Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
> > ---
> >  drivers/scsi/ufs/ufshcd.c | 3 +++
> >  drivers/scsi/ufs/ufshcd.h | 5 +++++
> >  2 files changed, 8 insertions(+)
> > 
> > diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> > index 3f9caafa91bf..811f3467879c 100644
> > --- a/drivers/scsi/ufs/ufshcd.c
> > +++ b/drivers/scsi/ufs/ufshcd.c
> > @@ -5035,6 +5035,9 @@ static int ufshcd_slave_configure(struct scsi_device *sdev)
> >  	 */
> >  	sdev->silence_suspend = 1;
> >  
> > +	if (hba->quirks & UFSHCD_QUIRK_BROKEN_FUA)
> > +		sdev->broken_fua = 1;
> > +
> >  	ufshcd_crypto_register(hba, q);
> >  
> >  	return 0;
> > diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
> > index 94f545be183a..6c480c6741d6 100644
> > --- a/drivers/scsi/ufs/ufshcd.h
> > +++ b/drivers/scsi/ufs/ufshcd.h
> > @@ -602,6 +602,11 @@ enum ufshcd_quirks {
> >  	 * support physical host configuration.
> >  	 */
> >  	UFSHCD_QUIRK_SKIP_PH_CONFIGURATION		= 1 << 16,
> > +
> > +	/*
> > +	 * This quirk disables FUA support.
> > +	 */
> > +	UFSHCD_QUIRK_BROKEN_FUA				= 1 << 17,
> >  };
> 
> "Broken" is ambiguous.  IIUC, the issue is that FUA performance is very bad, not
> that it doesn't work.  Can you clarify the intent in the comment?

My intent is FUA was supposed to be better than write+cache_flush.

> 
> Also, this patch does nothing by itself.  Which UFS host driver(s) need this
> quirk bit?  Can you update them to use it?  Or do they all need this, in which
> case a quirk bit would be unnecessary?

Likewise other quick bits, using this is up to SoC or UFS vendors. I
think that combination is up to OEMs who is building the product.

> 
> - Eric

Re: [PATCH] scsi: ufs: add a quirk to disable FUA support

[Eric Biggers]

On Tue, May 31, 2022 at 01:53:33PM -0700, Jaegeuk Kim wrote:
> > Also, this patch does nothing by itself.  Which UFS host driver(s) need this
> > quirk bit?  Can you update them to use it?  Or do they all need this, in which
> > case a quirk bit would be unnecessary?
> 
> Likewise other quick bits, using this is up to SoC or UFS vendors. I
> think that combination is up to OEMs who is building the product.

Of the UFS host drivers in the upstream kernel, which ones actually need this?

- Eric

Re: [PATCH] scsi: ufs: add a quirk to disable FUA support

[Asutosh Das]

On Tue, May 31 2022 at 14:25 -0700, Eric Biggers wrote:
>On Tue, May 31, 2022 at 01:53:33PM -0700, Jaegeuk Kim wrote:
>> > Also, this patch does nothing by itself.  Which UFS host driver(s) need this
>> > quirk bit?  Can you update them to use it?  Or do they all need this, in which
>> > case a quirk bit would be unnecessary?
>>
>> Likewise other quick bits, using this is up to SoC or UFS vendors. I
>> think that combination is up to OEMs who is building the product.
>
>Of the UFS host drivers in the upstream kernel, which ones actually need this?
>

Qualcomm UFSHC would need this.
>- Eric

Re: [PATCH] scsi: ufs: add a quirk to disable FUA support

[Adrian Hunter]

On 31/05/22 23:10, Jaegeuk Kim wrote:
> UFS stack shows very low performance of FUA comparing to write and cache_flush.
> Let's add a quirk to adjust it.
> 
> E.g., average latency according to the chunk size of write
> 
> Write(us/KB)	4	64	256	1024	2048
> FUA		873.792	754.604	995.624	1011.67	1067.99
> CACHE_FLUSH	824.703	712.98	800.307	1019.5	1037.37

Wouldn't it depend on how much data might be in the cache?
Do you have real-world use-cases where the difference is measurable?

> 
> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
> ---
>  drivers/scsi/ufs/ufshcd.c | 3 +++
>  drivers/scsi/ufs/ufshcd.h | 5 +++++
>  2 files changed, 8 insertions(+)
> 
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index 3f9caafa91bf..811f3467879c 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -5035,6 +5035,9 @@ static int ufshcd_slave_configure(struct scsi_device *sdev)
>  	 */
>  	sdev->silence_suspend = 1;
>  
> +	if (hba->quirks & UFSHCD_QUIRK_BROKEN_FUA)
> +		sdev->broken_fua = 1;
> +
>  	ufshcd_crypto_register(hba, q);
>  
>  	return 0;
> diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
> index 94f545be183a..6c480c6741d6 100644
> --- a/drivers/scsi/ufs/ufshcd.h
> +++ b/drivers/scsi/ufs/ufshcd.h
> @@ -602,6 +602,11 @@ enum ufshcd_quirks {
>  	 * support physical host configuration.
>  	 */
>  	UFSHCD_QUIRK_SKIP_PH_CONFIGURATION		= 1 << 16,
> +
> +	/*
> +	 * This quirk disables FUA support.
> +	 */
> +	UFSHCD_QUIRK_BROKEN_FUA				= 1 << 17,

Wouldn't it be more appropriate to make it a UFS_DEVICE_QUIRK_
since it presumably depends on the UFS device not the host controller?

Also, as already commented by others, there needs to be a user of
the quirk

>  };
>  
>  enum ufshcd_caps {

Re: [PATCH] scsi: ufs: add a quirk to disable FUA support

[Bart Van Assche]

On 5/31/22 22:24, Adrian Hunter wrote:
> On 31/05/22 23:10, Jaegeuk Kim wrote:
>> +	/*
>> +	 * This quirk disables FUA support.
>> +	 */
>> +	UFSHCD_QUIRK_BROKEN_FUA				= 1 << 17,
> 
> Wouldn't it be more appropriate to make it a UFS_DEVICE_QUIRK_
> since it presumably depends on the UFS device not the host controller?
> 
> Also, as already commented by others, there needs to be a user of
> the quirk

Another possibility is to use the generic SCSI blacklist mechanism. See 
also the scsi_static_device_list array. See also /proc/scsi/device_info. 
 From scsi_devinfo.c:

/*
  * proc_scsi_dev_info_write - allow additions to scsi_dev_info_list via
  * /proc.
  *
  * Description: Adds a black/white list entry for vendor and model with
  * an integer value of flag to the scsi device info list.
  * To use, echo "vendor:model:flag" > /proc/scsi/device_info
  */

Thanks,

Bart.

Re: [PATCH] scsi: ufs: add a quirk to disable FUA support

[Jaegeuk Kim]

On 06/01, Bart Van Assche wrote:
> On 5/31/22 22:24, Adrian Hunter wrote:
> > On 31/05/22 23:10, Jaegeuk Kim wrote:
> > > +	/*
> > > +	 * This quirk disables FUA support.
> > > +	 */
> > > +	UFSHCD_QUIRK_BROKEN_FUA				= 1 << 17,
> > 
> > Wouldn't it be more appropriate to make it a UFS_DEVICE_QUIRK_
> > since it presumably depends on the UFS device not the host controller?
> > 
> > Also, as already commented by others, there needs to be a user of
> > the quirk
> 
> Another possibility is to use the generic SCSI blacklist mechanism. See also
> the scsi_static_device_list array. See also /proc/scsi/device_info. From
> scsi_devinfo.c:
> 
> /*
>  * proc_scsi_dev_info_write - allow additions to scsi_dev_info_list via
>  * /proc.
>  *
>  * Description: Adds a black/white list entry for vendor and model with
>  * an integer value of flag to the scsi device info list.
>  * To use, echo "vendor:model:flag" > /proc/scsi/device_info
>  */

Good to know. Thank you, Bart.

> 
> Thanks,
> 
> Bart.

Re: [PATCH] scsi: ufs: add a quirk to disable FUA support

[Jaegeuk Kim]

On 06/01, Adrian Hunter wrote:
> On 31/05/22 23:10, Jaegeuk Kim wrote:
> > UFS stack shows very low performance of FUA comparing to write and cache_flush.
> > Let's add a quirk to adjust it.
> > 
> > E.g., average latency according to the chunk size of write
> > 
> > Write(us/KB)	4	64	256	1024	2048
> > FUA		873.792	754.604	995.624	1011.67	1067.99
> > CACHE_FLUSH	824.703	712.98	800.307	1019.5	1037.37
> 
> Wouldn't it depend on how much data might be in the cache?

I've got this average latency from 100 commands of write+cache_flush vs.
write(FUA). I think the cached data should be the same as this chunk
size.

> Do you have real-world use-cases where the difference is measurable?

I'm approaching this based on 1) f2fs uses FUA for checkpoint and fsync,
and 2) iomap uses FUA for O_DIRECT|O_DSYNC case [1].

[1] https://lore.kernel.org/lkml/20220527205955.3251982-1-jaegeuk@kernel.org/

> 
> > 
> > Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
> > ---
> >  drivers/scsi/ufs/ufshcd.c | 3 +++
> >  drivers/scsi/ufs/ufshcd.h | 5 +++++
> >  2 files changed, 8 insertions(+)
> > 
> > diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> > index 3f9caafa91bf..811f3467879c 100644
> > --- a/drivers/scsi/ufs/ufshcd.c
> > +++ b/drivers/scsi/ufs/ufshcd.c
> > @@ -5035,6 +5035,9 @@ static int ufshcd_slave_configure(struct scsi_device *sdev)
> >  	 */
> >  	sdev->silence_suspend = 1;
> >  
> > +	if (hba->quirks & UFSHCD_QUIRK_BROKEN_FUA)
> > +		sdev->broken_fua = 1;
> > +
> >  	ufshcd_crypto_register(hba, q);
> >  
> >  	return 0;
> > diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
> > index 94f545be183a..6c480c6741d6 100644
> > --- a/drivers/scsi/ufs/ufshcd.h
> > +++ b/drivers/scsi/ufs/ufshcd.h
> > @@ -602,6 +602,11 @@ enum ufshcd_quirks {
> >  	 * support physical host configuration.
> >  	 */
> >  	UFSHCD_QUIRK_SKIP_PH_CONFIGURATION		= 1 << 16,
> > +
> > +	/*
> > +	 * This quirk disables FUA support.
> > +	 */
> > +	UFSHCD_QUIRK_BROKEN_FUA				= 1 << 17,
> 
> Wouldn't it be more appropriate to make it a UFS_DEVICE_QUIRK_
> since it presumably depends on the UFS device not the host controller?
> 
> Also, as already commented by others, there needs to be a user of
> the quirk

Since I asked SoC vendors can verify the performance with this quirk,
I need to wait for their reply. Meanwhile, I'm willing to disable FUA in Pixel
devices, which I cannot post any patch directly to LKML.

Agreed that, if there's no other user in upstream, I'm okay to drop
this.

> 
> >  };
> >  
> >  enum ufshcd_caps {