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;
To:     Puma Hsu <pumahsu@google.com>, Greg KH <gregkh@linuxfoundation.org>
Cc:     mathias.nyman@intel.com, Sergey Shtylyov <s.shtylyov@omp.ru>,
        Albert Wang <albertccwang@google.com>,
        linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org,
        stable@vger.kernel.org
References: <20211229112551.3483931-1-pumahsu@google.com>
 <Yd1tUKhyZf26OVNQ@kroah.com>
 <CAGCq0LZb8nQDvcz=LswWi4qKd-65ys6iPjTKh=46dVtYLDEUVw@mail.gmail.com>
 <Yd/g/ywBWZG7gF8v@kroah.com>
 <CAGCq0LZ3i8VaMfRWNKvH_-ms0TgNqKA6f+Zx7M=iz1t_-smW+g@mail.gmail.com>
From:   Mathias Nyman <mathias.nyman@linux.intel.com>
Subject: Re: [PATCH v3] xhci: re-initialize the HC during resume if HCE was
 set
Message-ID: <fe411488-c67c-62ab-4709-98621b9f199b@linux.intel.com>
Date:   Mon, 17 Jan 2022 14:22:10 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Firefox/78.0 Thunderbird/78.14.0
MIME-Version: 1.0
In-Reply-To: <CAGCq0LZ3i8VaMfRWNKvH_-ms0TgNqKA6f+Zx7M=iz1t_-smW+g@mail.gmail.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 8bit
Precedence: bulk


>>> This seems like a big hammer for when the host controller throws an
>>>> error.  Why is this the only place that it should be checked for?  What
>>>> caused the error that can now allow it to be fixed?
>>>
>>> I believe this is not the only place that the host controller may set
>>> HCE, the host controller may set HCE anytime it sees an error in my
>>> opinion, not only in suspend or resume.
>>
>> Then where else should it be checked?  Where else will your silicon set
>> this bit as part of the normal operating process?
> 
> We observed this flag while resume in our silicon so far. According to the XHCI
> specification 4.24.1, “Software should implement an algorithm for checking the
> HCE flag if the xHC is not responding.”, so maybe it would be better
> to implement
> a new API to recover host controller whenever the driver side finds no response
> from host controller in the future.
> 

As all the code to reset the host during resume already exists, and is well tried
due to issues in resume being so common, I think it makes sense to add the HCE case 
here as well. It's a simple fix that makes the life of users better.

That said we shouldn't hide the reason for reset like this.
Print a debug message telling about the HCE so that everybody working with xHCI
can see it and start fixing the rootcause.

Another HCE check could be added to command timeout code, but just to show a
warning for now.
Reset might not always clear HCE, and we don't want to be stuck in a reset loop.
This check  could be a separate patch

-Mathias