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:
 <OS8PR06MB7541897BB2E71E8847B04C0CF212A@OS8PR06MB7541.apcprd06.prod.outlook.com>
Date: Mon, 22 Sep 2025 02:03:40 +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
> 
> > 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?

Update the information that I test with ast2600 (which is no rsts)
When uhci->rsts is null, reset_control_deassert(uhci->rsts) will return 0.
So It still work. I think this is support both rsts and non-rsts platform.

https://github.com/torvalds/linux/blob/master/drivers/reset/core.c#L468-L472


> >
> > > +	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.

No need to check if (uhci->rsts) with test with non-rsts 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
> > >