[PATCH RFC 0/3] mmc: block: Fix tuning (by avoiding it) for RPMB

[PATCH RFC 0/3] mmc: block: Fix tuning (by avoiding it) for RPMB

[Adrian Hunter]

Hi

The RPMB partition only allows certain commands.  In particular,
the tuning command (CMD21) is not allowed -  refer JEDEC eMMC
standard v5.1 section 6.2.2 Command restrictions.

That means commands will begin failing if re-tuning is needed
while switched to the RPMB partition.

Here are 4 options to fix the problem:


1. The approach taken by this patch set:

To avoid tuning for RPMB, switch to High Speed mode from HS200
or HS400 mode if re-tuning has been enabled.  And switch back
when leaving RPMB.

Advantages: Directly does what needs to be done.

Disadvantages: Assumes the mode switching will work.


2. Same as 1 but disable the tuning modes by removing them from
card->mmc_avail_type and doing a full mmc_reset().

Advantages: Simpler to program

Disadvantages: Doing a full reset is slower.  Doing a full reset
is not an expected consequence of accessing RPMB.


3. Simply disable re-tuning and attempt to recover from any errors.

Disadvantages: Due to the interdependent nature of RPMB reads/writes
it might not be possible to recover without returning an error to the
RPMB user.  Also it would be difficult to test if the recovery actually
worked in all cases.


4. Do a partiton switch as part of re-tuning.

Disadvantages: Makes re-tuning more complicated.  Would require moving
the control of partition switching into the core.  CRC errors arising
from the need to re-tune might break the interdependent nature of RPMB
operations.


As a final note, if a solution is found then we might be able to revert
commit 4e93b9a6abc0 ("mmc: card: Don't access RPMB partitions for normal
read/write").


Adrian Hunter (3):
      mmc: mmc: Factor out mmc_hs200_to_hs()
      mmc: mmc: Factor out mmc_hs400_to_hs() and __mmc_hs_to_hs200()
      mmc: block: Fix tuning (by avoiding it) for RPMB

 drivers/mmc/card/block.c |  36 +++++++++++
 drivers/mmc/core/mmc.c   | 156 ++++++++++++++++++++++++++++++++++++++++++-----
 include/linux/mmc/card.h |   2 +-
 include/linux/mmc/core.h |   3 +
 include/linux/mmc/host.h |   5 ++
 5 files changed, 186 insertions(+), 16 deletions(-)


Regards
Adrian

[PATCH RFC 1/3] mmc: mmc: Factor out mmc_hs200_to_hs()

[Adrian Hunter]

Factor out mmc_hs200_to_hs() so it can be re-used
in a later patch.

Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
---
 drivers/mmc/core/mmc.c | 47 +++++++++++++++++++++++++++++++++--------------
 1 file changed, 33 insertions(+), 14 deletions(-)

diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
index 28b477d397b1..a3fff994e6af 100644
--- a/drivers/mmc/core/mmc.c
+++ b/drivers/mmc/core/mmc.c
@@ -1053,21 +1053,14 @@ static int mmc_switch_status(struct mmc_card *card)
 	return mmc_switch_status_error(card->host, status);
 }
 
-static int mmc_select_hs400(struct mmc_card *card)
+static int mmc_hs200_to_hs(struct mmc_card *card)
 {
 	struct mmc_host *host = card->host;
 	bool send_status = true;
 	unsigned int max_dtr;
-	int err = 0;
+	int err;
 	u8 val;
 
-	/*
-	 * HS400 mode requires 8-bit bus width
-	 */
-	if (!(card->mmc_avail_type & EXT_CSD_CARD_TYPE_HS400 &&
-	      host->ios.bus_width == MMC_BUS_WIDTH_8))
-		return 0;
-
 	if (host->caps & MMC_CAP_WAIT_WHILE_BUSY)
 		send_status = false;
 
@@ -1081,11 +1074,8 @@ static int mmc_select_hs400(struct mmc_card *card)
 			   EXT_CSD_HS_TIMING, val,
 			   card->ext_csd.generic_cmd6_time,
 			   true, send_status, true);
-	if (err) {
-		pr_err("%s: switch to high-speed from hs200 failed, err:%d\n",
-			mmc_hostname(host), err);
-		return err;
-	}
+	if (err)
+		goto out_err;
 
 	/* Set host controller to HS timing */
 	mmc_set_timing(card->host, MMC_TIMING_MMC_HS);
@@ -1096,6 +1086,35 @@ static int mmc_select_hs400(struct mmc_card *card)
 			goto out_err;
 	}
 
+	return 0;
+
+out_err:
+	pr_err("%s: switch to high-speed from hs200 failed, error %d\n",
+	       mmc_hostname(host), err);
+	return err;
+}
+
+static int mmc_select_hs400(struct mmc_card *card)
+{
+	struct mmc_host *host = card->host;
+	bool send_status = true;
+	int err = 0;
+	u8 val;
+
+	/*
+	 * HS400 mode requires 8-bit bus width
+	 */
+	if (!(card->mmc_avail_type & EXT_CSD_CARD_TYPE_HS400 &&
+	      host->ios.bus_width == MMC_BUS_WIDTH_8))
+		return 0;
+
+	if (host->caps & MMC_CAP_WAIT_WHILE_BUSY)
+		send_status = false;
+
+	err = mmc_hs200_to_hs(card);
+	if (err)
+		goto out_err;
+
 	/* Switch card to DDR */
 	err = mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
 			 EXT_CSD_BUS_WIDTH,
-- 
1.9.1

[PATCH RFC 2/3] mmc: mmc: Factor out mmc_hs400_to_hs() and __mmc_hs_to_hs200()

[Adrian Hunter]

Factor out mmc_hs400_to_hs() and __mmc_hs_to_hs200() so they can be
re-used in a later patch.

Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
---
 drivers/mmc/core/mmc.c | 40 +++++++++++++++++++++++++++++++++++++++-
 1 file changed, 39 insertions(+), 1 deletion(-)

diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
index a3fff994e6af..4f771c6088f7 100644
--- a/drivers/mmc/core/mmc.c
+++ b/drivers/mmc/core/mmc.c
@@ -1162,7 +1162,7 @@ int mmc_hs200_to_hs400(struct mmc_card *card)
 	return mmc_select_hs400(card);
 }
 
-int mmc_hs400_to_hs200(struct mmc_card *card)
+static int mmc_hs400_to_hs(struct mmc_card *card)
 {
 	struct mmc_host *host = card->host;
 	bool send_status = true;
@@ -1208,6 +1208,24 @@ int mmc_hs400_to_hs200(struct mmc_card *card)
 			goto out_err;
 	}
 
+	return 0;
+
+out_err:
+	pr_err("%s: %s failed, error %d\n", mmc_hostname(card->host),
+	       __func__, err);
+	return err;
+}
+
+static int __mmc_hs_to_hs200(struct mmc_card *card)
+{
+	struct mmc_host *host = card->host;
+	bool send_status = true;
+	int err;
+	u8 val;
+
+	if (host->caps & MMC_CAP_WAIT_WHILE_BUSY)
+		send_status = false;
+
 	/* Switch HS to HS200 */
 	val = EXT_CSD_TIMING_HS200 |
 	      card->drive_strength << EXT_CSD_DRV_STR_SHIFT;
@@ -1235,6 +1253,26 @@ out_err:
 	return err;
 }
 
+int mmc_hs400_to_hs200(struct mmc_card *card)
+{
+	int err;
+
+	err = mmc_hs400_to_hs(card);
+	if (err)
+		goto out_err;
+
+	err = __mmc_hs_to_hs200(card);
+	if (err)
+		goto out_err;
+
+	return 0;
+
+out_err:
+	pr_err("%s: %s failed, error %d\n", mmc_hostname(card->host),
+	       __func__, err);
+	return err;
+}
+
 static void mmc_select_driver_type(struct mmc_card *card)
 {
 	int card_drv_type, drive_strength, drv_type;
-- 
1.9.1

[PATCH RFC 3/3] mmc: block: Fix tuning (by avoiding it) for RPMB

[Adrian Hunter]

The RPMB partition only allows certain commands.  In particular,
the tuning command (CMD21) is not allowed -  refer JEDEC eMMC
standard v5.1 section 6.2.2 Command restrictions.

To avoid tuning for RPMB, switch to High Speed mode from HS200
or HS400 mode if re-tuning has been enabled.  And switch back
when leaving RPMB.

In the case of HS400, this uses mode transitions that get used
anyway for re-tuning, so we may expect them to work.

In the case of HS200, this assumes it is OK to switch HS200 to
High Speed mode, which is at least not contradicted by the JEDEC
standard.

No attempt is made to recover from any error on the expectation
that subsequent block request failures will cause recovery via
mmc_blk_reset().

Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
---
 drivers/mmc/card/block.c | 36 +++++++++++++++++++++++++
 drivers/mmc/core/mmc.c   | 69 ++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/mmc/card.h |  2 +-
 include/linux/mmc/core.h |  3 +++
 include/linux/mmc/host.h |  5 ++++
 5 files changed, 114 insertions(+), 1 deletion(-)

diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
index 8a0147dfed27..596edef4dd24 100644
--- a/drivers/mmc/card/block.c
+++ b/drivers/mmc/card/block.c
@@ -733,6 +733,36 @@ static const struct block_device_operations mmc_bdops = {
 #endif
 };
 
+static int mmc_blk_pre_rpmb(struct mmc_card *card, struct mmc_blk_data *md)
+{
+	int ret;
+
+	if (md->part_type != EXT_CSD_PART_CONFIG_ACC_RPMB)
+		return 0;
+
+	ret = mmc_exit_tuning_mode(card);
+	if (ret < 0)
+		return ret;
+
+	card->post_rpmb_mode = ret;
+
+	return 0;
+}
+
+static void mmc_blk_post_rpmb(struct mmc_card *card,
+			      struct mmc_blk_data *main_md)
+{
+	int ret;
+
+	if (main_md->part_curr != EXT_CSD_PART_CONFIG_ACC_RPMB)
+		return;
+
+	ret = mmc_reenter_tuning_mode(card, card->post_rpmb_mode);
+	if (ret)
+		pr_err("%s: Post RPMB, failed to reenter mode %u, error %d\n",
+		       mmc_hostname(card->host), card->post_rpmb_mode, ret);
+}
+
 static inline int mmc_blk_part_switch(struct mmc_card *card,
 				      struct mmc_blk_data *md)
 {
@@ -745,6 +775,10 @@ static inline int mmc_blk_part_switch(struct mmc_card *card,
 	if (mmc_card_mmc(card)) {
 		u8 part_config = card->ext_csd.part_config;
 
+		ret = mmc_blk_pre_rpmb(card, md);
+		if (ret)
+			return ret;
+
 		part_config &= ~EXT_CSD_PART_CONFIG_ACC_MASK;
 		part_config |= md->part_type;
 
@@ -755,6 +789,8 @@ static inline int mmc_blk_part_switch(struct mmc_card *card,
 			return ret;
 
 		card->ext_csd.part_config = part_config;
+
+		mmc_blk_post_rpmb(card, main_md);
 	}
 
 	main_md->part_curr = md->part_type;
diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
index 4f771c6088f7..37aa0baf4a76 100644
--- a/drivers/mmc/core/mmc.c
+++ b/drivers/mmc/core/mmc.c
@@ -1408,6 +1408,75 @@ static int mmc_hs200_tuning(struct mmc_card *card)
 	return mmc_execute_tuning(card);
 }
 
+static int mmc_hs_to_hs200(struct mmc_card *card)
+{
+	int err;
+
+	err = __mmc_hs_to_hs200(card);
+	if (err)
+		return err;
+
+	err = mmc_hs200_tuning(card);
+	if (err)
+		return err;
+
+	return 0;
+}
+
+int mmc_exit_tuning_mode(struct mmc_card *card)
+{
+	struct mmc_host *host = card->host;
+	int err;
+
+	if (!mmc_retune_enabled(host))
+		return 0;
+
+	switch (host->ios.timing) {
+	case MMC_TIMING_MMC_HS200:
+		/*
+		 * Shouldn't need re-tuning because the frequency first gets
+		 * reduced to the High Speed frequency.
+		 */
+		mmc_retune_disable(host);
+		err = mmc_hs200_to_hs(card);
+		if (err < 0)
+			return err;
+		return MMC_TIMING_MMC_HS200;
+	case MMC_TIMING_MMC_HS400:
+		/*
+		 * Must disable re-tuning to stop it interfering with the mode
+		 * switch, which is OK because we are following the same
+		 * sequence that re-tuning follows anyway.
+		 */
+		mmc_retune_disable(host);
+		err = mmc_hs400_to_hs(card);
+		if (err < 0)
+			return err;
+		return MMC_TIMING_MMC_HS400;
+	default:
+		return 0;
+	}
+}
+EXPORT_SYMBOL(mmc_exit_tuning_mode);
+
+int mmc_reenter_tuning_mode(struct mmc_card *card, u8 mode)
+{
+	int err;
+
+	switch (mode) {
+	case MMC_TIMING_MMC_HS200:
+		return mmc_hs_to_hs200(card);
+	case MMC_TIMING_MMC_HS400:
+		err = mmc_hs_to_hs200(card);
+		if (err)
+			return err;
+		return mmc_hs200_to_hs400(card);
+	default:
+		return 0;
+	}
+}
+EXPORT_SYMBOL(mmc_reenter_tuning_mode);
+
 /*
  * Handle the detection and initialisation of a card.
  *
diff --git a/include/linux/mmc/card.h b/include/linux/mmc/card.h
index eb0151bac50c..fdf5e5341386 100644
--- a/include/linux/mmc/card.h
+++ b/include/linux/mmc/card.h
@@ -279,7 +279,7 @@ struct mmc_card {
 #define MMC_QUIRK_SEC_ERASE_TRIM_BROKEN (1<<10)	/* Skip secure for erase/trim */
 #define MMC_QUIRK_BROKEN_IRQ_POLLING	(1<<11)	/* Polling SDIO_CCCR_INTx could create a fake interrupt */
 #define MMC_QUIRK_TRIM_BROKEN	(1<<12)		/* Skip trim */
-
+	u8			post_rpmb_mode;	/* Mode to restore after RPMB */
 
 	unsigned int		erase_size;	/* erase size in sectors */
  	unsigned int		erase_shift;	/* if erase unit is power 2 */
diff --git a/include/linux/mmc/core.h b/include/linux/mmc/core.h
index b01e77de1a74..a5aeb44d3fc0 100644
--- a/include/linux/mmc/core.h
+++ b/include/linux/mmc/core.h
@@ -182,6 +182,9 @@ extern int mmc_set_blockcount(struct mmc_card *card, unsigned int blockcount,
 extern int mmc_hw_reset(struct mmc_host *host);
 extern int mmc_can_reset(struct mmc_card *card);
 
+extern int mmc_exit_tuning_mode(struct mmc_card *card);
+extern int mmc_reenter_tuning_mode(struct mmc_card *card, u8 mode);
+
 extern void mmc_set_data_timeout(struct mmc_data *, const struct mmc_card *);
 extern unsigned int mmc_align_data_size(struct mmc_card *, unsigned int);
 
diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
index 85800b48241f..6767adec8c84 100644
--- a/include/linux/mmc/host.h
+++ b/include/linux/mmc/host.h
@@ -526,4 +526,9 @@ static inline void mmc_retune_recheck(struct mmc_host *host)
 		host->retune_now = 1;
 }
 
+static inline bool mmc_retune_enabled(struct mmc_host *host)
+{
+	return host->can_retune;
+}
+
 #endif /* LINUX_MMC_HOST_H */
-- 
1.9.1

Re: [PATCH RFC 0/3] mmc: block: Fix tuning (by avoiding it) for RPMB

[Adrian Hunter]

cc a couple more people

On 21/04/16 16:28, Adrian Hunter wrote:
> Hi
> 
> The RPMB partition only allows certain commands.  In particular,
> the tuning command (CMD21) is not allowed -  refer JEDEC eMMC
> standard v5.1 section 6.2.2 Command restrictions.
> 
> That means commands will begin failing if re-tuning is needed
> while switched to the RPMB partition.
> 
> Here are 4 options to fix the problem:
> 
> 
> 1. The approach taken by this patch set:
> 
> To avoid tuning for RPMB, switch to High Speed mode from HS200
> or HS400 mode if re-tuning has been enabled.  And switch back
> when leaving RPMB.
> 
> Advantages: Directly does what needs to be done.
> 
> Disadvantages: Assumes the mode switching will work.
> 
> 
> 2. Same as 1 but disable the tuning modes by removing them from
> card->mmc_avail_type and doing a full mmc_reset().
> 
> Advantages: Simpler to program
> 
> Disadvantages: Doing a full reset is slower.  Doing a full reset
> is not an expected consequence of accessing RPMB.
> 
> 
> 3. Simply disable re-tuning and attempt to recover from any errors.
> 
> Disadvantages: Due to the interdependent nature of RPMB reads/writes
> it might not be possible to recover without returning an error to the
> RPMB user.  Also it would be difficult to test if the recovery actually
> worked in all cases.
> 
> 
> 4. Do a partiton switch as part of re-tuning.
> 
> Disadvantages: Makes re-tuning more complicated.  Would require moving
> the control of partition switching into the core.  CRC errors arising
> from the need to re-tune might break the interdependent nature of RPMB
> operations.
> 
> 
> As a final note, if a solution is found then we might be able to revert
> commit 4e93b9a6abc0 ("mmc: card: Don't access RPMB partitions for normal
> read/write").
> 
> 
> Adrian Hunter (3):
>       mmc: mmc: Factor out mmc_hs200_to_hs()
>       mmc: mmc: Factor out mmc_hs400_to_hs() and __mmc_hs_to_hs200()
>       mmc: block: Fix tuning (by avoiding it) for RPMB
> 
>  drivers/mmc/card/block.c |  36 +++++++++++
>  drivers/mmc/core/mmc.c   | 156 ++++++++++++++++++++++++++++++++++++++++++-----
>  include/linux/mmc/card.h |   2 +-
>  include/linux/mmc/core.h |   3 +
>  include/linux/mmc/host.h |   5 ++
>  5 files changed, 186 insertions(+), 16 deletions(-)
> 
> 
> Regards
> Adrian
> 

Re: [PATCH RFC 3/3] mmc: block: Fix tuning (by avoiding it) for RPMB

[Ulf Hansson]

On 21 April 2016 at 15:28, Adrian Hunter <adrian.hunter@intel.com> wrote:
> The RPMB partition only allows certain commands.  In particular,
> the tuning command (CMD21) is not allowed -  refer JEDEC eMMC
> standard v5.1 section 6.2.2 Command restrictions.
>
> To avoid tuning for RPMB, switch to High Speed mode from HS200
> or HS400 mode if re-tuning has been enabled.  And switch back
> when leaving RPMB.

I would rather just disable re-tuning during this period, instead of
changing the speed mode.
The primary reason to why, is because the latency it would introduce
to first switch to HS speed then back to HS200/400.

My concern is not the throughput as I expect read/writes request to an
RPMB partition is rather small.

Of course I realize that we need to take care when disable re-tuning.
Perhaps we can solve that by a re-try mechanism if the RPMB request
fails, and thus perform the re-tuning as part of the re-try?

Kind regards
Uffe

>
> In the case of HS400, this uses mode transitions that get used
> anyway for re-tuning, so we may expect them to work.
>
> In the case of HS200, this assumes it is OK to switch HS200 to
> High Speed mode, which is at least not contradicted by the JEDEC
> standard.
>
> No attempt is made to recover from any error on the expectation
> that subsequent block request failures will cause recovery via
> mmc_blk_reset().
>
> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
> ---
>  drivers/mmc/card/block.c | 36 +++++++++++++++++++++++++
>  drivers/mmc/core/mmc.c   | 69 ++++++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/mmc/card.h |  2 +-
>  include/linux/mmc/core.h |  3 +++
>  include/linux/mmc/host.h |  5 ++++
>  5 files changed, 114 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
> index 8a0147dfed27..596edef4dd24 100644
> --- a/drivers/mmc/card/block.c
> +++ b/drivers/mmc/card/block.c
> @@ -733,6 +733,36 @@ static const struct block_device_operations mmc_bdops = {
>  #endif
>  };
>
> +static int mmc_blk_pre_rpmb(struct mmc_card *card, struct mmc_blk_data *md)
> +{
> +       int ret;
> +
> +       if (md->part_type != EXT_CSD_PART_CONFIG_ACC_RPMB)
> +               return 0;
> +
> +       ret = mmc_exit_tuning_mode(card);
> +       if (ret < 0)
> +               return ret;
> +
> +       card->post_rpmb_mode = ret;
> +
> +       return 0;
> +}
> +
> +static void mmc_blk_post_rpmb(struct mmc_card *card,
> +                             struct mmc_blk_data *main_md)
> +{
> +       int ret;
> +
> +       if (main_md->part_curr != EXT_CSD_PART_CONFIG_ACC_RPMB)
> +               return;
> +
> +       ret = mmc_reenter_tuning_mode(card, card->post_rpmb_mode);
> +       if (ret)
> +               pr_err("%s: Post RPMB, failed to reenter mode %u, error %d\n",
> +                      mmc_hostname(card->host), card->post_rpmb_mode, ret);
> +}
> +
>  static inline int mmc_blk_part_switch(struct mmc_card *card,
>                                       struct mmc_blk_data *md)
>  {
> @@ -745,6 +775,10 @@ static inline int mmc_blk_part_switch(struct mmc_card *card,
>         if (mmc_card_mmc(card)) {
>                 u8 part_config = card->ext_csd.part_config;
>
> +               ret = mmc_blk_pre_rpmb(card, md);
> +               if (ret)
> +                       return ret;
> +
>                 part_config &= ~EXT_CSD_PART_CONFIG_ACC_MASK;
>                 part_config |= md->part_type;
>
> @@ -755,6 +789,8 @@ static inline int mmc_blk_part_switch(struct mmc_card *card,
>                         return ret;
>
>                 card->ext_csd.part_config = part_config;
> +
> +               mmc_blk_post_rpmb(card, main_md);
>         }
>
>         main_md->part_curr = md->part_type;
> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
> index 4f771c6088f7..37aa0baf4a76 100644
> --- a/drivers/mmc/core/mmc.c
> +++ b/drivers/mmc/core/mmc.c
> @@ -1408,6 +1408,75 @@ static int mmc_hs200_tuning(struct mmc_card *card)
>         return mmc_execute_tuning(card);
>  }
>
> +static int mmc_hs_to_hs200(struct mmc_card *card)
> +{
> +       int err;
> +
> +       err = __mmc_hs_to_hs200(card);
> +       if (err)
> +               return err;
> +
> +       err = mmc_hs200_tuning(card);
> +       if (err)
> +               return err;
> +
> +       return 0;
> +}
> +
> +int mmc_exit_tuning_mode(struct mmc_card *card)
> +{
> +       struct mmc_host *host = card->host;
> +       int err;
> +
> +       if (!mmc_retune_enabled(host))
> +               return 0;
> +
> +       switch (host->ios.timing) {
> +       case MMC_TIMING_MMC_HS200:
> +               /*
> +                * Shouldn't need re-tuning because the frequency first gets
> +                * reduced to the High Speed frequency.
> +                */
> +               mmc_retune_disable(host);
> +               err = mmc_hs200_to_hs(card);
> +               if (err < 0)
> +                       return err;
> +               return MMC_TIMING_MMC_HS200;
> +       case MMC_TIMING_MMC_HS400:
> +               /*
> +                * Must disable re-tuning to stop it interfering with the mode
> +                * switch, which is OK because we are following the same
> +                * sequence that re-tuning follows anyway.
> +                */
> +               mmc_retune_disable(host);
> +               err = mmc_hs400_to_hs(card);
> +               if (err < 0)
> +                       return err;
> +               return MMC_TIMING_MMC_HS400;
> +       default:
> +               return 0;
> +       }
> +}
> +EXPORT_SYMBOL(mmc_exit_tuning_mode);
> +
> +int mmc_reenter_tuning_mode(struct mmc_card *card, u8 mode)
> +{
> +       int err;
> +
> +       switch (mode) {
> +       case MMC_TIMING_MMC_HS200:
> +               return mmc_hs_to_hs200(card);
> +       case MMC_TIMING_MMC_HS400:
> +               err = mmc_hs_to_hs200(card);
> +               if (err)
> +                       return err;
> +               return mmc_hs200_to_hs400(card);
> +       default:
> +               return 0;
> +       }
> +}
> +EXPORT_SYMBOL(mmc_reenter_tuning_mode);
> +
>  /*
>   * Handle the detection and initialisation of a card.
>   *
> diff --git a/include/linux/mmc/card.h b/include/linux/mmc/card.h
> index eb0151bac50c..fdf5e5341386 100644
> --- a/include/linux/mmc/card.h
> +++ b/include/linux/mmc/card.h
> @@ -279,7 +279,7 @@ struct mmc_card {
>  #define MMC_QUIRK_SEC_ERASE_TRIM_BROKEN (1<<10)        /* Skip secure for erase/trim */
>  #define MMC_QUIRK_BROKEN_IRQ_POLLING   (1<<11) /* Polling SDIO_CCCR_INTx could create a fake interrupt */
>  #define MMC_QUIRK_TRIM_BROKEN  (1<<12)         /* Skip trim */
> -
> +       u8                      post_rpmb_mode; /* Mode to restore after RPMB */
>
>         unsigned int            erase_size;     /* erase size in sectors */
>         unsigned int            erase_shift;    /* if erase unit is power 2 */
> diff --git a/include/linux/mmc/core.h b/include/linux/mmc/core.h
> index b01e77de1a74..a5aeb44d3fc0 100644
> --- a/include/linux/mmc/core.h
> +++ b/include/linux/mmc/core.h
> @@ -182,6 +182,9 @@ extern int mmc_set_blockcount(struct mmc_card *card, unsigned int blockcount,
>  extern int mmc_hw_reset(struct mmc_host *host);
>  extern int mmc_can_reset(struct mmc_card *card);
>
> +extern int mmc_exit_tuning_mode(struct mmc_card *card);
> +extern int mmc_reenter_tuning_mode(struct mmc_card *card, u8 mode);
> +
>  extern void mmc_set_data_timeout(struct mmc_data *, const struct mmc_card *);
>  extern unsigned int mmc_align_data_size(struct mmc_card *, unsigned int);
>
> diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
> index 85800b48241f..6767adec8c84 100644
> --- a/include/linux/mmc/host.h
> +++ b/include/linux/mmc/host.h
> @@ -526,4 +526,9 @@ static inline void mmc_retune_recheck(struct mmc_host *host)
>                 host->retune_now = 1;
>  }
>
> +static inline bool mmc_retune_enabled(struct mmc_host *host)
> +{
> +       return host->can_retune;
> +}
> +
>  #endif /* LINUX_MMC_HOST_H */
> --
> 1.9.1
>

Re: [PATCH RFC 3/3] mmc: block: Fix tuning (by avoiding it) for RPMB

[Adrian Hunter]

On 28/04/16 13:34, Ulf Hansson wrote:
> On 21 April 2016 at 15:28, Adrian Hunter <adrian.hunter@intel.com> wrote:
>> The RPMB partition only allows certain commands.  In particular,
>> the tuning command (CMD21) is not allowed -  refer JEDEC eMMC
>> standard v5.1 section 6.2.2 Command restrictions.
>>
>> To avoid tuning for RPMB, switch to High Speed mode from HS200
>> or HS400 mode if re-tuning has been enabled.  And switch back
>> when leaving RPMB.
> 
> I would rather just disable re-tuning during this period, instead of
> changing the speed mode.
> The primary reason to why, is because the latency it would introduce
> to first switch to HS speed then back to HS200/400.

I wouldn't expect RPMB accesses to be frequent enough for the latency to matter.

> 
> My concern is not the throughput as I expect read/writes request to an
> RPMB partition is rather small.
> 
> Of course I realize that we need to take care when disable re-tuning.
> Perhaps we can solve that by a re-try mechanism if the RPMB request
> fails, and thus perform the re-tuning as part of the re-try?

The interdependent nature of RPMB commands suggests that re-trying is not
possible.  It seems to me that you would have to make up a new set of
commands and start again. i.e. return an error to the user so that they can
start again.

Another dependency is that we always need to re-tune after host runtime
suspend, which is why we always hit this problem when RPMB is accessed.  So
to avoid errors you would either need to disable runtime PM when the RPMB
partition is selected (which might be a long time if we don't get an access
to another partition), or always switch back to the main partition (not sure
if that would mess up the RPMB command sequence though).

> 
> Kind regards
> Uffe
> 
>>
>> In the case of HS400, this uses mode transitions that get used
>> anyway for re-tuning, so we may expect them to work.
>>
>> In the case of HS200, this assumes it is OK to switch HS200 to
>> High Speed mode, which is at least not contradicted by the JEDEC
>> standard.
>>
>> No attempt is made to recover from any error on the expectation
>> that subsequent block request failures will cause recovery via
>> mmc_blk_reset().
>>
>> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
>> ---
>>  drivers/mmc/card/block.c | 36 +++++++++++++++++++++++++
>>  drivers/mmc/core/mmc.c   | 69 ++++++++++++++++++++++++++++++++++++++++++++++++
>>  include/linux/mmc/card.h |  2 +-
>>  include/linux/mmc/core.h |  3 +++
>>  include/linux/mmc/host.h |  5 ++++
>>  5 files changed, 114 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
>> index 8a0147dfed27..596edef4dd24 100644
>> --- a/drivers/mmc/card/block.c
>> +++ b/drivers/mmc/card/block.c
>> @@ -733,6 +733,36 @@ static const struct block_device_operations mmc_bdops = {
>>  #endif
>>  };
>>
>> +static int mmc_blk_pre_rpmb(struct mmc_card *card, struct mmc_blk_data *md)
>> +{
>> +       int ret;
>> +
>> +       if (md->part_type != EXT_CSD_PART_CONFIG_ACC_RPMB)
>> +               return 0;
>> +
>> +       ret = mmc_exit_tuning_mode(card);
>> +       if (ret < 0)
>> +               return ret;
>> +
>> +       card->post_rpmb_mode = ret;
>> +
>> +       return 0;
>> +}
>> +
>> +static void mmc_blk_post_rpmb(struct mmc_card *card,
>> +                             struct mmc_blk_data *main_md)
>> +{
>> +       int ret;
>> +
>> +       if (main_md->part_curr != EXT_CSD_PART_CONFIG_ACC_RPMB)
>> +               return;
>> +
>> +       ret = mmc_reenter_tuning_mode(card, card->post_rpmb_mode);
>> +       if (ret)
>> +               pr_err("%s: Post RPMB, failed to reenter mode %u, error %d\n",
>> +                      mmc_hostname(card->host), card->post_rpmb_mode, ret);
>> +}
>> +
>>  static inline int mmc_blk_part_switch(struct mmc_card *card,
>>                                       struct mmc_blk_data *md)
>>  {
>> @@ -745,6 +775,10 @@ static inline int mmc_blk_part_switch(struct mmc_card *card,
>>         if (mmc_card_mmc(card)) {
>>                 u8 part_config = card->ext_csd.part_config;
>>
>> +               ret = mmc_blk_pre_rpmb(card, md);
>> +               if (ret)
>> +                       return ret;
>> +
>>                 part_config &= ~EXT_CSD_PART_CONFIG_ACC_MASK;
>>                 part_config |= md->part_type;
>>
>> @@ -755,6 +789,8 @@ static inline int mmc_blk_part_switch(struct mmc_card *card,
>>                         return ret;
>>
>>                 card->ext_csd.part_config = part_config;
>> +
>> +               mmc_blk_post_rpmb(card, main_md);
>>         }
>>
>>         main_md->part_curr = md->part_type;
>> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
>> index 4f771c6088f7..37aa0baf4a76 100644
>> --- a/drivers/mmc/core/mmc.c
>> +++ b/drivers/mmc/core/mmc.c
>> @@ -1408,6 +1408,75 @@ static int mmc_hs200_tuning(struct mmc_card *card)
>>         return mmc_execute_tuning(card);
>>  }
>>
>> +static int mmc_hs_to_hs200(struct mmc_card *card)
>> +{
>> +       int err;
>> +
>> +       err = __mmc_hs_to_hs200(card);
>> +       if (err)
>> +               return err;
>> +
>> +       err = mmc_hs200_tuning(card);
>> +       if (err)
>> +               return err;
>> +
>> +       return 0;
>> +}
>> +
>> +int mmc_exit_tuning_mode(struct mmc_card *card)
>> +{
>> +       struct mmc_host *host = card->host;
>> +       int err;
>> +
>> +       if (!mmc_retune_enabled(host))
>> +               return 0;
>> +
>> +       switch (host->ios.timing) {
>> +       case MMC_TIMING_MMC_HS200:
>> +               /*
>> +                * Shouldn't need re-tuning because the frequency first gets
>> +                * reduced to the High Speed frequency.
>> +                */
>> +               mmc_retune_disable(host);
>> +               err = mmc_hs200_to_hs(card);
>> +               if (err < 0)
>> +                       return err;
>> +               return MMC_TIMING_MMC_HS200;
>> +       case MMC_TIMING_MMC_HS400:
>> +               /*
>> +                * Must disable re-tuning to stop it interfering with the mode
>> +                * switch, which is OK because we are following the same
>> +                * sequence that re-tuning follows anyway.
>> +                */
>> +               mmc_retune_disable(host);
>> +               err = mmc_hs400_to_hs(card);
>> +               if (err < 0)
>> +                       return err;
>> +               return MMC_TIMING_MMC_HS400;
>> +       default:
>> +               return 0;
>> +       }
>> +}
>> +EXPORT_SYMBOL(mmc_exit_tuning_mode);
>> +
>> +int mmc_reenter_tuning_mode(struct mmc_card *card, u8 mode)
>> +{
>> +       int err;
>> +
>> +       switch (mode) {
>> +       case MMC_TIMING_MMC_HS200:
>> +               return mmc_hs_to_hs200(card);
>> +       case MMC_TIMING_MMC_HS400:
>> +               err = mmc_hs_to_hs200(card);
>> +               if (err)
>> +                       return err;
>> +               return mmc_hs200_to_hs400(card);
>> +       default:
>> +               return 0;
>> +       }
>> +}
>> +EXPORT_SYMBOL(mmc_reenter_tuning_mode);
>> +
>>  /*
>>   * Handle the detection and initialisation of a card.
>>   *
>> diff --git a/include/linux/mmc/card.h b/include/linux/mmc/card.h
>> index eb0151bac50c..fdf5e5341386 100644
>> --- a/include/linux/mmc/card.h
>> +++ b/include/linux/mmc/card.h
>> @@ -279,7 +279,7 @@ struct mmc_card {
>>  #define MMC_QUIRK_SEC_ERASE_TRIM_BROKEN (1<<10)        /* Skip secure for erase/trim */
>>  #define MMC_QUIRK_BROKEN_IRQ_POLLING   (1<<11) /* Polling SDIO_CCCR_INTx could create a fake interrupt */
>>  #define MMC_QUIRK_TRIM_BROKEN  (1<<12)         /* Skip trim */
>> -
>> +       u8                      post_rpmb_mode; /* Mode to restore after RPMB */
>>
>>         unsigned int            erase_size;     /* erase size in sectors */
>>         unsigned int            erase_shift;    /* if erase unit is power 2 */
>> diff --git a/include/linux/mmc/core.h b/include/linux/mmc/core.h
>> index b01e77de1a74..a5aeb44d3fc0 100644
>> --- a/include/linux/mmc/core.h
>> +++ b/include/linux/mmc/core.h
>> @@ -182,6 +182,9 @@ extern int mmc_set_blockcount(struct mmc_card *card, unsigned int blockcount,
>>  extern int mmc_hw_reset(struct mmc_host *host);
>>  extern int mmc_can_reset(struct mmc_card *card);
>>
>> +extern int mmc_exit_tuning_mode(struct mmc_card *card);
>> +extern int mmc_reenter_tuning_mode(struct mmc_card *card, u8 mode);
>> +
>>  extern void mmc_set_data_timeout(struct mmc_data *, const struct mmc_card *);
>>  extern unsigned int mmc_align_data_size(struct mmc_card *, unsigned int);
>>
>> diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
>> index 85800b48241f..6767adec8c84 100644
>> --- a/include/linux/mmc/host.h
>> +++ b/include/linux/mmc/host.h
>> @@ -526,4 +526,9 @@ static inline void mmc_retune_recheck(struct mmc_host *host)
>>                 host->retune_now = 1;
>>  }
>>
>> +static inline bool mmc_retune_enabled(struct mmc_host *host)
>> +{
>> +       return host->can_retune;
>> +}
>> +
>>  #endif /* LINUX_MMC_HOST_H */
>> --
>> 1.9.1
>>
> 

Re: [PATCH RFC 3/3] mmc: block: Fix tuning (by avoiding it) for RPMB

[Ulf Hansson]

On 28 April 2016 at 13:02, Adrian Hunter <adrian.hunter@intel.com> wrote:
> On 28/04/16 13:34, Ulf Hansson wrote:
>> On 21 April 2016 at 15:28, Adrian Hunter <adrian.hunter@intel.com> wrote:
>>> The RPMB partition only allows certain commands.  In particular,
>>> the tuning command (CMD21) is not allowed -  refer JEDEC eMMC
>>> standard v5.1 section 6.2.2 Command restrictions.
>>>
>>> To avoid tuning for RPMB, switch to High Speed mode from HS200
>>> or HS400 mode if re-tuning has been enabled.  And switch back
>>> when leaving RPMB.
>>
>> I would rather just disable re-tuning during this period, instead of
>> changing the speed mode.
>> The primary reason to why, is because the latency it would introduce
>> to first switch to HS speed then back to HS200/400.
>
> I wouldn't expect RPMB accesses to be frequent enough for the latency to matter.
>
>>
>> My concern is not the throughput as I expect read/writes request to an
>> RPMB partition is rather small.
>>
>> Of course I realize that we need to take care when disable re-tuning.
>> Perhaps we can solve that by a re-try mechanism if the RPMB request
>> fails, and thus perform the re-tuning as part of the re-try?
>
> The interdependent nature of RPMB commands suggests that re-trying is not
> possible.  It seems to me that you would have to make up a new set of
> commands and start again. i.e. return an error to the user so that they can
> start again.

Ok.

So perhaps returning -EAGAIN could work!?

>
> Another dependency is that we always need to re-tune after host runtime
> suspend, which is why we always hit this problem when RPMB is accessed.  So

Just to make sure I understand correctly; I would imagine you hit the
problem *only* when the RPMB partition was already selected, right?

Because that would then skip the switch command, and you will
therefore try to re-tune after the partition has already been switched
to?

> to avoid errors you would either need to disable runtime PM when the RPMB
> partition is selected (which might be a long time if we don't get an access
> to another partition), or always switch back to the main partition (not sure
> if that would mess up the RPMB command sequence though).

I wouldn't mind that we switch back to the main partition when we have
served an RPMB IOCTL request. Of course not in between every mmc
request, in case of using the MULTI IOCTL.

That would prevent the next regular mmc request on the main partition
to not have to switch partition and thus get decreased latency.

Kind regards
Uffe

Re: [PATCH RFC 3/3] mmc: block: Fix tuning (by avoiding it) for RPMB

[Adrian Hunter]

On 28/04/16 14:46, Ulf Hansson wrote:
> On 28 April 2016 at 13:02, Adrian Hunter <adrian.hunter@intel.com> wrote:
>> On 28/04/16 13:34, Ulf Hansson wrote:
>>> On 21 April 2016 at 15:28, Adrian Hunter <adrian.hunter@intel.com> wrote:
>>>> The RPMB partition only allows certain commands.  In particular,
>>>> the tuning command (CMD21) is not allowed -  refer JEDEC eMMC
>>>> standard v5.1 section 6.2.2 Command restrictions.
>>>>
>>>> To avoid tuning for RPMB, switch to High Speed mode from HS200
>>>> or HS400 mode if re-tuning has been enabled.  And switch back
>>>> when leaving RPMB.
>>>
>>> I would rather just disable re-tuning during this period, instead of
>>> changing the speed mode.
>>> The primary reason to why, is because the latency it would introduce
>>> to first switch to HS speed then back to HS200/400.
>>
>> I wouldn't expect RPMB accesses to be frequent enough for the latency to matter.
>>
>>>
>>> My concern is not the throughput as I expect read/writes request to an
>>> RPMB partition is rather small.
>>>
>>> Of course I realize that we need to take care when disable re-tuning.
>>> Perhaps we can solve that by a re-try mechanism if the RPMB request
>>> fails, and thus perform the re-tuning as part of the re-try?
>>
>> The interdependent nature of RPMB commands suggests that re-trying is not
>> possible.  It seems to me that you would have to make up a new set of
>> commands and start again. i.e. return an error to the user so that they can
>> start again.
> 
> Ok.
> 
> So perhaps returning -EAGAIN could work!?

I don't think existing code would expect that.  Doesn't seem like level of
service I would expect from the kernel.

And the concern is, that being an error path, it gets overlooked.

> 
>>
>> Another dependency is that we always need to re-tune after host runtime
>> suspend, which is why we always hit this problem when RPMB is accessed.  So
> 
> Just to make sure I understand correctly; I would imagine you hit the
> problem *only* when the RPMB partition was already selected, right?

Yes

> 
> Because that would then skip the switch command, and you will
> therefore try to re-tune after the partition has already been switched
> to?

Yes

> 
>> to avoid errors you would either need to disable runtime PM when the RPMB
>> partition is selected (which might be a long time if we don't get an access
>> to another partition), or always switch back to the main partition (not sure
>> if that would mess up the RPMB command sequence though).
> 
> I wouldn't mind that we switch back to the main partition when we have
> served an RPMB IOCTL request. Of course not in between every mmc
> request, in case of using the MULTI IOCTL.
> 
> That would prevent the next regular mmc request on the main partition
> to not have to switch partition and thus get decreased latency.

Doesn't stop us getting CRC errors because the eMMC needs tuning while in
the RPMB partition though.

Re: [PATCH RFC 3/3] mmc: block: Fix tuning (by avoiding it) for RPMB

[Ulf Hansson]

On 28 April 2016 at 15:02, Adrian Hunter <adrian.hunter@intel.com> wrote:
> On 28/04/16 14:46, Ulf Hansson wrote:
>> On 28 April 2016 at 13:02, Adrian Hunter <adrian.hunter@intel.com> wrote:
>>> On 28/04/16 13:34, Ulf Hansson wrote:
>>>> On 21 April 2016 at 15:28, Adrian Hunter <adrian.hunter@intel.com> wrote:
>>>>> The RPMB partition only allows certain commands.  In particular,
>>>>> the tuning command (CMD21) is not allowed -  refer JEDEC eMMC
>>>>> standard v5.1 section 6.2.2 Command restrictions.
>>>>>
>>>>> To avoid tuning for RPMB, switch to High Speed mode from HS200
>>>>> or HS400 mode if re-tuning has been enabled.  And switch back
>>>>> when leaving RPMB.
>>>>
>>>> I would rather just disable re-tuning during this period, instead of
>>>> changing the speed mode.
>>>> The primary reason to why, is because the latency it would introduce
>>>> to first switch to HS speed then back to HS200/400.
>>>
>>> I wouldn't expect RPMB accesses to be frequent enough for the latency to matter.
>>>
>>>>
>>>> My concern is not the throughput as I expect read/writes request to an
>>>> RPMB partition is rather small.
>>>>
>>>> Of course I realize that we need to take care when disable re-tuning.
>>>> Perhaps we can solve that by a re-try mechanism if the RPMB request
>>>> fails, and thus perform the re-tuning as part of the re-try?
>>>
>>> The interdependent nature of RPMB commands suggests that re-trying is not
>>> possible.  It seems to me that you would have to make up a new set of
>>> commands and start again. i.e. return an error to the user so that they can
>>> start again.
>>
>> Ok.
>>
>> So perhaps returning -EAGAIN could work!?
>
> I don't think existing code would expect that.  Doesn't seem like level of
> service I would expect from the kernel.
>
> And the concern is, that being an error path, it gets overlooked.

I guess you are right.

>
>>
>>>
>>> Another dependency is that we always need to re-tune after host runtime
>>> suspend, which is why we always hit this problem when RPMB is accessed.  So
>>
>> Just to make sure I understand correctly; I would imagine you hit the
>> problem *only* when the RPMB partition was already selected, right?
>
> Yes
>
>>
>> Because that would then skip the switch command, and you will
>> therefore try to re-tune after the partition has already been switched
>> to?
>
> Yes
>
>>
>>> to avoid errors you would either need to disable runtime PM when the RPMB
>>> partition is selected (which might be a long time if we don't get an access
>>> to another partition), or always switch back to the main partition (not sure
>>> if that would mess up the RPMB command sequence though).
>>
>> I wouldn't mind that we switch back to the main partition when we have
>> served an RPMB IOCTL request. Of course not in between every mmc
>> request, in case of using the MULTI IOCTL.
>>
>> That would prevent the next regular mmc request on the main partition
>> to not have to switch partition and thus get decreased latency.
>
> Doesn't stop us getting CRC errors because the eMMC needs tuning while in
> the RPMB partition though.

That's true. My idea was that we should return -EAGAIN as error code
to user space for these scenarios, but I guess it's not a good idea.

I have given your suggested approach some more thinking. Besides the
latency impact, don't you think it's rather risky to switch speed
modes back an forth?
We don't know whether cards+controllers are really able to cope with
that, even if they should?

Perhaps we could instead force a re-tune to be done before switching
to the RPMB partition and thus also before each RPMB request? That
decreases/removes the probability of getting a CRC errors because of a
needed re-tune, right?

Kind regards
Uffe

Re: [PATCH RFC 3/3] mmc: block: Fix tuning (by avoiding it) for RPMB

[Adrian Hunter]

On 02/05/16 11:24, Ulf Hansson wrote:
> On 28 April 2016 at 15:02, Adrian Hunter <adrian.hunter@intel.com> wrote:
>> On 28/04/16 14:46, Ulf Hansson wrote:
>>> On 28 April 2016 at 13:02, Adrian Hunter <adrian.hunter@intel.com> wrote:
>>>> On 28/04/16 13:34, Ulf Hansson wrote:
>>>>> On 21 April 2016 at 15:28, Adrian Hunter <adrian.hunter@intel.com> wrote:
>>>>>> The RPMB partition only allows certain commands.  In particular,
>>>>>> the tuning command (CMD21) is not allowed -  refer JEDEC eMMC
>>>>>> standard v5.1 section 6.2.2 Command restrictions.
>>>>>>
>>>>>> To avoid tuning for RPMB, switch to High Speed mode from HS200
>>>>>> or HS400 mode if re-tuning has been enabled.  And switch back
>>>>>> when leaving RPMB.
>>>>>
>>>>> I would rather just disable re-tuning during this period, instead of
>>>>> changing the speed mode.
>>>>> The primary reason to why, is because the latency it would introduce
>>>>> to first switch to HS speed then back to HS200/400.
>>>>
>>>> I wouldn't expect RPMB accesses to be frequent enough for the latency to matter.
>>>>
>>>>>
>>>>> My concern is not the throughput as I expect read/writes request to an
>>>>> RPMB partition is rather small.
>>>>>
>>>>> Of course I realize that we need to take care when disable re-tuning.
>>>>> Perhaps we can solve that by a re-try mechanism if the RPMB request
>>>>> fails, and thus perform the re-tuning as part of the re-try?
>>>>
>>>> The interdependent nature of RPMB commands suggests that re-trying is not
>>>> possible.  It seems to me that you would have to make up a new set of
>>>> commands and start again. i.e. return an error to the user so that they can
>>>> start again.
>>>
>>> Ok.
>>>
>>> So perhaps returning -EAGAIN could work!?
>>
>> I don't think existing code would expect that.  Doesn't seem like level of
>> service I would expect from the kernel.
>>
>> And the concern is, that being an error path, it gets overlooked.
> 
> I guess you are right.
> 
>>
>>>
>>>>
>>>> Another dependency is that we always need to re-tune after host runtime
>>>> suspend, which is why we always hit this problem when RPMB is accessed.  So
>>>
>>> Just to make sure I understand correctly; I would imagine you hit the
>>> problem *only* when the RPMB partition was already selected, right?
>>
>> Yes
>>
>>>
>>> Because that would then skip the switch command, and you will
>>> therefore try to re-tune after the partition has already been switched
>>> to?
>>
>> Yes
>>
>>>
>>>> to avoid errors you would either need to disable runtime PM when the RPMB
>>>> partition is selected (which might be a long time if we don't get an access
>>>> to another partition), or always switch back to the main partition (not sure
>>>> if that would mess up the RPMB command sequence though).
>>>
>>> I wouldn't mind that we switch back to the main partition when we have
>>> served an RPMB IOCTL request. Of course not in between every mmc
>>> request, in case of using the MULTI IOCTL.
>>>
>>> That would prevent the next regular mmc request on the main partition
>>> to not have to switch partition and thus get decreased latency.
>>
>> Doesn't stop us getting CRC errors because the eMMC needs tuning while in
>> the RPMB partition though.
> 
> That's true. My idea was that we should return -EAGAIN as error code
> to user space for these scenarios, but I guess it's not a good idea.
> 
> I have given your suggested approach some more thinking. Besides the
> latency impact, don't you think it's rather risky to switch speed
> modes back an forth?
> We don't know whether cards+controllers are really able to cope with
> that, even if they should?

Yes there is some risk, although that is what we already have to do for
re-tuning in the HS400 case.  Also I would expect it to fail straight away
if it was going to i.e. people testing their systems would know.  Given that
we don't have a solution at all at the moment, that seemed acceptable.

> 
> Perhaps we could instead force a re-tune to be done before switching
> to the RPMB partition and thus also before each RPMB request? That
> decreases/removes the probability of getting a CRC errors because of a
> needed re-tune, right?

Yes re-tuning beforehand should work.  I would suggest switching straight
back afterwards as well to avoid the possibility of going out of tune while
still switched to RPMB.

I am happy to implement that, if you agree.

Re: [PATCH RFC 3/3] mmc: block: Fix tuning (by avoiding it) for RPMB

[Ulf Hansson]

On 2 May 2016 at 11:31, Adrian Hunter <adrian.hunter@intel.com> wrote:
> On 02/05/16 11:24, Ulf Hansson wrote:
>> On 28 April 2016 at 15:02, Adrian Hunter <adrian.hunter@intel.com> wrote:
>>> On 28/04/16 14:46, Ulf Hansson wrote:
>>>> On 28 April 2016 at 13:02, Adrian Hunter <adrian.hunter@intel.com> wrote:
>>>>> On 28/04/16 13:34, Ulf Hansson wrote:
>>>>>> On 21 April 2016 at 15:28, Adrian Hunter <adrian.hunter@intel.com> wrote:
>>>>>>> The RPMB partition only allows certain commands.  In particular,
>>>>>>> the tuning command (CMD21) is not allowed -  refer JEDEC eMMC
>>>>>>> standard v5.1 section 6.2.2 Command restrictions.
>>>>>>>
>>>>>>> To avoid tuning for RPMB, switch to High Speed mode from HS200
>>>>>>> or HS400 mode if re-tuning has been enabled.  And switch back
>>>>>>> when leaving RPMB.
>>>>>>
>>>>>> I would rather just disable re-tuning during this period, instead of
>>>>>> changing the speed mode.
>>>>>> The primary reason to why, is because the latency it would introduce
>>>>>> to first switch to HS speed then back to HS200/400.
>>>>>
>>>>> I wouldn't expect RPMB accesses to be frequent enough for the latency to matter.
>>>>>
>>>>>>
>>>>>> My concern is not the throughput as I expect read/writes request to an
>>>>>> RPMB partition is rather small.
>>>>>>
>>>>>> Of course I realize that we need to take care when disable re-tuning.
>>>>>> Perhaps we can solve that by a re-try mechanism if the RPMB request
>>>>>> fails, and thus perform the re-tuning as part of the re-try?
>>>>>
>>>>> The interdependent nature of RPMB commands suggests that re-trying is not
>>>>> possible.  It seems to me that you would have to make up a new set of
>>>>> commands and start again. i.e. return an error to the user so that they can
>>>>> start again.
>>>>
>>>> Ok.
>>>>
>>>> So perhaps returning -EAGAIN could work!?
>>>
>>> I don't think existing code would expect that.  Doesn't seem like level of
>>> service I would expect from the kernel.
>>>
>>> And the concern is, that being an error path, it gets overlooked.
>>
>> I guess you are right.
>>
>>>
>>>>
>>>>>
>>>>> Another dependency is that we always need to re-tune after host runtime
>>>>> suspend, which is why we always hit this problem when RPMB is accessed.  So
>>>>
>>>> Just to make sure I understand correctly; I would imagine you hit the
>>>> problem *only* when the RPMB partition was already selected, right?
>>>
>>> Yes
>>>
>>>>
>>>> Because that would then skip the switch command, and you will
>>>> therefore try to re-tune after the partition has already been switched
>>>> to?
>>>
>>> Yes
>>>
>>>>
>>>>> to avoid errors you would either need to disable runtime PM when the RPMB
>>>>> partition is selected (which might be a long time if we don't get an access
>>>>> to another partition), or always switch back to the main partition (not sure
>>>>> if that would mess up the RPMB command sequence though).
>>>>
>>>> I wouldn't mind that we switch back to the main partition when we have
>>>> served an RPMB IOCTL request. Of course not in between every mmc
>>>> request, in case of using the MULTI IOCTL.
>>>>
>>>> That would prevent the next regular mmc request on the main partition
>>>> to not have to switch partition and thus get decreased latency.
>>>
>>> Doesn't stop us getting CRC errors because the eMMC needs tuning while in
>>> the RPMB partition though.
>>
>> That's true. My idea was that we should return -EAGAIN as error code
>> to user space for these scenarios, but I guess it's not a good idea.
>>
>> I have given your suggested approach some more thinking. Besides the
>> latency impact, don't you think it's rather risky to switch speed
>> modes back an forth?
>> We don't know whether cards+controllers are really able to cope with
>> that, even if they should?
>
> Yes there is some risk, although that is what we already have to do for
> re-tuning in the HS400 case.  Also I would expect it to fail straight away
> if it was going to i.e. people testing their systems would know.  Given that
> we don't have a solution at all at the moment, that seemed acceptable.
>
>>
>> Perhaps we could instead force a re-tune to be done before switching
>> to the RPMB partition and thus also before each RPMB request? That
>> decreases/removes the probability of getting a CRC errors because of a
>> needed re-tune, right?
>
> Yes re-tuning beforehand should work.  I would suggest switching straight
> back afterwards as well to avoid the possibility of going out of tune while
> still switched to RPMB.

Yes, that's what I meant as well.

>
> I am happy to implement that, if you agree.
>

Okay, so let's try this!

Kind regards
Uffe

RE: [PATCH RFC 0/3] mmc: block: Fix tuning (by avoiding it) for RPMB

[Winkler, Tomas]

> 
> cc a couple more people
> 
> On 21/04/16 16:28, Adrian Hunter wrote:
> > Hi
> >
> > The RPMB partition only allows certain commands.  In particular, the
> > tuning command (CMD21) is not allowed -  refer JEDEC eMMC standard
> > v5.1 section 6.2.2 Command restrictions.
> >
> > That means commands will begin failing if re-tuning is needed while
> > switched to the RPMB partition.
> >
> > Here are 4 options to fix the problem:
> >
> >
> > 1. The approach taken by this patch set:
> >
> > To avoid tuning for RPMB, switch to High Speed mode from HS200 or
> > HS400 mode if re-tuning has been enabled.  And switch back when
> > leaving RPMB.
> >
> > Advantages: Directly does what needs to be done.
> >
> > Disadvantages: Assumes the mode switching will work.
> >
> >
> > 2. Same as 1 but disable the tuning modes by removing them from
> > card->mmc_avail_type and doing a full mmc_reset().
> >
> > Advantages: Simpler to program
> >
> > Disadvantages: Doing a full reset is slower.  Doing a full reset is
> > not an expected consequence of accessing RPMB.
> >
> >
> > 3. Simply disable re-tuning and attempt to recover from any errors.
> >
> > Disadvantages: Due to the interdependent nature of RPMB reads/writes
> > it might not be possible to recover without returning an error to the
> > RPMB user.  Also it would be difficult to test if the recovery
> > actually worked in all cases.
> >
> >
> > 4. Do a partiton switch as part of re-tuning.
> >
> > Disadvantages: Makes re-tuning more complicated.  Would require moving
> > the control of partition switching into the core.  CRC errors arising
> > from the need to re-tune might break the interdependent nature of RPMB
> > operations.
> >
> >
> > As a final note, if a solution is found then we might be able to
> > revert commit 4e93b9a6abc0 ("mmc: card: Don't access RPMB partitions
> > for normal read/write").
> >
This still doesn't bring us to that point, mostly for writing as each transaction has to have  a valid MAC computed. 
There is another layer required I hope matching the RPMB sybsystem as we suggested. 
I would prefer it won't be attached to block layer as is, yet we already have legacy SW stack so I see it painfull. 

Thanks
Tomas