Re: [PATCH] Input: pwm-beeper - defer pwm config if pwm can sleep

[Thierry Reding]

On Mon, Feb 22, 2016 at 11:46:39AM -0800, Dmitry Torokhov wrote:
> On Wed, Feb 17, 2016 at 02:19:26PM +0100, Manfred Schlaegl wrote:
> > If the pwm can sleep defer actions to it using a worker.
> > A similar approach was used in leds-pwm (c971ff185)
> > 
> > Trigger:
> > On a Freescale i.MX53 based board we ran into "BUG: scheduling while
> > atomic" because input_inject_event locks interrupts, but
> > imx_pwm_config_v2 sleeps.
> > 
> > Tested on Freescale i.MX53 SoC with 4.5-rc1 and 4.1.
> > 
> > Unmodified applicable to
> >  * 4.5-rc4
> >  * 4.4.1 (stable)
> >  * 4.3.5 (stable)
> >  * 4.1.18 (longterm)
> > 
> > Modified applicable to
> >  * 3.18.27 (longterm)
> > 
> > Signed-off-by: Manfred Schlaegl <manfred.schlaegl@xxxxxx>
> > ---
> >  drivers/input/misc/pwm-beeper.c | 62 +++++++++++++++++++++++++++++------------
> >  1 file changed, 44 insertions(+), 18 deletions(-)
> > 
> > diff --git a/drivers/input/misc/pwm-beeper.c b/drivers/input/misc/pwm-beeper.c
> > index f2261ab..c160b5e 100644
> > --- a/drivers/input/misc/pwm-beeper.c
> > +++ b/drivers/input/misc/pwm-beeper.c
> > @@ -20,21 +20,42 @@
> >  #include <linux/platform_device.h>
> >  #include <linux/pwm.h>
> >  #include <linux/slab.h>
> > +#include <linux/workqueue.h>
> >  
> >  struct pwm_beeper {
> >  	struct input_dev *input;
> >  	struct pwm_device *pwm;
> > +	struct work_struct work;
> >  	unsigned long period;
> > +	bool can_sleep;
> 
> I wonder if it is not better to always schedule work, regardless of
> whether PWM may sleep or not.

I agree with Dmitry. Users of the PWM API should always assume that
calls to the PWM API might sleep. Conditionalizing on pwm_can_sleep()
isn't a good idea, since that function is scheduled to be removed. In
fact it's been returning true unconditionally since v4.5, so the fast
path is dead code anyway.

Thierry

Attachment: signature.asc
Description: PGP signature