linux-kernel - Re: [PATCH] usb: phy: Restore deferred probing path

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: <20141204132200.GC7262@ulmo.nvidia.com>
Date:	Thu, 4 Dec 2014 14:22:02 +0100
From:	Thierry Reding <thierry.reding@...il.com>
To:	Sergei Shtylyov <sergei.shtylyov@...entembedded.com>
Cc:	Felipe Balbi <balbi@...com>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	linux-usb@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] usb: phy: Restore deferred probing path

On Thu, Dec 04, 2014 at 03:23:06PM +0300, Sergei Shtylyov wrote:
> Hello.
> 
> On 12/4/2014 3:06 PM, Thierry Reding wrote:
> 
> >From: Thierry Reding <treding@...dia.com>
> 
> >Commit 1290a958d48e ("usb: phy: propagate __of_usb_find_phy()'s error on
> >failure") broke platforms that rely on deferred probing to order probing
> >of PHY and host controller drivers. The reason is that the commit simply
> >propagates errors from __of_usb_find_phy(), which returns -ENODEV if no
> >PHY has been registered yet for a given device tree node. The only case
> >in which -EPROBE_DEFER would now be returned is if try_module_get() did
> >fail, which does not make sense.
> 
> >The correct thing to do is to return -EPROBE_DEFER if a PHY hasn't been
> >registered yet. The only condition under which it makes sense to return
> >-ENODEV is if the device tree node representing the PHY has been
> >disabled (via the status property) because in that case the PHY will
> >never be registered.
> 
> >This patch addresses the problem by making __of_usb_find_phy() return an
> >appropriate error code while keeping in line with the above-mentioned
> >commit to propagate error codes rather than overwriting them. At the
> >same time the check for a valid PHY is decoupled from the check for the
> >try_module_get() call and a separate error code is returned if the
> >latter fails.
> 
> >Signed-off-by: Thierry Reding <treding@...dia.com>
> >---
> >  drivers/usb/phy/phy.c | 14 ++++++++++----
> >  1 file changed, 10 insertions(+), 4 deletions(-)
> 
> >diff --git a/drivers/usb/phy/phy.c b/drivers/usb/phy/phy.c
> >index b4066a001ba0..2f9735b35338 100644
> >--- a/drivers/usb/phy/phy.c
> >+++ b/drivers/usb/phy/phy.c
> [...]
> >@@ -190,10 +193,13 @@ struct usb_phy *devm_usb_get_phy_by_phandle(struct device *dev,
> >  	spin_lock_irqsave(&phy_lock, flags);
> >
> >  	phy = __of_usb_find_phy(node);
> >-	if (IS_ERR(phy) || !try_module_get(phy->dev->driver->owner)) {
> >-		if (!IS_ERR(phy))
> >-			phy = ERR_PTR(-EPROBE_DEFER);
> >+	if (IS_ERR(phy)) {
> >+		devres_free(ptr);
> >+		goto err1;
> >+	}
> >
> >+	if (!try_module_get(phy->dev->driver->owner)) {
> >+		phy = ERR_PTR(-ENODEV);
> >  		devres_free(ptr);
> >  		goto err1;
> >  	}
> 
>    I think devres_free() should now be called in one place, under the new
> 'err2' label.

That'd make things very confusing, though. devres_free() should only be
called on error, whereas both err1 and err0 are shared with the success
case as well. If we were to do what you suggest we'd end up with
something like this:

		...

		phy = __of_usb_find_phy(node);
		if (IS_ERR(phy))
			goto err2;

		if (!try_module_get(...)) {
			phy = ERR_PTR(-ENODEV);
			goto err2;
		}

		*ptr = phy;
		devres_add(dev, ptr);

		get_device(phy->dev);

		goto err1;

	err2:
		devres_free(ptr);

	err1:
		spin_unlock_irqrestore(&phy_lock, flags);

	err0:
		of_node_put(node);

		return phy;
	}

That's pretty spaghetti-like.

Thierry

Content of type "application/pgp-signature" skipped