[PATCH 0/2] mmc: sdhci: Fix for sdhci suspend function

[PATCH 0/2] mmc: sdhci: Fix for sdhci suspend function

[Aaron Lu]

Fixed a bug in sdhci_suspend_host, the tuning timer should be
deactivated instead of reactivated when suspending.

Handle failure case in sdhci_suspend_host, so that calling function
does not need to take care of the error recovery.

mmc: sdhci: Fix tuning timer incorrect setting when suspending host
mmc: sdhci: Deal with failure case in sdhci_suspend_host

 drivers/mmc/host/sdhci-pci.c |    9 ++-------
 drivers/mmc/host/sdhci.c     |   30 ++++++++++++++++++++++--------
 2 files changed, 24 insertions(+), 15 deletions(-)

-- 
1.7.7.4

[PATCH 1/2] mmc: sdhci: Fix tuning timer incorrect setting when suspending host

[Aaron Lu]

When suspending host, the tuning timer shoule be deactivated.
And the HOST_NEEDS_TUNING flag should be set after tuning timer is
deactivated.

Signed-off-by: Philip Rakity <prakity@marvell.com>
Signed-off-by: Aaron Lu <aaron.lu@amd.com>
Cc: stable@kernel.org
---
 drivers/mmc/host/sdhci.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index ab6018f..2007d37 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -2346,9 +2346,8 @@ int sdhci_suspend_host(struct sdhci_host *host)
 	/* Disable tuning since we are suspending */
 	if (host->version >= SDHCI_SPEC_300 && host->tuning_count &&
 	    host->tuning_mode == SDHCI_TUNING_MODE_1) {
+		del_timer_sync(&host->tuning_timer);
 		host->flags &= ~SDHCI_NEEDS_RETUNING;
-		mod_timer(&host->tuning_timer, jiffies +
-			host->tuning_count * HZ);
 	}
 
 	ret = mmc_suspend_host(host->mmc);
-- 
1.7.7.4

[PATCH 2/2] mmc: sdhci: Deal with failure case in sdhci_suspend_host

[Aaron Lu]

If there are errors happened in sdhci_suspend_host, handle it so that
when the function returns with error, the host's behaviour is the same
before this function call, e.g. card detection is enabled and tuning
timer is active, etc.

Previously, sdhci_suspend_host will return the error code and rely on
the calling function to handle it. sdhci-pci will handle it in
sdhci_pci_suspend while sdhci-platform code will not.

Signed-off-by: Philip Rakity <prakity@marvell.com>
Signed-off-by: Aaron Lu <aaron.lu@amd.com>
---
 drivers/mmc/host/sdhci-pci.c |    9 ++-------
 drivers/mmc/host/sdhci.c     |   27 +++++++++++++++++++++------
 2 files changed, 23 insertions(+), 13 deletions(-)

diff --git a/drivers/mmc/host/sdhci-pci.c b/drivers/mmc/host/sdhci-pci.c
index d2e77fb..494d14b 100644
--- a/drivers/mmc/host/sdhci-pci.c
+++ b/drivers/mmc/host/sdhci-pci.c
@@ -1013,7 +1013,7 @@ static int sdhci_pci_suspend(struct device *dev)
 		ret = sdhci_suspend_host(slot->host);
 
 		if (ret)
-			goto err_pci_suspend;
+			return ret;
 
 		slot_pm_flags = slot->host->mmc->pm_flags;
 		if (slot_pm_flags & MMC_PM_WAKE_SDIO_IRQ)
@@ -1025,7 +1025,7 @@ static int sdhci_pci_suspend(struct device *dev)
 	if (chip->fixes && chip->fixes->suspend) {
 		ret = chip->fixes->suspend(chip);
 		if (ret)
-			goto err_pci_suspend;
+			return ret;
 	}
 
 	pci_save_state(pdev);
@@ -1042,11 +1042,6 @@ static int sdhci_pci_suspend(struct device *dev)
 	}
 
 	return 0;
-
-err_pci_suspend:
-	while (--i >= 0)
-		sdhci_resume_host(chip->slots[i]->host);
-	return ret;
 }
 
 static int sdhci_pci_resume(struct device *dev)
diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 2007d37..37aeb81 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -2339,25 +2339,40 @@ out:
 
 int sdhci_suspend_host(struct sdhci_host *host)
 {
-	int ret;
+	int ret, has_tuning_timer;
 
 	sdhci_disable_card_detection(host);
 
 	/* Disable tuning since we are suspending */
-	if (host->version >= SDHCI_SPEC_300 && host->tuning_count &&
-	    host->tuning_mode == SDHCI_TUNING_MODE_1) {
+	has_tuning_timer = host->version >= SDHCI_SPEC_300 &&
+		host->tuning_count && host->tuning_mode == SDHCI_TUNING_MODE_1;
+	if (has_tuning_timer) {
 		del_timer_sync(&host->tuning_timer);
 		host->flags &= ~SDHCI_NEEDS_RETUNING;
 	}
 
 	ret = mmc_suspend_host(host->mmc);
 	if (ret)
-		return ret;
+		goto err_suspend;
+
+	if (host->vmmc) {
+		ret = regulator_disable(host->vmmc);
+		if (ret)
+			goto err_suspend;
+	}
 
 	free_irq(host->irq, host);
 
-	if (host->vmmc)
-		ret = regulator_disable(host->vmmc);
+	return 0;
+
+err_suspend:
+	if (has_tuning_timer) {
+		host->flags |= SDHCI_NEEDS_RETUNING;
+		mod_timer(&host->tuning_timer, jiffies +
+				host->tuning_count * HZ);
+	}
+
+	sdhci_enable_card_detection(host);
 
 	return ret;
 }
-- 
1.7.7.4

Re: [PATCH 2/2] mmc: sdhci: Deal with failure case in sdhci_suspend_host

[Adrian Hunter]

On 28/12/11 05:11, Aaron Lu wrote:
> If there are errors happened in sdhci_suspend_host, handle it so that
> when the function returns with error, the host's behaviour is the same
> before this function call, e.g. card detection is enabled and tuning
> timer is active, etc.
> 
> Previously, sdhci_suspend_host will return the error code and rely on
> the calling function to handle it. sdhci-pci will handle it in
> sdhci_pci_suspend while sdhci-platform code will not.
> 
> Signed-off-by: Philip Rakity <prakity@marvell.com>
> Signed-off-by: Aaron Lu <aaron.lu@amd.com>
> ---
>  drivers/mmc/host/sdhci-pci.c |    9 ++-------
>  drivers/mmc/host/sdhci.c     |   27 +++++++++++++++++++++------
>  2 files changed, 23 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/mmc/host/sdhci-pci.c b/drivers/mmc/host/sdhci-pci.c
> index d2e77fb..494d14b 100644
> --- a/drivers/mmc/host/sdhci-pci.c
> +++ b/drivers/mmc/host/sdhci-pci.c
> @@ -1013,7 +1013,7 @@ static int sdhci_pci_suspend(struct device *dev)
>  		ret = sdhci_suspend_host(slot->host);
>  
>  		if (ret)
> -			goto err_pci_suspend;
> +			return ret;
>  
>  		slot_pm_flags = slot->host->mmc->pm_flags;
>  		if (slot_pm_flags & MMC_PM_WAKE_SDIO_IRQ)
> @@ -1025,7 +1025,7 @@ static int sdhci_pci_suspend(struct device *dev)
>  	if (chip->fixes && chip->fixes->suspend) {
>  		ret = chip->fixes->suspend(chip);
>  		if (ret)
> -			goto err_pci_suspend;
> +			return ret;
>  	}
>  
>  	pci_save_state(pdev);
> @@ -1042,11 +1042,6 @@ static int sdhci_pci_suspend(struct device *dev)
>  	}
>  
>  	return 0;
> -
> -err_pci_suspend:
> -	while (--i >= 0)
> -		sdhci_resume_host(chip->slots[i]->host);
> -	return ret;

This doesn't look right.  This is about having multiple
host controllers on the same PCI device.  If those
hosts have been successfully suspended, then they must
be resumed on error.


>  }
>  
>  static int sdhci_pci_resume(struct device *dev)
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index 2007d37..37aeb81 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -2339,25 +2339,40 @@ out:
>  
>  int sdhci_suspend_host(struct sdhci_host *host)
>  {
> -	int ret;
> +	int ret, has_tuning_timer;
>  
>  	sdhci_disable_card_detection(host);
>  
>  	/* Disable tuning since we are suspending */
> -	if (host->version >= SDHCI_SPEC_300 && host->tuning_count &&
> -	    host->tuning_mode == SDHCI_TUNING_MODE_1) {
> +	has_tuning_timer = host->version >= SDHCI_SPEC_300 &&
> +		host->tuning_count && host->tuning_mode == SDHCI_TUNING_MODE_1;
> +	if (has_tuning_timer) {
>  		del_timer_sync(&host->tuning_timer);
>  		host->flags &= ~SDHCI_NEEDS_RETUNING;
>  	}
>  
>  	ret = mmc_suspend_host(host->mmc);
>  	if (ret)
> -		return ret;
> +		goto err_suspend;
> +
> +	if (host->vmmc) {
> +		ret = regulator_disable(host->vmmc);
> +		if (ret)
> +			goto err_suspend;
> +	}
>  
>  	free_irq(host->irq, host);
>  
> -	if (host->vmmc)
> -		ret = regulator_disable(host->vmmc);
> +	return 0;
> +
> +err_suspend:
> +	if (has_tuning_timer) {
> +		host->flags |= SDHCI_NEEDS_RETUNING;
> +		mod_timer(&host->tuning_timer, jiffies +
> +				host->tuning_count * HZ);
> +	}
> +
> +	sdhci_enable_card_detection(host);
>  
>  	return ret;
>  }

Re: [PATCH 1/2] mmc: sdhci: Fix tuning timer incorrect setting when suspending host

[Adrian Hunter]

On 28/12/11 05:11, Aaron Lu wrote:
> When suspending host, the tuning timer shoule be deactivated.
> And the HOST_NEEDS_TUNING flag should be set after tuning timer is
> deactivated.
> 
> Signed-off-by: Philip Rakity <prakity@marvell.com>
> Signed-off-by: Aaron Lu <aaron.lu@amd.com>
> Cc: stable@kernel.org

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

> ---
>  drivers/mmc/host/sdhci.c |    3 +--
>  1 files changed, 1 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index ab6018f..2007d37 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -2346,9 +2346,8 @@ int sdhci_suspend_host(struct sdhci_host *host)
>  	/* Disable tuning since we are suspending */
>  	if (host->version >= SDHCI_SPEC_300 && host->tuning_count &&
>  	    host->tuning_mode == SDHCI_TUNING_MODE_1) {
> +		del_timer_sync(&host->tuning_timer);
>  		host->flags &= ~SDHCI_NEEDS_RETUNING;
> -		mod_timer(&host->tuning_timer, jiffies +
> -			host->tuning_count * HZ);
>  	}
>  
>  	ret = mmc_suspend_host(host->mmc);

Re: [PATCH 2/2] mmc: sdhci: Deal with failure case in sdhci_suspend_host

[Aaron Lu]

On Wed, Dec 28, 2011 at 02:23:32PM +0200, Adrian Hunter wrote:
> > -
> > -err_pci_suspend:
> > -	while (--i >= 0)
> > -		sdhci_resume_host(chip->slots[i]->host);
> > -	return ret;
> 
> This doesn't look right.  This is about having multiple
> host controllers on the same PCI device.  If those
> hosts have been successfully suspended, then they must
> be resumed on error.
>

You are right, I failed to understand the code.
Will send the patch again without touching the pci recover code, thanks.

> 
> >  }
> >