* Re: [PATCH v2] usb: ohci: add check for start frame in host controller functional states
2021-09-29 14:59 ` [PATCH v2] usb: ohci: add check for start frame in host controller functional states Alan Stern
@ 2021-09-29 15:01 ` Alan Stern
2021-10-08 7:18 ` zhuyinbo
1 sibling, 0 replies; 3+ messages in thread
From: Alan Stern @ 2021-09-29 15:01 UTC (permalink / raw)
To: Yinbo Zhu
Cc: Greg Kroah-Hartman, linux-usb, linux-kernel, Greg Kroah-Hartman,
Patchwork Bot
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
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH v2] usb: ohci: add check for start frame in host controller functional states
2021-09-29 14:59 ` [PATCH v2] usb: ohci: add check for start frame in host controller functional states Alan Stern
2021-09-29 15:01 ` Alan Stern
@ 2021-10-08 7:18 ` zhuyinbo
1 sibling, 0 replies; 3+ messages in thread
From: zhuyinbo @ 2021-10-08 7:18 UTC (permalink / raw)
To: Alan Stern
Cc: Greg Kroah-Hartman, linux-usb, linux-kernel, Greg Kroah-Hartman,
Patchwork Bot
在 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, ®s->control);
>> +
>> + if ((ints & OHCI_CTRL_HCFS) != OHCI_USB_OPER) {
>> + ohci_writel(ohci, OHCI_INTR_SF, ®s->intrdisable);
>> + (void)ohci_readl(ohci, ®s->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, ®s->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, ®s->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, ®s->intrdisable);
+ (void)ohci_readl(ohci, ®s->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
>>
^ permalink raw reply [flat|nested] 3+ messages in thread