Re: [PATCH v2] usb: ohci: add check for start frame in host controller functional states

Re: [PATCH v2] usb: ohci: add check for start frame in host controller functional states

[Alan Stern]

On Wed, Sep 29, 2021 at 06:09:27PM +0800, Yinbo Zhu wrote:
> The pm states of ohci controller include UsbOperational, UsbReset, UsbSuspend

Those aren't really PM states.  The specification calls them "USB 
states".

> , and UsbResume. Among them, only the UsbOperational state supports launching
--^

This comma should come directly after the word "launching", with no 
space in between.

> the start frame for host controller according the ohci protocol spec, but in
> S3/S4 press test procedure, it may happen that the start frame was launched

What is the S3/S4 press test?  I don't recall hearing of it before.

> in other pm states and cause ohci works abnormally then kernel will allways
> report rcu CallTrace. This patch was to add check for start frame in host
> controller functional states for fixing above issue.

The patch doesn't check for start of frames, that is, it doesn't check 
the INTR_SF bit in the intrstatus register.  Instead it checks whether 
controller is in the UsbOperational state.  And the patch also sets 
INTR_SF in the intrdisable register -- you do not mention this in the 
description.

> Signed-off-by: Yinbo Zhu <zhuyinbo@loongson.cn>
> ---
> Change in v2:
> 		Revise the punctuation.	
> 
>  drivers/usb/host/ohci-hcd.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/usb/host/ohci-hcd.c b/drivers/usb/host/ohci-hcd.c
> index 1f5e693..f0aeae5 100644
> --- a/drivers/usb/host/ohci-hcd.c
> +++ b/drivers/usb/host/ohci-hcd.c
> @@ -881,6 +881,13 @@ static irqreturn_t ohci_irq (struct usb_hcd *hcd)
>  	struct ohci_regs __iomem *regs = ohci->regs;
>  	int			ints;
>  
> +	ints = ohci_readl(ohci, &regs->control);
> +
> +	if ((ints & OHCI_CTRL_HCFS) != OHCI_USB_OPER) {
> +		ohci_writel(ohci, OHCI_INTR_SF, &regs->intrdisable);
> +		(void)ohci_readl(ohci, &regs->intrdisable);
> +	}

The driver is already supposed to prevent this problem by writing the 
OHCI_INTR_SF flag to the intrdisable register when start-of-frame 
interrupts aren't needed.  Maybe what you should do is change this code 
lower down in ohci_irq():

	if ((ints & OHCI_INTR_SF) != 0 && !ohci->ed_rm_list
			&& ohci->rh_state == OHCI_RH_RUNNING)
		ohci_writel (ohci, OHCI_INTR_SF, &regs->intrdisable);

by getting rid of the test for OHCI_RH_RUNNING.

Alan Stern

> +
>  	/* Read interrupt status (and flush pending writes).  We ignore the
>  	 * optimization of checking the LSB of hcca->done_head; it doesn't
>  	 * work on all systems (edge triggering for OHCI can be a factor).
> -- 
> 1.8.3.1
> 

Re: [PATCH v2] usb: ohci: add check for start frame in host controller functional states

[Alan Stern]

On Wed, Sep 29, 2021 at 10:59:05AM -0400, Alan Stern wrote:
> On Wed, Sep 29, 2021 at 06:09:27PM +0800, Yinbo Zhu wrote:
> > The pm states of ohci controller include UsbOperational, UsbReset, UsbSuspend
> 
> Those aren't really PM states.  The specification calls them "USB 
> states".
> 
> > , and UsbResume. Among them, only the UsbOperational state supports launching
> --^
> 
> This comma should come directly after the word "launching", with no 

Sorry, I meant the word "UsbSuspend", not "launching".

Alan Stern

Re: [PATCH v2] usb: ohci: add check for start frame in host controller functional states

[zhuyinbo]

在 2021/9/29 下午10:59, Alan Stern 写道:
> On Wed, Sep 29, 2021 at 06:09:27PM +0800, Yinbo Zhu wrote:
>> The pm states of ohci controller include UsbOperational, UsbReset, UsbSuspend
> > Those aren't really PM states.  The specification calls them "USB 
> > states".
I had replace "PM states" with "USB states" in v3 version patch
>
>> , and UsbResume. Among them, only the UsbOperational state supports launching
> --^
>
> > This comma should come directly after the word "launching", with no 
> > space in between.
> okay, I got it.
>> the start frame for host controller according the ohci protocol spec, but in
>> S3/S4 press test procedure, it may happen that the start frame was launched
> > What is the S3/S4 press test?  I don't recall hearing of it before.
S3 test is that suspend to memory, S4 test is that system suspend to disk.
>
>> in other pm states and cause ohci works abnormally then kernel will allways
>> report rcu CallTrace. This patch was to add check for start frame in host
>> controller functional states for fixing above issue.
> > The patch doesn't check for start of frames, that is, it doesn't check 
> > the INTR_SF bit in the intrstatus register.  Instead it checks whether 
> > controller is in the UsbOperational state.  And the patch also sets 
> > INTR_SF in the intrdisable register -- you do not mention this in the 
> > description.
> okay, I got it, and I had made a appropriate commit changes that according to what you advice in v3 version patch.
>> Signed-off-by: Yinbo Zhu <zhuyinbo@loongson.cn>
>> ---
>> Change in v2:
>> 		Revise the punctuation.	
>>
>>   drivers/usb/host/ohci-hcd.c | 7 +++++++
>>   1 file changed, 7 insertions(+)
>>
>> diff --git a/drivers/usb/host/ohci-hcd.c b/drivers/usb/host/ohci-hcd.c
>> index 1f5e693..f0aeae5 100644
>> --- a/drivers/usb/host/ohci-hcd.c
>> +++ b/drivers/usb/host/ohci-hcd.c
>> @@ -881,6 +881,13 @@ static irqreturn_t ohci_irq (struct usb_hcd *hcd)
>>   	struct ohci_regs __iomem *regs = ohci->regs;
>>   	int			ints;
>>   
>> +	ints = ohci_readl(ohci, &regs->control);
>> +
>> +	if ((ints & OHCI_CTRL_HCFS) != OHCI_USB_OPER) {
>> +		ohci_writel(ohci, OHCI_INTR_SF, &regs->intrdisable);
>> +		(void)ohci_readl(ohci, &regs->intrdisable);
>> +	}
> > The driver is already supposed to prevent this problem by writing the 
> > OHCI_INTR_SF flag to the intrdisable register when start-of-frame 
> > interrupts aren't needed.  Maybe what you should do is change this code 
> > lower down in ohci_irq():
>
> >	if ((ints & OHCI_INTR_SF) != 0 && !ohci->ed_rm_list
> >			&& ohci->rh_state == OHCI_RH_RUNNING)
> >		ohci_writel (ohci, OHCI_INTR_SF, &regs->intrdisable);
>
> > by getting rid of the test for OHCI_RH_RUNNING.
>
> > Alan Stern

Hi Alan Stern,

       Above code condition that the key point is ohci->ed_rm_list is 
NULL, but my target of my patch is to disable SoF interrupt when hc isn't

UsbOperation state and it doesn't matter with that ohci->ed_rm_list 
whether is NULL. In addition the ohci->rh_state is to describe root hub

state that include halt, suspend,run and  it isn't exactly the same as 
hc state.

      following code is the v3 version patch,  please you check.

         ohci_work(ohci);
-       if ((ints & OHCI_INTR_SF) != 0 && !ohci->ed_rm_list
-                       && ohci->rh_state == OHCI_RH_RUNNING)
+
+       ctl = ohci_readl(ohci, &regs->control);
+
+       if (((ints & OHCI_INTR_SF) != 0 && !ohci->ed_rm_list
+                       && ohci->rh_state == OHCI_RH_RUNNING) ||
+                       ((ctl & OHCI_CTRL_HCFS) != OHCI_USB_OPER)) {
                 ohci_writel (ohci, OHCI_INTR_SF, &regs->intrdisable);
+               (void)ohci_readl(ohci, &regs->intrdisable);
+       }

>
>> +
>>   	/* Read interrupt status (and flush pending writes).  We ignore the
>>   	 * optimization of checking the LSB of hcca->done_head; it doesn't
>>   	 * work on all systems (edge triggering for OHCI can be a factor).
>> -- 
>> 1.8.3.1
>>