Hi, On Tue, Nov 21, 2023 at 1:53 AM Hans de Goede <hdegoede@xxxxxxxxxx> wrote: > > > Right after this loop, you have: > > > > if (ret) > > return ret; > > > > That will return with the mutex held. It needs to be a "goto > > abort_reset". You'd also need to init `use_override` then, I think. > > Ah, good catch, I will fix this for the next version. > > Assuming there will be a next version. Did you read the cover-letter > part about the moving of the wait for reset to after the descriptor > read not fixing the missing reset ack 100% but rather only 50% or > so of the time ? > > And do you have any opinion on if we should still move forward with > this patch-set or not ? I'd tend to leave it to your judgement. I have a bias towards landing it because it improves probe speed in a way that matches what the spec suggests and, IMO, probe speed is important. It also has the potential to avoid the need for quirks on some devices, even if it didn't work out that way for the device you tested with. The only downside you have listed is the potential for regressions, but that's something that's a risk for nearly any change. This doesn't feel like an excessively risky change to me and, as you've pointed out, it's documented in the spec. If someone reports a regression then it seems like we should address it as it comes... > > I'll also note that it seems awkward that > > `clear_bit(I2C_HID_RESET_PENDING, &ihid->flags)` is scattered in so > > many places for error handling, but I couldn't really find a better > > way to do it. :-P > > I guess we could just no clear it? Only the wait for reset > wait_event_timeout() cares about its value and if we run that > a second time then the bit will be set to 1 again before calling > it anyways... Not sure I like my own suggestion here, but > it is an option. I'm afraid it may bite us later thogh if we > ever decide to check for the bit in another place. Yeah, I didn't have any great ideas either and I think it's fine as you have it. It just bothered me as I was reviewing and so I figured I'd mention it in case some brilliant idea occurred to you. ;-)
