[PATCH] mmc: block: enable cache-flushing when mmc cache is on

[PATCH] mmc: block: enable cache-flushing when mmc cache is on

[Michael Wu]

The mmc core enable cache on default. But it only enables cache-flushing
when host supports cmd23 and eMMC supports reliable write.
For hosts which do not support cmd23 or eMMCs which do not support
reliable write, the cache can not be flushed by `sync` command.
This may leads to cache data lost.
This patch enables cache-flushing as long as cache is enabled, no
matter host supports cmd23 and/or eMMC supports reliable write or not.

Signed-off-by: Michael Wu <michael@allwinnertech.com>
---
 drivers/mmc/core/block.c | 20 ++++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)

diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c
index 689eb9afeeed..1e508c079c1e 100644
--- a/drivers/mmc/core/block.c
+++ b/drivers/mmc/core/block.c
@@ -2279,6 +2279,8 @@ static struct mmc_blk_data *mmc_blk_alloc_req(struct mmc_card *card,
 	struct mmc_blk_data *md;
 	int devidx, ret;
 	char cap_str[10];
+	bool enable_cache = false;
+	bool enable_fua = false;
 
 	devidx = ida_simple_get(&mmc_blk_ida, 0, max_devices, GFP_KERNEL);
 	if (devidx < 0) {
@@ -2375,12 +2377,18 @@ static struct mmc_blk_data *mmc_blk_alloc_req(struct mmc_card *card,
 			md->flags |= MMC_BLK_CMD23;
 	}
 
-	if (mmc_card_mmc(card) &&
-	    md->flags & MMC_BLK_CMD23 &&
-	    ((card->ext_csd.rel_param & EXT_CSD_WR_REL_PARAM_EN) ||
-	     card->ext_csd.rel_sectors)) {
-		md->flags |= MMC_BLK_REL_WR;
-		blk_queue_write_cache(md->queue.queue, true, true);
+	if (mmc_card_mmc(card)) {
+		if (md->flags & MMC_BLK_CMD23 &&
+			((card->ext_csd.rel_param & EXT_CSD_WR_REL_PARAM_EN) ||
+			card->ext_csd.rel_sectors)) {
+			md->flags |= MMC_BLK_REL_WR;
+			enable_fua = true;
+		}
+
+		if (mmc_cache_enabled(card->host))
+			enable_cache = true;
+
+		blk_queue_write_cache(md->queue.queue, enable_cache, enable_fua);
 	}
 
 	string_get_size((u64)size, 512, STRING_UNITS_2,
-- 
2.29.0

Re: [PATCH] mmc: block: enable cache-flushing when mmc cache is on

[Adrian Hunter]

On 12/03/2022 06:43, Michael Wu wrote:
> The mmc core enable cache on default. But it only enables cache-flushing
> when host supports cmd23 and eMMC supports reliable write.
> For hosts which do not support cmd23 or eMMCs which do not support
> reliable write, the cache can not be flushed by `sync` command.
> This may leads to cache data lost.
> This patch enables cache-flushing as long as cache is enabled, no
> matter host supports cmd23 and/or eMMC supports reliable write or not.
> 

Fixes tag?

> Signed-off-by: Michael Wu <michael@allwinnertech.com>
> ---
>  drivers/mmc/core/block.c | 20 ++++++++++++++------
>  1 file changed, 14 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c
> index 689eb9afeeed..1e508c079c1e 100644
> --- a/drivers/mmc/core/block.c
> +++ b/drivers/mmc/core/block.c
> @@ -2279,6 +2279,8 @@ static struct mmc_blk_data *mmc_blk_alloc_req(struct mmc_card *card,
>  	struct mmc_blk_data *md;
>  	int devidx, ret;
>  	char cap_str[10];
> +	bool enable_cache = false;
> +	bool enable_fua = false;
>  
>  	devidx = ida_simple_get(&mmc_blk_ida, 0, max_devices, GFP_KERNEL);
>  	if (devidx < 0) {
> @@ -2375,12 +2377,18 @@ static struct mmc_blk_data *mmc_blk_alloc_req(struct mmc_card *card,
>  			md->flags |= MMC_BLK_CMD23;
>  	}
>  
> -	if (mmc_card_mmc(card) &&
> -	    md->flags & MMC_BLK_CMD23 &&
> -	    ((card->ext_csd.rel_param & EXT_CSD_WR_REL_PARAM_EN) ||
> -	     card->ext_csd.rel_sectors)) {
> -		md->flags |= MMC_BLK_REL_WR;
> -		blk_queue_write_cache(md->queue.queue, true, true);
> +	if (mmc_card_mmc(card)) {
> +		if (md->flags & MMC_BLK_CMD23 &&
> +			((card->ext_csd.rel_param & EXT_CSD_WR_REL_PARAM_EN) ||
> +			card->ext_csd.rel_sectors)) {
> +			md->flags |= MMC_BLK_REL_WR;
> +			enable_fua = true;
> +		}
> +
> +		if (mmc_cache_enabled(card->host))
> +			enable_cache = true;
> +
> +		blk_queue_write_cache(md->queue.queue, enable_cache, enable_fua);
>  	}

Seems like we should inform block layer about SD card cache also

>  
>  	string_get_size((u64)size, 512, STRING_UNITS_2,

Re: [PATCH] mmc: block: enable cache-flushing when mmc cache is on

[Michael Wu]

On 14/03/2022 14:54, Adrian Hunter wrote:
> On 12/03/2022 06:43, Michael Wu wrote:
>> The mmc core enable cache on default. But it only enables cache-flushing
>> when host supports cmd23 and eMMC supports reliable write.
>> For hosts which do not support cmd23 or eMMCs which do not support
>> reliable write, the cache can not be flushed by `sync` command.
>> This may leads to cache data lost.
>> This patch enables cache-flushing as long as cache is enabled, no
>> matter host supports cmd23 and/or eMMC supports reliable write or not.
>>
> 
> Fixes tag?
> 

Hi Adrian,
My patch intend to fix the cache problem brought by the following two 
patches:

Fixes: d0c97cfb81ebc ("mmc: core: Use CMD23 for multiblock transfers 
when we can.")
Fixes: e9d5c746246c8 ("mmc/block: switch to using blk_queue_write_cache()")

I'm not sure if this is what you referred to ("Fixes tag"). Please 
correct me if I misunderstood.

>> Signed-off-by: Michael Wu <michael@allwinnertech.com>
>> ---
>>   drivers/mmc/core/block.c | 20 ++++++++++++++------
>>   1 file changed, 14 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c
>> index 689eb9afeeed..1e508c079c1e 100644
>> --- a/drivers/mmc/core/block.c
>> +++ b/drivers/mmc/core/block.c
>> @@ -2279,6 +2279,8 @@ static struct mmc_blk_data *mmc_blk_alloc_req(struct mmc_card *card,
>>   	struct mmc_blk_data *md;
>>   	int devidx, ret;
>>   	char cap_str[10];
>> +	bool enable_cache = false;
>> +	bool enable_fua = false;
>>   
>>   	devidx = ida_simple_get(&mmc_blk_ida, 0, max_devices, GFP_KERNEL);
>>   	if (devidx < 0) {
>> @@ -2375,12 +2377,18 @@ static struct mmc_blk_data *mmc_blk_alloc_req(struct mmc_card *card,
>>   			md->flags |= MMC_BLK_CMD23;
>>   	}
>>   
>> -	if (mmc_card_mmc(card) &&
>> -	    md->flags & MMC_BLK_CMD23 &&
>> -	    ((card->ext_csd.rel_param & EXT_CSD_WR_REL_PARAM_EN) ||
>> -	     card->ext_csd.rel_sectors)) {
>> -		md->flags |= MMC_BLK_REL_WR;
>> -		blk_queue_write_cache(md->queue.queue, true, true);
>> +	if (mmc_card_mmc(card)) {
>> +		if (md->flags & MMC_BLK_CMD23 &&
>> +			((card->ext_csd.rel_param & EXT_CSD_WR_REL_PARAM_EN) ||
>> +			card->ext_csd.rel_sectors)) {
>> +			md->flags |= MMC_BLK_REL_WR;
>> +			enable_fua = true;
>> +		}
>> +
>> +		if (mmc_cache_enabled(card->host))
>> +			enable_cache = true;
>> +
>> +		blk_queue_write_cache(md->queue.queue, enable_cache, enable_fua);
>>   	}
> 
> Seems like we should inform block layer about SD card cache also
> 

I saw another mail by Avri Altman, which says few days will be needed to 
ask internally. Shall I wait or make another change here on 'inform 
block layer about SD card cache'?

>>   
>>   	string_get_size((u64)size, 512, STRING_UNITS_2,

-- 
Best Regards,
Michael Wu

RE: [PATCH] mmc: block: enable cache-flushing when mmc cache is on

[Avri Altman]

> On 14/03/2022 14:54, Adrian Hunter wrote:
> > On 12/03/2022 06:43, Michael Wu wrote:
> >> The mmc core enable cache on default. But it only enables
> >> cache-flushing when host supports cmd23 and eMMC supports reliable
> write.
> >> For hosts which do not support cmd23 or eMMCs which do not support
> >> reliable write, the cache can not be flushed by `sync` command.
> >> This may leads to cache data lost.
> >> This patch enables cache-flushing as long as cache is enabled, no
> >> matter host supports cmd23 and/or eMMC supports reliable write or not.
> >>
> >
> > Fixes tag?
> >
> 
> Hi Adrian,
> My patch intend to fix the cache problem brought by the following two
> patches:
> 
> Fixes: d0c97cfb81ebc ("mmc: core: Use CMD23 for multiblock transfers when
> we can.")
> Fixes: e9d5c746246c8 ("mmc/block: switch to using blk_queue_write_cache()")
> 
> I'm not sure if this is what you referred to ("Fixes tag"). Please correct me if I
> misunderstood.
> 
> >> Signed-off-by: Michael Wu <michael@allwinnertech.com>
> >> ---
> >>   drivers/mmc/core/block.c | 20 ++++++++++++++------
> >>   1 file changed, 14 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c
> >> index 689eb9afeeed..1e508c079c1e 100644
> >> --- a/drivers/mmc/core/block.c
> >> +++ b/drivers/mmc/core/block.c
> >> @@ -2279,6 +2279,8 @@ static struct mmc_blk_data
> *mmc_blk_alloc_req(struct mmc_card *card,
> >>      struct mmc_blk_data *md;
> >>      int devidx, ret;
> >>      char cap_str[10];
> >> +    bool enable_cache = false;
> >> +    bool enable_fua = false;
> >>
> >>      devidx = ida_simple_get(&mmc_blk_ida, 0, max_devices, GFP_KERNEL);
> >>      if (devidx < 0) {
> >> @@ -2375,12 +2377,18 @@ static struct mmc_blk_data
> *mmc_blk_alloc_req(struct mmc_card *card,
> >>                      md->flags |= MMC_BLK_CMD23;
> >>      }
> >>
> >> -    if (mmc_card_mmc(card) &&
> >> -        md->flags & MMC_BLK_CMD23 &&
> >> -        ((card->ext_csd.rel_param & EXT_CSD_WR_REL_PARAM_EN) ||
> >> -         card->ext_csd.rel_sectors)) {
> >> -            md->flags |= MMC_BLK_REL_WR;
> >> -            blk_queue_write_cache(md->queue.queue, true, true);
> >> +    if (mmc_card_mmc(card)) {
> >> +            if (md->flags & MMC_BLK_CMD23 &&
> >> +                    ((card->ext_csd.rel_param & EXT_CSD_WR_REL_PARAM_EN)
> ||
> >> +                    card->ext_csd.rel_sectors)) {
> >> +                    md->flags |= MMC_BLK_REL_WR;
> >> +                    enable_fua = true;
> >> +            }
> >> +
> >> +            if (mmc_cache_enabled(card->host))
> >> +                    enable_cache = true;
> >> +
> >> +            blk_queue_write_cache(md->queue.queue, enable_cache,
> >> + enable_fua);
> >>      }
> >
> > Seems like we should inform block layer about SD card cache also
> >
> 
> I saw another mail by Avri Altman, which says few days will be needed to ask
> internally. Shall I wait or make another change here on 'inform block layer
> about SD card cache'?
Please don't wait.

Thanks,
Avri

> 
> >>
> >>      string_get_size((u64)size, 512, STRING_UNITS_2,
> 
> --
> Best Regards,
> Michael Wu

Re: [PATCH] mmc: block: enable cache-flushing when mmc cache is on

[Michael Wu]

On 14/03/2022 17:37, Avri Altman wrote:
>> On 14/03/2022 14:54, Adrian Hunter wrote:
>>> On 12/03/2022 06:43, Michael Wu wrote:
>>>> The mmc core enable cache on default. But it only enables
>>>> cache-flushing when host supports cmd23 and eMMC supports reliable
>> write.
>>>> For hosts which do not support cmd23 or eMMCs which do not support
>>>> reliable write, the cache can not be flushed by `sync` command.
>>>> This may leads to cache data lost.
>>>> This patch enables cache-flushing as long as cache is enabled, no
>>>> matter host supports cmd23 and/or eMMC supports reliable write or not.
>>>>
>>>
>>> Fixes tag?
>>>
>>
>> Hi Adrian,
>> My patch intend to fix the cache problem brought by the following two
>> patches:
>>
>> Fixes: d0c97cfb81ebc ("mmc: core: Use CMD23 for multiblock transfers when
>> we can.")
>> Fixes: e9d5c746246c8 ("mmc/block: switch to using blk_queue_write_cache()")
>>
>> I'm not sure if this is what you referred to ("Fixes tag"). Please correct me if I
>> misunderstood.
>>
>>>> Signed-off-by: Michael Wu <michael@allwinnertech.com>
>>>> ---
>>>>    drivers/mmc/core/block.c | 20 ++++++++++++++------
>>>>    1 file changed, 14 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c
>>>> index 689eb9afeeed..1e508c079c1e 100644
>>>> --- a/drivers/mmc/core/block.c
>>>> +++ b/drivers/mmc/core/block.c
>>>> @@ -2279,6 +2279,8 @@ static struct mmc_blk_data
>> *mmc_blk_alloc_req(struct mmc_card *card,
>>>>       struct mmc_blk_data *md;
>>>>       int devidx, ret;
>>>>       char cap_str[10];
>>>> +    bool enable_cache = false;
>>>> +    bool enable_fua = false;
>>>>
>>>>       devidx = ida_simple_get(&mmc_blk_ida, 0, max_devices, GFP_KERNEL);
>>>>       if (devidx < 0) {
>>>> @@ -2375,12 +2377,18 @@ static struct mmc_blk_data
>> *mmc_blk_alloc_req(struct mmc_card *card,
>>>>                       md->flags |= MMC_BLK_CMD23;
>>>>       }
>>>>
>>>> -    if (mmc_card_mmc(card) &&
>>>> -        md->flags & MMC_BLK_CMD23 &&
>>>> -        ((card->ext_csd.rel_param & EXT_CSD_WR_REL_PARAM_EN) ||
>>>> -         card->ext_csd.rel_sectors)) {
>>>> -            md->flags |= MMC_BLK_REL_WR;
>>>> -            blk_queue_write_cache(md->queue.queue, true, true);
>>>> +    if (mmc_card_mmc(card)) {
>>>> +            if (md->flags & MMC_BLK_CMD23 &&
>>>> +                    ((card->ext_csd.rel_param & EXT_CSD_WR_REL_PARAM_EN)
>> ||
>>>> +                    card->ext_csd.rel_sectors)) {
>>>> +                    md->flags |= MMC_BLK_REL_WR;
>>>> +                    enable_fua = true;
>>>> +            }
>>>> +
>>>> +            if (mmc_cache_enabled(card->host))
>>>> +                    enable_cache = true;
>>>> +
>>>> +            blk_queue_write_cache(md->queue.queue, enable_cache,
>>>> + enable_fua);
>>>>       }
>>>
>>> Seems like we should inform block layer about SD card cache also
>>>
>>
>> I saw another mail by Avri Altman, which says few days will be needed to ask
>> internally. Shall I wait or make another change here on 'inform block layer
>> about SD card cache'?
> Please don't wait.
> 
> Thanks,
> Avri
> 
>>
>>>>
>>>>       string_get_size((u64)size, 512, STRING_UNITS_2,
>>
>> --
>> Best Regards,
>> Michael Wu
Hi Avril & Adrian,
Thanks for your efforts. Could we have an agreement now --

1. enabling-cache and cmd23/reliable-write should be independent;

 > On 14/03/2022 18:32, Adrian Hunter wrote:
 >> On 14/03/2022 09:26, Avri Altman wrote:
 >>> Hi,
 >>>> The mmc core enable cache on default. But it only enables 
cache-flushing
 >>>> when host supports cmd23 and eMMC supports reliable write.
 >>>> For hosts which do not support cmd23 or eMMCs which do not support
 >>>> reliable write, the cache can not be flushed by `sync` command.
 >>>> This may leads to cache data lost.
 >>>> This patch enables cache-flushing as long as cache is enabled, no
 >>>> matter host supports cmd23 and/or eMMC supports reliable write or
 >>>> not.
 >>> I looked in the spec and indeed couldn't find why enabling cache is
 >>> dependent of cmd23/reliable write.
 >>> Nor I was able to find the original commit log.
 >>
 >> Reliable write was added first, so it might have been an oversight:
 >>
 >> commit 881d1c25f765938a95def5afe39486ce39f9fc96
 >> Author: Seungwon Jeon <tgih.jun@samsung.com>
 >> Date:   Fri Oct 14 14:03:21 2011 +0900
 >>
 >>      mmc: core: Add cache control for eMMC4.5 device
 >>
 >>      This patch adds cache feature of eMMC4.5 Spec.
 >>      If device supports cache capability, host can utilize some
 >>      specific operations.
 >>
 >>      Signed-off-by: Seungwon Jeon <tgih.jun@samsung.com>
 >>      Signed-off-by: Jaehoon Chung <jh80.chung@samsung.com>
 >>      Signed-off-by: Chris Ball <cjb@laptop.org>

Here's what I found in the spec JESD84-B51:
 > 6.6.31 Cache
 > Caching of data shall apply only for the single block
 > read/write(CMD17/24), pre-defined multiple block
 > read/write(CMD23+CMD18/25) and open ended multiple block
 > read/write(CMD18/25+CMD12) commands and excludes any other access
 > e.g., to the register space(e.g., CMD6).
Which means with CMD18/25+CMD12 (without using CMD23), the cache can 
also be enabled. Maybe this could be an evidence of the independence 
between enabling-cache and cmd23/reliable-write?

2. We don't consider supporting SD in this change.

 > On 14/03/2022 19:10, Avri Altman wrote:
 >> Here is what our SD system guys wrote:
 >> " In SD we don’t support reliable write and this eMMC driver may not 
 >>    be utilizing the cache feature we added in SD5.0.
 >>   The method of cache flush is different between SD and eMMC."
 >>
 >> So adding SD seems to be out of scope of this change.

Is there anything else I can do about this patch? Thanks again.

-- 
Best Regards,
Michael Wu

RE: [PATCH] mmc: block: enable cache-flushing when mmc cache is on

[Avri Altman]

> Hi Avril & Adrian,
> Thanks for your efforts. Could we have an agreement now --
> 
> 1. enabling-cache and cmd23/reliable-write should be independent;
> 
> Here's what I found in the spec JESD84-B51:
>  > 6.6.31 Cache
>  > Caching of data shall apply only for the single block
>  > read/write(CMD17/24), pre-defined multiple block
>  > read/write(CMD23+CMD18/25) and open ended multiple block
>  > read/write(CMD18/25+CMD12) commands and excludes any other access
>  > e.g., to the register space(e.g., CMD6).
> Which means with CMD18/25+CMD12 (without using CMD23), the cache can
> also be enabled. Maybe this could be an evidence of the independence
> between enabling-cache and cmd23/reliable-write?
Acked-by: Avri Altman <avri.altman@wdc.com>

Thanks,
Avri

> 
> 2. We don't consider supporting SD in this change.
> 
>  > On 14/03/2022 19:10, Avri Altman wrote:
>  >> Here is what our SD system guys wrote:
>  >> " In SD we don’t support reliable write and this eMMC driver may not
>  >>    be utilizing the cache feature we added in SD5.0.
>  >>   The method of cache flush is different between SD and eMMC."
>  >>
>  >> So adding SD seems to be out of scope of this change.
> 
> Is there anything else I can do about this patch? Thanks again.
> 
> --
> Best Regards,
> Michael Wu

Re: [PATCH] mmc: block: enable cache-flushing when mmc cache is on

[Adrian Hunter]

On 16.3.2022 13.09, Avri Altman wrote:
>> Hi Avril & Adrian,
>> Thanks for your efforts. Could we have an agreement now --
>>
>> 1. enabling-cache and cmd23/reliable-write should be independent;
>>
>> Here's what I found in the spec JESD84-B51:
>>  > 6.6.31 Cache
>>  > Caching of data shall apply only for the single block
>>  > read/write(CMD17/24), pre-defined multiple block
>>  > read/write(CMD23+CMD18/25) and open ended multiple block
>>  > read/write(CMD18/25+CMD12) commands and excludes any other access
>>  > e.g., to the register space(e.g., CMD6).
>> Which means with CMD18/25+CMD12 (without using CMD23), the cache can
>> also be enabled. Maybe this could be an evidence of the independence
>> between enabling-cache and cmd23/reliable-write?
> Acked-by: Avri Altman <avri.altman@wdc.com>
> 
> Thanks,
> Avri
> 
>>
>> 2. We don't consider supporting SD in this change.
>>
>>  > On 14/03/2022 19:10, Avri Altman wrote:
>>  >> Here is what our SD system guys wrote:
>>  >> " In SD we don’t support reliable write and this eMMC driver may not
>>  >>    be utilizing the cache feature we added in SD5.0.
>>  >>   The method of cache flush is different between SD and eMMC."
>>  >>
>>  >> So adding SD seems to be out of scope of this change.
>>
>> Is there anything else I can do about this patch? Thanks again.

So we are not going to let the block layer know about SD cache?
Or is it a separate change?

Re: [PATCH] mmc: block: enable cache-flushing when mmc cache is on

[Christian Löhle]

>So we are not going to let the block layer know about SD cache?
>Or is it a separate change?

I have some code for this laying around, but as it requires reading, parsing and writing Function Registers,
in particular PEH, it's a lot of boilerplate code to get the functionality, but I'll clean it up and send a patch in the coming weeks.





From: Adrian Hunter <adrian.hunter@intel.com>
Sent: Wednesday, March 16, 2022 12:28 PM
To: Avri Altman; Michael Wu; ulf.hansson@linaro.org; beanhuo@micron.com; porzio@gmail.com
Cc: linux-mmc@vger.kernel.org; linux-kernel@vger.kernel.org; allwinner-opensource-support
Subject: Re: [PATCH] mmc: block: enable cache-flushing when mmc cache is on
    
On 16.3.2022 13.09, Avri Altman wrote:
>> Hi Avril & Adrian,
>> Thanks for your efforts. Could we have an agreement now --
>>
>> 1. enabling-cache and cmd23/reliable-write should be independent;
>>
>> Here's what I found in the spec JESD84-B51:
>>  > 6.6.31 Cache
>>  > Caching of data shall apply only for the single block
>>  > read/write(CMD17/24), pre-defined multiple block
>>  > read/write(CMD23+CMD18/25) and open ended multiple block
>>  > read/write(CMD18/25+CMD12) commands and excludes any other access
>>  > e.g., to the register space(e.g., CMD6).
>> Which means with CMD18/25+CMD12 (without using CMD23), the cache can
>> also be enabled. Maybe this could be an evidence of the independence
>> between enabling-cache and cmd23/reliable-write?
> Acked-by: Avri Altman <avri.altman@wdc.com>
> 
> Thanks,
> Avri
> 
>>
>> 2. We don't consider supporting SD in this change.
>>
>>  > On 14/03/2022 19:10, Avri Altman wrote:
>>  >> Here is what our SD system guys wrote:
>>  >> " In SD we don’t support reliable write and this eMMC driver may not
>>  >>    be utilizing the cache feature we added in SD5.0.
>>  >>   The method of cache flush is different between SD and eMMC."
>>  >>
>>  >> So adding SD seems to be out of scope of this change.
>>
>> Is there anything else I can do about this patch? Thanks again.

So we are not going to let the block layer know about SD cache?
Or is it a separate change?
    =
Hyperstone GmbH | Reichenaustr. 39a  | 78467 Konstanz
Managing Director: Dr. Jan Peter Berns.
Commercial register of local courts: Freiburg HRB381782

Re: [PATCH] mmc: block: enable cache-flushing when mmc cache is on

[Adrian Hunter]

On 16.3.2022 16.46, Christian Löhle wrote:
>> So we are not going to let the block layer know about SD cache?
>> Or is it a separate change?
> 
> I have some code for this laying around, but as it requires reading, parsing and writing Function Registers,
> in particular PEH, it's a lot of boilerplate code to get the functionality, but I'll clean it up and send a patch in the coming weeks.
> 

We have the sd cache flush.  We would presumably just need to call blk_queue_write_cache()
for the !mmc_card_mmc(card) case e.g.

	if (mmc_has_reliable_write(card)) {
		md->flags |= MMC_BLK_REL_WR;
		enable_fua = true;
	}

	if (mmc_cache_enabled(card->host))
		enable_cache = true;

	blk_queue_write_cache(md->queue.queue, enable_cache, enable_fua);

Avri, were you objecting to that?

> 
> 
> 
> 
> From: Adrian Hunter <adrian.hunter@intel.com>
> Sent: Wednesday, March 16, 2022 12:28 PM
> To: Avri Altman; Michael Wu; ulf.hansson@linaro.org; beanhuo@micron.com; porzio@gmail.com
> Cc: linux-mmc@vger.kernel.org; linux-kernel@vger.kernel.org; allwinner-opensource-support
> Subject: Re: [PATCH] mmc: block: enable cache-flushing when mmc cache is on
>     
> On 16.3.2022 13.09, Avri Altman wrote:
>>> Hi Avril & Adrian,
>>> Thanks for your efforts. Could we have an agreement now --
>>>
>>> 1. enabling-cache and cmd23/reliable-write should be independent;
>>>
>>> Here's what I found in the spec JESD84-B51:
>>>   > 6.6.31 Cache
>>>   > Caching of data shall apply only for the single block
>>>   > read/write(CMD17/24), pre-defined multiple block
>>>   > read/write(CMD23+CMD18/25) and open ended multiple block
>>>   > read/write(CMD18/25+CMD12) commands and excludes any other access
>>>   > e.g., to the register space(e.g., CMD6).
>>> Which means with CMD18/25+CMD12 (without using CMD23), the cache can
>>> also be enabled. Maybe this could be an evidence of the independence
>>> between enabling-cache and cmd23/reliable-write?
>> Acked-by: Avri Altman <avri.altman@wdc.com>
>>
>> Thanks,
>> Avri
>>
>>>
>>> 2. We don't consider supporting SD in this change.
>>>
>>>   > On 14/03/2022 19:10, Avri Altman wrote:
>>>   >> Here is what our SD system guys wrote:
>>>   >> " In SD we don’t support reliable write and this eMMC driver may not
>>>   >>    be utilizing the cache feature we added in SD5.0.
>>>   >>   The method of cache flush is different between SD and eMMC."
>>>   >>
>>>   >> So adding SD seems to be out of scope of this change.
>>>
>>> Is there anything else I can do about this patch? Thanks again.
> 
> So we are not going to let the block layer know about SD cache?
> Or is it a separate change?
>     =
> Hyperstone GmbH | Reichenaustr. 39a  | 78467 Konstanz
> Managing Director: Dr. Jan Peter Berns.
> Commercial register of local courts: Freiburg HRB381782
> 

Re: [PATCH] mmc: block: enable cache-flushing when mmc cache is on

[Ulf Hansson]

On Wed, 16 Mar 2022 at 17:08, Adrian Hunter <adrian.hunter@intel.com> wrote:
>
> On 16.3.2022 16.46, Christian Löhle wrote:
> >> So we are not going to let the block layer know about SD cache?
> >> Or is it a separate change?
> >
> > I have some code for this laying around, but as it requires reading, parsing and writing Function Registers,
> > in particular PEH, it's a lot of boilerplate code to get the functionality, but I'll clean it up and send a patch in the coming weeks.
> >
>
> We have the sd cache flush.  We would presumably just need to call blk_queue_write_cache()
> for the !mmc_card_mmc(card) case e.g.
>
>         if (mmc_has_reliable_write(card)) {
>                 md->flags |= MMC_BLK_REL_WR;
>                 enable_fua = true;
>         }
>
>         if (mmc_cache_enabled(card->host))
>                 enable_cache = true;
>
>         blk_queue_write_cache(md->queue.queue, enable_cache, enable_fua);

To me, this seems like the most reasonable thing to do.

However, I have to admit that it's not clear to me, if there was a
good reason to why commit f4c5522b0a88 ("mmc: Reliable write
support.") also added support for REQ_FLUSH (write back cache) and why
not only REQ_FUA. I assumed this was wrong too, right?

When it comes to patches for stable kernels. mmc_cache_enabled() was
introduced quite recently in v5.13, so for older kernels that call
needs to be replaced with something else.

In any case, the relevant commits that can be considered as needs to
be fixed seems like these:
commit f4c5522b0a88 ("mmc: Reliable write support.")
commit 881d1c25f765 ("mmc: core: Add cache control for eMMC4.5 device")
commit 130206a615a9 ("mmc: core: Add support for cache ctrl for SD cards")

[...]

Kind regards
Uffe

Re: [PATCH] mmc: block: enable cache-flushing when mmc cache is on

[Ulf Hansson]

On Thu, 17 Mar 2022 at 10:14, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>
> On Wed, 16 Mar 2022 at 17:08, Adrian Hunter <adrian.hunter@intel.com> wrote:
> >
> > On 16.3.2022 16.46, Christian Löhle wrote:
> > >> So we are not going to let the block layer know about SD cache?
> > >> Or is it a separate change?
> > >
> > > I have some code for this laying around, but as it requires reading, parsing and writing Function Registers,
> > > in particular PEH, it's a lot of boilerplate code to get the functionality, but I'll clean it up and send a patch in the coming weeks.
> > >
> >
> > We have the sd cache flush.  We would presumably just need to call blk_queue_write_cache()
> > for the !mmc_card_mmc(card) case e.g.
> >
> >         if (mmc_has_reliable_write(card)) {
> >                 md->flags |= MMC_BLK_REL_WR;
> >                 enable_fua = true;
> >         }
> >
> >         if (mmc_cache_enabled(card->host))
> >                 enable_cache = true;
> >
> >         blk_queue_write_cache(md->queue.queue, enable_cache, enable_fua);
>
> To me, this seems like the most reasonable thing to do.
>
> However, I have to admit that it's not clear to me, if there was a
> good reason to why commit f4c5522b0a88 ("mmc: Reliable write
> support.") also added support for REQ_FLUSH (write back cache) and why
> not only REQ_FUA. I assumed this was wrong too, right?
>
> When it comes to patches for stable kernels. mmc_cache_enabled() was
> introduced quite recently in v5.13, so for older kernels that call
> needs to be replaced with something else.
>
> In any case, the relevant commits that can be considered as needs to
> be fixed seems like these:
> commit f4c5522b0a88 ("mmc: Reliable write support.")
> commit 881d1c25f765 ("mmc: core: Add cache control for eMMC4.5 device")
> commit 130206a615a9 ("mmc: core: Add support for cache ctrl for SD cards")
>
> [...]

Michael, are you planning to send a v2 for this? Or are there any
parts that are still unclear to you?

Kind regards
Uffe

Re: [PATCH] mmc: block: enable cache-flushing when mmc cache is on

[Michael Wu]

On 24/03/2022 19:27, Ulf Hansson wrote:
> On Thu, 17 Mar 2022 at 10:14, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>>
>> On Wed, 16 Mar 2022 at 17:08, Adrian Hunter <adrian.hunter@intel.com> wrote:
>>>
>>> On 16.3.2022 16.46, Christian Löhle wrote:
>>>>> So we are not going to let the block layer know about SD cache?
>>>>> Or is it a separate change?
>>>>
>>>> I have some code for this laying around, but as it requires reading, parsing and writing Function Registers,
>>>> in particular PEH, it's a lot of boilerplate code to get the functionality, but I'll clean it up and send a patch in the coming weeks.
>>>>
>>>
>>> We have the sd cache flush.  We would presumably just need to call blk_queue_write_cache()
>>> for the !mmc_card_mmc(card) case e.g.
>>>
>>>          if (mmc_has_reliable_write(card)) {
>>>                  md->flags |= MMC_BLK_REL_WR;
>>>                  enable_fua = true;
>>>          }
>>>
>>>          if (mmc_cache_enabled(card->host))
>>>                  enable_cache = true;
>>>
>>>          blk_queue_write_cache(md->queue.queue, enable_cache, enable_fua);
>>
>> To me, this seems like the most reasonable thing to do.
>>
>> However, I have to admit that it's not clear to me, if there was a
>> good reason to why commit f4c5522b0a88 ("mmc: Reliable write
>> support.") also added support for REQ_FLUSH (write back cache) and why
>> not only REQ_FUA. I assumed this was wrong too, right?
>>

Hi Ulf,

1. I've found the reason. If we only enable REQ_FUA, there won't be any 
effect -- The block layer won't send any request with FUA flag to the 
driver.
If we want REQ_FUA to take effect, we must enable REQ_FLUSH. But on the 
contrary, REQ_FLUSH does not rely on REQ_FUA.
In the previous patch(commit f4c5522b0a88 ("mmc: Reliable write 
support.")), REQ_FLUSH was added to make REQ_FUA effective. I've done 
experiments to prove this.

2. Why block layer requires REQ_FLUSH to make REQ_FUA effective? I did 
not find the reason. Does anyone know about this? Thank you.

>> When it comes to patches for stable kernels. mmc_cache_enabled() was
>> introduced quite recently in v5.13, so for older kernels that call
>> needs to be replaced with something else.
>>
>> In any case, the relevant commits that can be considered as needs to
>> be fixed seems like these:
>> commit f4c5522b0a88 ("mmc: Reliable write support.")
>> commit 881d1c25f765 ("mmc: core: Add cache control for eMMC4.5 device")
>> commit 130206a615a9 ("mmc: core: Add support for cache ctrl for SD cards")
>>
>> [...]
> 
> Michael, are you planning to send a v2 for this? Or are there any
> parts that are still unclear to you?

Dear Ulf, Sorry for the delay. I was trying to figure out the SD cache 
stuff, so a few day was taken...

> 
> Kind regards
> Uffe

-- 
Best Regards,
Michael Wu

Re: [PATCH] mmc: block: enable cache-flushing when mmc cache is on

[Ulf Hansson]

On Fri, 25 Mar 2022 at 06:46, Michael Wu <michael@allwinnertech.com> wrote:
>
> On 24/03/2022 19:27, Ulf Hansson wrote:
> > On Thu, 17 Mar 2022 at 10:14, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> >>
> >> On Wed, 16 Mar 2022 at 17:08, Adrian Hunter <adrian.hunter@intel.com> wrote:
> >>>
> >>> On 16.3.2022 16.46, Christian Löhle wrote:
> >>>>> So we are not going to let the block layer know about SD cache?
> >>>>> Or is it a separate change?
> >>>>
> >>>> I have some code for this laying around, but as it requires reading, parsing and writing Function Registers,
> >>>> in particular PEH, it's a lot of boilerplate code to get the functionality, but I'll clean it up and send a patch in the coming weeks.
> >>>>
> >>>
> >>> We have the sd cache flush.  We would presumably just need to call blk_queue_write_cache()
> >>> for the !mmc_card_mmc(card) case e.g.
> >>>
> >>>          if (mmc_has_reliable_write(card)) {
> >>>                  md->flags |= MMC_BLK_REL_WR;
> >>>                  enable_fua = true;
> >>>          }
> >>>
> >>>          if (mmc_cache_enabled(card->host))
> >>>                  enable_cache = true;
> >>>
> >>>          blk_queue_write_cache(md->queue.queue, enable_cache, enable_fua);
> >>
> >> To me, this seems like the most reasonable thing to do.
> >>
> >> However, I have to admit that it's not clear to me, if there was a
> >> good reason to why commit f4c5522b0a88 ("mmc: Reliable write
> >> support.") also added support for REQ_FLUSH (write back cache) and why
> >> not only REQ_FUA. I assumed this was wrong too, right?
> >>
>
> Hi Ulf,
>
> 1. I've found the reason. If we only enable REQ_FUA, there won't be any
> effect -- The block layer won't send any request with FUA flag to the
> driver.
> If we want REQ_FUA to take effect, we must enable REQ_FLUSH. But on the
> contrary, REQ_FLUSH does not rely on REQ_FUA.
> In the previous patch(commit f4c5522b0a88 ("mmc: Reliable write
> support.")), REQ_FLUSH was added to make REQ_FUA effective. I've done
> experiments to prove this.

Thanks for doing the research and for confirming.

Note that this is also pretty well documented in
Documentation/block/writeback_cache_control.rst.

>
> 2. Why block layer requires REQ_FLUSH to make REQ_FUA effective? I did
> not find the reason. Does anyone know about this? Thank you.

The REQ_FLUSH indicates that the storage device has a write back
cache, which also can be flushed in some device specific way.

The REQ_FUA (Force Unit Access), tells that the data can be written to
the storage device, in a way that when the I/O request is completed,
the data is fully written to the device (the data must not be left in
the write back cache). In other words, REQ_FUA doesn't make sense
unless REQ_FLUSH is supported too.

$subject patch should also conform to this pattern.

However, it's still questionable to me whether we want to support
REQ_FUA through the eMMC reliable write command - in case we also have
support for managing the eMMC cache separately. It looks to me that
the reason why we currently support REQ_FUA, is because back in the
days when there was only the eMMC reliable write command available, it
was simply the best we could do. But it was never really a good fit.

I am starting to think that we may consider dropping REQ_FUA, if we
have the option to manage the eMMC cache separately - no matter
whether the eMMC reliable write command is supported or not. In this
case, REQ_FLUSH is sufficient and also a better match to what we
really can support.

>
> >> When it comes to patches for stable kernels. mmc_cache_enabled() was
> >> introduced quite recently in v5.13, so for older kernels that call
> >> needs to be replaced with something else.
> >>
> >> In any case, the relevant commits that can be considered as needs to
> >> be fixed seems like these:
> >> commit f4c5522b0a88 ("mmc: Reliable write support.")
> >> commit 881d1c25f765 ("mmc: core: Add cache control for eMMC4.5 device")
> >> commit 130206a615a9 ("mmc: core: Add support for cache ctrl for SD cards")
> >>
> >> [...]
> >
> > Michael, are you planning to send a v2 for this? Or are there any
> > parts that are still unclear to you?
>
> Dear Ulf, Sorry for the delay. I was trying to figure out the SD cache
> stuff, so a few day was taken...

No problem, I have been busy too. :-)

Kind regards
Uffe

Re: [PATCH] mmc: block: enable cache-flushing when mmc cache is on

[Michael Wu]

On 25/03/2022 18:13, Ulf Hansson wrote:
> On Fri, 25 Mar 2022 at 06:46, Michael Wu <michael@allwinnertech.com> wrote:
>>
>> On 24/03/2022 19:27, Ulf Hansson wrote:
>>> On Thu, 17 Mar 2022 at 10:14, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>>>>
>>>> On Wed, 16 Mar 2022 at 17:08, Adrian Hunter <adrian.hunter@intel.com> wrote:
>>>>>
>>>>> On 16.3.2022 16.46, Christian Löhle wrote:
>>>>>>> So we are not going to let the block layer know about SD cache?
>>>>>>> Or is it a separate change?
>>>>>>
>>>>>> I have some code for this laying around, but as it requires reading, parsing and writing Function Registers,
>>>>>> in particular PEH, it's a lot of boilerplate code to get the functionality, but I'll clean it up and send a patch in the coming weeks.
>>>>>>
>>>>>
>>>>> We have the sd cache flush.  We would presumably just need to call blk_queue_write_cache()
>>>>> for the !mmc_card_mmc(card) case e.g.
>>>>>
>>>>>           if (mmc_has_reliable_write(card)) {
>>>>>                   md->flags |= MMC_BLK_REL_WR;
>>>>>                   enable_fua = true;
>>>>>           }
>>>>>
>>>>>           if (mmc_cache_enabled(card->host))
>>>>>                   enable_cache = true;
>>>>>
>>>>>           blk_queue_write_cache(md->queue.queue, enable_cache, enable_fua);
>>>>
>>>> To me, this seems like the most reasonable thing to do.
>>>>
>>>> However, I have to admit that it's not clear to me, if there was a
>>>> good reason to why commit f4c5522b0a88 ("mmc: Reliable write
>>>> support.") also added support for REQ_FLUSH (write back cache) and why
>>>> not only REQ_FUA. I assumed this was wrong too, right?
>>>>
>>
>> Hi Ulf,
>>
>> 1. I've found the reason. If we only enable REQ_FUA, there won't be any
>> effect -- The block layer won't send any request with FUA flag to the
>> driver.
>> If we want REQ_FUA to take effect, we must enable REQ_FLUSH. But on the
>> contrary, REQ_FLUSH does not rely on REQ_FUA.
>> In the previous patch(commit f4c5522b0a88 ("mmc: Reliable write
>> support.")), REQ_FLUSH was added to make REQ_FUA effective. I've done
>> experiments to prove this.
> 
> Thanks for doing the research and for confirming.
> 
> Note that this is also pretty well documented in
> Documentation/block/writeback_cache_control.rst.

Thanks for reminding. I'm clear now.

> 
>>
>> 2. Why block layer requires REQ_FLUSH to make REQ_FUA effective? I did
>> not find the reason. Does anyone know about this? Thank you.
> 
> The REQ_FLUSH indicates that the storage device has a write back
> cache, which also can be flushed in some device specific way.
> 
> The REQ_FUA (Force Unit Access), tells that the data can be written to
> the storage device, in a way that when the I/O request is completed,
> the data is fully written to the device (the data must not be left in
> the write back cache). In other words, REQ_FUA doesn't make sense
> unless REQ_FLUSH is supported too.
> 

Thank you for your answer.

> $subject patch should also conform to this pattern.

I'm not sure if I understood this in a right way... Did you mean I 
should modify the subject of this mail/patch?

> 
> However, it's still questionable to me whether we want to support
> REQ_FUA through the eMMC reliable write command - in case we also have
> support for managing the eMMC cache separately. It looks to me that
> the reason why we currently support REQ_FUA, is because back in the
> days when there was only the eMMC reliable write command available, it
> was simply the best we could do. But it was never really a good fit.
> 
> I am starting to think that we may consider dropping REQ_FUA, if we
> have the option to manage the eMMC cache separately - no matter
> whether the eMMC reliable write command is supported or not. In this
> case, REQ_FLUSH is sufficient and also a better match to what we
> really can support.

Hi Ulf,
As to dropping REQ_FUA, I don't know if it is a good idea, but generally 
we are facing three possible situations:

1. If both cache and reliable-write are available, both REQ_FUA and 
REQ_FLUSH can be supported at the same time. In this case, with 
available cache, the behavior of reliable-write is to write eMMC while 
skipping cache, which is consistent with the current kernel's definition 
of REQ_FUA. What's more, most eMMCs now support both cache and 
reliable-write command.
2. If only reliable-write is available, REQ_FUA should not be supported, 
which is consistent with the current standard in another way. But I 
don't think eMMCs that only support reliable-write can be easily found 
nowadays.
3. If only cache is available, we just use REQ_FLUSH. It is not in 
conflict with keeping REQ_FUA.

Maybe, is it more reasonable to reserve FUA and use if/else to pick it 
up or down, considering the compatibility? I mean, in most cases, FUA 
and FLUSH are complementary. So it seems more feasible with branch to 
choose.

>>
>>>> When it comes to patches for stable kernels. mmc_cache_enabled() was
>>>> introduced quite recently in v5.13, so for older kernels that call
>>>> needs to be replaced with something else.
>>>>
>>>> In any case, the relevant commits that can be considered as needs to
>>>> be fixed seems like these:
>>>> commit f4c5522b0a88 ("mmc: Reliable write support.")
>>>> commit 881d1c25f765 ("mmc: core: Add cache control for eMMC4.5 device")
>>>> commit 130206a615a9 ("mmc: core: Add support for cache ctrl for SD cards")
>>>>
>>>> [...]
>>>
>>> Michael, are you planning to send a v2 for this? Or are there any
>>> parts that are still unclear to you?
>>
>> Dear Ulf, Sorry for the delay. I was trying to figure out the SD cache
>> stuff, so a few day was taken...
> 
> No problem, I have been busy too. :-)
> 
> Kind regards
> Uffe

-- 
Best Regards,
Michael Wu

Re: [PATCH] mmc: block: enable cache-flushing when mmc cache is on

[Ulf Hansson]

On Mon, 28 Mar 2022 at 12:11, Michael Wu <michael@allwinnertech.com> wrote:
>
> On 25/03/2022 18:13, Ulf Hansson wrote:
> > On Fri, 25 Mar 2022 at 06:46, Michael Wu <michael@allwinnertech.com> wrote:
> >>
> >> On 24/03/2022 19:27, Ulf Hansson wrote:
> >>> On Thu, 17 Mar 2022 at 10:14, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> >>>>
> >>>> On Wed, 16 Mar 2022 at 17:08, Adrian Hunter <adrian.hunter@intel.com> wrote:
> >>>>>
> >>>>> On 16.3.2022 16.46, Christian Löhle wrote:
> >>>>>>> So we are not going to let the block layer know about SD cache?
> >>>>>>> Or is it a separate change?
> >>>>>>
> >>>>>> I have some code for this laying around, but as it requires reading, parsing and writing Function Registers,
> >>>>>> in particular PEH, it's a lot of boilerplate code to get the functionality, but I'll clean it up and send a patch in the coming weeks.
> >>>>>>
> >>>>>
> >>>>> We have the sd cache flush.  We would presumably just need to call blk_queue_write_cache()
> >>>>> for the !mmc_card_mmc(card) case e.g.
> >>>>>
> >>>>>           if (mmc_has_reliable_write(card)) {
> >>>>>                   md->flags |= MMC_BLK_REL_WR;
> >>>>>                   enable_fua = true;
> >>>>>           }
> >>>>>
> >>>>>           if (mmc_cache_enabled(card->host))
> >>>>>                   enable_cache = true;
> >>>>>
> >>>>>           blk_queue_write_cache(md->queue.queue, enable_cache, enable_fua);
> >>>>
> >>>> To me, this seems like the most reasonable thing to do.
> >>>>
> >>>> However, I have to admit that it's not clear to me, if there was a
> >>>> good reason to why commit f4c5522b0a88 ("mmc: Reliable write
> >>>> support.") also added support for REQ_FLUSH (write back cache) and why
> >>>> not only REQ_FUA. I assumed this was wrong too, right?
> >>>>
> >>
> >> Hi Ulf,
> >>
> >> 1. I've found the reason. If we only enable REQ_FUA, there won't be any
> >> effect -- The block layer won't send any request with FUA flag to the
> >> driver.
> >> If we want REQ_FUA to take effect, we must enable REQ_FLUSH. But on the
> >> contrary, REQ_FLUSH does not rely on REQ_FUA.
> >> In the previous patch(commit f4c5522b0a88 ("mmc: Reliable write
> >> support.")), REQ_FLUSH was added to make REQ_FUA effective. I've done
> >> experiments to prove this.
> >
> > Thanks for doing the research and for confirming.
> >
> > Note that this is also pretty well documented in
> > Documentation/block/writeback_cache_control.rst.
>
> Thanks for reminding. I'm clear now.
>
> >
> >>
> >> 2. Why block layer requires REQ_FLUSH to make REQ_FUA effective? I did
> >> not find the reason. Does anyone know about this? Thank you.
> >
> > The REQ_FLUSH indicates that the storage device has a write back
> > cache, which also can be flushed in some device specific way.
> >
> > The REQ_FUA (Force Unit Access), tells that the data can be written to
> > the storage device, in a way that when the I/O request is completed,
> > the data is fully written to the device (the data must not be left in
> > the write back cache). In other words, REQ_FUA doesn't make sense
> > unless REQ_FLUSH is supported too.
> >
>
> Thank you for your answer.
>
> > $subject patch should also conform to this pattern.
>
> I'm not sure if I understood this in a right way... Did you mean I
> should modify the subject of this mail/patch?

No, I just meant that the code in the patch should conform to this.

If REQ_FUA is set, REQ_FLUSH must be set too.

>
> >
> > However, it's still questionable to me whether we want to support
> > REQ_FUA through the eMMC reliable write command - in case we also have
> > support for managing the eMMC cache separately. It looks to me that
> > the reason why we currently support REQ_FUA, is because back in the
> > days when there was only the eMMC reliable write command available, it
> > was simply the best we could do. But it was never really a good fit.
> >
> > I am starting to think that we may consider dropping REQ_FUA, if we
> > have the option to manage the eMMC cache separately - no matter
> > whether the eMMC reliable write command is supported or not. In this
> > case, REQ_FLUSH is sufficient and also a better match to what we
> > really can support.
>
> Hi Ulf,
> As to dropping REQ_FUA, I don't know if it is a good idea, but generally
> we are facing three possible situations:
>
> 1. If both cache and reliable-write are available, both REQ_FUA and
> REQ_FLUSH can be supported at the same time. In this case, with
> available cache, the behavior of reliable-write is to write eMMC while
> skipping cache, which is consistent with the current kernel's definition
> of REQ_FUA. What's more, most eMMCs now support both cache and
> reliable-write command.

Yes, this seems reasonable.


> 2. If only reliable-write is available, REQ_FUA should not be supported,
> which is consistent with the current standard in another way. But I
> don't think eMMCs that only support reliable-write can be easily found
> nowadays.

If we drop REQ_FUA for this case, I am worried that we might break use
cases for those older eMMC devices.

So, no, let's keep REQ_FUA and REQ_FLUSH if reliable-write is supported.

> 3. If only cache is available, we just use REQ_FLUSH. It is not in
> conflict with keeping REQ_FUA.

Right.

>
> Maybe, is it more reasonable to reserve FUA and use if/else to pick it
> up or down, considering the compatibility? I mean, in most cases, FUA
> and FLUSH are complementary. So it seems more feasible with branch to
> choose.

Let's summarize what I think we should do then:

 if (reliable-write supported) {
     enable_fua = true;
     enable_cache = true;
}

if (mmc_cache_enabled)
     enable_cache = true;

blk_queue_write_cache(md->queue.queue, enable_cache, enable_fua);

Does this seem reasonable to you?

[...]

Kind regards
Uffe

Re: [PATCH] mmc: block: enable cache-flushing when mmc cache is on

[Michael Wu]

On 28/03/2022 19:38, Ulf Hansson wrote:
> On Mon, 28 Mar 2022 at 12:11, Michael Wu <michael@allwinnertech.com> wrote:
>>
>> On 25/03/2022 18:13, Ulf Hansson wrote:
>>> On Fri, 25 Mar 2022 at 06:46, Michael Wu <michael@allwinnertech.com> wrote:
>>>>
>>>> On 24/03/2022 19:27, Ulf Hansson wrote:
>>>>> On Thu, 17 Mar 2022 at 10:14, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>>>>>>
>>>>>> On Wed, 16 Mar 2022 at 17:08, Adrian Hunter <adrian.hunter@intel.com> wrote:
>>>>>>>
>>>>>>> On 16.3.2022 16.46, Christian Löhle wrote:
>>>>>>>>> So we are not going to let the block layer know about SD cache?
>>>>>>>>> Or is it a separate change?
>>>>>>>>
>>>>>>>> I have some code for this laying around, but as it requires reading, parsing and writing Function Registers,
>>>>>>>> in particular PEH, it's a lot of boilerplate code to get the functionality, but I'll clean it up and send a patch in the coming weeks.
>>>>>>>>
>>>>>>>
>>>>>>> We have the sd cache flush.  We would presumably just need to call blk_queue_write_cache()
>>>>>>> for the !mmc_card_mmc(card) case e.g.
>>>>>>>
>>>>>>>            if (mmc_has_reliable_write(card)) {
>>>>>>>                    md->flags |= MMC_BLK_REL_WR;
>>>>>>>                    enable_fua = true;
>>>>>>>            }
>>>>>>>
>>>>>>>            if (mmc_cache_enabled(card->host))
>>>>>>>                    enable_cache = true;
>>>>>>>
>>>>>>>            blk_queue_write_cache(md->queue.queue, enable_cache, enable_fua);
>>>>>>
>>>>>> To me, this seems like the most reasonable thing to do.
>>>>>>
>>>>>> However, I have to admit that it's not clear to me, if there was a
>>>>>> good reason to why commit f4c5522b0a88 ("mmc: Reliable write
>>>>>> support.") also added support for REQ_FLUSH (write back cache) and why
>>>>>> not only REQ_FUA. I assumed this was wrong too, right?
>>>>>>
>>>>
>>>> Hi Ulf,
>>>>
>>>> 1. I've found the reason. If we only enable REQ_FUA, there won't be any
>>>> effect -- The block layer won't send any request with FUA flag to the
>>>> driver.
>>>> If we want REQ_FUA to take effect, we must enable REQ_FLUSH. But on the
>>>> contrary, REQ_FLUSH does not rely on REQ_FUA.
>>>> In the previous patch(commit f4c5522b0a88 ("mmc: Reliable write
>>>> support.")), REQ_FLUSH was added to make REQ_FUA effective. I've done
>>>> experiments to prove this.
>>>
>>> Thanks for doing the research and for confirming.
>>>
>>> Note that this is also pretty well documented in
>>> Documentation/block/writeback_cache_control.rst.
>>
>> Thanks for reminding. I'm clear now.
>>
>>>
>>>>
>>>> 2. Why block layer requires REQ_FLUSH to make REQ_FUA effective? I did
>>>> not find the reason. Does anyone know about this? Thank you.
>>>
>>> The REQ_FLUSH indicates that the storage device has a write back
>>> cache, which also can be flushed in some device specific way.
>>>
>>> The REQ_FUA (Force Unit Access), tells that the data can be written to
>>> the storage device, in a way that when the I/O request is completed,
>>> the data is fully written to the device (the data must not be left in
>>> the write back cache). In other words, REQ_FUA doesn't make sense
>>> unless REQ_FLUSH is supported too.
>>>
>>
>> Thank you for your answer.
>>
>>> $subject patch should also conform to this pattern.
>>
>> I'm not sure if I understood this in a right way... Did you mean I
>> should modify the subject of this mail/patch?
> 
> No, I just meant that the code in the patch should conform to this.

No problem. Please have a look at the code below.

> 
> If REQ_FUA is set, REQ_FLUSH must be set too.
> 
>>
>>>
>>> However, it's still questionable to me whether we want to support
>>> REQ_FUA through the eMMC reliable write command - in case we also have
>>> support for managing the eMMC cache separately. It looks to me that
>>> the reason why we currently support REQ_FUA, is because back in the
>>> days when there was only the eMMC reliable write command available, it
>>> was simply the best we could do. But it was never really a good fit.
>>>
>>> I am starting to think that we may consider dropping REQ_FUA, if we
>>> have the option to manage the eMMC cache separately - no matter
>>> whether the eMMC reliable write command is supported or not. In this
>>> case, REQ_FLUSH is sufficient and also a better match to what we
>>> really can support.
>>
>> Hi Ulf,
>> As to dropping REQ_FUA, I don't know if it is a good idea, but generally
>> we are facing three possible situations:
>>
>> 1. If both cache and reliable-write are available, both REQ_FUA and
>> REQ_FLUSH can be supported at the same time. In this case, with
>> available cache, the behavior of reliable-write is to write eMMC while
>> skipping cache, which is consistent with the current kernel's definition
>> of REQ_FUA. What's more, most eMMCs now support both cache and
>> reliable-write command.
> 
> Yes, this seems reasonable.
> 
> 
>> 2. If only reliable-write is available, REQ_FUA should not be supported,
>> which is consistent with the current standard in another way. But I
>> don't think eMMCs that only support reliable-write can be easily found
>> nowadays.
> 
> If we drop REQ_FUA for this case, I am worried that we might break use
> cases for those older eMMC devices.
> 
> So, no, let's keep REQ_FUA and REQ_FLUSH if reliable-write is supported.

OK. Let's keep them.

> 
>> 3. If only cache is available, we just use REQ_FLUSH. It is not in
>> conflict with keeping REQ_FUA.
> 
> Right.
> 
>>
>> Maybe, is it more reasonable to reserve FUA and use if/else to pick it
>> up or down, considering the compatibility? I mean, in most cases, FUA
>> and FLUSH are complementary. So it seems more feasible with branch to
>> choose.
> 
> Let's summarize what I think we should do then:
> 
>   if (reliable-write supported) {
>       enable_fua = true;
>       enable_cache = true;
> }
> 
> if (mmc_cache_enabled)
>       enable_cache = true;
> 
> blk_queue_write_cache(md->queue.queue, enable_cache, enable_fua);
> 
> Does this seem reasonable to you?

Yes. Let me attach the complete code here:

     if (md->flags & MMC_BLK_CMD23 &&
         ((card->ext_csd.rel_param & EXT_CSD_WR_REL_PARAM_EN) ||
         card->ext_csd.rel_sectors)) {
         md->flags |= MMC_BLK_REL_WR;
         enable_fua = true;
         enable_cache = true;
     }

     if (mmc_cache_enabled(card->host))
         enable_cache = true;

     blk_queue_write_cache(md->queue.queue, enable_cache, enable_fua);


If this is good, I'll submit a patch-v2 soon.

> 
> [...]
> 
> Kind regards
> Uffe

-- 
Best Regards,
Michael Wu

Re: [PATCH] mmc: block: enable cache-flushing when mmc cache is on

[Ulf Hansson]

On Tue, 29 Mar 2022 at 11:08, Michael Wu <michael@allwinnertech.com> wrote:
>
> On 28/03/2022 19:38, Ulf Hansson wrote:
> > On Mon, 28 Mar 2022 at 12:11, Michael Wu <michael@allwinnertech.com> wrote:
> >>
> >> On 25/03/2022 18:13, Ulf Hansson wrote:
> >>> On Fri, 25 Mar 2022 at 06:46, Michael Wu <michael@allwinnertech.com> wrote:
> >>>>
> >>>> On 24/03/2022 19:27, Ulf Hansson wrote:
> >>>>> On Thu, 17 Mar 2022 at 10:14, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> >>>>>>
> >>>>>> On Wed, 16 Mar 2022 at 17:08, Adrian Hunter <adrian.hunter@intel.com> wrote:
> >>>>>>>
> >>>>>>> On 16.3.2022 16.46, Christian Löhle wrote:
> >>>>>>>>> So we are not going to let the block layer know about SD cache?
> >>>>>>>>> Or is it a separate change?
> >>>>>>>>
> >>>>>>>> I have some code for this laying around, but as it requires reading, parsing and writing Function Registers,
> >>>>>>>> in particular PEH, it's a lot of boilerplate code to get the functionality, but I'll clean it up and send a patch in the coming weeks.
> >>>>>>>>
> >>>>>>>
> >>>>>>> We have the sd cache flush.  We would presumably just need to call blk_queue_write_cache()
> >>>>>>> for the !mmc_card_mmc(card) case e.g.
> >>>>>>>
> >>>>>>>            if (mmc_has_reliable_write(card)) {
> >>>>>>>                    md->flags |= MMC_BLK_REL_WR;
> >>>>>>>                    enable_fua = true;
> >>>>>>>            }
> >>>>>>>
> >>>>>>>            if (mmc_cache_enabled(card->host))
> >>>>>>>                    enable_cache = true;
> >>>>>>>
> >>>>>>>            blk_queue_write_cache(md->queue.queue, enable_cache, enable_fua);
> >>>>>>
> >>>>>> To me, this seems like the most reasonable thing to do.
> >>>>>>
> >>>>>> However, I have to admit that it's not clear to me, if there was a
> >>>>>> good reason to why commit f4c5522b0a88 ("mmc: Reliable write
> >>>>>> support.") also added support for REQ_FLUSH (write back cache) and why
> >>>>>> not only REQ_FUA. I assumed this was wrong too, right?
> >>>>>>
> >>>>
> >>>> Hi Ulf,
> >>>>
> >>>> 1. I've found the reason. If we only enable REQ_FUA, there won't be any
> >>>> effect -- The block layer won't send any request with FUA flag to the
> >>>> driver.
> >>>> If we want REQ_FUA to take effect, we must enable REQ_FLUSH. But on the
> >>>> contrary, REQ_FLUSH does not rely on REQ_FUA.
> >>>> In the previous patch(commit f4c5522b0a88 ("mmc: Reliable write
> >>>> support.")), REQ_FLUSH was added to make REQ_FUA effective. I've done
> >>>> experiments to prove this.
> >>>
> >>> Thanks for doing the research and for confirming.
> >>>
> >>> Note that this is also pretty well documented in
> >>> Documentation/block/writeback_cache_control.rst.
> >>
> >> Thanks for reminding. I'm clear now.
> >>
> >>>
> >>>>
> >>>> 2. Why block layer requires REQ_FLUSH to make REQ_FUA effective? I did
> >>>> not find the reason. Does anyone know about this? Thank you.
> >>>
> >>> The REQ_FLUSH indicates that the storage device has a write back
> >>> cache, which also can be flushed in some device specific way.
> >>>
> >>> The REQ_FUA (Force Unit Access), tells that the data can be written to
> >>> the storage device, in a way that when the I/O request is completed,
> >>> the data is fully written to the device (the data must not be left in
> >>> the write back cache). In other words, REQ_FUA doesn't make sense
> >>> unless REQ_FLUSH is supported too.
> >>>
> >>
> >> Thank you for your answer.
> >>
> >>> $subject patch should also conform to this pattern.
> >>
> >> I'm not sure if I understood this in a right way... Did you mean I
> >> should modify the subject of this mail/patch?
> >
> > No, I just meant that the code in the patch should conform to this.
>
> No problem. Please have a look at the code below.
>
> >
> > If REQ_FUA is set, REQ_FLUSH must be set too.
> >
> >>
> >>>
> >>> However, it's still questionable to me whether we want to support
> >>> REQ_FUA through the eMMC reliable write command - in case we also have
> >>> support for managing the eMMC cache separately. It looks to me that
> >>> the reason why we currently support REQ_FUA, is because back in the
> >>> days when there was only the eMMC reliable write command available, it
> >>> was simply the best we could do. But it was never really a good fit.
> >>>
> >>> I am starting to think that we may consider dropping REQ_FUA, if we
> >>> have the option to manage the eMMC cache separately - no matter
> >>> whether the eMMC reliable write command is supported or not. In this
> >>> case, REQ_FLUSH is sufficient and also a better match to what we
> >>> really can support.
> >>
> >> Hi Ulf,
> >> As to dropping REQ_FUA, I don't know if it is a good idea, but generally
> >> we are facing three possible situations:
> >>
> >> 1. If both cache and reliable-write are available, both REQ_FUA and
> >> REQ_FLUSH can be supported at the same time. In this case, with
> >> available cache, the behavior of reliable-write is to write eMMC while
> >> skipping cache, which is consistent with the current kernel's definition
> >> of REQ_FUA. What's more, most eMMCs now support both cache and
> >> reliable-write command.
> >
> > Yes, this seems reasonable.
> >
> >
> >> 2. If only reliable-write is available, REQ_FUA should not be supported,
> >> which is consistent with the current standard in another way. But I
> >> don't think eMMCs that only support reliable-write can be easily found
> >> nowadays.
> >
> > If we drop REQ_FUA for this case, I am worried that we might break use
> > cases for those older eMMC devices.
> >
> > So, no, let's keep REQ_FUA and REQ_FLUSH if reliable-write is supported.
>
> OK. Let's keep them.
>
> >
> >> 3. If only cache is available, we just use REQ_FLUSH. It is not in
> >> conflict with keeping REQ_FUA.
> >
> > Right.
> >
> >>
> >> Maybe, is it more reasonable to reserve FUA and use if/else to pick it
> >> up or down, considering the compatibility? I mean, in most cases, FUA
> >> and FLUSH are complementary. So it seems more feasible with branch to
> >> choose.
> >
> > Let's summarize what I think we should do then:
> >
> >   if (reliable-write supported) {
> >       enable_fua = true;
> >       enable_cache = true;
> > }
> >
> > if (mmc_cache_enabled)
> >       enable_cache = true;
> >
> > blk_queue_write_cache(md->queue.queue, enable_cache, enable_fua);
> >
> > Does this seem reasonable to you?
>
> Yes. Let me attach the complete code here:
>
>      if (md->flags & MMC_BLK_CMD23 &&
>          ((card->ext_csd.rel_param & EXT_CSD_WR_REL_PARAM_EN) ||
>          card->ext_csd.rel_sectors)) {
>          md->flags |= MMC_BLK_REL_WR;
>          enable_fua = true;
>          enable_cache = true;
>      }
>
>      if (mmc_cache_enabled(card->host))
>          enable_cache = true;
>
>      blk_queue_write_cache(md->queue.queue, enable_cache, enable_fua);
>
>
> If this is good, I'll submit a patch-v2 soon.

Yes, this looks good to me!

Kind regards
Uffe

RE: [PATCH] mmc: block: enable cache-flushing when mmc cache is on

[Avri Altman]

 
> On 16.3.2022 16.46, Christian Löhle wrote:
> >> So we are not going to let the block layer know about SD cache?
> >> Or is it a separate change?
> >
> > I have some code for this laying around, but as it requires reading,
> > parsing and writing Function Registers, in particular PEH, it's a lot of
> boilerplate code to get the functionality, but I'll clean it up and send a patch
> in the coming weeks.
> >
> 
> We have the sd cache flush.  We would presumably just need to call
> blk_queue_write_cache() for the !mmc_card_mmc(card) case e.g.
> 
>         if (mmc_has_reliable_write(card)) {
>                 md->flags |= MMC_BLK_REL_WR;
>                 enable_fua = true;
>         }
> 
>         if (mmc_cache_enabled(card->host))
>                 enable_cache = true;
> 
>         blk_queue_write_cache(md->queue.queue, enable_cache, enable_fua);
> 
> Avri, were you objecting to that?
Hi, Sorry for my late response.
Yes - That's fine.

Thanks,
Avri

> 
> >
> >
> >
> >
> > From: Adrian Hunter <adrian.hunter@intel.com>
> > Sent: Wednesday, March 16, 2022 12:28 PM
> > To: Avri Altman; Michael Wu; ulf.hansson@linaro.org;
> > beanhuo@micron.com; porzio@gmail.com
> > Cc: linux-mmc@vger.kernel.org; linux-kernel@vger.kernel.org;
> > allwinner-opensource-support
> > Subject: Re: [PATCH] mmc: block: enable cache-flushing when mmc cache
> > is on
> >
> > On 16.3.2022 13.09, Avri Altman wrote:
> >>> Hi Avril & Adrian,
> >>> Thanks for your efforts. Could we have an agreement now --
> >>>
> >>> 1. enabling-cache and cmd23/reliable-write should be independent;
> >>>
> >>> Here's what I found in the spec JESD84-B51:
> >>>   > 6.6.31 Cache
> >>>   > Caching of data shall apply only for the single block
> >>>   > read/write(CMD17/24), pre-defined multiple block
> >>>   > read/write(CMD23+CMD18/25) and open ended multiple block
> >>>   > read/write(CMD18/25+CMD12) commands and excludes any other
> access
> >>>   > e.g., to the register space(e.g., CMD6).
> >>> Which means with CMD18/25+CMD12 (without using CMD23), the cache
> can
> >>> also be enabled. Maybe this could be an evidence of the independence
> >>> between enabling-cache and cmd23/reliable-write?
> >> Acked-by: Avri Altman <avri.altman@wdc.com>
> >>
> >> Thanks,
> >> Avri
> >>
> >>>
> >>> 2. We don't consider supporting SD in this change.
> >>>
> >>>   > On 14/03/2022 19:10, Avri Altman wrote:
> >>>   >> Here is what our SD system guys wrote:
> >>>   >> " In SD we don’t support reliable write and this eMMC driver may not
> >>>   >>    be utilizing the cache feature we added in SD5.0.
> >>>   >>   The method of cache flush is different between SD and eMMC."
> >>>   >>
> >>>   >> So adding SD seems to be out of scope of this change.
> >>>
> >>> Is there anything else I can do about this patch? Thanks again.
> >
> > So we are not going to let the block layer know about SD cache?
> > Or is it a separate change?
> >     =
> > Hyperstone GmbH | Reichenaustr. 39a  | 78467 Konstanz Managing
> > Director: Dr. Jan Peter Berns.
> > Commercial register of local courts: Freiburg HRB381782
> >

RE: [PATCH] mmc: block: enable cache-flushing when mmc cache is on

[Avri Altman]

> > Seems like we should inform block layer about SD card cache also
> >
> 
> I saw another mail by Avri Altman, which says few days will be needed to ask
> internally. Shall I wait or make another change here on 'inform block layer
> about SD card cache'?
Here is what our SD system guys wrote:
" In SD we don’t support reliable write and this eMMC driver may not be utilizing the cache feature we added in SD5.0.
 The method of cache flush is different between SD and eMMC."

So adding SD seems to be out of scope of this change.

Thanks,
Avri

> 
> >>
> >>      string_get_size((u64)size, 512, STRING_UNITS_2,
> 
> --
> Best Regards,
> Michael Wu

RE: [PATCH] mmc: block: enable cache-flushing when mmc cache is on

[Avri Altman]

Hi,
> The mmc core enable cache on default. But it only enables cache-flushing
> when host supports cmd23 and eMMC supports reliable write.
> For hosts which do not support cmd23 or eMMCs which do not support
> reliable write, the cache can not be flushed by `sync` command.
> This may leads to cache data lost.
> This patch enables cache-flushing as long as cache is enabled, no matter host
> supports cmd23 and/or eMMC supports reliable write or not.
I looked in the spec and indeed couldn't find why enabling cache is dependent of cmd23/reliable write.
Nor I was able to find the original commit log.

Please allow few days to ask internally.

Thanks,
Avri

Re: [PATCH] mmc: block: enable cache-flushing when mmc cache is on

[Adrian Hunter]

On 14/03/2022 09:26, Avri Altman wrote:
> Hi,
>> The mmc core enable cache on default. But it only enables cache-flushing
>> when host supports cmd23 and eMMC supports reliable write.
>> For hosts which do not support cmd23 or eMMCs which do not support
>> reliable write, the cache can not be flushed by `sync` command.
>> This may leads to cache data lost.
>> This patch enables cache-flushing as long as cache is enabled, no matter host
>> supports cmd23 and/or eMMC supports reliable write or not.
> I looked in the spec and indeed couldn't find why enabling cache is dependent of cmd23/reliable write.
> Nor I was able to find the original commit log.

Reliable write was added first, so it might have been an oversight:

commit 881d1c25f765938a95def5afe39486ce39f9fc96
Author: Seungwon Jeon <tgih.jun@samsung.com>
Date:   Fri Oct 14 14:03:21 2011 +0900

    mmc: core: Add cache control for eMMC4.5 device
    
    This patch adds cache feature of eMMC4.5 Spec.
    If device supports cache capability, host can utilize some specific
    operations.
    
    Signed-off-by: Seungwon Jeon <tgih.jun@samsung.com>
    Signed-off-by: Jaehoon Chung <jh80.chung@samsung.com>
    Signed-off-by: Chris Ball <cjb@laptop.org>



> 
> Please allow few days to ask internally.
> 
> Thanks,
> Avri