Re: [PATCH 1/3] HID: logitech-hidpp: use devres to manage FF private data

[Dmitry Torokhov]

On Fri, Oct 11, 2019 at 04:52:04PM +0200, Benjamin Tissoires wrote:
> Hi Andrey,
> 
> On Mon, Oct 7, 2019 at 7:13 AM Andrey Smirnov <andrew.smirnov@xxxxxxxxx> wrote:
> >
> > To simplify resource management in commit that follows as well as to
> > save a couple of extra kfree()s and simplify hidpp_ff_deinit() switch
> > driver code to use devres to manage the life-cycle of FF private data.
> >
> > Signed-off-by: Andrey Smirnov <andrew.smirnov@xxxxxxxxx>
> > Cc: Jiri Kosina <jikos@xxxxxxxxxx>
> > Cc: Benjamin Tissoires <benjamin.tissoires@xxxxxxxxxx>
> > Cc: Henrik Rydberg <rydberg@xxxxxxxxxxx>
> > Cc: Sam Bazely <sambazley@xxxxxxxxxxxx>
> > Cc: Pierre-Loup A. Griffais <pgriffais@xxxxxxxxxxxxxxxxx>
> > Cc: Austin Palmer <austinp@xxxxxxxxxxxxxxxxx>
> > Cc: linux-input@xxxxxxxxxxxxxxx
> > Cc: linux-kernel@xxxxxxxxxxxxxxx
> > Cc: stable@xxxxxxxxxxxxxxx
> 
> This patch doesn't seem to fix any error, is there a reason to send it
> to stable? (besides as a dependency of the rest of the series).
> 
> > ---
> >  drivers/hid/hid-logitech-hidpp.c | 53 +++++++++++++++++---------------
> >  1 file changed, 29 insertions(+), 24 deletions(-)
> >
> > diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
> > index 0179f7ed77e5..58eb928224e5 100644
> > --- a/drivers/hid/hid-logitech-hidpp.c
> > +++ b/drivers/hid/hid-logitech-hidpp.c
> > @@ -2079,6 +2079,11 @@ static void hidpp_ff_destroy(struct ff_device *ff)
> >         struct hidpp_ff_private_data *data = ff->private;
> >
> >         kfree(data->effect_ids);
> 
> Is there any reasons we can not also devm alloc data->effect_ids?
> 
> > +       /*
> > +        * Set private to NULL to prevent input_ff_destroy() from
> > +        * freeing our devres allocated memory
> 
> Ouch. There is something wrong here: input_ff_destroy() calls
> kfree(ff->private), when the data has not been allocated by
> input_ff_create(). This seems to lack a little bit of symmetry.

Yeah, ff and ff-memless essentially take over the private data assigned
to them. They were done before devm and the lifetime of the "private"
data pieces was tied to the lifetime of the input device to simplify
error handling and teardown.

Maybe we should clean it up a bit... I'm open to suggestions.

In this case maybe best way is to get rid of hidpp_ff_destroy() and not
set ff->private and rely on devm to free the buffers. One can get to
device private data from ff methods via input_get_drvdata() since they
all (except destroy) are passed input device pointer.

Thanks.

-- 
Dmitry