Patchwork [RFC,v3,04/13] ahci-platform: Undo pdata->resume on resume failure

login
register
mail settings
Submitter Hans de Goede
Date Jan. 18, 2014, 11:48 p.m.
Message ID <1390088935-7193-5-git-send-email-hdegoede@redhat.com>
Download mbox | patch
Permalink /patch/312343/
State Not Applicable
Delegated to: David Miller
Headers show

Comments

Hans de Goede - Jan. 18, 2014, 11:48 p.m.
When the ahci_resume fails the error handling code tries to undo all changes
made, but it was not undoing the results of pdata->resume.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/ata/ahci_platform.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)
Tejun Heo - Jan. 19, 2014, 11:27 a.m.
Hello,

On Sun, Jan 19, 2014 at 12:48:46AM +0100, Hans de Goede wrote:
> When the ahci_resume fails the error handling code tries to undo all changes
> made, but it was not undoing the results of pdata->resume.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/ata/ahci_platform.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/ata/ahci_platform.c b/drivers/ata/ahci_platform.c
> index dc1ef73..41720cb 100644
> --- a/drivers/ata/ahci_platform.c
> +++ b/drivers/ata/ahci_platform.c
> @@ -307,7 +307,7 @@ static int ahci_resume(struct device *dev)
>  	if (dev->power.power_state.event == PM_EVENT_SUSPEND) {
>  		rc = ahci_reset_controller(host);
>  		if (rc)
> -			goto disable_unprepare_clk;
> +			goto pdata_suspend;
>  
>  		ahci_init_controller(host);
>  	}
> @@ -316,6 +316,9 @@ static int ahci_resume(struct device *dev)
>  
>  	return 0;
>  
> +pdata_suspend:
> +	if (pdata && pdata->suspend)
> +		pdata->suspend(dev);
>  disable_unprepare_clk:
>  	if (!IS_ERR(hpriv->clk))
>  		clk_disable_unprepare(hpriv->clk);

Hmmmm... resume isn't an operation you can revert without side-effect
when the whole system is waking up from sleep.  e.g. think about what
should happen the driver is removed and loaded again - it should be
able to reinitialized the device, which is unlikely to work if the
device is suspended at the platform level.  If resume fails, the right
state to be in is "failed with as much as resumed" instead of
"suspended".

Thanks.
Hans de Goede - Jan. 19, 2014, 6:40 p.m.
Hi,

On 01/19/2014 12:27 PM, Tejun Heo wrote:
> Hello,
>
> On Sun, Jan 19, 2014 at 12:48:46AM +0100, Hans de Goede wrote:
>> When the ahci_resume fails the error handling code tries to undo all changes
>> made, but it was not undoing the results of pdata->resume.
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>   drivers/ata/ahci_platform.c | 5 ++++-
>>   1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/ata/ahci_platform.c b/drivers/ata/ahci_platform.c
>> index dc1ef73..41720cb 100644
>> --- a/drivers/ata/ahci_platform.c
>> +++ b/drivers/ata/ahci_platform.c
>> @@ -307,7 +307,7 @@ static int ahci_resume(struct device *dev)
>>   	if (dev->power.power_state.event == PM_EVENT_SUSPEND) {
>>   		rc = ahci_reset_controller(host);
>>   		if (rc)
>> -			goto disable_unprepare_clk;
>> +			goto pdata_suspend;
>>
>>   		ahci_init_controller(host);
>>   	}
>> @@ -316,6 +316,9 @@ static int ahci_resume(struct device *dev)
>>
>>   	return 0;
>>
>> +pdata_suspend:
>> +	if (pdata && pdata->suspend)
>> +		pdata->suspend(dev);
>>   disable_unprepare_clk:
>>   	if (!IS_ERR(hpriv->clk))
>>   		clk_disable_unprepare(hpriv->clk);
>
> Hmmmm... resume isn't an operation you can revert without side-effect
> when the whole system is waking up from sleep.  e.g. think about what
> should happen the driver is removed and loaded again - it should be
> able to reinitialized the device, which is unlikely to work if the
> device is suspended at the platform level.  If resume fails, the right
> state to be in is "failed with as much as resumed" instead of
> "suspended".

That sounds like your advocating for just returning from resume on the
first error without undoing any of the previous steps, have I gotten that
right?

That sounds as sensible as any other approach on resume errors
(there are IMHO no good answers), if that is what you mean, shall I do a
patch in the next versions of my patch-set doing that ?

Regards,

Hans
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Tejun Heo - Jan. 19, 2014, 7:13 p.m.
Hello,

On Sun, Jan 19, 2014 at 07:40:21PM +0100, Hans de Goede wrote:
> That sounds like your advocating for just returning from resume on the
> first error without undoing any of the previous steps, have I gotten that
> right?

Yeah, or just ignore reset failure and proceed.

> That sounds as sensible as any other approach on resume errors
> (there are IMHO no good answers), if that is what you mean, shall I do a

Suspending back is the wrong answer tho.

> patch in the next versions of my patch-set doing that ?

Isn't just dropping this patch enough tho?

Thanks.
Hans de Goede - Jan. 19, 2014, 7:34 p.m.
Hi,

On 01/19/2014 08:13 PM, Tejun Heo wrote:
> Hello,
>
> On Sun, Jan 19, 2014 at 07:40:21PM +0100, Hans de Goede wrote:
>> That sounds like your advocating for just returning from resume on the
>> first error without undoing any of the previous steps, have I gotten that
>> right?
>
> Yeah, or just ignore reset failure and proceed.

That seems like a bad idea IMHO, if reset failed something is seriously
amiss and just continuing as nothing happened seems unhelpful.

>> That sounds as sensible as any other approach on resume errors
>> (there are IMHO no good answers), if that is what you mean, shall I do a
>
> Suspending back is the wrong answer tho.
>
>> patch in the next versions of my patch-set doing that ?
>
> Isn't just dropping this patch enough tho?

Well the current error handling still re-disables the clks on resume failure,
if you want to proceed with resume as far as possible, rather then return to
a suspended state it seems sensible to just leave the clocks on as well.

Also disabling the clocks on resume failure, followed by a rmmod will cause
a WARN_ON to trigger in the clock-framework when ahci_host_stop tries to
disable the clks for a second time.

Regards,

Hans
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Tejun Heo - Jan. 19, 2014, 7:42 p.m.
On Sun, Jan 19, 2014 at 08:34:51PM +0100, Hans de Goede wrote:
> Well the current error handling still re-disables the clks on resume failure,
> if you want to proceed with resume as far as possible, rather then return to

Let's put it as "put it in a state where it can be reinitialized
afterwards".

Thanks.
Hans de Goede - Jan. 19, 2014, 7:53 p.m.
Hi,

On 01/19/2014 08:42 PM, Tejun Heo wrote:
> On Sun, Jan 19, 2014 at 08:34:51PM +0100, Hans de Goede wrote:
>> Well the current error handling still re-disables the clks on resume failure,
>> if you want to proceed with resume as far as possible, rather then return to
>
> Let's put it as "put it in a state where it can be reinitialized
> afterwards".

Ok, I'll drop the patch then.

Regards,

Hans
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/drivers/ata/ahci_platform.c b/drivers/ata/ahci_platform.c
index dc1ef73..41720cb 100644
--- a/drivers/ata/ahci_platform.c
+++ b/drivers/ata/ahci_platform.c
@@ -307,7 +307,7 @@  static int ahci_resume(struct device *dev)
 	if (dev->power.power_state.event == PM_EVENT_SUSPEND) {
 		rc = ahci_reset_controller(host);
 		if (rc)
-			goto disable_unprepare_clk;
+			goto pdata_suspend;
 
 		ahci_init_controller(host);
 	}
@@ -316,6 +316,9 @@  static int ahci_resume(struct device *dev)
 
 	return 0;
 
+pdata_suspend:
+	if (pdata && pdata->suspend)
+		pdata->suspend(dev);
 disable_unprepare_clk:
 	if (!IS_ERR(hpriv->clk))
 		clk_disable_unprepare(hpriv->clk);