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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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.

> +        */
> +       ff->private = NULL;
>  }
>
>  static int hidpp_ff_init(struct hidpp_device *hidpp, u8 feature_index)
> @@ -2090,7 +2095,7 @@ static int hidpp_ff_init(struct hidpp_device *hidpp, u8 feature_index)
>         const u16 bcdDevice = le16_to_cpu(udesc->bcdDevice);
>         struct ff_device *ff;
>         struct hidpp_report response;
> -       struct hidpp_ff_private_data *data;
> +       struct hidpp_ff_private_data *data = hidpp->private_data;
>         int error, j, num_slots;
>         u8 version;
>
> @@ -2129,18 +2134,13 @@ static int hidpp_ff_init(struct hidpp_device *hidpp, u8 feature_index)
>                 return error;
>         }
>
> -       data = kzalloc(sizeof(*data), GFP_KERNEL);
> -       if (!data)
> -               return -ENOMEM;
>         data->effect_ids = kcalloc(num_slots, sizeof(int), GFP_KERNEL);
> -       if (!data->effect_ids) {
> -               kfree(data);
> +       if (!data->effect_ids)
>                 return -ENOMEM;
> -       }
> +
>         data->wq = create_singlethread_workqueue("hidpp-ff-sendqueue");
>         if (!data->wq) {
>                 kfree(data->effect_ids);
> -               kfree(data);
>                 return -ENOMEM;
>         }
>
> @@ -2199,28 +2199,15 @@ static int hidpp_ff_init(struct hidpp_device *hidpp, u8 feature_index)
>         return 0;
>  }
>
> -static int hidpp_ff_deinit(struct hid_device *hid)
> +static void hidpp_ff_deinit(struct hid_device *hid)
>  {
> -       struct hid_input *hidinput = list_entry(hid->inputs.next, struct hid_input, list);
> -       struct input_dev *dev = hidinput->input;
> -       struct hidpp_ff_private_data *data;
> -
> -       if (!dev) {
> -               hid_err(hid, "Struct input_dev not found!\n");
> -               return -EINVAL;
> -       }
> +       struct hidpp_device *hidpp = hid_get_drvdata(hid);
> +       struct hidpp_ff_private_data *data = hidpp->private_data;
>
>         hid_info(hid, "Unloading HID++ force feedback.\n");
> -       data = dev->ff->private;
> -       if (!data) {

I am pretty sure we might need to keep a test on data not being null.

> -               hid_err(hid, "Private data not found!\n");
> -               return -EINVAL;
> -       }
>
>         destroy_workqueue(data->wq);
>         device_remove_file(&hid->dev, &dev_attr_range);
> -
> -       return 0;
>  }

This whole hunk seems unrelated to the devm change.
Can you extract a patch that just stores hidpp_ff_private_data in
hidpp->private_data and then cleans up hidpp_ff_deinit() before
switching it to devm? (or maybe not, see below)

After a few more thoughts, I don't think this mixing of devm and non
devm is a good idea:
we could say that the hidpp_ff_private_data and its effect_ids are
both children of the input_dev, not the hid_device. And we would
expect the whole thing to simplify with devm, but it's not, because ff
is not supposed to be used with devm.

I have a feeling the whole ff logic is wrong in term of where things
should be cleaned up, but I can not come up with a good hint on where
to start. For example, destroy_workqueue() is called in
hidpp_ff_deinit() where it might be racy, and maybe we should call it
in hidpp_ff_destroy()...

Last, you should base this patch on top of the for-next branch, we
recently merged a fix for the FF drivers in the HID subsystem:
https://git.kernel.org/pub/scm/linux/kernel/git/hid/hid.git/commit/?h=for-next&id=d9d4b1e46d9543a82c23f6df03f4ad697dab361b

Would it be too complex to drop this patch from the series and do a
proper follow up cleanup series that might not need to go to stable?

Cheers,
Benjamin

>
>
> @@ -2725,6 +2712,20 @@ static int k400_connect(struct hid_device *hdev, bool connected)
>
>  #define HIDPP_PAGE_G920_FORCE_FEEDBACK                 0x8123
>
> +static int g920_allocate(struct hid_device *hdev)
> +{
> +       struct hidpp_device *hidpp = hid_get_drvdata(hdev);
> +       struct hidpp_ff_private_data *data;
> +
> +       data = devm_kzalloc(&hdev->dev, sizeof(*data), GFP_KERNEL);
> +       if (!data)
> +               return -ENOMEM;
> +
> +       hidpp->private_data = data;
> +
> +       return 0;
> +}
> +
>  static int g920_get_config(struct hidpp_device *hidpp)
>  {
>         u8 feature_type;
> @@ -3561,6 +3562,10 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
>                 ret = k400_allocate(hdev);
>                 if (ret)
>                         return ret;
> +       } else if (hidpp->quirks & HIDPP_QUIRK_CLASS_G920) {
> +               ret = g920_allocate(hdev);
> +               if (ret)
> +                       return ret;
>         }
>
>         INIT_WORK(&hidpp->work, delayed_work_cb);
> --
> 2.21.0
>





[Index of Archives]     [Linux Kernel]     [Kernel Development Newbies]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite Hiking]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux