linux-kernel - Re: [PATCH] drivers: pwm: pwm-atmel: implement suspend/resume functions

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <60b4ac51-6cee-04b3-f74a-3d2693e1f021@microchip.com>
Date:   Tue, 11 Apr 2017 11:33:44 +0300
From:   m18063 <Claudiu.Beznea@...rochip.com>
To:     Boris Brezillon <boris.brezillon@...e-electrons.com>,
        Thierry Reding <thierry.reding@...il.com>
CC:     <linux-pwm@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
        <linux-arm-kernel@...ts.infradead.org>,
        <alexandre.belloni@...e-electrons.com>,
        <nicolas.ferre@...rochip.com>
Subject: Re: [PATCH] drivers: pwm: pwm-atmel: implement suspend/resume
 functions



On 10.04.2017 19:27, Boris Brezillon wrote:
> On Mon, 10 Apr 2017 18:01:37 +0200
> Boris Brezillon <boris.brezillon@...e-electrons.com> wrote:
> 
>> On Mon, 10 Apr 2017 17:10:11 +0200
>> Thierry Reding <thierry.reding@...il.com> wrote:
>>
>>> On Mon, Apr 10, 2017 at 04:35:58PM +0200, Boris Brezillon wrote:  
>>>> On Mon, 10 Apr 2017 17:20:20 +0300
>>>> Claudiu Beznea <claudiu.beznea@...rochip.com> wrote:
>>>>     
>>>>> Implement suspend and resume power management specific
>>>>> function to allow PWM controller to correctly suspend
>>>>> and resume.
>>>>>
>>>>> Signed-off-by: Claudiu Beznea <claudiu.beznea@...rochip.com>
>>>>> ---
>>>>>  drivers/pwm/pwm-atmel.c | 81 +++++++++++++++++++++++++++++++++++++++++++++++++
>>>>>  1 file changed, 81 insertions(+)
>>>>>
>>>>> diff --git a/drivers/pwm/pwm-atmel.c b/drivers/pwm/pwm-atmel.c
>>>>> index 530d7dc..75177c6 100644
>>>>> --- a/drivers/pwm/pwm-atmel.c
>>>>> +++ b/drivers/pwm/pwm-atmel.c
>>>>> @@ -58,6 +58,8 @@
>>>>>  #define PWM_MAX_PRD		0xFFFF
>>>>>  #define PRD_MAX_PRES		10
>>>>>  
>>>>> +#define PWM_MAX_CH_NUM		(4)
>>>>> +
>>>>>  struct atmel_pwm_registers {
>>>>>  	u8 period;
>>>>>  	u8 period_upd;
>>>>> @@ -65,11 +67,18 @@ struct atmel_pwm_registers {
>>>>>  	u8 duty_upd;
>>>>>  };
>>>>>  
>>>>> +struct atmel_pwm_pm_ctx {
>>>>> +	u32 cmr;
>>>>> +	u32 cdty;
>>>>> +	u32 cprd;
>>>>> +};
>>>>> +
>>>>>  struct atmel_pwm_chip {
>>>>>  	struct pwm_chip chip;
>>>>>  	struct clk *clk;
>>>>>  	void __iomem *base;
>>>>>  	const struct atmel_pwm_registers *regs;
>>>>> +	struct atmel_pwm_pm_ctx ctx[PWM_MAX_CH_NUM];    
>>>>
>>>> Hm, I'm pretty sure you can rely on the current PWM state and call
>>>> atmel_pwm_apply() at resume time instead of doing that. See what I did
>>>> here [1].
>>>>
>>>> Thierry, maybe it's time to start thinking about a generic solution to
>>>> save/restore PWM states.    
>>>
>>> Generally speaking I think applying the states are the right way to go.
>>> Ideally the PWM core could simply resume all of the PWM channels that a
>>> device exports and the ->apply() callback would be enough to restore
>>> that. I'm not sure if that's going to work with current implementations,
>>> though. Your pwm-atmel-hlcdc patch certainly indicates that we're not
>>> quite there yet.
>>>
>>> On the other hand, I'm beginning to think that maybe PWMs are too low-
>>> level for this kind of suspend/resume. For example if you use the PWM to
>>> control a backlight brightness, restoring it via the driver core's
>>> resume hook is potentially going to turn it back on at the wrong time. I
>>> have a feeling that we might be better off just pushing this up to the
>>> PWM users. A slight special case might be sysfs, for which no external
>>> user driver exists. But we already have separate data structures to keep
>>> track of sysfs-related context, so suspend/resume support could be added
>>> there.  
>>
>> Yep, you're probably right, we should let the PWM user take care of
>> re-applying the PWM state, because it's the only one having enough
>> knowledge about what the PWM is really driving to take a wise decision.
> 
> Note that we need drivers to implement both ->apply() and ->get_state()
> for this approach to work correctly, and we also need some help from
> the core to reset the PWM states at resume time, otherwise
> pwm_apply_state() will just compare the old state to the new one, see
> that they match and never call the ->apply() method.
> 
> Another solution would be to remove the memcmp here [1] and
> unconditionally call ->apply().
There are drivers which checks, in ->apply() hooks, the current PWM state
before applying the new state or take actions based on differences
b/w current and new PWM states. Removing memcmp without resetting
the PWM state would lead to wrong states in those drivers.
 
> 
> [1]http://lxr.free-electrons.com/source/drivers/pwm/core.c#L466
>