linux-kernel - Re: [RFC PATCH 12/21] reset: uniphier: add core support for UniPhier reset driver

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

Date:	Wed, 11 May 2016 12:34:34 +0200
From:	Philipp Zabel <p.zabel@...gutronix.de>
To:	Masahiro Yamada <yamada.masahiro@...ionext.com>
Cc:	linux-clk@...r.kernel.org, Arnd Bergmann <arnd@...db.de>,
	Guenter Roeck <linux@...ck-us.net>,
	Kalle Valo <kvalo@...eaurora.org>,
	Jiri Slaby <jslaby@...e.com>,
	Mauro Carvalho Chehab <mchehab@....samsung.com>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	"David S. Miller" <davem@...emloft.net>,
	linux-arm-kernel <linux-arm-kernel@...ts.infradead.org>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Andrew Morton <akpm@...ux-foundation.org>
Subject: Re: [RFC PATCH 12/21] reset: uniphier: add core support for
 UniPhier reset driver

Am Mittwoch, den 11.05.2016, 11:46 +0900 schrieb Masahiro Yamada:
> Hi Philipp,
> 
> 
> 2016-05-10 22:54 GMT+09:00 Philipp Zabel <p.zabel@...gutronix.de>:
> > Am Dienstag, den 10.05.2016, 18:50 +0900 schrieb Masahiro Yamada:
> > [...]
> >> +static int uniphier_reset_update(struct reset_controller_dev *rcdev,
> >> +                              unsigned long id, bool assert)
> >> +{
> >> +     struct uniphier_reset_priv *priv = to_uniphier_reset_priv(rcdev);
> >> +     const struct uniphier_reset_data *p;
> >> +     bool handled = false;
> >> +
> >> +     for (p = priv->data; p->id != UNIPHIER_RESET_ID_END; p++) {
> >> +             unsigned int val;
> >> +             int ret;
> >> +
> >> +             if (p->id != id)
> >> +                     continue;
> >> +
> >> +             val = p->deassert_val;
> >> +             if (assert)
> >> +                     val = ~val;
> >> +
> >> +             ret = regmap_write_bits(priv->regmap, p->reg, p->mask, val);
> >
> > What is the difference between mask and deassert_val? Couldn't you just
> > assign
> >         val = assert ? 0 : p->mask;
> > ?
> 
> 
> I need to handle both active-high resets and active-low resets.

I see. I hadn't seen any active-high resets in your lists yet. If you
need them, you obviously can't simplify this much.

> I thought two ways to do that.
> 
> 
> [1] Have mask and a flag indicating active-low/active-high,
>     like follows:
> 
>      if (flag & UNIPHIER_RST_ACTIVE_LOW)
>           assert = !assert;
>      val = assert ? 0 : p->mask;
> 
> [2] Have mask and deassert_val as in this patch
>
> [1] cannot manage a case where one register contains
> active-low bits and active-high bits mixed in it.
> 
> 
> 
> For example, let's say reset bits are BIT(1) and BIT(0).
> 
> [2] can solve this case as follows:
> 
> (a) If both bit1 and bit0 are active-high.
>      .mask = BIT(1) | BIT(0);
>      .deassert_val = 0;
> 
> (b) If bit1 is active-high and bit0 is active-low
>      .mask = BIT(1) | BIT(0);
>      .deassert_val =  BIT(0);
> 
> (c) If bit1 is active-low and bit0 is active-high
>      .mask = BIT(1) | BIT(0);
>      .deassert_val =  BIT(1);
> 
> (d) If both bit1 and bit0 are active-low
>      .mask = BIT(1) | BIT(0);
>      .deassert_val = BIT(1) | BIT(0);
> 
> 
> I have not been hit by such a complicated case though.

In general it is a good idea not to add complexity for theoretical
cases, on the other hand [1] isn't really much less complex. Can I ask
you to invert the logic, though, and use assert_val instead?

regards
Philipp