Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Fri, 23 Dec 2022 15:01:54 +0100
From:   Johan Hovold <johan@kernel.org>
To:     Matthias Kaehlcke <mka@chromium.org>
Cc:     Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        linux-kernel@vger.kernel.org, linux-usb@vger.kernel.org,
        Stefan Wahren <stefan.wahren@i2se.com>, stable@vger.kernel.org,
        Douglas Anderson <dianders@chromium.org>,
        Ravi Chandra Sadineni <ravisadineni@chromium.org>
Subject: Re: [PATCH v2 1/2] usb: misc: onboard_usb_hub: Don't create platform
 devices for DT nodes without 'vdd-supply'
Message-ID: <Y6W00vQm3jfLflUJ@hovoldconsulting.com>
References: <20221222022605.v2.1.If5e7ec83b1782e4dffa6ea759416a27326c8231d@changeid>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20221222022605.v2.1.If5e7ec83b1782e4dffa6ea759416a27326c8231d@changeid>
Precedence: bulk

On Thu, Dec 22, 2022 at 02:26:44AM +0000, Matthias Kaehlcke wrote:
> The primary task of the onboard_usb_hub driver is to control the
> power of an onboard USB hub. The driver gets the regulator from the
> device tree property "vdd-supply" of the hub's DT node. Some boards
> have device tree nodes for USB hubs supported by this driver, but
> don't specify a "vdd-supply". This is not an error per se, it just
> means that the onboard hub driver can't be used for these hubs, so
> don't create platform devices for such nodes.
> 
> This change doesn't completely fix the reported regression. It
> should fix it for the RPi 3 B Plus and boards with similar hub
> configurations (compatible DT nodes without "vdd-supply"), boards
> that actually use the onboard hub driver could still be impacted
> by the race conditions discussed in that thread. Not creating the
> platform devices for nodes without "vdd-supply" is the right
> thing to do, independently from the race condition, which will
> be fixed in future patch.
> 
> Fixes: 8bc063641ceb ("usb: misc: Add onboard_usb_hub driver")
> Link: https://lore.kernel.org/r/d04bcc45-3471-4417-b30b-5cf9880d785d@i2se.com/
> Reported-by: Stefan Wahren <stefan.wahren@i2se.com>
> Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> ---
> 
> Changes in v2:
> - don't create platform devices when "vdd-supply" is missing,
>   rather than returning an error from _find_onboard_hub()
> - check for "vdd-supply" not "vdd" (Johan)

Please try to remember to CC people providing feedback on your patches.

> - updated subject and commit message
> - added 'Link' tag (regzbot)
> 
>  drivers/usb/misc/onboard_usb_hub_pdevs.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/drivers/usb/misc/onboard_usb_hub_pdevs.c b/drivers/usb/misc/onboard_usb_hub_pdevs.c
> index ed22a18f4ab7..8cea53b0907e 100644
> --- a/drivers/usb/misc/onboard_usb_hub_pdevs.c
> +++ b/drivers/usb/misc/onboard_usb_hub_pdevs.c
> @@ -101,6 +101,19 @@ void onboard_hub_create_pdevs(struct usb_device *parent_hub, struct list_head *p
>  			}
>  		}
>  
> +		/*
> +		 * The primary task of the onboard_usb_hub driver is to control
> +		 * the power of an USB onboard hub. Some boards have device tree
> +		 * nodes for USB hubs supported by this driver, but don't
> +		 * specify a "vdd-supply", which is needed by the driver. This is
> +		 * not a DT error per se, it just means that the onboard hub
> +		 * driver can't be used with these nodes, so don't create a
> +		 * a platform device for such a node.
> +		 */
> +		if (!of_get_property(np, "vdd-supply", NULL) &&
> +		    !of_get_property(npc, "vdd-supply", NULL))
> +			goto node_put;

So as I mentioned elsewhere, this doesn't look right. It is the
responsibility of the platform driver to manage its resources and it may
not even need a supply.

I see now that you have already matched on the compatible property above
so that you only create the platform device for the devices that (may)
need it. 

It seems the assumptions that this driver was written under needs to be
revisited.

> +
>  		pdev = of_platform_device_create(np, NULL, &parent_hub->dev);
>  		if (!pdev) {
>  			dev_err(&parent_hub->dev,

Johan