linux-kernel - Re: [PATCH] usb: dwc3: of-simple: use managed and shared reset control

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: <1522742421.5089.1.camel@pengutronix.de>
Date:   Tue, 03 Apr 2018 10:00:21 +0200
From:   Philipp Zabel <p.zabel@...gutronix.de>
To:     Masahiro Yamada <yamada.masahiro@...ionext.com>,
        Felipe Balbi <felipe.balbi@...ux.intel.com>,
        linux-usb@...r.kernel.org
Cc:     Vivek Gautam <vivek.gautam@...eaurora.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Felipe Balbi <balbi@...nel.org>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] usb: dwc3: of-simple: use managed and shared reset
 control

On Thu, 2018-03-29 at 15:07 +0900, Masahiro Yamada wrote:
> This driver handles the reset control in a common manner; deassert
> resets before use, assert them after use.  There is no good reason
> why it should be exclusive.

Is this preemptive cleanup, or do you have hardware on the horizon that
shares these reset lines with other peripherals?

> Also, use devm_ for clean-up.
> 
> Signed-off-by: Masahiro Yamada <yamada.masahiro@...ionext.com>
> ---
> 
> CCing Philipp Zabel.
> I see his sob in commit 06c47e6286d5.

At the time I was concerned with the reset_control_array addition and
didn't look closely at the exclusive vs shared issue.

>  drivers/usb/dwc3/dwc3-of-simple.c | 7 ++-----
>  1 file changed, 2 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/usb/dwc3/dwc3-of-simple.c b/drivers/usb/dwc3/dwc3-of-simple.c
> index e54c362..bd6ab65 100644
> --- a/drivers/usb/dwc3/dwc3-of-simple.c
> +++ b/drivers/usb/dwc3/dwc3-of-simple.c
> @@ -91,7 +91,7 @@ static int dwc3_of_simple_probe(struct platform_device *pdev)
>  	platform_set_drvdata(pdev, simple);
>  	simple->dev = dev;
>  
> -	simple->resets = of_reset_control_array_get_optional_exclusive(np);
> +	simple->resets = devm_reset_control_array_get_optional_shared(dev);

>From the usage in the driver, it does indeed look like _shared reset
usage is appropriate. I assume that the hardware has no need for the
reset to be asserted right before probe or after remove, it just
requires that the reset line is kept deasserted while the driver is
probed.

>  	if (IS_ERR(simple->resets)) {
>  		ret = PTR_ERR(simple->resets);
>  		dev_err(dev, "failed to get device resets, err=%d\n", ret);
> @@ -100,7 +100,7 @@ static int dwc3_of_simple_probe(struct platform_device *pdev)
>  
>  	ret = reset_control_deassert(simple->resets);
>  	if (ret)
> -		goto err_resetc_put;
> +		return ret;
>  
>  	ret = dwc3_of_simple_clk_init(simple, of_count_phandle_with_args(np,
>  						"clocks", "#clock-cells"));
> @@ -126,8 +126,6 @@ static int dwc3_of_simple_probe(struct platform_device *pdev)
>  err_resetc_assert:
>  	reset_control_assert(simple->resets);
>  
> -err_resetc_put:
> -	reset_control_put(simple->resets);
>  	return ret;
>  }
>  
> @@ -146,7 +144,6 @@ static int dwc3_of_simple_remove(struct platform_device *pdev)
>  	simple->num_clocks = 0;
>  
>  	reset_control_assert(simple->resets);
> -	reset_control_put(simple->resets);
>  
>  	pm_runtime_put_sync(dev);
>  	pm_runtime_disable(dev);

Changing to devm_ changes the order here. Whether or not it could be a
problem to assert the reset only after pm_runtime_put (or potentially
never), I can't say. I assume this is a non-issue, but somebody who
knows the hardware better would have to decide.

regards
Philipp