linux-kernel - Re: [PATCH v4 1/7] clk: kona: allow nested ccu_write

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]

Date:	Mon, 02 Jun 2014 14:05:56 -0700
From:	Mike Turquette <mturquette@...aro.org>
To:	Alex Elder <elder@...aro.org>, mporter@...aro.org,
	bcm@...thebug.org
Cc:	linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4 1/7] clk: kona: allow nested ccu_write_enable() requests

Quoting Alex Elder (2014-05-30 20:46:46)
> On 05/30/2014 06:28 PM, Mike Turquette wrote:
> > Quoting Alex Elder (2014-05-30 13:53:02)
> >> Use a counter rather than a Boolean to track whether write access to
> >> a CCU has been enabled or not.  This will allow more than one of
> >> these requests to be nested.
> >>
> >> Note that __ccu_write_enable() and __ccu_write_disable() calls all
> >> come in pairs, and they are always surrounded immediately by calls
> >> to ccu_lock() and ccu_unlock().
> >>
> >> Signed-off-by: Alex Elder <elder@...aro.org>
> >> ---
> >>  drivers/clk/bcm/clk-kona.c | 14 ++++----------
> >>  drivers/clk/bcm/clk-kona.h |  2 +-
> >>  2 files changed, 5 insertions(+), 11 deletions(-)
> >>
> >> diff --git a/drivers/clk/bcm/clk-kona.c b/drivers/clk/bcm/clk-kona.c
> >> index 95af2e6..ee8e988 100644
> >> --- a/drivers/clk/bcm/clk-kona.c
> >> +++ b/drivers/clk/bcm/clk-kona.c
> >> @@ -170,13 +170,8 @@ static inline void ccu_unlock(struct ccu_data *ccu, unsigned long flags)
> >>   */
> >>  static inline void __ccu_write_enable(struct ccu_data *ccu)
> > 
> > Per Documentation/CodingStyle, chapter 15, "the inline disease", it
> > might be best to not inline these functions.
> 
> This was not intentional.  I normally only inline things
> defined in header files, and maybe this is an artifact of
> having been in a header at one time.  I don't know, I'll get
> rid of the inline.
> 
> > 
> >>  {
> >> -       if (ccu->write_enabled) {
> >> -               pr_err("%s: access already enabled for %s\n", __func__,
> >> -                       ccu->name);
> >> -               return;
> >> -       }
> >> -       ccu->write_enabled = true;
> >> -       __ccu_write(ccu, 0, CCU_ACCESS_PASSWORD | 1);
> >> +       if (!ccu->write_enabled++)
> >> +               __ccu_write(ccu, 0, CCU_ACCESS_PASSWORD | 1);
> >>  }
> >>  
> >>  static inline void __ccu_write_disable(struct ccu_data *ccu)
> >> @@ -186,9 +181,8 @@ static inline void __ccu_write_disable(struct ccu_data *ccu)
> >>                         ccu->name);
> >>                 return;
> >>         }
> >> -
> >> -       __ccu_write(ccu, 0, CCU_ACCESS_PASSWORD);
> >> -       ccu->write_enabled = false;
> >> +       if (!--ccu->write_enabled)
> >> +               __ccu_write(ccu, 0, CCU_ACCESS_PASSWORD);
> > 
> > What happens if calls to __ccu_write_enable and __ccu_write_disable are
> > unbalanced? It would be better to catch that case and throw a WARN:
> 
> You can't see it in the diff, but that's what happens
> (well, it's a pr_err(), not a WARN()).   I think a WARN()
> is probably right in this case though.
> 
> >       if (WARN_ON(ccu->write_enabled == 0))
> >               return;
> > 
> >       if (--ccu->write_enabled > 0)
> >               return;
> > 
> >       __ccu_write(ccu, 0, CCU_ACCESS_PASSWORD);
> > 
> >>  }
> >>  
> >>  /*
> >> diff --git a/drivers/clk/bcm/clk-kona.h b/drivers/clk/bcm/clk-kona.h
> >> index 2537b30..e9a8466 100644
> >> --- a/drivers/clk/bcm/clk-kona.h
> >> +++ b/drivers/clk/bcm/clk-kona.h
> >> @@ -478,7 +478,7 @@ struct ccu_policy {
> >>  struct ccu_data {
> >>         void __iomem *base;     /* base of mapped address space */
> >>         spinlock_t lock;        /* serialization lock */
> >> -       bool write_enabled;     /* write access is currently enabled */
> >> +       u32 write_enabled;      /* write access enable count */
> > 
> > Why u32? An unsigned int will do just nicely here.
> 
> That's a preference of mine.  I almost always favor
> using u32, etc. because they are compact, and explicit
> about the size and signedness.  I "know" an int is 32
> bits, but I still prefer being explicit.
> 
> I'll interpret this as a preference on your part for
> unsigned int, and I have no problem making that change.

It's not a big deal, I was just curious why. Feel free to use whatever
solution you prefer here.

Regards,
Mike

> 
>                                         -Alex
> 
> > Regards,
> > Mike
> > 
> >>         struct ccu_policy policy;
> >>         struct list_head links; /* for ccu_list */
> >>         struct device_node *node;
> >> -- 
> >> 1.9.1
> >>
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/