linux-kernel - Re: pwm: atmel: PWM may not properly disable

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 for Android: free password hash cracker in your pocket

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

Message-ID: <20160512121402.GA26824@ulmo.ba.sec>
Date:	Thu, 12 May 2016 14:14:02 +0200
From:	Thierry Reding <thierry.reding@...il.com>
To:	Guillermo Rodriguez Garcia <guille.rodriguez@...il.com>
Cc:	linux-kernel@...r.kernel.org, linux-pwm@...r.kernel.org,
	Nicolas Ferre <nicolas.ferre@...el.com>
Subject: Re: pwm: atmel: PWM may not properly disable

On Thu, May 12, 2016 at 01:49:12PM +0200, Guillermo Rodriguez Garcia wrote:
> Hello,
> 
> [...]
> >>> One thing that I'd request is that instead of the cpu_relax() you use a
> >>> usleep_range() within the loop instead. I assume it can potentially take
> >>> a long time for the current period to finish, so busy looping isn't such
> >>> a great idea. You could possibly use the current period_ns to derive a
> >>> meaningful value to pass to usleep_range().
> >>
> >> I am not sure yet but I believe disabling does not really need to wait for the
> >> current period to finish (at least the datasheets do not mention this anywhere).
> >> I think that the after writing to PWM_DIS, the actual disable operation is
> >> initiated immediately in the PWM subsystem, but is executed asynchronously
> >> and requires the pwm_clk to complete. If this assumption is correct, perhaps
> >> it is enough to do one single read from PWM_SR so that the disable operation
> >> has had the chance to propagate. This is again assuming that all operations
> >> are executed sequentially within the PWM subsystem.
> >>
> >> If the above is correct, then we would not need a loop at all.
> >
> > I was wrong. The required delay indeed seems to depend on the current PWM
> > frequency, suggesting that indeed disabling does not take effect until
> > the current
> > period is finished.
> >
> > I will prepare a patch using usleep_range instead of cpu_relax.
> 
> I have found a problem while preparing this. If I use usleep_range I
> keep running
> into "BUG: scheduling while atomic". This is because I am using the PWM to
> drive a buzzer with pwm-beeper, and pwm-beeper currently crashes if the PWM
> driver sleeps. Apparently this patch is needed:
> 
> https://lkml.org/lkml/2016/2/22/757
> 
> However this has not been merged yet.
> 
> How should I proceed ?

The PWM API really shouldn't be used within atomic contexts. There was a
change recently that marked all of the PWM devices as "might sleep". The
reason for the change was that we introduced a mutex in pwm_enable() and
hence every user would have to deal with this eventually. That mutex has
since been removed again, but the fact remains that users shouldn't
assume that a PWM can be used in atomic context, because the PWM chip
could equally well be behind a slow bus such as I2C and hence sleep for
every register access.

So the correct thing to do would be to follow what leds-pwm did and
implement a workqueue. Also might as well make it the only code path as
Dmitry suggested in the linked thread, I don't see any point in any kind
of fast path here.

Thierry

Download attachment "signature.asc" of type "application/pgp-signature" (820 bytes)