sl811h_suspend() and PM_EVENT_PRETHAW state handling

sl811h_suspend() and PM_EVENT_PRETHAW state handling

[Shuah Khan]

sl811h_suspend() seems to be the odd routine in the way it handles the 
PM_EVENT_PRETHAW state. It treats it same as PM_EVENT_SUSPEND and 
PM_EVENT_HIBERNATE. All other uses I could find treat it same as 
PM_EVENT_FREEZE and PM_EVENT_QUIESCE. Makes sense since PM_EVENT_PRETHAW 
is PM_EVENT_QUIESCE.

#define PM_EVENT_PRETHAW PM_EVENT_QUIESCE

Reference: Commit 185849991d592497e43bcd264c6152af1261ffe2 introduced 
PM_EVENT_PRETHAW state to sl811h_suspend().

Couple of questions?

- Why does sl811h_suspend() treat PM_EVENT_PRETHAW different from 
PM_EVENT_FREEZE?

There is no problem with this code as such, since state is passed in. 
However, this usage conflicts with the rest of the usages and the way 
pm_op() routine maps PM_EVENT_PRETHAW/PM_EVENT_QUIESCE to freeze() pm_ops.

         case PM_EVENT_FREEZE:
         case PM_EVENT_QUIESCE:
                 return ops->freeze;

Assuming the handling PM_EVENT_PRETHAW is correct in this routine, what 
would be the right mapping for this legacy handling to dev_pm_ops?

-- Shuah

Shuah Khan, Linux Kernel Developer - Open Source Group Samsung Research 
America (Silicon Valley) shuah.kh@samsung.com | (970) 672-0658

Re: sl811h_suspend() and PM_EVENT_PRETHAW state handling

[Alan Stern]

On Tue, 6 Aug 2013, Shuah Khan wrote:

> sl811h_suspend() seems to be the odd routine in the way it handles the 
> PM_EVENT_PRETHAW state. It treats it same as PM_EVENT_SUSPEND and 
> PM_EVENT_HIBERNATE. All other uses I could find treat it same as 
> PM_EVENT_FREEZE and PM_EVENT_QUIESCE. Makes sense since PM_EVENT_PRETHAW 
> is PM_EVENT_QUIESCE.
> 
> #define PM_EVENT_PRETHAW PM_EVENT_QUIESCE
> 
> Reference: Commit 185849991d592497e43bcd264c6152af1261ffe2 introduced 
> PM_EVENT_PRETHAW state to sl811h_suspend().
> 
> Couple of questions?
> 
> - Why does sl811h_suspend() treat PM_EVENT_PRETHAW different from 
> PM_EVENT_FREEZE?

Because with FREEZE, the driver wants to retain the current bus state.  
With PRETHAW, it doesn't care about retaining the bus state.

At least, that's how it looks to me.  The original author of this 
driver died a couple of years ago.  I don't know if anyone is using it 
any more.

> There is no problem with this code as such, since state is passed in. 
> However, this usage conflicts with the rest of the usages and the way 
> pm_op() routine maps PM_EVENT_PRETHAW/PM_EVENT_QUIESCE to freeze() pm_ops.
> 
>          case PM_EVENT_FREEZE:
>          case PM_EVENT_QUIESCE:
>                  return ops->freeze;
> 
> Assuming the handling PM_EVENT_PRETHAW is correct in this routine, what 
> would be the right mapping for this legacy handling to dev_pm_ops?

It depends on the driver; there is no one answer.

Alan Stern

Re: sl811h_suspend() and PM_EVENT_PRETHAW state handling

[Shuah Khan]

On 08/06/2013 12:39 PM, Alan Stern wrote:
> On Tue, 6 Aug 2013, Shuah Khan wrote:
>
>> sl811h_suspend() seems to be the odd routine in the way it handles the
>> PM_EVENT_PRETHAW state. It treats it same as PM_EVENT_SUSPEND and
>> PM_EVENT_HIBERNATE. All other uses I could find treat it same as
>> PM_EVENT_FREEZE and PM_EVENT_QUIESCE. Makes sense since PM_EVENT_PRETHAW
>> is PM_EVENT_QUIESCE.
>>
>> #define PM_EVENT_PRETHAW PM_EVENT_QUIESCE
>>
>> Reference: Commit 185849991d592497e43bcd264c6152af1261ffe2 introduced
>> PM_EVENT_PRETHAW state to sl811h_suspend().
>>
>> Couple of questions?
>>
>> - Why does sl811h_suspend() treat PM_EVENT_PRETHAW different from
>> PM_EVENT_FREEZE?
>
> Because with FREEZE, the driver wants to retain the current bus state.
> With PRETHAW, it doesn't care about retaining the bus state.
>
> At least, that's how it looks to me.  The original author of this
> driver died a couple of years ago.  I don't know if anyone is using it
> any more.
>
>> There is no problem with this code as such, since state is passed in.
>> However, this usage conflicts with the rest of the usages and the way
>> pm_op() routine maps PM_EVENT_PRETHAW/PM_EVENT_QUIESCE to freeze() pm_ops.
>>
>>           case PM_EVENT_FREEZE:
>>           case PM_EVENT_QUIESCE:
>>                   return ops->freeze;
>>
>> Assuming the handling PM_EVENT_PRETHAW is correct in this routine, what
>> would be the right mapping for this legacy handling to dev_pm_ops?
>
> It depends on the driver; there is no one answer.
>
> Alan Stern
>
>

With the dev_pm_ops model, drivers have to provide interfaces for each 
one of these states. In this case, there will be a conflict since 
pm_op() treats this state as freeze where as the driver wants to do 
treat it as a suspend/hibernate. In the case of legacy pm_ops, state is 
passed in as a parameter and driver could take special action if need 
be, based on the state, however in dev_pm_ops model, state is not passed 
in. Instead it is handled with state specific pm_ops interfaces.

For example, if this driver were to be converted to dev_pm_ops, it would 
require a freeze interface which will call sl811h_bus_suspend(). Once 
that is done, PM_EVENT_PRETHAW will be mapped to freeze() ops and 
sl811h_bus_suspend() will be called instead of port_power(sl811, 0);

What I am getting at is, there is no provision to handle the special 
case for PM_EVENT_PRETHAW like in the case of this driver when using 
dev_pm_ops.

-- Shuah

Shuah Khan, Linux Kernel Developer - Open Source Group Samsung Research 
America (Silicon Valley) shuah.kh@samsung.com | (970) 672-0658

Re: sl811h_suspend() and PM_EVENT_PRETHAW state handling

[Alan Stern]

On Tue, 6 Aug 2013, Shuah Khan wrote:

> With the dev_pm_ops model, drivers have to provide interfaces for each 
> one of these states.

No, they don't.  They can leave out interfaces if they want.

>  In this case, there will be a conflict since 
> pm_op() treats this state as freeze where as the driver wants to do 
> treat it as a suspend/hibernate. In the case of legacy pm_ops, state is 
> passed in as a parameter and driver could take special action if need 
> be, based on the state, however in dev_pm_ops model, state is not passed 
> in. Instead it is handled with state specific pm_ops interfaces.
> 
> For example, if this driver were to be converted to dev_pm_ops, it would 
> require a freeze interface which will call sl811h_bus_suspend(). Once 
> that is done, PM_EVENT_PRETHAW will be mapped to freeze() ops and 
> sl811h_bus_suspend() will be called instead of port_power(sl811, 0);
> 
> What I am getting at is, there is no provision to handle the special 
> case for PM_EVENT_PRETHAW like in the case of this driver when using 
> dev_pm_ops.

Okay.  So what?

Alan Stern

Re: sl811h_suspend() and PM_EVENT_PRETHAW state handling

[Shuah Khan]

On 08/06/2013 03:22 PM, Alan Stern wrote:
> On Tue, 6 Aug 2013, Shuah Khan wrote:
>
>> With the dev_pm_ops model, drivers have to provide interfaces for each
>> one of these states.
>
> No, they don't.  They can leave out interfaces if they want.

Yes. Agreed. There is no need to provide each and every interface. Only 
the ones driver wishes to handle.

>
>>   In this case, there will be a conflict since
>> pm_op() treats this state as freeze where as the driver wants to do
>> treat it as a suspend/hibernate. In the case of legacy pm_ops, state is
>> passed in as a parameter and driver could take special action if need
>> be, based on the state, however in dev_pm_ops model, state is not passed
>> in. Instead it is handled with state specific pm_ops interfaces.
>>
>> For example, if this driver were to be converted to dev_pm_ops, it would
>> require a freeze interface which will call sl811h_bus_suspend(). Once
>> that is done, PM_EVENT_PRETHAW will be mapped to freeze() ops and
>> sl811h_bus_suspend() will be called instead of port_power(sl811, 0);
>>
>> What I am getting at is, there is no provision to handle the special
>> case for PM_EVENT_PRETHAW like in the case of this driver when using
>> dev_pm_ops.
>
> Okay.  So what?
>

I am exploring to see if there is a deficiency in dev_pm_ops 
infrastructure that needs addressing. Based on this example, there is a 
need for a way to allow drivers that want to do something state specific 
that is different from the defined framework if need be.

-- Shuah

Shuah Khan, Linux Kernel Developer - Open Source Group Samsung Research 
America (Silicon Valley) shuah.kh@samsung.com | (970) 672-0658

Re: sl811h_suspend() and PM_EVENT_PRETHAW state handling

[Shuah Khan]

On 08/06/2013 04:28 PM, Rafael J. Wysocki wrote:
> On Tuesday, August 06, 2013 09:38:36 PM Shuah Khan wrote:
>> On 08/06/2013 03:22 PM, Alan Stern wrote:
>>> On Tue, 6 Aug 2013, Shuah Khan wrote:
>>>
>>>> With the dev_pm_ops model, drivers have to provide interfaces for each
>>>> one of these states.
>>>
>>> No, they don't.  They can leave out interfaces if they want.
>>
>> Yes. Agreed. There is no need to provide each and every interface. Only
>> the ones driver wishes to handle.
>>
>>>
>>>>    In this case, there will be a conflict since
>>>> pm_op() treats this state as freeze where as the driver wants to do
>>>> treat it as a suspend/hibernate. In the case of legacy pm_ops, state is
>>>> passed in as a parameter and driver could take special action if need
>>>> be, based on the state, however in dev_pm_ops model, state is not passed
>>>> in. Instead it is handled with state specific pm_ops interfaces.
>>>>
>>>> For example, if this driver were to be converted to dev_pm_ops, it would
>>>> require a freeze interface which will call sl811h_bus_suspend(). Once
>>>> that is done, PM_EVENT_PRETHAW will be mapped to freeze() ops and
>>>> sl811h_bus_suspend() will be called instead of port_power(sl811, 0);
>>>>
>>>> What I am getting at is, there is no provision to handle the special
>>>> case for PM_EVENT_PRETHAW like in the case of this driver when using
>>>> dev_pm_ops.
>>>
>>> Okay.  So what?
>>>
>>
>> I am exploring to see if there is a deficiency in dev_pm_ops
>> infrastructure that needs addressing.
>
> No, there isn't.  We determined a few years ago that "freeze" could always be
> used instead of "prethaw".  Moreover, there actually is a "thaw" after that
> stage if the image restoration fails, so please just use "freeze" here.  Or
> just skip it if it is not really needed.
>

Thanks Rafael!

-- Shuah

Shuah Khan, Linux Kernel Developer - Open Source Group Samsung Research 
America (Silicon Valley) shuah.kh@samsung.com | (970) 672-0658

Re: sl811h_suspend() and PM_EVENT_PRETHAW state handling

[Rafael J. Wysocki]

On Tuesday, August 06, 2013 09:38:36 PM Shuah Khan wrote:
> On 08/06/2013 03:22 PM, Alan Stern wrote:
> > On Tue, 6 Aug 2013, Shuah Khan wrote:
> >
> >> With the dev_pm_ops model, drivers have to provide interfaces for each
> >> one of these states.
> >
> > No, they don't.  They can leave out interfaces if they want.
> 
> Yes. Agreed. There is no need to provide each and every interface. Only 
> the ones driver wishes to handle.
> 
> >
> >>   In this case, there will be a conflict since
> >> pm_op() treats this state as freeze where as the driver wants to do
> >> treat it as a suspend/hibernate. In the case of legacy pm_ops, state is
> >> passed in as a parameter and driver could take special action if need
> >> be, based on the state, however in dev_pm_ops model, state is not passed
> >> in. Instead it is handled with state specific pm_ops interfaces.
> >>
> >> For example, if this driver were to be converted to dev_pm_ops, it would
> >> require a freeze interface which will call sl811h_bus_suspend(). Once
> >> that is done, PM_EVENT_PRETHAW will be mapped to freeze() ops and
> >> sl811h_bus_suspend() will be called instead of port_power(sl811, 0);
> >>
> >> What I am getting at is, there is no provision to handle the special
> >> case for PM_EVENT_PRETHAW like in the case of this driver when using
> >> dev_pm_ops.
> >
> > Okay.  So what?
> >
> 
> I am exploring to see if there is a deficiency in dev_pm_ops 
> infrastructure that needs addressing.

No, there isn't.  We determined a few years ago that "freeze" could always be
used instead of "prethaw".  Moreover, there actually is a "thaw" after that
stage if the image restoration fails, so please just use "freeze" here.  Or
just skip it if it is not really needed.

Thanks,
Rafael


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.