Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20220902091535.3572333-1-raychi@google.com> <YxIX+jqWFfwAWYot@rowland.harvard.edu>
 <CAPBYUsApTYex027qBe-=EyUxDHb8MMQscX+2jqZ98zXxN-0tHA@mail.gmail.com>
 <YxI4ZViLkZOjN/Bh@rowland.harvard.edu> <CAPBYUsCEjMSJ8P8ZM1_W+S1DOWFTOM0wJwi2fTukfxSGucYhnQ@mail.gmail.com>
 <YxYTRSSeNqooy7lz@rowland.harvard.edu>
In-Reply-To: <YxYTRSSeNqooy7lz@rowland.harvard.edu>
From:   Ray Chi <raychi@google.com>
Date:   Thu, 8 Sep 2022 19:11:01 +0800
Message-ID: <CAPBYUsD5peoF0WNQnjS3xc6R_tCXH3685AKdf3MbUhk8Tkxf_g@mail.gmail.com>
Subject: Re: [Patch v2] usb: core: stop USB enumeration if too many retries
To:     Alan Stern <stern@rowland.harvard.edu>
Cc:     Greg KH <gregkh@linuxfoundation.org>,
        mathias.nyman@linux.intel.com,
        Albert Wang <albertccwang@google.com>,
        Badhri Jagan Sridharan <badhri@google.com>,
        Puma Hsu <pumahsu@google.com>, linux-usb@vger.kernel.org,
        linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Mon, Sep 5, 2022 at 11:18 PM Alan Stern <stern@rowland.harvard.edu> wrote:
>
> On Mon, Sep 05, 2022 at 04:36:16PM +0800, Ray Chi wrote:
> > On Sat, Sep 3, 2022 at 1:07 AM Alan Stern <stern@rowland.harvard.edu> wrote:
> > > I don't understand.  If you don't know whether the accessory is broken,
> > > how do you know whether to set the quirk?
> > >
> > > On the other hand, if you always set the quirk even before you know
> > > whether the accessory is broken, why make it a quirk at all?  Why not
> > > make it the normal behavior of the driver?
> > >
> >
> > Since our device has a watchdog mechanism, when the device connects to
> > a broken accessory, the kernel panic will happen. This problem didn't happen
> > in all USB Hosts, so I want to use the quirk to fix this problem for those hosts
> > with a watchdog mechanism.
>
> Okay.  So this shouldn't be a quirk; it should apply all the time to any
> hub where the host controller has this watchdog mechanism.
>
> > > Why not set CONFIG_USB_FEW_INIT_RETRIES instead?
> > >
> >
> > https://source.android.com/docs/core/architecture/kernel/android-common
> > According to Android Common Kernel, I can't only add this config to one project.
> > In addition, it can't stop enumeration so that the timeout problem
> > still happens.
>
> This is the first time you have mentioned either the watchdog mechanism
> or the fact that this is intended for Android.  It would have been a lot
> better if both of these facts were included in the initial patch
> description.  You can't expect people to evaluate a new patch properly
> if they don't have a clear picture of what it was meant for.
>
> > > might describe in detail a situation where the quirk is needed,
> > > explaining what sort of behavior of the system would lead you to set the
> > > quirk, and why.
> > >
> >
> > There is a kernel panic when the device connects to the broken accessory.
> > I tried to modify the initial_descriptor_timeout. When the accessory is not
> > working, the total time is 6.5s (get descriptor retry) + 5*2 seconds
> > (set address of xhci timeout).
> > The time is so long to cause kernel panic for the device. This is why I want to
> > stop enumeration instead reducing the retries or timeout.
>
> It sounds like what you need is a "quick initialization" option that
> will limit the timeout lengths and the numbers of retries, and will
> cause the system to ignore connections on a port once an initialization
> has failed.  There should also be a way to make the system stop ignoring
> a port, perhaps by writing to a sysfs file.
>

I will remove the quirk and use the sysfs file to do.

> In addition, there should be an automatic algorithm to determine which
> hub ports this option will apply to.  I don't think you want it to be
> based on a quirk, because you shouldn't need to wait for a kernel panic
> before realizing that the quirk is needed -- that's why the algorithm
> has to be automatic.
>
> Can you write a new patch that works more like this?
>

I had two ideas to determine whether the port should apply the option or not.
One is a timeout and the other one is using a retry count. If using
the retry count,
I think it is close to current retry definitions. Maybe we can modify
the design.
If I use the timeout, I need more time to think of a better way to do it for the
synchronization problem. Currently, they are rough ideas. I will
upload the commit
if I have a better solution to determine the behavior automatically.

I will upload a v3 patch using sysfs file to fix the current problem.

> Alan Stern

Thanks,
Ray