Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH v2] usb: ohci: add check for start frame in host
 controller functional states
To:     Alan Stern <stern@rowland.harvard.edu>
Cc:     Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org,
        Greg Kroah-Hartman <greg@kroah.com>,
        Patchwork Bot <patchwork-bot@kernel.org>
References: <1632910167-23554-1-git-send-email-zhuyinbo@loongson.cn>
 <20210929145905.GA428239@rowland.harvard.edu>
From:   zhuyinbo <zhuyinbo@loongson.cn>
Message-ID: <0cbc2fc2-4e33-5529-a07a-8c0ee41c800e@loongson.cn>
Date:   Fri, 8 Oct 2021 15:18:09 +0800
User-Agent: Mozilla/5.0 (X11; Linux mips64; rv:68.0) Gecko/20100101
 Thunderbird/68.7.0
MIME-Version: 1.0
In-Reply-To: <20210929145905.GA428239@rowland.harvard.edu>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Content-Language: en-US
Precedence: bulk


在 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
>>