linux-kernel - Re: [PATCH] gpio: gpio-dwapb: add optional reset

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]

Message-ID: <1507719418.22082.1.camel@pengutronix.de>
Date:   Wed, 11 Oct 2017 12:56:58 +0200
From:   Philipp Zabel <p.zabel@...gutronix.de>
To:     Linus Walleij <linus.walleij@...aro.org>,
        Alan Tull <atull@...nel.org>
Cc:     Hoan Tran <hotran@....com>,
        "linux-gpio@...r.kernel.org" <linux-gpio@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] gpio: gpio-dwapb: add optional reset

Hi Alan, Linus,

On Wed, 2017-10-11 at 10:31 +0200, Linus Walleij wrote:
> On Tue, Oct 10, 2017 at 11:26 PM, Alan Tull <atull@...nel.org> wrote:
> 
> > Some platforms require reset to be released to allow register
> > access.
> > 
> > Signed-off-by: Alan Tull <atull@...nel.org>
> 
> Fair enough.
> 
> (...)
> > +       rst = devm_reset_control_get_optional_exclusive(dev, NULL);

The way this reset control is used, it looks like you could use _shared
instead of _exclusive here. This relaxes the guarantees made by the API
a bit and may allow this driver to work with more reset controllers.

> > +       if (IS_ERR(rst)) {
> > +               if (PTR_ERR(rst) == -EPROBE_DEFER)
> > +                       return PTR_ERR(rst);

The _optional variant of reset_control_get returns NULL if no reset is
specified in the device tree. If an error value is returned, it is
always an actual error (invalid device tree contents, reset is specified
in the device tree but the driver returns an error, etc.).
This should just be:

	if (IS_ERR(rst))
		return PTR_ERR(rst);

> > +       } else {
> > +               reset_control_deassert(rst);
> > +               gpio->rst = rst;

And this should be made unconditional. reset_control_deassert just
ignores rst == NULL.

> > +       }
> 
> I do not see why any error other than -EPROBE_DEFER
> should be ignored?
> 
> I guess the _optional API returns NULL if there is no
> reset line so it should be fine to just return the error on
> any error.

Correct. The _optional API together with NULL reset control handles
allows to simplify handling of optional resets in the consumer drivers.

> > +       if (gpio->rst)
> > +               reset_control_assert(gpio->rst);
> 
> Is this the right way to handle an optional reset line?

Just as the deassert above, this should be made unconditional.

regards
Philipp