From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756544AbcEEIFx (ORCPT ); Thu, 5 May 2016 04:05:53 -0400 Received: from mx1.redhat.com ([209.132.183.28]:47575 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751617AbcEEIFr (ORCPT ); Thu, 5 May 2016 04:05:47 -0400 Subject: Re: [PATCH v3 2/2] usb: host: ehci-tegra: Avoid getting the same reset twice To: Thierry Reding , Stephen Warren References: <1462372800-30900-1-git-send-email-thierry.reding@gmail.com> <1462372800-30900-2-git-send-email-thierry.reding@gmail.com> <572A3008.4020602@wwwdotorg.org> <20160504202544.GB21480@mithrandir.ba.sec> Cc: Alan Stern , Greg Kroah-Hartman , Alexandre Courbot , Jon Hunter , linux-usb@vger.kernel.org, linux-tegra@vger.kernel.org, linux-kernel@vger.kernel.org, Philipp Zabel From: Hans de Goede Message-ID: Date: Thu, 5 May 2016 10:05:39 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.0 MIME-Version: 1.0 In-Reply-To: <20160504202544.GB21480@mithrandir.ba.sec> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi, On 04-05-16 22:25, Thierry Reding wrote: > On Wed, May 04, 2016 at 11:23:20AM -0600, Stephen Warren wrote: >> On 05/04/2016 08:40 AM, Thierry Reding wrote: >>> From: Thierry Reding >>> >>> 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? > > Yes. This is really only for aesthetics. The direct assignment doesn't > fit within 80 columns, and wrapping it looks ugly whichever way you do > it. > >>> 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? > > Hah! Interestingly my initial attempt at fixing this was to introduce a > lock to serialize these, because I thought that was what was going on. I > don't think this function can ever run concurrently for different > devices because the driver core already serializes probes (unless a > driver specifically requests asynchronous probing, which this one > doesn't). Why not just use the new shared reset functionality ? It is easy to use, that way you can drop some of the special handling in the driver and you're code actually reflects the hardware (which IMHO has a shared reset). Regards, Hans