Re: [PATCH] HID: ghlive: support for ghlive ps3/wii u dongles

[Pascal Giard <pascal.giard@etsmtl.ca>]

On Thu, Oct 29, 2020 at 11:26 AM Jiri Kosina <jikos@kernel.org> wrote:
>
> On Thu, 8 Oct 2020, Pascal Giard wrote:
>
> > This commit introduces the Guitar Hero Live driver which adds support
> > for the PS3 and Wii U dongles.
>
> Pascal,
>
> thanks for the patch.
>

Dear Jiri,

thank you for reviewing my patch.

> [ ... snip ... ]
>
> > ---
> >  drivers/hid/Kconfig      |   6 ++
> >  drivers/hid/Makefile     |   1 +
> >  drivers/hid/hid-ghlive.c | 220 +++++++++++++++++++++++++++++++++++++++
>
> Would it make more sense (with respect to how we are structuring/naming
> the hid drivers) to incorporate this into hid-sony (irrespective of
> currently ongoing discussions about actually splitting that driver :) )?
>

I think it would be most appropriate, yes.

Note that there are 3 other dongles out there:
- the xbox360 dongle does not need any special treatment, it just
works with hid-generic;
- the ps4 dongle obviously makes sense to go into hid-sony (although
no one has reversed engineered that one (yet));
- the xboxone dongle: that's an unknown one to me. I don't have any
information about that one unfortunately and do not own one.

I wrote this as a separate hid driver as I saw that email [1] from
Roderick Colenbrander in which he expressed a preference for a
seperate driver in cases where the device is not from Sony proper.

> > +static void ghl_magic_poke(struct timer_list *t)
> > +{
> > +     struct ghlive_sc *sc = from_timer(sc, t, poke_timer);
> > +
> > +     int ret;
> > +     unsigned int pipe;
> > +     struct urb *urb;
> > +     struct usb_ctrlrequest *cr;
> > +     const u16 poke_size =
> > +             ARRAY_SIZE(ghl_ps3wiiu_magic_data);
> > +     u8 *databuf;
> > +
> > +     pipe = usb_sndctrlpipe(sc->usbdev, 0);
> > +
> > +     cr = kzalloc(sizeof(*cr), GFP_ATOMIC);
> > +     if (!cr)
> > +             goto resched;
> > +
> > +     databuf = kzalloc(poke_size, GFP_ATOMIC);
> > +     if (!databuf) {
> > +             kfree(cr);
> > +             goto resched;
> > +     }
> > +
> > +     urb = usb_alloc_urb(0, GFP_ATOMIC);
> > +     if (!urb) {
> > +             kfree(databuf);
> > +             kfree(cr);
> > +             goto resched;
>
>
> So if one of the allocations above succeeds and a subsequent one fails,
> you're going to try re-allocate all of them next time again, leaking the
> ones that previously succeeded, right?
>

I attempted to avoid such a case. IIUC there are 4 possible scenarios
tied to those 3 allocs (cr, databuf, and urb):
1) alloc of cr fails. nothing to be freed, we reschedule;
2) alloc of cr succeeds, alloc of databuf fails. we free cr and we reschedule;
3) allocs of cr and databuf succeed, alloc of urb fails. we free cr
and databuf, and we reschedule;
4) all allocs succeed, we submit the urb, and free urb. once the
control packet is sent, the callback is called and we free cr and
databuf.

Am I missing something? It's VERY possible that I am as this is my
first patch and I wrote this by looking at other peoples' code. The
only thing that comes to mind is if poke is called again before the
control packet actually gets sent (and the callback called), in which
case I'm not sure what would happen. But with a poke interval of 10
seconds, is that probability close enough to 0 to be ignored?

Thanks again for your valuable input, :-)

-Pascal
[1] https://marc.info/?l=linux-input&m=157273970001101&w=2