All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH V2 0/3] mmc: block: Fix tuning (by avoiding it) for RPMB
@ 2016-05-04 11:38 Adrian Hunter
  2016-05-04 11:38 ` [PATCH V2 1/3] mmc: core: Add a facility to "pause" re-tuning Adrian Hunter
                   ` (4 more replies)
  0 siblings, 5 replies; 16+ messages in thread
From: Adrian Hunter @ 2016-05-04 11:38 UTC (permalink / raw)
  To: Ulf Hansson; +Cc: linux-mmc, Tomas Winkler

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.

As we discussed here:

	http://marc.info/?l=linux-mmc&m=146218765801869

this patch set takes a new approach which is:

	Re-tune before switching to the RPMB partition
	Don't allow re-tuning while switched to RPMB
	Switch back from the RPMB partition immediately

I gave it a brief test with a HS400 eMMC and using mmc utils
to access RPMB.  Despite all the partition switches and
re-tuning in between, all RPMB operations were successful.


Changes in V2:

	New approach entirely


Adrian Hunter (3):
      mmc: core: Add a facility to "pause" re-tuning
      mmc: block: Always switch back to main area after RPMB access
      mmc: block: Pause re-tuning while switched to the RPMB partition

 drivers/mmc/card/block.c | 19 ++++++++++++++++++-
 drivers/mmc/core/host.c  | 22 ++++++++++++++++++++++
 include/linux/mmc/host.h |  4 ++++
 3 files changed, 44 insertions(+), 1 deletion(-)


Regards
Adrian

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH V2 1/3] mmc: core: Add a facility to "pause" re-tuning
  2016-05-04 11:38 [PATCH V2 0/3] mmc: block: Fix tuning (by avoiding it) for RPMB Adrian Hunter
@ 2016-05-04 11:38 ` Adrian Hunter
  2016-05-10 12:24   ` Ulf Hansson
  2016-05-04 11:38 ` [PATCH V2 2/3] mmc: block: Always switch back to main area after RPMB access Adrian Hunter
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 16+ messages in thread
From: Adrian Hunter @ 2016-05-04 11:38 UTC (permalink / raw)
  To: Ulf Hansson; +Cc: linux-mmc, Tomas Winkler

Re-tuning is not possible when switched to the RPMB
partition.  However re-tuning should not be needed
if re-tuning is done immediately before switching,
a small set of operations is done, and then we
immediately switch back to the main partition.

To ensure that re-tuning can't be done for a short
while, add a facility to "pause" re-tuning.

The existing facility to hold / release re-tuning
is used but it also flags re-tuning as needed to cause
re-tuning before the next command (which will be the
switch to RPMB).

We also need to "unpause" in the recovery path, which
is catered for by adding it to mmc_retune_disable().

Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
---
 drivers/mmc/core/host.c  | 22 ++++++++++++++++++++++
 include/linux/mmc/host.h |  4 ++++
 2 files changed, 26 insertions(+)

diff --git a/drivers/mmc/core/host.c b/drivers/mmc/core/host.c
index e0a3ee16c0d3..302e5858755a 100644
--- a/drivers/mmc/core/host.c
+++ b/drivers/mmc/core/host.c
@@ -68,8 +68,30 @@ void mmc_retune_enable(struct mmc_host *host)
 			  jiffies + host->retune_period * HZ);
 }
 
+/*
+ * Pause re-tuning for a small set of operations.  The pause begins after the
+ * next command and after first doing re-tuning.
+ */
+void mmc_retune_pause(struct mmc_host *host)
+{
+	if (!host->retune_paused) {
+		host->retune_paused = 1;
+		mmc_retune_needed(host);
+		mmc_retune_hold(host);
+	}
+}
+
+void mmc_retune_unpause(struct mmc_host *host)
+{
+	if (host->retune_paused) {
+		host->retune_paused = 0;
+		mmc_retune_release(host);
+	}
+}
+
 void mmc_retune_disable(struct mmc_host *host)
 {
+	mmc_retune_unpause(host);
 	host->can_retune = 0;
 	del_timer_sync(&host->retune_timer);
 	host->retune_now = 0;
diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
index 85800b48241f..45cde8cd39f2 100644
--- a/include/linux/mmc/host.h
+++ b/include/linux/mmc/host.h
@@ -329,6 +329,7 @@ struct mmc_host {
 	unsigned int		can_retune:1;	/* re-tuning can be used */
 	unsigned int		doing_retune:1;	/* re-tuning in progress */
 	unsigned int		retune_now:1;	/* do re-tuning at next req */
+	unsigned int		retune_paused:1; /* re-tuning is temporarily disabled */
 
 	int			rescan_disable;	/* disable card detection */
 	int			rescan_entered;	/* used with nonremovable devices */
@@ -526,4 +527,7 @@ static inline void mmc_retune_recheck(struct mmc_host *host)
 		host->retune_now = 1;
 }
 
+void mmc_retune_pause(struct mmc_host *host);
+void mmc_retune_unpause(struct mmc_host *host);
+
 #endif /* LINUX_MMC_HOST_H */
-- 
1.9.1


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH V2 2/3] mmc: block: Always switch back to main area after RPMB access
  2016-05-04 11:38 [PATCH V2 0/3] mmc: block: Fix tuning (by avoiding it) for RPMB Adrian Hunter
  2016-05-04 11:38 ` [PATCH V2 1/3] mmc: core: Add a facility to "pause" re-tuning Adrian Hunter
@ 2016-05-04 11:38 ` Adrian Hunter
  2016-05-04 11:38 ` [PATCH V2 3/3] mmc: block: Pause re-tuning while switched to the RPMB partition Adrian Hunter
                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 16+ messages in thread
From: Adrian Hunter @ 2016-05-04 11:38 UTC (permalink / raw)
  To: Ulf Hansson; +Cc: linux-mmc, Tomas Winkler

In preparation to support the use of the RPMB partition with transfer
modes that might require re-tuning, always switch back to the main
area after RPMB access.

RPMB is accessible only via IOCTL so only those paths are affected.

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

diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
index 9ce679255775..a65043854458 100644
--- a/drivers/mmc/card/block.c
+++ b/drivers/mmc/card/block.c
@@ -618,6 +618,10 @@ static int mmc_blk_ioctl_cmd(struct block_device *bdev,
 
 	ioc_err = __mmc_blk_ioctl_cmd(card, md, idata);
 
+	/* Always switch back to main area after RPMB access */
+	if (md->area_type & MMC_BLK_DATA_AREA_RPMB)
+		mmc_blk_part_switch(card, dev_get_drvdata(&card->dev));
+
 	mmc_put_card(card);
 
 	err = mmc_blk_ioctl_copy_to_user(ic_ptr, idata);
@@ -685,6 +689,10 @@ static int mmc_blk_ioctl_multi_cmd(struct block_device *bdev,
 	for (i = 0; i < num_of_cmds && !ioc_err; i++)
 		ioc_err = __mmc_blk_ioctl_cmd(card, md, idata[i]);
 
+	/* Always switch back to main area after RPMB access */
+	if (md->area_type & MMC_BLK_DATA_AREA_RPMB)
+		mmc_blk_part_switch(card, dev_get_drvdata(&card->dev));
+
 	mmc_put_card(card);
 
 	/* copy to user if data and response */
-- 
1.9.1


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH V2 3/3] mmc: block: Pause re-tuning while switched to the RPMB partition
  2016-05-04 11:38 [PATCH V2 0/3] mmc: block: Fix tuning (by avoiding it) for RPMB Adrian Hunter
  2016-05-04 11:38 ` [PATCH V2 1/3] mmc: core: Add a facility to "pause" re-tuning Adrian Hunter
  2016-05-04 11:38 ` [PATCH V2 2/3] mmc: block: Always switch back to main area after RPMB access Adrian Hunter
@ 2016-05-04 11:38 ` Adrian Hunter
  2016-05-04 11:54 ` [PATCH V2 0/3] mmc: block: Fix tuning (by avoiding it) for RPMB Winkler, Tomas
  2016-05-10 10:28 ` Ulf Hansson
  4 siblings, 0 replies; 16+ messages in thread
From: Adrian Hunter @ 2016-05-04 11:38 UTC (permalink / raw)
  To: Ulf Hansson; +Cc: linux-mmc, Tomas Winkler

Re-tuning is not possible when switched to the RPMB
partition.  However re-tuning should not be needed
if re-tuning is done immediately before switching,
a small set of operations is done, and then we
immediately switch back to the main partition.

A previous patch ensured that we immediately switch
back to the main partition.

This patch uses the new facility to "pause" re-tuning
before switching to the RPMB partition, and to "unpause"
it after switching from the RPMB partition.

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

diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
index a65043854458..565009b190cd 100644
--- a/drivers/mmc/card/block.c
+++ b/drivers/mmc/card/block.c
@@ -756,16 +756,25 @@ static inline int mmc_blk_part_switch(struct mmc_card *card,
 	if (mmc_card_mmc(card)) {
 		u8 part_config = card->ext_csd.part_config;
 
+		if (md->part_type == EXT_CSD_PART_CONFIG_ACC_RPMB)
+			mmc_retune_pause(card->host);
+
 		part_config &= ~EXT_CSD_PART_CONFIG_ACC_MASK;
 		part_config |= md->part_type;
 
 		ret = mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
 				 EXT_CSD_PART_CONFIG, part_config,
 				 card->ext_csd.part_time);
-		if (ret)
+		if (ret) {
+			if (md->part_type == EXT_CSD_PART_CONFIG_ACC_RPMB)
+				mmc_retune_unpause(card->host);
 			return ret;
+		}
 
 		card->ext_csd.part_config = part_config;
+
+		if (main_md->part_curr == EXT_CSD_PART_CONFIG_ACC_RPMB)
+			mmc_retune_unpause(card->host);
 	}
 
 	main_md->part_curr = md->part_type;
-- 
1.9.1


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* RE: [PATCH V2 0/3] mmc: block: Fix tuning (by avoiding it) for RPMB
  2016-05-04 11:38 [PATCH V2 0/3] mmc: block: Fix tuning (by avoiding it) for RPMB Adrian Hunter
                   ` (2 preceding siblings ...)
  2016-05-04 11:38 ` [PATCH V2 3/3] mmc: block: Pause re-tuning while switched to the RPMB partition Adrian Hunter
@ 2016-05-04 11:54 ` Winkler, Tomas
  2016-05-10 10:28 ` Ulf Hansson
  4 siblings, 0 replies; 16+ messages in thread
From: Winkler, Tomas @ 2016-05-04 11:54 UTC (permalink / raw)
  To: Hunter, Adrian, Ulf Hansson; +Cc: linux-mmc



> -----Original Message-----
> From: Hunter, Adrian
> Sent: Wednesday, May 04, 2016 14:38
> To: Ulf Hansson <ulf.hansson@linaro.org>
> Cc: linux-mmc <linux-mmc@vger.kernel.org>; Winkler, Tomas
> <tomas.winkler@intel.com>
> Subject: [PATCH V2 0/3] mmc: block: Fix tuning (by avoiding it) for RPMB
> 
> 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.
> 
> As we discussed here:
> 
> 	http://marc.info/?l=linux-mmc&m=146218765801869
> 
> this patch set takes a new approach which is:
> 
> 	Re-tune before switching to the RPMB partition
> 	Don't allow re-tuning while switched to RPMB
> 	Switch back from the RPMB partition immediately
> 
> I gave it a brief test with a HS400 eMMC and using mmc utils to access RPMB.
> Despite all the partition switches and re-tuning in between, all RPMB
> operations were successful.
> 
> 
> Changes in V2:
> 
> 	New approach entirely
> 
> 
> Adrian Hunter (3):
>       mmc: core: Add a facility to "pause" re-tuning
>       mmc: block: Always switch back to main area after RPMB access
>       mmc: block: Pause re-tuning while switched to the RPMB partition
> 
>  drivers/mmc/card/block.c | 19 ++++++++++++++++++-
> drivers/mmc/core/host.c  | 22 ++++++++++++++++++++++
> include/linux/mmc/host.h |  4 ++++
>  3 files changed, 44 insertions(+), 1 deletion(-)
> 

The series looks good, it will be easy to adjust to my RPMB patch.
Thanks
Tomas




^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH V2 0/3] mmc: block: Fix tuning (by avoiding it) for RPMB
  2016-05-04 11:38 [PATCH V2 0/3] mmc: block: Fix tuning (by avoiding it) for RPMB Adrian Hunter
                   ` (3 preceding siblings ...)
  2016-05-04 11:54 ` [PATCH V2 0/3] mmc: block: Fix tuning (by avoiding it) for RPMB Winkler, Tomas
@ 2016-05-10 10:28 ` Ulf Hansson
  4 siblings, 0 replies; 16+ messages in thread
From: Ulf Hansson @ 2016-05-10 10:28 UTC (permalink / raw)
  To: Adrian Hunter; +Cc: linux-mmc, Tomas Winkler

On 4 May 2016 at 13:38, Adrian Hunter <adrian.hunter@intel.com> 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.
>
> As we discussed here:
>
>         http://marc.info/?l=linux-mmc&m=146218765801869
>
> this patch set takes a new approach which is:
>
>         Re-tune before switching to the RPMB partition
>         Don't allow re-tuning while switched to RPMB
>         Switch back from the RPMB partition immediately
>
> I gave it a brief test with a HS400 eMMC and using mmc utils
> to access RPMB.  Despite all the partition switches and
> re-tuning in between, all RPMB operations were successful.
>
>
> Changes in V2:
>
>         New approach entirely
>
>
> Adrian Hunter (3):
>       mmc: core: Add a facility to "pause" re-tuning
>       mmc: block: Always switch back to main area after RPMB access
>       mmc: block: Pause re-tuning while switched to the RPMB partition
>
>  drivers/mmc/card/block.c | 19 ++++++++++++++++++-
>  drivers/mmc/core/host.c  | 22 ++++++++++++++++++++++
>  include/linux/mmc/host.h |  4 ++++
>  3 files changed, 44 insertions(+), 1 deletion(-)
>
>
> Regards
> Adrian

Thanks, applied for next!

Kind regards
Uffe

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH V2 1/3] mmc: core: Add a facility to "pause" re-tuning
  2016-05-04 11:38 ` [PATCH V2 1/3] mmc: core: Add a facility to "pause" re-tuning Adrian Hunter
@ 2016-05-10 12:24   ` Ulf Hansson
  2016-05-10 13:03     ` Adrian Hunter
  0 siblings, 1 reply; 16+ messages in thread
From: Ulf Hansson @ 2016-05-10 12:24 UTC (permalink / raw)
  To: Adrian Hunter; +Cc: linux-mmc, Tomas Winkler

On 4 May 2016 at 13:38, Adrian Hunter <adrian.hunter@intel.com> wrote:
> Re-tuning is not possible when switched to the RPMB
> partition.  However re-tuning should not be needed
> if re-tuning is done immediately before switching,
> a small set of operations is done, and then we
> immediately switch back to the main partition.
>
> To ensure that re-tuning can't be done for a short
> while, add a facility to "pause" re-tuning.
>
> The existing facility to hold / release re-tuning
> is used but it also flags re-tuning as needed to cause
> re-tuning before the next command (which will be the
> switch to RPMB).
>
> We also need to "unpause" in the recovery path, which
> is catered for by adding it to mmc_retune_disable().
>
> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
> ---
>  drivers/mmc/core/host.c  | 22 ++++++++++++++++++++++
>  include/linux/mmc/host.h |  4 ++++
>  2 files changed, 26 insertions(+)
>
> diff --git a/drivers/mmc/core/host.c b/drivers/mmc/core/host.c
> index e0a3ee16c0d3..302e5858755a 100644
> --- a/drivers/mmc/core/host.c
> +++ b/drivers/mmc/core/host.c
> @@ -68,8 +68,30 @@ void mmc_retune_enable(struct mmc_host *host)
>                           jiffies + host->retune_period * HZ);
>  }
>
> +/*
> + * Pause re-tuning for a small set of operations.  The pause begins after the
> + * next command and after first doing re-tuning.
> + */
> +void mmc_retune_pause(struct mmc_host *host)
> +{
> +       if (!host->retune_paused) {
> +               host->retune_paused = 1;
> +               mmc_retune_needed(host);
> +               mmc_retune_hold(host);
> +       }
> +}
> +

When the mmc block device driver is built as a module, this doesn't
build. I will drop the series from my next branch to sort this out.

Should we export these via EXPORT_SYMBOL_GPL, or implement them as
inline functions?

This also made me think about the SDIO/WLAN driver issue, during
system PM suspend/resume, which also needed temporary to disable
re-tuning.

*If* we are going to export these, I want to make it works for the
SDIO case well...

Kind regards
Uffe

> +void mmc_retune_unpause(struct mmc_host *host)
> +{
> +       if (host->retune_paused) {
> +               host->retune_paused = 0;
> +               mmc_retune_release(host);
> +       }
> +}
> +
>  void mmc_retune_disable(struct mmc_host *host)
>  {
> +       mmc_retune_unpause(host);
>         host->can_retune = 0;
>         del_timer_sync(&host->retune_timer);
>         host->retune_now = 0;
> diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
> index 85800b48241f..45cde8cd39f2 100644
> --- a/include/linux/mmc/host.h
> +++ b/include/linux/mmc/host.h
> @@ -329,6 +329,7 @@ struct mmc_host {
>         unsigned int            can_retune:1;   /* re-tuning can be used */
>         unsigned int            doing_retune:1; /* re-tuning in progress */
>         unsigned int            retune_now:1;   /* do re-tuning at next req */
> +       unsigned int            retune_paused:1; /* re-tuning is temporarily disabled */
>
>         int                     rescan_disable; /* disable card detection */
>         int                     rescan_entered; /* used with nonremovable devices */
> @@ -526,4 +527,7 @@ static inline void mmc_retune_recheck(struct mmc_host *host)
>                 host->retune_now = 1;
>  }
>
> +void mmc_retune_pause(struct mmc_host *host);
> +void mmc_retune_unpause(struct mmc_host *host);
> +
>  #endif /* LINUX_MMC_HOST_H */
> --
> 1.9.1
>

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH V2 1/3] mmc: core: Add a facility to "pause" re-tuning
  2016-05-10 12:24   ` Ulf Hansson
@ 2016-05-10 13:03     ` Adrian Hunter
  2016-05-11  6:48       ` Ulf Hansson
  0 siblings, 1 reply; 16+ messages in thread
From: Adrian Hunter @ 2016-05-10 13:03 UTC (permalink / raw)
  To: Ulf Hansson; +Cc: linux-mmc, Tomas Winkler

On 10/05/16 15:24, Ulf Hansson wrote:
> On 4 May 2016 at 13:38, Adrian Hunter <adrian.hunter@intel.com> wrote:
>> Re-tuning is not possible when switched to the RPMB
>> partition.  However re-tuning should not be needed
>> if re-tuning is done immediately before switching,
>> a small set of operations is done, and then we
>> immediately switch back to the main partition.
>>
>> To ensure that re-tuning can't be done for a short
>> while, add a facility to "pause" re-tuning.
>>
>> The existing facility to hold / release re-tuning
>> is used but it also flags re-tuning as needed to cause
>> re-tuning before the next command (which will be the
>> switch to RPMB).
>>
>> We also need to "unpause" in the recovery path, which
>> is catered for by adding it to mmc_retune_disable().
>>
>> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
>> ---
>>  drivers/mmc/core/host.c  | 22 ++++++++++++++++++++++
>>  include/linux/mmc/host.h |  4 ++++
>>  2 files changed, 26 insertions(+)
>>
>> diff --git a/drivers/mmc/core/host.c b/drivers/mmc/core/host.c
>> index e0a3ee16c0d3..302e5858755a 100644
>> --- a/drivers/mmc/core/host.c
>> +++ b/drivers/mmc/core/host.c
>> @@ -68,8 +68,30 @@ void mmc_retune_enable(struct mmc_host *host)
>>                           jiffies + host->retune_period * HZ);
>>  }
>>
>> +/*
>> + * Pause re-tuning for a small set of operations.  The pause begins after the
>> + * next command and after first doing re-tuning.
>> + */
>> +void mmc_retune_pause(struct mmc_host *host)
>> +{
>> +       if (!host->retune_paused) {
>> +               host->retune_paused = 1;
>> +               mmc_retune_needed(host);
>> +               mmc_retune_hold(host);
>> +       }
>> +}
>> +
> 
> When the mmc block device driver is built as a module, this doesn't
> build. I will drop the series from my next branch to sort this out.

Oops. Sorry!

> Should we export these via EXPORT_SYMBOL_GPL, or implement them as
> inline functions?

They need to be exported.  I tend to go with what else is in the same file
i.e. host.c is exporting using EXPORT_SYMBOL()

> 
> This also made me think about the SDIO/WLAN driver issue, during
> system PM suspend/resume, which also needed temporary to disable
> re-tuning.
> 
> *If* we are going to export these, I want to make it works for the
> SDIO case well...

SDIO case is slightly different, and SDIO uses its own header file sdio_func.h.

> 
> Kind regards
> Uffe
> 
>> +void mmc_retune_unpause(struct mmc_host *host)
>> +{
>> +       if (host->retune_paused) {
>> +               host->retune_paused = 0;
>> +               mmc_retune_release(host);
>> +       }
>> +}
>> +
>>  void mmc_retune_disable(struct mmc_host *host)
>>  {
>> +       mmc_retune_unpause(host);
>>         host->can_retune = 0;
>>         del_timer_sync(&host->retune_timer);
>>         host->retune_now = 0;
>> diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
>> index 85800b48241f..45cde8cd39f2 100644
>> --- a/include/linux/mmc/host.h
>> +++ b/include/linux/mmc/host.h
>> @@ -329,6 +329,7 @@ struct mmc_host {
>>         unsigned int            can_retune:1;   /* re-tuning can be used */
>>         unsigned int            doing_retune:1; /* re-tuning in progress */
>>         unsigned int            retune_now:1;   /* do re-tuning at next req */
>> +       unsigned int            retune_paused:1; /* re-tuning is temporarily disabled */
>>
>>         int                     rescan_disable; /* disable card detection */
>>         int                     rescan_entered; /* used with nonremovable devices */
>> @@ -526,4 +527,7 @@ static inline void mmc_retune_recheck(struct mmc_host *host)
>>                 host->retune_now = 1;
>>  }
>>
>> +void mmc_retune_pause(struct mmc_host *host);
>> +void mmc_retune_unpause(struct mmc_host *host);
>> +
>>  #endif /* LINUX_MMC_HOST_H */
>> --
>> 1.9.1
>>
> 


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH V2 1/3] mmc: core: Add a facility to "pause" re-tuning
  2016-05-10 13:03     ` Adrian Hunter
@ 2016-05-11  6:48       ` Ulf Hansson
  2016-05-11  9:00         ` Adrian Hunter
  0 siblings, 1 reply; 16+ messages in thread
From: Ulf Hansson @ 2016-05-11  6:48 UTC (permalink / raw)
  To: Adrian Hunter; +Cc: linux-mmc, Tomas Winkler

On 10 May 2016 at 15:03, Adrian Hunter <adrian.hunter@intel.com> wrote:
> On 10/05/16 15:24, Ulf Hansson wrote:
>> On 4 May 2016 at 13:38, Adrian Hunter <adrian.hunter@intel.com> wrote:
>>> Re-tuning is not possible when switched to the RPMB
>>> partition.  However re-tuning should not be needed
>>> if re-tuning is done immediately before switching,
>>> a small set of operations is done, and then we
>>> immediately switch back to the main partition.
>>>
>>> To ensure that re-tuning can't be done for a short
>>> while, add a facility to "pause" re-tuning.
>>>
>>> The existing facility to hold / release re-tuning
>>> is used but it also flags re-tuning as needed to cause
>>> re-tuning before the next command (which will be the
>>> switch to RPMB).
>>>
>>> We also need to "unpause" in the recovery path, which
>>> is catered for by adding it to mmc_retune_disable().
>>>
>>> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
>>> ---
>>>  drivers/mmc/core/host.c  | 22 ++++++++++++++++++++++
>>>  include/linux/mmc/host.h |  4 ++++
>>>  2 files changed, 26 insertions(+)
>>>
>>> diff --git a/drivers/mmc/core/host.c b/drivers/mmc/core/host.c
>>> index e0a3ee16c0d3..302e5858755a 100644
>>> --- a/drivers/mmc/core/host.c
>>> +++ b/drivers/mmc/core/host.c
>>> @@ -68,8 +68,30 @@ void mmc_retune_enable(struct mmc_host *host)
>>>                           jiffies + host->retune_period * HZ);
>>>  }
>>>
>>> +/*
>>> + * Pause re-tuning for a small set of operations.  The pause begins after the
>>> + * next command and after first doing re-tuning.
>>> + */
>>> +void mmc_retune_pause(struct mmc_host *host)
>>> +{
>>> +       if (!host->retune_paused) {
>>> +               host->retune_paused = 1;
>>> +               mmc_retune_needed(host);
>>> +               mmc_retune_hold(host);
>>> +       }
>>> +}
>>> +
>>
>> When the mmc block device driver is built as a module, this doesn't
>> build. I will drop the series from my next branch to sort this out.
>
> Oops. Sorry!
>
>> Should we export these via EXPORT_SYMBOL_GPL, or implement them as
>> inline functions?
>
> They need to be exported.  I tend to go with what else is in the same file
> i.e. host.c is exporting using EXPORT_SYMBOL()

Yes, okay!

>
>>
>> This also made me think about the SDIO/WLAN driver issue, during
>> system PM suspend/resume, which also needed temporary to disable
>> re-tuning.
>>
>> *If* we are going to export these, I want to make it works for the
>> SDIO case well...
>
> SDIO case is slightly different, and SDIO uses its own header file sdio_func.h.

I what way is it different?

Regarding the header file, my point is that I want to keep the numbers
of exported functions to a minimum.

Do you think there is way to combine these two use cases, such only
one pair of new functions would be needed?

Kind regards
Uffe

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH V2 1/3] mmc: core: Add a facility to "pause" re-tuning
  2016-05-11  6:48       ` Ulf Hansson
@ 2016-05-11  9:00         ` Adrian Hunter
  2016-05-12  6:14           ` Adrian Hunter
  0 siblings, 1 reply; 16+ messages in thread
From: Adrian Hunter @ 2016-05-11  9:00 UTC (permalink / raw)
  To: Ulf Hansson; +Cc: linux-mmc, Tomas Winkler

On 11/05/16 09:48, Ulf Hansson wrote:
> On 10 May 2016 at 15:03, Adrian Hunter <adrian.hunter@intel.com> wrote:
>> On 10/05/16 15:24, Ulf Hansson wrote:
>>> On 4 May 2016 at 13:38, Adrian Hunter <adrian.hunter@intel.com> wrote:
>>>> Re-tuning is not possible when switched to the RPMB
>>>> partition.  However re-tuning should not be needed
>>>> if re-tuning is done immediately before switching,
>>>> a small set of operations is done, and then we
>>>> immediately switch back to the main partition.
>>>>
>>>> To ensure that re-tuning can't be done for a short
>>>> while, add a facility to "pause" re-tuning.
>>>>
>>>> The existing facility to hold / release re-tuning
>>>> is used but it also flags re-tuning as needed to cause
>>>> re-tuning before the next command (which will be the
>>>> switch to RPMB).
>>>>
>>>> We also need to "unpause" in the recovery path, which
>>>> is catered for by adding it to mmc_retune_disable().
>>>>
>>>> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
>>>> ---
>>>>  drivers/mmc/core/host.c  | 22 ++++++++++++++++++++++
>>>>  include/linux/mmc/host.h |  4 ++++
>>>>  2 files changed, 26 insertions(+)
>>>>
>>>> diff --git a/drivers/mmc/core/host.c b/drivers/mmc/core/host.c
>>>> index e0a3ee16c0d3..302e5858755a 100644
>>>> --- a/drivers/mmc/core/host.c
>>>> +++ b/drivers/mmc/core/host.c
>>>> @@ -68,8 +68,30 @@ void mmc_retune_enable(struct mmc_host *host)
>>>>                           jiffies + host->retune_period * HZ);
>>>>  }
>>>>
>>>> +/*
>>>> + * Pause re-tuning for a small set of operations.  The pause begins after the
>>>> + * next command and after first doing re-tuning.
>>>> + */
>>>> +void mmc_retune_pause(struct mmc_host *host)
>>>> +{
>>>> +       if (!host->retune_paused) {
>>>> +               host->retune_paused = 1;
>>>> +               mmc_retune_needed(host);
>>>> +               mmc_retune_hold(host);
>>>> +       }
>>>> +}
>>>> +
>>>
>>> When the mmc block device driver is built as a module, this doesn't
>>> build. I will drop the series from my next branch to sort this out.
>>
>> Oops. Sorry!
>>
>>> Should we export these via EXPORT_SYMBOL_GPL, or implement them as
>>> inline functions?
>>
>> They need to be exported.  I tend to go with what else is in the same file
>> i.e. host.c is exporting using EXPORT_SYMBOL()
> 
> Yes, okay!
> 
>>
>>>
>>> This also made me think about the SDIO/WLAN driver issue, during
>>> system PM suspend/resume, which also needed temporary to disable
>>> re-tuning.
>>>
>>> *If* we are going to export these, I want to make it works for the
>>> SDIO case well...
>>
>> SDIO case is slightly different, and SDIO uses its own header file sdio_func.h.
> 
> I what way is it different?

In the RPMB case there are 3 things to do:
	1. Do re-tuning at next command
	2. Hold re-tuning
	3. Release re-tuning

In the SDIO case there are 3 things to do:
	1. Prevent re-tuning at next command
	2. Hold re-tuning
	3. Release re-tuning

So the first thing is different.

> 
> Regarding the header file, my point is that I want to keep the numbers
> of exported functions to a minimum.
> 
> Do you think there is way to combine these two use cases, such only
> one pair of new functions would be needed?

To make them the same we would need to add a parameter to mmc_retune_pause()
i.e. something like

void mmc_retune_pause(struct mmc_host *host, bool retune_now)
{
	if (!host->retune_paused) {
		host->retune_paused = 1;
		mmc_retune_hold(host);
		if (retune_now)
			mmc_retune_needed(host);
		else
			host->retune_now = 0;
	}
}

For SDIO we would need to put the function declarations in sdio_func.h as
well as host.h.

Shall I make a V3 of these patches like that?


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH V2 1/3] mmc: core: Add a facility to "pause" re-tuning
  2016-05-11  9:00         ` Adrian Hunter
@ 2016-05-12  6:14           ` Adrian Hunter
  2016-05-12 13:20             ` Ulf Hansson
  0 siblings, 1 reply; 16+ messages in thread
From: Adrian Hunter @ 2016-05-12  6:14 UTC (permalink / raw)
  To: Ulf Hansson; +Cc: linux-mmc, Tomas Winkler

On 11/05/16 12:00, Adrian Hunter wrote:
> On 11/05/16 09:48, Ulf Hansson wrote:
>> On 10 May 2016 at 15:03, Adrian Hunter <adrian.hunter@intel.com> wrote:
>>> On 10/05/16 15:24, Ulf Hansson wrote:
>>>> On 4 May 2016 at 13:38, Adrian Hunter <adrian.hunter@intel.com> wrote:
>>>>> Re-tuning is not possible when switched to the RPMB
>>>>> partition.  However re-tuning should not be needed
>>>>> if re-tuning is done immediately before switching,
>>>>> a small set of operations is done, and then we
>>>>> immediately switch back to the main partition.
>>>>>
>>>>> To ensure that re-tuning can't be done for a short
>>>>> while, add a facility to "pause" re-tuning.
>>>>>
>>>>> The existing facility to hold / release re-tuning
>>>>> is used but it also flags re-tuning as needed to cause
>>>>> re-tuning before the next command (which will be the
>>>>> switch to RPMB).
>>>>>
>>>>> We also need to "unpause" in the recovery path, which
>>>>> is catered for by adding it to mmc_retune_disable().
>>>>>
>>>>> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
>>>>> ---
>>>>>  drivers/mmc/core/host.c  | 22 ++++++++++++++++++++++
>>>>>  include/linux/mmc/host.h |  4 ++++
>>>>>  2 files changed, 26 insertions(+)
>>>>>
>>>>> diff --git a/drivers/mmc/core/host.c b/drivers/mmc/core/host.c
>>>>> index e0a3ee16c0d3..302e5858755a 100644
>>>>> --- a/drivers/mmc/core/host.c
>>>>> +++ b/drivers/mmc/core/host.c
>>>>> @@ -68,8 +68,30 @@ void mmc_retune_enable(struct mmc_host *host)
>>>>>                           jiffies + host->retune_period * HZ);
>>>>>  }
>>>>>
>>>>> +/*
>>>>> + * Pause re-tuning for a small set of operations.  The pause begins after the
>>>>> + * next command and after first doing re-tuning.
>>>>> + */
>>>>> +void mmc_retune_pause(struct mmc_host *host)
>>>>> +{
>>>>> +       if (!host->retune_paused) {
>>>>> +               host->retune_paused = 1;
>>>>> +               mmc_retune_needed(host);
>>>>> +               mmc_retune_hold(host);
>>>>> +       }
>>>>> +}
>>>>> +
>>>>
>>>> When the mmc block device driver is built as a module, this doesn't
>>>> build. I will drop the series from my next branch to sort this out.
>>>
>>> Oops. Sorry!
>>>
>>>> Should we export these via EXPORT_SYMBOL_GPL, or implement them as
>>>> inline functions?
>>>
>>> They need to be exported.  I tend to go with what else is in the same file
>>> i.e. host.c is exporting using EXPORT_SYMBOL()
>>
>> Yes, okay!
>>
>>>
>>>>
>>>> This also made me think about the SDIO/WLAN driver issue, during
>>>> system PM suspend/resume, which also needed temporary to disable
>>>> re-tuning.
>>>>
>>>> *If* we are going to export these, I want to make it works for the
>>>> SDIO case well...
>>>
>>> SDIO case is slightly different, and SDIO uses its own header file sdio_func.h.
>>
>> I what way is it different?
> 
> In the RPMB case there are 3 things to do:
> 	1. Do re-tuning at next command
> 	2. Hold re-tuning
> 	3. Release re-tuning
> 
> In the SDIO case there are 3 things to do:
> 	1. Prevent re-tuning at next command
> 	2. Hold re-tuning
> 	3. Release re-tuning
> 
> So the first thing is different.
> 
>>
>> Regarding the header file, my point is that I want to keep the numbers
>> of exported functions to a minimum.
>>
>> Do you think there is way to combine these two use cases, such only
>> one pair of new functions would be needed?
> 
> To make them the same we would need to add a parameter to mmc_retune_pause()
> i.e. something like
> 
> void mmc_retune_pause(struct mmc_host *host, bool retune_now)
> {
> 	if (!host->retune_paused) {
> 		host->retune_paused = 1;
> 		mmc_retune_hold(host);
> 		if (retune_now)
> 			mmc_retune_needed(host);
> 		else
> 			host->retune_now = 0;
> 	}
> }
> 
> For SDIO we would need to put the function declarations in sdio_func.h as
> well as host.h.
> 
> Shall I make a V3 of these patches like that?

I looked again at sdio_func.h and it seems to have its own paradigm i.e. it
is a completely separate set of functions that take the SDIO function as a
parameter, and that hide and encapsulate core and host functions.

It would be inconsistent with that paradigm to expose mmc_retune_pause() and
mmc_retune_unpause() there.  Is that what you want to do?


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH V2 1/3] mmc: core: Add a facility to "pause" re-tuning
  2016-05-12 13:20             ` Ulf Hansson
@ 2016-05-12 13:19               ` Adrian Hunter
  2016-05-16 12:35                 ` [PATCH V3 " Adrian Hunter
  0 siblings, 1 reply; 16+ messages in thread
From: Adrian Hunter @ 2016-05-12 13:19 UTC (permalink / raw)
  To: Ulf Hansson; +Cc: linux-mmc, Tomas Winkler

On 12/05/16 16:20, Ulf Hansson wrote:
> On 12 May 2016 at 08:14, Adrian Hunter <adrian.hunter@intel.com> wrote:
>> On 11/05/16 12:00, Adrian Hunter wrote:
>>> On 11/05/16 09:48, Ulf Hansson wrote:
>>>> On 10 May 2016 at 15:03, Adrian Hunter <adrian.hunter@intel.com> wrote:
>>>>> On 10/05/16 15:24, Ulf Hansson wrote:
>>>>>> On 4 May 2016 at 13:38, Adrian Hunter <adrian.hunter@intel.com> wrote:
>>>>>>> Re-tuning is not possible when switched to the RPMB
>>>>>>> partition.  However re-tuning should not be needed
>>>>>>> if re-tuning is done immediately before switching,
>>>>>>> a small set of operations is done, and then we
>>>>>>> immediately switch back to the main partition.
>>>>>>>
>>>>>>> To ensure that re-tuning can't be done for a short
>>>>>>> while, add a facility to "pause" re-tuning.
>>>>>>>
>>>>>>> The existing facility to hold / release re-tuning
>>>>>>> is used but it also flags re-tuning as needed to cause
>>>>>>> re-tuning before the next command (which will be the
>>>>>>> switch to RPMB).
>>>>>>>
>>>>>>> We also need to "unpause" in the recovery path, which
>>>>>>> is catered for by adding it to mmc_retune_disable().
>>>>>>>
>>>>>>> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
>>>>>>> ---
>>>>>>>  drivers/mmc/core/host.c  | 22 ++++++++++++++++++++++
>>>>>>>  include/linux/mmc/host.h |  4 ++++
>>>>>>>  2 files changed, 26 insertions(+)
>>>>>>>
>>>>>>> diff --git a/drivers/mmc/core/host.c b/drivers/mmc/core/host.c
>>>>>>> index e0a3ee16c0d3..302e5858755a 100644
>>>>>>> --- a/drivers/mmc/core/host.c
>>>>>>> +++ b/drivers/mmc/core/host.c
>>>>>>> @@ -68,8 +68,30 @@ void mmc_retune_enable(struct mmc_host *host)
>>>>>>>                           jiffies + host->retune_period * HZ);
>>>>>>>  }
>>>>>>>
>>>>>>> +/*
>>>>>>> + * Pause re-tuning for a small set of operations.  The pause begins after the
>>>>>>> + * next command and after first doing re-tuning.
>>>>>>> + */
>>>>>>> +void mmc_retune_pause(struct mmc_host *host)
>>>>>>> +{
>>>>>>> +       if (!host->retune_paused) {
>>>>>>> +               host->retune_paused = 1;
>>>>>>> +               mmc_retune_needed(host);
>>>>>>> +               mmc_retune_hold(host);
>>>>>>> +       }
>>>>>>> +}
>>>>>>> +
>>>>>>
>>>>>> When the mmc block device driver is built as a module, this doesn't
>>>>>> build. I will drop the series from my next branch to sort this out.
>>>>>
>>>>> Oops. Sorry!
>>>>>
>>>>>> Should we export these via EXPORT_SYMBOL_GPL, or implement them as
>>>>>> inline functions?
>>>>>
>>>>> They need to be exported.  I tend to go with what else is in the same file
>>>>> i.e. host.c is exporting using EXPORT_SYMBOL()
>>>>
>>>> Yes, okay!
>>>>
>>>>>
>>>>>>
>>>>>> This also made me think about the SDIO/WLAN driver issue, during
>>>>>> system PM suspend/resume, which also needed temporary to disable
>>>>>> re-tuning.
>>>>>>
>>>>>> *If* we are going to export these, I want to make it works for the
>>>>>> SDIO case well...
>>>>>
>>>>> SDIO case is slightly different, and SDIO uses its own header file sdio_func.h.
>>>>
>>>> I what way is it different?
>>>
>>> In the RPMB case there are 3 things to do:
>>>       1. Do re-tuning at next command
>>>       2. Hold re-tuning
>>>       3. Release re-tuning
>>>
>>> In the SDIO case there are 3 things to do:
>>>       1. Prevent re-tuning at next command
>>>       2. Hold re-tuning
>>>       3. Release re-tuning
>>>
>>> So the first thing is different.
>>>
>>>>
>>>> Regarding the header file, my point is that I want to keep the numbers
>>>> of exported functions to a minimum.
>>>>
>>>> Do you think there is way to combine these two use cases, such only
>>>> one pair of new functions would be needed?
>>>
>>> To make them the same we would need to add a parameter to mmc_retune_pause()
>>> i.e. something like
>>>
>>> void mmc_retune_pause(struct mmc_host *host, bool retune_now)
>>> {
>>>       if (!host->retune_paused) {
>>>               host->retune_paused = 1;
>>>               mmc_retune_hold(host);
>>>               if (retune_now)
>>>                       mmc_retune_needed(host);
>>>               else
>>>                       host->retune_now = 0;
>>>       }
>>> }
>>>
>>> For SDIO we would need to put the function declarations in sdio_func.h as
>>> well as host.h.
>>>
>>> Shall I make a V3 of these patches like that?
> 
> No.
> 
>>
>> I looked again at sdio_func.h and it seems to have its own paradigm i.e. it
>> is a completely separate set of functions that take the SDIO function as a
>> parameter, and that hide and encapsulate core and host functions.
>>
>> It would be inconsistent with that paradigm to expose mmc_retune_pause() and
>> mmc_retune_unpause() there.  Is that what you want to do?
> 
> I agree, we shouldn't mess up the SDIO API with these functions.
> 
> Instead, let's keep it simple and just leave out the SDIO case for
> now. So do EXPORT_SYMBOL for those APIs you added in $subject patch,
> without further changes.
> 
> Okay?

Yes please :-)


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH V2 1/3] mmc: core: Add a facility to "pause" re-tuning
  2016-05-12  6:14           ` Adrian Hunter
@ 2016-05-12 13:20             ` Ulf Hansson
  2016-05-12 13:19               ` Adrian Hunter
  0 siblings, 1 reply; 16+ messages in thread
From: Ulf Hansson @ 2016-05-12 13:20 UTC (permalink / raw)
  To: Adrian Hunter; +Cc: linux-mmc, Tomas Winkler

On 12 May 2016 at 08:14, Adrian Hunter <adrian.hunter@intel.com> wrote:
> On 11/05/16 12:00, Adrian Hunter wrote:
>> On 11/05/16 09:48, Ulf Hansson wrote:
>>> On 10 May 2016 at 15:03, Adrian Hunter <adrian.hunter@intel.com> wrote:
>>>> On 10/05/16 15:24, Ulf Hansson wrote:
>>>>> On 4 May 2016 at 13:38, Adrian Hunter <adrian.hunter@intel.com> wrote:
>>>>>> Re-tuning is not possible when switched to the RPMB
>>>>>> partition.  However re-tuning should not be needed
>>>>>> if re-tuning is done immediately before switching,
>>>>>> a small set of operations is done, and then we
>>>>>> immediately switch back to the main partition.
>>>>>>
>>>>>> To ensure that re-tuning can't be done for a short
>>>>>> while, add a facility to "pause" re-tuning.
>>>>>>
>>>>>> The existing facility to hold / release re-tuning
>>>>>> is used but it also flags re-tuning as needed to cause
>>>>>> re-tuning before the next command (which will be the
>>>>>> switch to RPMB).
>>>>>>
>>>>>> We also need to "unpause" in the recovery path, which
>>>>>> is catered for by adding it to mmc_retune_disable().
>>>>>>
>>>>>> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
>>>>>> ---
>>>>>>  drivers/mmc/core/host.c  | 22 ++++++++++++++++++++++
>>>>>>  include/linux/mmc/host.h |  4 ++++
>>>>>>  2 files changed, 26 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/mmc/core/host.c b/drivers/mmc/core/host.c
>>>>>> index e0a3ee16c0d3..302e5858755a 100644
>>>>>> --- a/drivers/mmc/core/host.c
>>>>>> +++ b/drivers/mmc/core/host.c
>>>>>> @@ -68,8 +68,30 @@ void mmc_retune_enable(struct mmc_host *host)
>>>>>>                           jiffies + host->retune_period * HZ);
>>>>>>  }
>>>>>>
>>>>>> +/*
>>>>>> + * Pause re-tuning for a small set of operations.  The pause begins after the
>>>>>> + * next command and after first doing re-tuning.
>>>>>> + */
>>>>>> +void mmc_retune_pause(struct mmc_host *host)
>>>>>> +{
>>>>>> +       if (!host->retune_paused) {
>>>>>> +               host->retune_paused = 1;
>>>>>> +               mmc_retune_needed(host);
>>>>>> +               mmc_retune_hold(host);
>>>>>> +       }
>>>>>> +}
>>>>>> +
>>>>>
>>>>> When the mmc block device driver is built as a module, this doesn't
>>>>> build. I will drop the series from my next branch to sort this out.
>>>>
>>>> Oops. Sorry!
>>>>
>>>>> Should we export these via EXPORT_SYMBOL_GPL, or implement them as
>>>>> inline functions?
>>>>
>>>> They need to be exported.  I tend to go with what else is in the same file
>>>> i.e. host.c is exporting using EXPORT_SYMBOL()
>>>
>>> Yes, okay!
>>>
>>>>
>>>>>
>>>>> This also made me think about the SDIO/WLAN driver issue, during
>>>>> system PM suspend/resume, which also needed temporary to disable
>>>>> re-tuning.
>>>>>
>>>>> *If* we are going to export these, I want to make it works for the
>>>>> SDIO case well...
>>>>
>>>> SDIO case is slightly different, and SDIO uses its own header file sdio_func.h.
>>>
>>> I what way is it different?
>>
>> In the RPMB case there are 3 things to do:
>>       1. Do re-tuning at next command
>>       2. Hold re-tuning
>>       3. Release re-tuning
>>
>> In the SDIO case there are 3 things to do:
>>       1. Prevent re-tuning at next command
>>       2. Hold re-tuning
>>       3. Release re-tuning
>>
>> So the first thing is different.
>>
>>>
>>> Regarding the header file, my point is that I want to keep the numbers
>>> of exported functions to a minimum.
>>>
>>> Do you think there is way to combine these two use cases, such only
>>> one pair of new functions would be needed?
>>
>> To make them the same we would need to add a parameter to mmc_retune_pause()
>> i.e. something like
>>
>> void mmc_retune_pause(struct mmc_host *host, bool retune_now)
>> {
>>       if (!host->retune_paused) {
>>               host->retune_paused = 1;
>>               mmc_retune_hold(host);
>>               if (retune_now)
>>                       mmc_retune_needed(host);
>>               else
>>                       host->retune_now = 0;
>>       }
>> }
>>
>> For SDIO we would need to put the function declarations in sdio_func.h as
>> well as host.h.
>>
>> Shall I make a V3 of these patches like that?

No.

>
> I looked again at sdio_func.h and it seems to have its own paradigm i.e. it
> is a completely separate set of functions that take the SDIO function as a
> parameter, and that hide and encapsulate core and host functions.
>
> It would be inconsistent with that paradigm to expose mmc_retune_pause() and
> mmc_retune_unpause() there.  Is that what you want to do?

I agree, we shouldn't mess up the SDIO API with these functions.

Instead, let's keep it simple and just leave out the SDIO case for
now. So do EXPORT_SYMBOL for those APIs you added in $subject patch,
without further changes.

Okay?

Kind regards
Uffe

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH V3 1/3] mmc: core: Add a facility to "pause" re-tuning
  2016-05-12 13:19               ` Adrian Hunter
@ 2016-05-16 12:35                 ` Adrian Hunter
  2016-05-17 15:06                   ` Ulf Hansson
  0 siblings, 1 reply; 16+ messages in thread
From: Adrian Hunter @ 2016-05-16 12:35 UTC (permalink / raw)
  To: Ulf Hansson; +Cc: linux-mmc, Tomas Winkler

Re-tuning is not possible when switched to the RPMB
partition.  However re-tuning should not be needed
if re-tuning is done immediately before switching,
a small set of operations is done, and then we
immediately switch back to the main partition.

To ensure that re-tuning can't be done for a short
while, add a facility to "pause" re-tuning.

The existing facility to hold / release re-tuning
is used but it also flags re-tuning as needed to cause
re-tuning before the next command (which will be the
switch to RPMB).

We also need to "unpause" in the recovery path, which
is catered for by adding it to mmc_retune_disable().

Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
---


Changes in V3:

	Added:
		EXPORT_SYMBOL(mmc_retune_pause);
		EXPORT_SYMBOL(mmc_retune_unpause);

Changes in V2:

	New approach entirely


 drivers/mmc/core/host.c  | 24 ++++++++++++++++++++++++
 include/linux/mmc/host.h |  4 ++++
 2 files changed, 28 insertions(+)

diff --git a/drivers/mmc/core/host.c b/drivers/mmc/core/host.c
index e0a3ee16c0d3..1be42fab1a30 100644
--- a/drivers/mmc/core/host.c
+++ b/drivers/mmc/core/host.c
@@ -68,8 +68,32 @@ void mmc_retune_enable(struct mmc_host *host)
 			  jiffies + host->retune_period * HZ);
 }
 
+/*
+ * Pause re-tuning for a small set of operations.  The pause begins after the
+ * next command and after first doing re-tuning.
+ */
+void mmc_retune_pause(struct mmc_host *host)
+{
+	if (!host->retune_paused) {
+		host->retune_paused = 1;
+		mmc_retune_needed(host);
+		mmc_retune_hold(host);
+	}
+}
+EXPORT_SYMBOL(mmc_retune_pause);
+
+void mmc_retune_unpause(struct mmc_host *host)
+{
+	if (host->retune_paused) {
+		host->retune_paused = 0;
+		mmc_retune_release(host);
+	}
+}
+EXPORT_SYMBOL(mmc_retune_unpause);
+
 void mmc_retune_disable(struct mmc_host *host)
 {
+	mmc_retune_unpause(host);
 	host->can_retune = 0;
 	del_timer_sync(&host->retune_timer);
 	host->retune_now = 0;
diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
index 85800b48241f..45cde8cd39f2 100644
--- a/include/linux/mmc/host.h
+++ b/include/linux/mmc/host.h
@@ -329,6 +329,7 @@ struct mmc_host {
 	unsigned int		can_retune:1;	/* re-tuning can be used */
 	unsigned int		doing_retune:1;	/* re-tuning in progress */
 	unsigned int		retune_now:1;	/* do re-tuning at next req */
+	unsigned int		retune_paused:1; /* re-tuning is temporarily disabled */
 
 	int			rescan_disable;	/* disable card detection */
 	int			rescan_entered;	/* used with nonremovable devices */
@@ -526,4 +527,7 @@ static inline void mmc_retune_recheck(struct mmc_host *host)
 		host->retune_now = 1;
 }
 
+void mmc_retune_pause(struct mmc_host *host);
+void mmc_retune_unpause(struct mmc_host *host);
+
 #endif /* LINUX_MMC_HOST_H */
-- 
1.9.1



^ permalink raw reply related	[flat|nested] 16+ messages in thread

* Re: [PATCH V3 1/3] mmc: core: Add a facility to "pause" re-tuning
  2016-05-16 12:35                 ` [PATCH V3 " Adrian Hunter
@ 2016-05-17 15:06                   ` Ulf Hansson
  2016-05-18  6:44                     ` Adrian Hunter
  0 siblings, 1 reply; 16+ messages in thread
From: Ulf Hansson @ 2016-05-17 15:06 UTC (permalink / raw)
  To: Adrian Hunter; +Cc: linux-mmc, Tomas Winkler

On 16 May 2016 at 14:35, Adrian Hunter <adrian.hunter@intel.com> wrote:
> Re-tuning is not possible when switched to the RPMB
> partition.  However re-tuning should not be needed
> if re-tuning is done immediately before switching,
> a small set of operations is done, and then we
> immediately switch back to the main partition.
>
> To ensure that re-tuning can't be done for a short
> while, add a facility to "pause" re-tuning.
>
> The existing facility to hold / release re-tuning
> is used but it also flags re-tuning as needed to cause
> re-tuning before the next command (which will be the
> switch to RPMB).
>
> We also need to "unpause" in the recovery path, which
> is catered for by adding it to mmc_retune_disable().
>
> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
> ---

Thanks, re-applied the series with this v3 version of patch 1/3.

If the tests from kernelci and in linux-next works out okay, I intend
to send this as fixes for 4.7 rc1.

Kind regards
Uffe

>
>
> Changes in V3:
>
>         Added:
>                 EXPORT_SYMBOL(mmc_retune_pause);
>                 EXPORT_SYMBOL(mmc_retune_unpause);
>
> Changes in V2:
>
>         New approach entirely
>
>
>  drivers/mmc/core/host.c  | 24 ++++++++++++++++++++++++
>  include/linux/mmc/host.h |  4 ++++
>  2 files changed, 28 insertions(+)
>
> diff --git a/drivers/mmc/core/host.c b/drivers/mmc/core/host.c
> index e0a3ee16c0d3..1be42fab1a30 100644
> --- a/drivers/mmc/core/host.c
> +++ b/drivers/mmc/core/host.c
> @@ -68,8 +68,32 @@ void mmc_retune_enable(struct mmc_host *host)
>                           jiffies + host->retune_period * HZ);
>  }
>
> +/*
> + * Pause re-tuning for a small set of operations.  The pause begins after the
> + * next command and after first doing re-tuning.
> + */
> +void mmc_retune_pause(struct mmc_host *host)
> +{
> +       if (!host->retune_paused) {
> +               host->retune_paused = 1;
> +               mmc_retune_needed(host);
> +               mmc_retune_hold(host);
> +       }
> +}
> +EXPORT_SYMBOL(mmc_retune_pause);
> +
> +void mmc_retune_unpause(struct mmc_host *host)
> +{
> +       if (host->retune_paused) {
> +               host->retune_paused = 0;
> +               mmc_retune_release(host);
> +       }
> +}
> +EXPORT_SYMBOL(mmc_retune_unpause);
> +
>  void mmc_retune_disable(struct mmc_host *host)
>  {
> +       mmc_retune_unpause(host);
>         host->can_retune = 0;
>         del_timer_sync(&host->retune_timer);
>         host->retune_now = 0;
> diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
> index 85800b48241f..45cde8cd39f2 100644
> --- a/include/linux/mmc/host.h
> +++ b/include/linux/mmc/host.h
> @@ -329,6 +329,7 @@ struct mmc_host {
>         unsigned int            can_retune:1;   /* re-tuning can be used */
>         unsigned int            doing_retune:1; /* re-tuning in progress */
>         unsigned int            retune_now:1;   /* do re-tuning at next req */
> +       unsigned int            retune_paused:1; /* re-tuning is temporarily disabled */
>
>         int                     rescan_disable; /* disable card detection */
>         int                     rescan_entered; /* used with nonremovable devices */
> @@ -526,4 +527,7 @@ static inline void mmc_retune_recheck(struct mmc_host *host)
>                 host->retune_now = 1;
>  }
>
> +void mmc_retune_pause(struct mmc_host *host);
> +void mmc_retune_unpause(struct mmc_host *host);
> +
>  #endif /* LINUX_MMC_HOST_H */
> --
> 1.9.1
>
>

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH V3 1/3] mmc: core: Add a facility to "pause" re-tuning
  2016-05-17 15:06                   ` Ulf Hansson
@ 2016-05-18  6:44                     ` Adrian Hunter
  0 siblings, 0 replies; 16+ messages in thread
From: Adrian Hunter @ 2016-05-18  6:44 UTC (permalink / raw)
  To: Ulf Hansson; +Cc: linux-mmc, Tomas Winkler

On 17/05/16 18:06, Ulf Hansson wrote:
> On 16 May 2016 at 14:35, Adrian Hunter <adrian.hunter@intel.com> wrote:
>> Re-tuning is not possible when switched to the RPMB
>> partition.  However re-tuning should not be needed
>> if re-tuning is done immediately before switching,
>> a small set of operations is done, and then we
>> immediately switch back to the main partition.
>>
>> To ensure that re-tuning can't be done for a short
>> while, add a facility to "pause" re-tuning.
>>
>> The existing facility to hold / release re-tuning
>> is used but it also flags re-tuning as needed to cause
>> re-tuning before the next command (which will be the
>> switch to RPMB).
>>
>> We also need to "unpause" in the recovery path, which
>> is catered for by adding it to mmc_retune_disable().
>>
>> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
>> ---
> 
> Thanks, re-applied the series with this v3 version of patch 1/3.
> 
> If the tests from kernelci and in linux-next works out okay, I intend
> to send this as fixes for 4.7 rc1.

Thank you!


^ permalink raw reply	[flat|nested] 16+ messages in thread

end of thread, other threads:[~2016-05-18  6:48 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-05-04 11:38 [PATCH V2 0/3] mmc: block: Fix tuning (by avoiding it) for RPMB Adrian Hunter
2016-05-04 11:38 ` [PATCH V2 1/3] mmc: core: Add a facility to "pause" re-tuning Adrian Hunter
2016-05-10 12:24   ` Ulf Hansson
2016-05-10 13:03     ` Adrian Hunter
2016-05-11  6:48       ` Ulf Hansson
2016-05-11  9:00         ` Adrian Hunter
2016-05-12  6:14           ` Adrian Hunter
2016-05-12 13:20             ` Ulf Hansson
2016-05-12 13:19               ` Adrian Hunter
2016-05-16 12:35                 ` [PATCH V3 " Adrian Hunter
2016-05-17 15:06                   ` Ulf Hansson
2016-05-18  6:44                     ` Adrian Hunter
2016-05-04 11:38 ` [PATCH V2 2/3] mmc: block: Always switch back to main area after RPMB access Adrian Hunter
2016-05-04 11:38 ` [PATCH V2 3/3] mmc: block: Pause re-tuning while switched to the RPMB partition Adrian Hunter
2016-05-04 11:54 ` [PATCH V2 0/3] mmc: block: Fix tuning (by avoiding it) for RPMB Winkler, Tomas
2016-05-10 10:28 ` Ulf Hansson

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.