Hi Doug,
Thank you for the reviews.
On 11/20/23 23:07, Doug Anderson wrote:
> Hi,
>
> On Mon, Nov 20, 2023 at 11:33 AM Hans de Goede <hdegoede@xxxxxxxxxx> wrote:
>>
>> @@ -741,12 +741,9 @@ static int i2c_hid_parse(struct hid_device *hid)
>> return -EINVAL;
>> }
>>
>> + mutex_lock(&ihid->reset_lock);
>> do {
>> - mutex_lock(&ihid->reset_lock);
>> ret = i2c_hid_start_hwreset(ihid);
>> - if (ret == 0)
>> - ret = i2c_hid_finish_hwreset(ihid);
>> - mutex_unlock(&ihid->reset_lock);
>> if (ret)
>> msleep(1000);
>> } while (tries-- > 0 && ret);
>
> 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'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.
Regards,
Hans
