Message-ID: <1489591269.2528.47.camel@pengutronix.de>
Subject: Re: [PATCH v2 13/14] usb: host: ehci-st: simplify optional reset
 handling
From: Philipp Zabel <p.zabel@pengutronix.de>
To: Alan Stern <stern@rowland.harvard.edu>
Cc: linux-usb@vger.kernel.org, Patrice Chotard <patrice.chotard@st.com>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        linux-kernel@vger.kernel.org
Date: Wed, 15 Mar 2017 16:21:09 +0100
In-Reply-To: <Pine.LNX.4.44L0.1703151031050.4455-100000@netrider.rowland.org>
References: <Pine.LNX.4.44L0.1703151031050.4455-100000@netrider.rowland.org>
Content-Type: text/plain; charset="UTF-8"
Mime-Version: 1.0
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 3070
Lines: 87

Hi Alan,

On Wed, 2017-03-15 at 10:35 -0400, Alan Stern wrote:
> On Wed, 15 Mar 2017, Philipp Zabel wrote:
> 
> > As of commit bb475230b8e5 ("reset: make optional functions really
> > optional"), the reset framework API calls use NULL pointers to describe
> > optional, non-present reset controls.
> 
> What does it use to describe genuine errors?

Negative error pointers, as before. The only difference is that instead
of returning -ENOENT if no resets are specified in the device tree, the
optional reset_control_get variants now return NULL.

> > This allows to return errors from devm_reset_control_get_optional_shared
> > unconditionally.
> > 
> > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> > ---
> >  drivers/usb/host/ehci-st.c | 8 ++------
> >  1 file changed, 2 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/usb/host/ehci-st.c b/drivers/usb/host/ehci-st.c
> > index be4a2788fc582..12e803d2c98df 100644
> > --- a/drivers/usb/host/ehci-st.c
> > +++ b/drivers/usb/host/ehci-st.c
> > @@ -210,18 +210,14 @@ static int st_ehci_platform_probe(struct platform_device *dev)
> >  		devm_reset_control_get_optional_shared(&dev->dev, "power");
> >  	if (IS_ERR(priv->pwr)) {
> >  		err = PTR_ERR(priv->pwr);
> > -		if (err == -EPROBE_DEFER)
> > -			goto err_put_clks;
> > -		priv->pwr = NULL;
> > +		goto err_put_clks;
> >  	}
> >  
> >  	priv->rst =
> >  		devm_reset_control_get_optional_shared(&dev->dev, "softreset");
> >  	if (IS_ERR(priv->rst)) {
> >  		err = PTR_ERR(priv->rst);
> > -		if (err == -EPROBE_DEFER)
> > -			goto err_put_clks;
> > -		priv->rst = NULL;
> > +		goto err_put_clks;
> >  	}
> >  
> >  	if (pdata->power_on) {
> 
> These changes do not agree with the patch description.  If any sort of 
> error besides EPROBE_DEFER occurs then:
>
> 	the old code sets priv->pwr or priv->rst to NULL and continues;
> 
> 	the new code returns with an error.
> 
> The only way the patch could be equivalent to the existing code would
> be if devm_reset_control_get_optional_shared() returns no errors other
> than EPROBE_DEFER.  But the patch description doesn't say this.
> 
> Alan Stern

You are right, devm_reset_control_get_optional_shared can return:

-ENOMEM, returned by devres_alloc in __devm_reset_control_get,

-EILSEQ, returned by of_property_match_string in __of_reset_control_get
if the "reset-names" DT property is broken,

-EINVAL, returned by of_parse_phandle_with_args in
__of_reset_control_get, if there is a reset phandle specified in the
device tree but it is pointing to an invalid reset controller node
(missing "#reset-cells" property or number of phandle arguments does not
match).

-EINVAL, returned by the reset controller driver's choice of of_xlate,
also if the reset is specified but somehow invalid.

So yes, if there was a genuine error in the device tree, we would now
return the error instead of silently ignoring it as before. I assume
that these errors were never intended to be ignored, it just happened to
be a hassle to separate them from the -ENOENT condition with the old
API.

regards
Philipp