Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Tue, 3 Nov 2020 11:55:22 +0100 (CET)
From:   Jiri Kosina <jikos@kernel.org>
To:     Pascal Giard <pascal.giard@etsmtl.ca>
cc:     Benjamin Tissoires <benjamin.tissoires@redhat.com>,
        linux-input@vger.kernel.org, linux-kernel@vger.kernel.org,
        Sanjay Govind <sanjay.govind9@gmail.com>
Subject: Re: [PATCH] HID: ghlive: support for ghlive ps3/wii u dongles
In-Reply-To: <CAJNNDmn1OBzRouNUcAmWSfj4piSHRFfc6V6gvb2D+2qYO1Ob7g@mail.gmail.com>
Message-ID: <nycvar.YFH.7.76.2011031152270.18859@cbobk.fhfr.pm>
References: <20201009022722.123943-1-pascal.giard@etsmtl.ca> <nycvar.YFH.7.76.2010291622380.18859@cbobk.fhfr.pm> <CAJNNDmn1OBzRouNUcAmWSfj4piSHRFfc6V6gvb2D+2qYO1Ob7g@mail.gmail.com>
User-Agent: Alpine 2.21 (LSU 202 2017-01-01)
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Precedence: bulk

On Thu, 29 Oct 2020, Pascal Giard wrote:

> > >  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.

Yeah, but before that happens, I think we should just continue with status 
quo, meaning putting the support into hid-sony, if noone has any 
objections.

> > 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? 

No, I probably didn't have my first coffee of the day when looking at your 
patch, sorry.

Still, the way it tries to allocate the buffers is sub-optimal, as if 
either (not necessarily the first one) fails, it retries the next 
iteration with attempting to allocate from scratch.

If the system really is in tight memory condition, it'd make more sense to 
keep the ones for which the allocation succeeded already allocated, and 
retry only the failed one(s).

Thanks,

-- 
Jiri Kosina
SUSE Labs