linux-kernel - Re: [PATCH v8 3/4] counter: ti-ecap-capture: capture driver support for ECAP

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] [day] [month] [year] [list]

Message-ID: <Yy2wDPITCZ/J01Xr@fedora>
Date:   Fri, 23 Sep 2022 09:09:32 -0400
From:   William Breathitt Gray <william.gray@...aro.org>
To:     Julien Panis <jpanis@...libre.com>
Cc:     robh+dt@...nel.org, krzysztof.kozlowski+dt@...aro.org,
        linux-iio@...r.kernel.org, linux-kernel@...r.kernel.org,
        devicetree@...r.kernel.org, mranostay@...com
Subject: Re: [PATCH v8 3/4] counter: ti-ecap-capture: capture driver support
 for ECAP

On Fri, Sep 23, 2022 at 02:32:26PM +0200, Julien Panis wrote:
> 
> 
> On 23/09/2022 14:17, Julien Panis wrote:
> > 
> > 
> > On 23/09/2022 13:35, William Breathitt Gray wrote:
> > > On Fri, Sep 23, 2022 at 09:23:26AM +0200, Julien Panis wrote:
> > > > 
> > > > On 23/09/2022 03:08, William Breathitt Gray wrote:
> > > > > On Thu, Sep 22, 2022 at 07:04:01PM +0200, Julien Panis wrote:
> > > > > > ECAP hardware on TI AM62x SoC supports capture feature.
> > > > > > It can be used
> > > > > > to timestamp events (falling/rising edges) detected on input signal.
> > > > > > 
> > > > > > This commit adds capture driver support for ECAP
> > > > > > hardware on AM62x SoC.
> > > > > > 
> > > > > > In the ECAP hardware, capture pin can also be configured to be in
> > > > > > PWM mode. Current implementation only supports capture
> > > > > > operating mode.
> > > > > > Hardware also supports timebase sync between multiple instances, but
> > > > > > this driver supports simple independent capture functionality.
> > > > > > 
> > > > > > Signed-off-by: Julien Panis <jpanis@...libre.com>
> > > > > Hello Julien,
> > > > > 
> > > > > Comments follow inline below.
> > > > > 
> > > > > > +/**
> > > > > > + * struct ecap_cnt_dev - device private data structure
> > > > > > + * @enabled: device state
> > > > > > + * @clk:     device clock
> > > > > > + * @regmap:  device register map
> > > > > > + * @nb_ovf:  number of overflows since capture start
> > > > > > + * @pm_ctx:  device context for PM operations
> > > > > > + */
> > > > > > +struct ecap_cnt_dev {
> > > > > > +    bool enabled;
> > > > > > +    struct clk *clk;
> > > > > > +    struct regmap *regmap;
> > > > > > +    atomic_t nb_ovf;
> > > > > > +    struct {
> > > > > > +        u8 ev_mode;
> > > > > > +        u32 time_cntr;
> > > > > > +    } pm_ctx;
> > > > > > +};
> > > > > Provide documentation for the ev_mode and time_cntr members. You
> > > > > probably need a lock as well to protect access to this structure or
> > > > > you'll end up with race problems.
> > > > Hi William,
> > > > 
> > > > How can I end up with race problems ? pm_ctx members are only
> > > > accessed at
> > > > suspend (after capture/IRQ are disabled) and resume (before
> > > > capture/IRQ are
> > > > re-enabled).
> > > > Is there any risk I did not identify ?
> > > > 
> > > > Julien
> > > I was thinking of the ecap_cnt_dev enabled member. The Counter callbacks
> > > may execute in concurrent threads, so races can appear when you access
> > > members of the ecap_cnt_dev structure in these callbacks.
> > > 
> > > Take for example this section of ecap_cnt_enable_write():
> > > 
> > >          if (enable == ecap_dev->enabled)
> > >                  return 0;
> > >          if (enable)
> > >                  ecap_cnt_capture_enable(counter);
> > >          else
> > >                  ecap_cnt_capture_disable(counter);
> > >          ecap_dev->enabled = enable
> > > 
> > > Suppose two threads try to enable the count capture. A race condition is
> > > present where the two threads could see ecap_dev->enabled as false and
> > > both proceed to call ecap_cnt_capture_enable(). This results in
> > > pm_runtime_get_sync() bumping the usage count twice and we're left with
> > > a mismatch the next time ecap_cnt_capture_disable() is called.
> > > 
> > > William Breathitt Gray
> > 
> > OK, If I understand well there's the same problem with IO access with
> > regmap ?
> > Julien
> 
> [ERRATUM] It seems that some spinlock is already used by regmap API.
> So, only the 'enabled' member needs a lock.

Keep in mind that although individual regmap API calls can be considered
atomic, the order of the calls made could be a point of race if there
are concurrent threads. However, in your particular driver it looks
you're fine as long as access to 'enabled' is protected.

William Breathitt Gray

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