linux-kernel - Re: [PATCH v3 2/2] usb: host: ehci-tegra: Avoid getting the same reset twice

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: <572A3008.4020602@wwwdotorg.org>
Date:	Wed, 4 May 2016 11:23:20 -0600
From:	Stephen Warren <swarren@...dotorg.org>
To:	Thierry Reding <thierry.reding@...il.com>
Cc:	Alan Stern <stern@...land.harvard.edu>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Alexandre Courbot <gnurou@...il.com>,
	Jon Hunter <jonathanh@...dia.com>, linux-usb@...r.kernel.org,
	linux-tegra@...r.kernel.org, linux-kernel@...r.kernel.org,
	Philipp Zabel <p.zabel@...gutronix.de>,
	Hans de Goede <hdegoede@...hat.com>
Subject: Re: [PATCH v3 2/2] usb: host: ehci-tegra: Avoid getting the same
 reset twice

On 05/04/2016 08:40 AM, Thierry Reding wrote:
> From: Thierry Reding <treding@...dia.com>
>
> Starting with commit 0b52297f2288 ("reset: Add support for shared reset
> controls") there is a reference count for reset control assertions. The
> goal is to allow resets to be shared by multiple devices and an assert
> will take effect only when all instances have asserted the reset.
>
> In order to preserve backwards-compatibility, all reset controls become
> exclusive by default. This is to ensure that reset_control_assert() can
> immediately assert in hardware.
>
> However, this new behaviour triggers the following warning in the EHCI
> driver for Tegra:
...
> The reason is that Tegra SoCs have three EHCI controllers, each with a
> separate reset line. However the first controller contains UTMI pads
> configuration registers that are shared with its siblings and that are
> reset as part of the first controller's reset. There is special code in
> the driver to assert and deassert this shared reset at probe time, and
> it does so irrespective of which controller is probed first to ensure
> that these shared registers are reset before any of the controllers are
> initialized. Unfortunately this means that if the first controller gets
> probed first, it will request its own reset line and will subsequently
> request the same reset line again (temporarily) to perform the reset.
> This used to work fine before the above-mentioned commit, but now
> triggers the new WARN.
>
> Work around this by making sure we reuse the controller's reset if the
> controller happens to be the first controller.

> diff --git a/drivers/usb/host/ehci-tegra.c b/drivers/usb/host/ehci-tegra.c

> @@ -81,15 +81,23 @@ static int tegra_reset_usb_controller(struct platform_device *pdev)

> +	bool has_utmi_pad_registers = false;
>
>   	phy_np = of_parse_phandle(pdev->dev.of_node, "nvidia,phy", 0);
>   	if (!phy_np)
>   		return -ENOENT;
>
> +	if (of_property_read_bool(phy_np, "nvidia,has-utmi-pad-registers"))
> +		has_utmi_pad_registers = true;

Isn't that just:

has_utmi_pad_registers = of_property_read_bool(phy_np,
     "nvidia,has-utmi-pad-registers");

... and then you can remove " = false" from the declaration too?

>   	if (!usb1_reset_attempted) {
>   		struct reset_control *usb1_reset;
>
> -		usb1_reset = of_reset_control_get(phy_np, "utmi-pads");
> +		if (!has_utmi_pad_registers)
> +			usb1_reset = of_reset_control_get(phy_np, "utmi-pads");
> +		else
> +			usb1_reset = tegra->rst;
...
>   		usb1_reset_attempted = true;
>   	}

This is a pre-existing issue, but what happens if the probes for two USB 
controllers run in parallel; there seems to be missing locking related 
to testing/setting usb1_reset_attempted, which could cause multiple 
parallel attempts to get the "utmi-pads" reset object, which would 
presumably cause essentially the same issue this patch is solving in 
other cases?