Re: [PATCH -next] usb: ehci: Read CMD_RUN instead of STS_HALT in ehci_halt with ZX-200

From: Greg KH <gregkh@linuxfoundation.org>
To: xy521521 <xy521521@gmail.com>
Cc: stern@rowland.harvard.edu, linux-usb@vger.kernel.org,
	linux-kernel@vger.kernel.org, EdisonZhang@zhaoxin.com,
	PeterWu@zhaoxin.com, cobechen@zhaoxin.com, wugaoquan@kylinos.cn,
	Hongyu Xie <xiehongyu1@kylinos.cn>
Subject: Re: [PATCH -next] usb: ehci: Read CMD_RUN instead of STS_HALT in ehci_halt with ZX-200
Date: Sat, 23 Jul 2022 11:31:40 +0200	[thread overview]
Message-ID: <Ytu//IorhlAiunLa@kroah.com> (raw)
In-Reply-To: <20220723073805.23491-1-xy521521@gmail.com>

On Sat, Jul 23, 2022 at 03:38:05PM +0800, xy521521 wrote:
> From: Hongyu Xie <xiehongyu1@kylinos.cn>
> 
> Forcing HC to halt state is ensured by reading STS_HALT field in USBSTS
> register every microsecond(2ms in total) after clearing CMD_RUN filed in
> USBCMD register during initialization.
> 
> But sometimes the STS_HALT field in USBSTS is not set during that 2ms, i.e
> ehci_handshake returns -ETIMEDOUT. And host controller won't work after
> that, so does the device attached on it. This was first found on a system
> with ZX-200 HC on it.
> 
> The interesting part is that if you ignore -ETIMEOUT returned from
> ehci_handshak or read CMD_RUN instead and continue the initialization, the
> HC works just fine.
> 
> So read CMD_RUN instead.

You do not define what a "ZX-200" is, please do so.

This feels like a bug in the hardware, right?  If so, why not make a new
quirk flag for it and handle it that way as odds are it probably is in
other devices based on this silicon.

> 
> Signed-off-by: Hongyu Xie <xiehongyu1@kylinos.cn>

Is thie relevant for stable kernels?  If so, how far back?

> ---
>  drivers/usb/host/ehci-hcd.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c
> index 684164fa9716..a935cfb79bcc 100644
> --- a/drivers/usb/host/ehci-hcd.c
> +++ b/drivers/usb/host/ehci-hcd.c
> @@ -181,6 +181,7 @@ static int tdi_in_host_mode (struct ehci_hcd *ehci)
>  static int ehci_halt (struct ehci_hcd *ehci)
>  {
>  	u32	temp;
> +	struct pci_dev  *pci_dev = to_pci_dev(ehci_to_hcd(ehci)->self.controller);

Wait, how do you know this is a PCI device?  What happens when you run
this on a ehci controller that is not a PCI device?  How well did you
test this change?

>  
>  	spin_lock_irq(&ehci->lock);
>  
> @@ -204,8 +205,14 @@ static int ehci_halt (struct ehci_hcd *ehci)
>  	spin_unlock_irq(&ehci->lock);
>  	synchronize_irq(ehci_to_hcd(ehci)->irq);
>  
> -	return ehci_handshake(ehci, &ehci->regs->status,
> -			  STS_HALT, STS_HALT, 16 * 125);
> +	if (((pci_dev->vendor == PCI_VENDOR_ID_ZHAOXIN) &&
> +		(pci_dev->device == 0x3104) &&
> +		((pci_dev->revision & 0xf0) == 0x90)))
> +		return ehci_handshake(ehci, &ehci->regs->command, CMD_RUN,
> +				0, 16 * 125);

What is the "0" here for?

> +	else

No need for the else statement, checkpatch should have caught that,
right?

thanks,

greg k-h