linux-kernel - RE: [PATCH v3 2/4] usb: uhci: Add reset control support

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:
 <OS8PR06MB75419B5722DD375F493A7902F213A@OS8PR06MB7541.apcprd06.prod.outlook.com>
Date: Sun, 21 Sep 2025 01:45:34 +0000
From: Ryan Chen <ryan_chen@...eedtech.com>
To: Alan Stern <stern@...land.harvard.edu>
CC: Greg Kroah-Hartman <gregkh@...uxfoundation.org>, Rob Herring
	<robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley
	<conor+dt@...nel.org>, Philipp Zabel <p.zabel@...gutronix.de>,
	"linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH v3 2/4] usb: uhci: Add reset control support

> Subject: Re: [PATCH v3 2/4] usb: uhci: Add reset control support
> 
> On Fri, Sep 19, 2025 at 10:57:10AM +0800, Ryan Chen wrote:
> > Some SoCs, such as the Aspeed AST2700, require the UHCI controller to
> > be taken out of reset before it can operate. Add optional reset
> > control support to the UHCI platform driver.
> >
> > The driver now acquires an optional reset line from device tree,
> > deasserts it during probe, and asserts it again in the error path and
> > shutdown.
> >
> > Signed-off-by: Ryan Chen <ryan_chen@...eedtech.com>
> > ---
> >  drivers/usb/host/uhci-hcd.h      |  1 +
> >  drivers/usb/host/uhci-platform.c | 18 ++++++++++++++++--
> >  2 files changed, 17 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/usb/host/uhci-hcd.h b/drivers/usb/host/uhci-hcd.h
> > index 13ee2a6144b2..4326d1f3ca76 100644
> > --- a/drivers/usb/host/uhci-hcd.h
> > +++ b/drivers/usb/host/uhci-hcd.h
> > @@ -445,6 +445,7 @@ struct uhci_hcd {
> >  	short load[MAX_PHASE];			/* Periodic allocations */
> >
> >  	struct clk *clk;			/* (optional) clock source */
> > +	struct reset_control *rsts;		/* (optional) clock reset */
> >
> >  	/* Reset host controller */
> >  	void	(*reset_hc) (struct uhci_hcd *uhci);
> > diff --git a/drivers/usb/host/uhci-platform.c
> > b/drivers/usb/host/uhci-platform.c
> > index 62318291f566..f255358d6242 100644
> > --- a/drivers/usb/host/uhci-platform.c
> > +++ b/drivers/usb/host/uhci-platform.c
> > @@ -11,6 +11,7 @@
> >  #include <linux/of.h>
> >  #include <linux/device.h>
> >  #include <linux/platform_device.h>
> > +#include <linux/reset.h>
> >
> >  static int uhci_platform_init(struct usb_hcd *hcd)  { @@ -132,17
> > +133,29 @@ static int uhci_hcd_platform_probe(struct platform_device
> *pdev)
> >  		goto err_rmr;
> >  	}
> >
> > +	uhci->rsts = devm_reset_control_array_get_optional_shared(&pdev->dev);
> > +	if (IS_ERR(uhci->rsts)) {
> > +		ret = PTR_ERR(uhci->rsts);
> > +		goto err_clk;
> > +	}
> > +	ret = reset_control_deassert(uhci->rsts);
> 
> Does this work right if uhci->rsts is NULL?
> 
> > +	if (ret)
> > +		goto err_clk;
> > +
> >  	ret = platform_get_irq(pdev, 0);
> >  	if (ret < 0)
> > -		goto err_clk;
> > +		goto err_reset;
> >
> >  	ret = usb_add_hcd(hcd, ret, IRQF_SHARED);
> >  	if (ret)
> > -		goto err_clk;
> > +		goto err_reset;
> >
> >  	device_wakeup_enable(hcd->self.controller);
> >  	return 0;
> >
> > +err_reset:
> > +	if (!IS_ERR_OR_NULL(uhci->rsts))
> > +		reset_control_assert(uhci->rsts);
> 
> How could this code ever execute if uhci->rsts is an ERR_PTR?
> 
> Also, why does this code test for NULL...

Will add with following.
	if (uhci->rsts) {
		ret = reset_control_deassert(uhci->rsts);
		if (ret)
			goto err_clk;
	}

I will test it with non-resets platform.
> 
> >  err_clk:
> >  	clk_disable_unprepare(uhci->clk);
> >  err_rmr:
> > @@ -156,6 +169,7 @@ static void uhci_hcd_platform_remove(struct
> platform_device *pdev)
> >  	struct usb_hcd *hcd = platform_get_drvdata(pdev);
> >  	struct uhci_hcd *uhci = hcd_to_uhci(hcd);
> >
> > +	reset_control_assert(uhci->rsts);
> 
> when this code doesn't?

Both reset_control_assert() and reset_control_deassert() already handle 
NULL and ERR_PTR safely in the reset framework.
https://github.com/torvalds/linux/blob/master/drivers/reset/core.c#L468-L472

> 
> Alan Stern
> 
> >  	clk_disable_unprepare(uhci->clk);
> >  	usb_remove_hcd(hcd);
> >  	usb_put_hcd(hcd);
> > --
> > 2.34.1
> >