Re: [PATCH v3 1/2] HID: bigben: use spinlock to safely schedule workers

[Pietro Borrello]

On Fri, 10 Feb 2023 at 15:26, Benjamin Tissoires
<benjamin.tissoires@xxxxxxxxxx> wrote:
>
> [...]
> >
> > -     if (bigben->removed || !report_field)
>
> You are removing an important test here: if (!report_field), please keep
> it.

To my understanding, that check was added in commit
918aa1ef104d ("HID: bigbenff: prevent null pointer dereference")
to prevent the NULL pointer crash, only fixing the crash point.
However, the true root cause was a missing check for output
reports patched in commit
c7bf714f8755 ("HID: check empty report_list in bigben_probe()"),
where the type-confused report list_entry was overlapping with
a NULL pointer, which was then causing the crash.
Let me know if there is any other path that may result in having a
report with no fields. In that case, it would make sense to keep the
check.

>
> > -             return;
> > +     spin_lock_irqsave(&bigben->lock, flags);
> >
> >       if (bigben->work_led) {
> >               bigben->work_led = false;
> > @@ -219,6 +229,8 @@ static void bigben_worker(struct work_struct *work)
> >               report_field->value[7] = 0x00; /* padding */
> >               hid_hw_request(bigben->hid, bigben->report, HID_REQ_SET_REPORT);
> >       }
> > +
> > +     spin_unlock_irqrestore(&bigben->lock, flags);
>
> Ouch, having hid_hw_request() called whithin a spinlock is definitely not
> something that should be done.
>
> However, the spinlock should be protecting 2 kinds of things:
> - any access to any value of struct bigben_device, but in an atomic way
>   (i.e. copy everything you need locally in a spinlock, then release it
>   and never read that struct again in that function).
> - the access to bigben->removed, which should be checked only in
>   bigben_schedule_work() and in the .remove() function.
>
> Please note that this is what the playstation driver does: it prepares
> the report under the spinlock (which is really fast) before sending the
> report to the device which can be slow and be interrupted.
>
> With that being said, it is clear that we need 2 patches for this one:
> - the first one introduces the spinlock and protects the concurrent
>   accesses to struct bigben_device (which is roughly everything below
>   with the changes I just said)
> - the second one introduces bigben_schedule_work() and piggy backs on
>   top of that new lock.

Thanks for clarifying. I will work on a v4 patch.
Best regards,
Pietro