On Mon, Sep 5, 2022 at 11:18 PM Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxx> 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
