Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1376713 > unrolled thread

Re: [PATCH v5 04/46] pwm: get rid of pwm->lock

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v5 04/46] pwm: get rid of pwm->lock Thierry Reding <thierry.reding@gmail.com> - 2016-04-12 13:30 +0200
    Re: [PATCH v5 04/46] pwm: get rid of pwm->lock Boris Brezillon <boris.brezillon@free-electrons.com> - 2016-04-12 13:40 +0200
      Re: [PATCH v5 04/46] pwm: get rid of pwm->lock Thierry Reding <thierry.reding@gmail.com> - 2016-04-12 13:50 +0200

#1376713 — Re: [PATCH v5 04/46] pwm: get rid of pwm->lock

[Multipart message — attachments visible in raw view] — view raw

On Wed, Mar 30, 2016 at 10:03:27PM +0200, Boris Brezillon wrote:
> PWM devices are not protected against concurrent accesses. The lock in
> pwm_device might let PWM users think it is, but it's actually only
> protecting the enabled state.
> 
> Removing this lock should be fine as long as all PWM users are aware that
> accesses to the PWM device have to be serialized, which seems to be the
> case for all of them except the sysfs interface.
> Patch the sysfs code by adding a lock to the pwm_export struct and making
> sure it's taken for all accesses to the exported PWM device.
> 
> Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> ---
>  drivers/pwm/core.c  | 19 ++++--------------
>  drivers/pwm/sysfs.c | 57 ++++++++++++++++++++++++++++++++++++++++++-----------
>  include/linux/pwm.h |  2 --
>  3 files changed, 50 insertions(+), 28 deletions(-)

This is a little overzealous. Only accesses that can cause races need to
be protected by the lock. All of the *_show() callbacks don't modify the
PWM device in any way, so there is no need to protect them against
concurrent accesses.

I've dropped the changes when applying.

Thanks,
Thierry

[toc] | [next] | [standalone]

#1376720

Hi Thierry,

On Tue, 12 Apr 2016 13:22:46 +0200
Thierry Reding <thierry.reding@gmail.com> wrote:

> On Wed, Mar 30, 2016 at 10:03:27PM +0200, Boris Brezillon wrote:
> > PWM devices are not protected against concurrent accesses. The lock in
> > pwm_device might let PWM users think it is, but it's actually only
> > protecting the enabled state.
> > 
> > Removing this lock should be fine as long as all PWM users are aware that
> > accesses to the PWM device have to be serialized, which seems to be the
> > case for all of them except the sysfs interface.
> > Patch the sysfs code by adding a lock to the pwm_export struct and making
> > sure it's taken for all accesses to the exported PWM device.
> > 
> > Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> > ---
> >  drivers/pwm/core.c  | 19 ++++--------------
> >  drivers/pwm/sysfs.c | 57 ++++++++++++++++++++++++++++++++++++++++++-----------
> >  include/linux/pwm.h |  2 --
> >  3 files changed, 50 insertions(+), 28 deletions(-)
> 
> This is a little overzealous. Only accesses that can cause races need to
> be protected by the lock. All of the *_show() callbacks don't modify the
> PWM device in any way, so there is no need to protect them against
> concurrent accesses.

This is probably true for this set of changes, but what will happen
when we'll switch to the atomic API? There's no guarantee that
pwm->state = *newstate is done atomically, and you may see a partially
updated state when calling pwm_get_state() while another thread is
calling pwm_apply_state().

-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[toc] | [prev] | [next] | [standalone]

#1376725

[Multipart message — attachments visible in raw view] — view raw

On Tue, Apr 12, 2016 at 01:32:55PM +0200, Boris Brezillon wrote:
> Hi Thierry,
> 
> On Tue, 12 Apr 2016 13:22:46 +0200
> Thierry Reding <thierry.reding@gmail.com> wrote:
> 
> > On Wed, Mar 30, 2016 at 10:03:27PM +0200, Boris Brezillon wrote:
> > > PWM devices are not protected against concurrent accesses. The lock in
> > > pwm_device might let PWM users think it is, but it's actually only
> > > protecting the enabled state.
> > > 
> > > Removing this lock should be fine as long as all PWM users are aware that
> > > accesses to the PWM device have to be serialized, which seems to be the
> > > case for all of them except the sysfs interface.
> > > Patch the sysfs code by adding a lock to the pwm_export struct and making
> > > sure it's taken for all accesses to the exported PWM device.
> > > 
> > > Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> > > ---
> > >  drivers/pwm/core.c  | 19 ++++--------------
> > >  drivers/pwm/sysfs.c | 57 ++++++++++++++++++++++++++++++++++++++++++-----------
> > >  include/linux/pwm.h |  2 --
> > >  3 files changed, 50 insertions(+), 28 deletions(-)
> > 
> > This is a little overzealous. Only accesses that can cause races need to
> > be protected by the lock. All of the *_show() callbacks don't modify the
> > PWM device in any way, so there is no need to protect them against
> > concurrent accesses.
> 
> This is probably true for this set of changes, but what will happen
> when we'll switch to the atomic API? There's no guarantee that
> pwm->state = *newstate is done atomically, and you may see a partially
> updated state when calling pwm_get_state() while another thread is
> calling pwm_apply_state().

I'd argue that for sysfs it doesn't matter since the userspace API is
non-atomic anyway. As such it doesn't really matter which part of the
state you're getting because only one field from the query is exposed
to userspace, hence the coherence of the state is irrelevant.

All other users should be taking care of the serialization themselves
already.

Thierry

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web