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;
MIME-Version: 1.0
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>
 <fe411488-c67c-62ab-4709-98621b9f199b@linux.intel.com>
In-Reply-To: <fe411488-c67c-62ab-4709-98621b9f199b@linux.intel.com>
From:   Puma Hsu <pumahsu@google.com>
Date:   Tue, 18 Jan 2022 14:50:43 +0800
Message-ID: <CAGCq0LYwrkCe4X77x__CC6qUPaSvRXoO3grKODfP+eSGZfNm7w@mail.gmail.com>
Subject: Re: [PATCH v3] xhci: re-initialize the HC during resume if HCE was set
To:     Mathias Nyman <mathias.nyman@linux.intel.com>
Cc:     Greg KH <gregkh@linuxfoundation.org>, 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
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Mon, Jan 17, 2022 at 8:20 PM Mathias Nyman
<mathias.nyman@linux.intel.com> wrote:
>
>
> >>> 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?  W=
hat
> >>>> 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 se=
t
> >> 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, =E2=80=9CSoftware should implement an algorithm f=
or checking the
> > HCE flag if the xHC is not responding.=E2=80=9D, so maybe it would be b=
etter
> > 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 we=
ll tried
> due to issues in resume being so common, I think it makes sense to add th=
e 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 wit=
h xHCI
> can see it and start fixing the rootcause.

I will print a debug message in next patch version.

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

Maybe I can try to make this in the future. Thank you for advising.

> -Mathias