RE: [PATCH V1] mmc: core: fix still flush cache when eMMC cache off

[Bean Huo (beanhuo)]

Hi, 

>On 19 March 2017 at 01:45, Bean Huo (beanhuo) <beanhuo@xxxxxxxxxx> wrote:
>> This patch fixes the issue that mmc_blk_issue_rq still flushes cache
>> when eMMC cache has already been off through user space tool, such as
>> mmc-utils.
>> The reason is that card->ext_csd.cache_ctrl isn't reset.
>
>First, why do you want to turn of the cache ctrl? Is the eMMC device having
>issues with it? Then we should invent a card quirk instead.


Why I turn off it? because I did power loss testing and validation, I should switch it off and on.
When I do some performance and power loss case validation on several Linux release versions,
I need to switch off or on cache through user space tool.
I can't confirm every user that likes me, But I think at least it is not reasonable to 
flush eMMC cache, when internal eMMC cache is off.

>Second, what errors do you encounter when the mmc core tries to flush the
>cache when it has been turned off? Can you please elaborate on this?


No error found, but firstly, please think about overhead introduced by useless flush cache, Unless you
Don't care this tiny time. second, under the condition of cache off, additional flush cache request still has impact on
Internal eMMC logic. I don't know What and how impact, but at least it is really exist.


>>
>> Signed-off-by: beanhuo <beanhuo@xxxxxxxxxx>
>> ---
>>  drivers/mmc/core/block.c | 9 +++++++++
>>  1 file changed, 9 insertions(+)
>>
>> diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c index
>> 1621fa0..fb3635ac 100644
>> --- a/drivers/mmc/core/block.c
>> +++ b/drivers/mmc/core/block.c
>> @@ -64,6 +64,7 @@ MODULE_ALIAS("mmc:block");
>>  #define MMC_BLK_TIMEOUT_MS  (10 * 60 * 1000)        /* 10 minute timeout */
>>  #define MMC_SANITIZE_REQ_TIMEOUT 240000  #define
>> MMC_EXTRACT_INDEX_FROM_ARG(x) ((x & 0x00FF0000) >> 16)
>> +#define MMC_EXTRACT_VALUE_FROM_ARG(x) ((x & 0x0000FF00) >> 8)
>>
>>  #define mmc_req_rel_wr(req)    ((req->cmd_flags & REQ_FUA) && \
>>                                   (rq_data_dir(req) == WRITE)) @@
>> -535,6 +536,14 @@ static int __mmc_blk_ioctl_cmd(struct mmc_card *card,
>struct mmc_blk_data *md,
>>                 return data.error;
>>         }
>>
>> +       if ((MMC_EXTRACT_INDEX_FROM_ARG(cmd.arg) ==
>EXT_CSD_CACHE_CTRL) &&
>> +           (cmd.opcode == MMC_SWITCH) && (card->ext_csd.cache_size > 0)) {
>> +               if (MMC_EXTRACT_VALUE_FROM_ARG(cmd.arg) & 1)
>> +                       card->ext_csd.cache_ctrl = 1;
>> +               else
>> +                       card->ext_csd.cache_ctrl = 0;
>> +       }
>> +
>
>I am sure "cache ctrl" isn't the only thing user space via mmc ioctl can cause
>problems for. The mmc core keep tracks of other ext csd states, etc, as well. I
>don't think it's worth to compensate and try to act accordingly to cover cases
>when user space has messed up.
>
>To be clear, it would have been entirely different if the something was changed
>via a mmc sysfs interface. Then we really should act accordingly, however for
>mmc ioctls it just becomes unmanageable due to its flexibility.

I will check this later. But flush cache seams now there is only one entry, and
It checks "cache_ctrl".

>
>>         /*
>>          * According to the SD specs, some commands require a delay after
>>          * issuing the command.
>> --
>> 2.7.4
>
>Kind regards
>Uffe
>--
>To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of
>a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at
>http://vger.kernel.org/majordomo-info.html