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

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

[Bean Huo (beanhuo)]

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.

Signed-off-by: beanhuo <beanhuo@micron.com>
---
 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;
+	}
+
 	/*
 	 * According to the SD specs, some commands require a delay after
 	 * issuing the command.
-- 
2.7.4

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

[Shawn Lin]

Hi

On 2017/3/19 8:45, Bean Huo (beanhuo) 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.

I did a quick test and see the case you refer to, so

Tested-by: Shawn Lin <shawn.lin@rock-chips.com>
Reviewed-by: Shawn Lin <shawn.lin@rock-chips.com>

> The reason is that card->ext_csd.cache_ctrl isn't reset.
>
> Signed-off-by: beanhuo <beanhuo@micron.com>
> ---
>  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;
> +	}
> +
>  	/*
>  	 * According to the SD specs, some commands require a delay after
>  	 * issuing the command.
>

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

[Bartlomiej Zolnierkiewicz]

On Sunday, March 19, 2017 12:45:40 AM Bean Huo 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.
> 
> Signed-off-by: beanhuo <beanhuo@micron.com>

Reviewed-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics

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

[Bean Huo (beanhuo)]

Ping Linux-mmc maintainer...

Tested-by: Shawn Lin <shawn.lin@rock-chips.com>
Reviewed-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
Reviewed-by: Shawn Lin <shawn.lin@rock-chips.com>

>On Sunday, March 19, 2017 12:45:40 AM Bean Huo 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.
>>
>> Signed-off-by: beanhuo <beanhuo@micron.com>
>
>
>Best regards,
>--
>Bartlomiej Zolnierkiewicz
>Samsung R&D Institute Poland
>Samsung Electronics
>
>--
>To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of
>a message to majordomo@vger.kernel.org More majordomo info at
>http://vger.kernel.org/majordomo-info.html

//beanhuo

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

[Ulf Hansson]

On 19 March 2017 at 01:45, Bean Huo (beanhuo) <beanhuo@micron.com> 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.

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.

>
> Signed-off-by: beanhuo <beanhuo@micron.com>
> ---
>  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.

>         /*
>          * According to the SD specs, some commands require a delay after
>          * issuing the command.
> --
> 2.7.4

Kind regards
Uffe

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@micron.com> 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@micron.com>
>> ---
>>  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@vger.kernel.org More majordomo info at
>http://vger.kernel.org/majordomo-info.html

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

[Ulf Hansson]

On 23 March 2017 at 11:45, Bean Huo (beanhuo) <beanhuo@micron.com> wrote:
> Hi,
>
>>On 19 March 2017 at 01:45, Bean Huo (beanhuo) <beanhuo@micron.com> 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.

Ah, I see. Your use-case seems reasonable while validating robustness
of the eMMC!

>
>>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.

Got it!

However I still don't like the mmc ioctls API to compensate and deal
with all "crazy-ness" that user-space may cause. Cache-ctrl is only
one case out of many.

I see two viable options to solve your problem.
1) Extend mmc_test with a new test(s) for cache ctrl and perhaps
suspend/resume. Isn't this actually exactly what you want?
2) Extend debugfs to be able to turn cache ctrl on/off.

[...]

Kind regards
Uffe

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

[Bean Huo (beanhuo)]

Hi, Uffe

>>>On 19 March 2017 at 01:45, Bean Huo (beanhuo) <beanhuo@micron.com>
>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.
>
>Ah, I see. Your use-case seems reasonable while validating robustness of the
>eMMC!
>
>>
>>>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.
>
>Got it!
>
>However I still don't like the mmc ioctls API to compensate and deal with all
>"crazy-ness" that user-space may cause. Cache-ctrl is only one case out of many.

Question is that not every Linux-mmc user really understand that shouldn't use "mmc ioctls API 
to compensate and deal with all crazy-ness that user-space may cause ".
no matter we like or dislike, Issue is already there; I think that most of eMMC user is now 
configuring eMMC through mmc-utils and mmc ioctls. This is a real condition.
Some automotive applications, Cache would not be enabled. 

Maybe I am shallow, mmc ioctls likes backdoor that let user access and configure eMMC some feature,
Even not too often. I still suggest to fix this in mmc ioctls, unless mmc iotcls being deleted, and mmc-utils
doesn't support Cache off/on.
Because eMMC internal cache has been disabled, but Linux-mmc still issue flush cache request, this is indeed
Not reasonable. It definitely increases system level overhead.

>I see two viable options to solve your problem.
>1) Extend mmc_test with a new test(s) for cache ctrl and perhaps suspend/resume.
>Isn't this actually exactly what you want?

>2) Extend debugfs to be able to turn cache ctrl on/off.

I can try, but this patch is just to fix mmc ctrls backdoor on eMMC flush cache.

>[...]
>
>Kind regards
>Uffe
>--
>To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of
>a message to majordomo@vger.kernel.org More majordomo info at
>http://vger.kernel.org/majordomo-info.html