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) [email protected] | (970) 672-0658
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
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) [email protected] | (970) 672-0658
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
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) [email protected] | (970) 672-0658
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.
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) [email protected] | (970) 672-0658