Hi Andrey,

On Mon, Oct 7, 2019 at 7:13 AM Andrey Smirnov andrew.smirnov@gmail.com 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@gmail.com Cc: Jiri Kosina jikos@kernel.org Cc: Benjamin Tissoires benjamin.tissoires@redhat.com Cc: Henrik Rydberg rydberg@bitmath.org Cc: Sam Bazely sambazley@fastmail.com Cc: Pierre-Loup A. Griffais pgriffais@valvesoftware.com Cc: Austin Palmer austinp@valvesoftware.com Cc: linux-input@vger.kernel.org Cc: linux-kernel@vger.kernel.org Cc: stable@vger.kernel.org

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

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

Hi Andrey,

On Fri, Oct 11, 2019 at 8:19 PM Andrey Smirnov andrew.smirnov@gmail.com wrote:

On Fri, Oct 11, 2019 at 7:52 AM Benjamin Tissoires benjamin.tissoires@redhat.com wrote:

...

Hi Andrey,

On Mon, Oct 7, 2019 at 7:13 AM Andrey Smirnov andrew.smirnov@gmail.com 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@gmail.com Cc: Jiri Kosina jikos@kernel.org Cc: Benjamin Tissoires benjamin.tissoires@redhat.com Cc: Henrik Rydberg rydberg@bitmath.org Cc: Sam Bazely sambazley@fastmail.com Cc: Pierre-Loup A. Griffais pgriffais@valvesoftware.com Cc: Austin Palmer austinp@valvesoftware.com Cc: linux-input@vger.kernel.org Cc: linux-kernel@vger.kernel.org Cc: stable@vger.kernel.org

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

Dependency is the only reason for this patch, but it is a pretty big reason. Prior to patches 1/3 and 2/3 FF private data was both allocated and passed off to FF layer via ff->private = data in a span of a few lines of code within hidpp_ff_init()/g920_get_config(). After, said pair is effectively split into two different functions, both needing access to FF private data, but called quite far apart in hidpp_probe(). Alternatives to patch 1/3 would be to either make sure that every error path in hidpp_prob() after the call to g920_allocate() is aware of allocated FF data, which seems like a nightmare, or, to create a temporary FF data, fill it in g920_get_config() and memdup() it in hidpp_ff_init(). Is that (the latter) the path that you prefer to take?

Hmm, I don't have a strong opinion on that. The point I don't like with the devres version is that it seems like a half-backed solution, as part of the driver would use devres while parts for the same purpose will not. I do not consider your code untrusted, but this is usually a reasonable source of leakages, so as a rule a thumb, devres should be used in a all or nothing fashion.

Basically, both alternative solutions would be OK with me, as long as the scope of each patch is reduced to the minimum (and having more than one is OK too).

...

...

---

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?

No, but I was trying to limit the scope of this patch.

...

...

•   /*
  

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

I agree, I think it's a wart in FF API design.

Yep, see Dmitry's answer for ideas :)

...

...

•    */
  

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

OK, sure. Could you be more explicit in your reasoning next time though? I am assuming this is because hid_hw_stop() might be called before?

Honestly, I don't have a good reason for it. I have seen enough of automatic static/dynamic checks with the same patterns to let my guts express that we need to check on the value of a pointer stored in private_data before dereferencing it.

If you are absolutely sure this is not need, a simple comment in the code is enough :)

...

...

•           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)

Well it appears you are against the idea of leveraging devres in this series, so discussing the fate of said hunk seems moot.

Well, I really value your work and I am very happy of it. It's just that for a patch/series aimed at stable, I rather have the patch series following the stable rules, which are that we should fix one thing only, and have the most simplest patch possible. I truly believe adding devres to cleanup the error path is the thing to do, but maybe not in this series.

...

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

Yeah, it probably should be moved to hidpp_ff_destroy(). Out of scope for this series though, I'll deal with it in a separate submission.

As per Dmitry's suggestion of removing hidpp_ff_destroy() maybe we should keep it that way, I am not entirely sure about how the races can happen (I don't think I even have one FF device I could test against).

...

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

Sure will do.

thanks \o/

...

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?

No it's alright. I'll submit a v2 of this series with only two patches and send a follow up after.

And thanks a lot again.

Cheers, Benjamin

On Fri, Oct 11, 2019 at 8:26 PM Dmitry Torokhov dmitry.torokhov@gmail.com wrote:

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@gmail.com 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@gmail.com Cc: Jiri Kosina jikos@kernel.org Cc: Benjamin Tissoires benjamin.tissoires@redhat.com Cc: Henrik Rydberg rydberg@bitmath.org Cc: Sam Bazely sambazley@fastmail.com Cc: Pierre-Loup A. Griffais pgriffais@valvesoftware.com Cc: Austin Palmer austinp@valvesoftware.com Cc: linux-input@vger.kernel.org Cc: linux-kernel@vger.kernel.org Cc: stable@vger.kernel.org

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.

Yeah, that stealing of the pointer is not good :) But OTOH, it helps

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

The problem I had when doing the review was that there is no easy way to have a "devm_input_ff_create_()", because the way it's built is already "devres-compatible": the destroy gets called by input core.

So I don't have a good answer to simplify in a transparent manner without breaking the API.

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.

Sounds like a good idea. However, it seems there might be a race when removing the workqueue: the workqueue gets deleted in hidpp_remove, when the input node will be freed by devres, so after the call of hidpp_remove.

So we should probably keep hidpp_ff_destroy() to clean up the ff bits, and instead move the content of hidpp_ff_deinit() into hidpp_ff_destroy() so we ensure proper ordering.

Andrey, note that ensuring the workqueue gets freed after the call of input_destroy_device is something that should definitively go into stable as this is a potential race problem.

Cheers, Benjamin

On Fri, Oct 11, 2019 at 01:33:03PM -0700, Dmitry Torokhov wrote:

On Fri, Oct 11, 2019 at 09:25:52PM +0200, Benjamin Tissoires wrote:

...

On Fri, Oct 11, 2019 at 8:26 PM Dmitry Torokhov dmitry.torokhov@gmail.com wrote:

...

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@gmail.com 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@gmail.com Cc: Jiri Kosina jikos@kernel.org Cc: Benjamin Tissoires benjamin.tissoires@redhat.com Cc: Henrik Rydberg rydberg@bitmath.org Cc: Sam Bazely sambazley@fastmail.com Cc: Pierre-Loup A. Griffais pgriffais@valvesoftware.com Cc: Austin Palmer austinp@valvesoftware.com Cc: linux-input@vger.kernel.org Cc: linux-kernel@vger.kernel.org Cc: stable@vger.kernel.org

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.

Yeah, that stealing of the pointer is not good :) But OTOH, it helps

...

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

The problem I had when doing the review was that there is no easy way to have a "devm_input_ff_create_()", because the way it's built is already "devres-compatible": the destroy gets called by input core.

I do not think we want devm_input_ff_create() explicitly, I think the fact that you can "build up" an input device by allocating it, then adding slots, poller, ff support, etc, and input core cleans it up is all good. It is just the ownership if the driver-private data block is not very obvious and is not compatible with allocating via devm.

...

So I don't have a good answer to simplify in a transparent manner without breaking the API.

...

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.

Sounds like a good idea. However, it seems there might be a race when removing the workqueue: the workqueue gets deleted in hidpp_remove, when the input node will be freed by devres, so after the call of hidpp_remove.

Yeah, well, that is a common issue with mixing devm and normal resources (and workqueue here is that "normal" resource), and we should either:

• not use devm
• use devm_add_action_or_reset() to work in custom actions that work freeing of non-managed resources into devm flow.

Actually, there is a door #3: use system workqueue. After all the work that Tejun done on workqueues it is very rare that one actually needs a dedicated workqueue (as works usually execute on one if the system worker threads that are shared with other workqueues anyway).

On Fri, Oct 11, 2019 at 11:34 PM Dmitry Torokhov dmitry.torokhov@gmail.com wrote:

On Fri, Oct 11, 2019 at 01:35:09PM -0700, Dmitry Torokhov wrote:

...

On Fri, Oct 11, 2019 at 01:33:03PM -0700, Dmitry Torokhov wrote:

...

On Fri, Oct 11, 2019 at 09:25:52PM +0200, Benjamin Tissoires wrote:

...

On Fri, Oct 11, 2019 at 8:26 PM Dmitry Torokhov dmitry.torokhov@gmail.com wrote:

...

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@gmail.com 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@gmail.com > Cc: Jiri Kosina jikos@kernel.org > Cc: Benjamin Tissoires benjamin.tissoires@redhat.com > Cc: Henrik Rydberg rydberg@bitmath.org > Cc: Sam Bazely sambazley@fastmail.com > Cc: Pierre-Loup A. Griffais pgriffais@valvesoftware.com > Cc: Austin Palmer austinp@valvesoftware.com > Cc: linux-input@vger.kernel.org > Cc: linux-kernel@vger.kernel.org > Cc: stable@vger.kernel.org

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.

Yeah, that stealing of the pointer is not good :) But OTOH, it helps

...

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

The problem I had when doing the review was that there is no easy way to have a "devm_input_ff_create_()", because the way it's built is already "devres-compatible": the destroy gets called by input core.

I do not think we want devm_input_ff_create() explicitly, I think the fact that you can "build up" an input device by allocating it, then adding slots, poller, ff support, etc, and input core cleans it up is all good. It is just the ownership if the driver-private data block is not very obvious and is not compatible with allocating via devm.

...

So I don't have a good answer to simplify in a transparent manner without breaking the API.

...

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.

Sounds like a good idea. However, it seems there might be a race when removing the workqueue: the workqueue gets deleted in hidpp_remove, when the input node will be freed by devres, so after the call of hidpp_remove.

Yeah, well, that is a common issue with mixing devm and normal resources (and workqueue here is that "normal" resource), and we should either:

• not use devm
• use devm_add_action_or_reset() to work in custom actions that work freeing of non-managed resources into devm flow.

Actually, there is a door #3: use system workqueue. After all the work that Tejun done on workqueues it is very rare that one actually needs a dedicated workqueue (as works usually execute on one if the system worker threads that are shared with other workqueues anyway).

And additional note about devm:

I think all HID input drivers that are using devm in probe, but do not have proper remove() function (and maybe even some with remove) are broken: hid_device_remove() calls hid_hw_stop() which potentially will shut off the transport. This happens before devm starts unwinding, so we still can be trying to communicate with the device in question, but the transport is gone.

Well, that is by design. A driver is supposed to call hid_hw_start() at the very end of its .probe(). And the supposed rule is that in the specific .remove(), you are to call first hid_hw_stop() to stop the transport layer underneath. That also means that in the HID subsystem, at least, you are not supposed to talk to the device during the devm teardown of the allocated data.

If you really need to communicate with the device during tear down, then you are supposed to write your own .remove, in which you control where the hid_hw_stop() happens.

We might have overlooked one or two, but I think we are on a good basis for now.

io_started/driver_input_lock is broken on removal as well as we release the lock when driver may very well be still talking to the device in devm teardown actions.

Again, this is not supposed to happen. Once hid_hw_stop() is called, we do not have access to the transport, so drivers can't talk to the device. So releasing/clearing the locks is supposed to be safe now.

I think we have similar kind of issues in other buses as well (i2c, spi, etc). For example, in i2c we remove the device from power domain before we actually complete devm unwinding.

I agree that this looks bad.

I would need to have a better look at it on Monday. Time to go on week end (this jet lag doesn't help me to go to sleep...)

Cheers, Benjamin

On Sat, Oct 12, 2019 at 1:24 AM Dmitry Torokhov dmitry.torokhov@gmail.com wrote:

On Sat, Oct 12, 2019 at 12:48:42AM +0200, Benjamin Tissoires wrote:

...

On Fri, Oct 11, 2019 at 11:34 PM Dmitry Torokhov dmitry.torokhov@gmail.com wrote:

...

On Fri, Oct 11, 2019 at 01:35:09PM -0700, Dmitry Torokhov wrote:

...

On Fri, Oct 11, 2019 at 01:33:03PM -0700, Dmitry Torokhov wrote:

...

On Fri, Oct 11, 2019 at 09:25:52PM +0200, Benjamin Tissoires wrote:

...

On Fri, Oct 11, 2019 at 8:26 PM Dmitry Torokhov dmitry.torokhov@gmail.com wrote: > > 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@gmail.com 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@gmail.com > > > Cc: Jiri Kosina jikos@kernel.org > > > Cc: Benjamin Tissoires benjamin.tissoires@redhat.com > > > Cc: Henrik Rydberg rydberg@bitmath.org > > > Cc: Sam Bazely sambazley@fastmail.com > > > Cc: Pierre-Loup A. Griffais pgriffais@valvesoftware.com > > > Cc: Austin Palmer austinp@valvesoftware.com > > > Cc: linux-input@vger.kernel.org > > > Cc: linux-kernel@vger.kernel.org > > > Cc: stable@vger.kernel.org > > > > 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.

Yeah, that stealing of the pointer is not good :) But OTOH, it helps

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

The problem I had when doing the review was that there is no easy way to have a "devm_input_ff_create_()", because the way it's built is already "devres-compatible": the destroy gets called by input core.

I do not think we want devm_input_ff_create() explicitly, I think the fact that you can "build up" an input device by allocating it, then adding slots, poller, ff support, etc, and input core cleans it up is all good. It is just the ownership if the driver-private data block is not very obvious and is not compatible with allocating via devm.

...

So I don't have a good answer to simplify in a transparent manner without breaking the API.

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

Sounds like a good idea. However, it seems there might be a race when removing the workqueue: the workqueue gets deleted in hidpp_remove, when the input node will be freed by devres, so after the call of hidpp_remove.

Yeah, well, that is a common issue with mixing devm and normal resources (and workqueue here is that "normal" resource), and we should either:

• not use devm
• use devm_add_action_or_reset() to work in custom actions that work freeing of non-managed resources into devm flow.

Actually, there is a door #3: use system workqueue. After all the work that Tejun done on workqueues it is very rare that one actually needs a dedicated workqueue (as works usually execute on one if the system worker threads that are shared with other workqueues anyway).

And additional note about devm:

I think all HID input drivers that are using devm in probe, but do not have proper remove() function (and maybe even some with remove) are broken: hid_device_remove() calls hid_hw_stop() which potentially will shut off the transport. This happens before devm starts unwinding, so we still can be trying to communicate with the device in question, but the transport is gone.

Well, that is by design. A driver is supposed to call hid_hw_start() at the very end of its .probe(). And the supposed rule is that in the specific .remove(), you are to call first hid_hw_stop() to stop the transport layer underneath. That also means that in the HID subsystem, at least, you are not supposed to talk to the device during the devm teardown of the allocated data.

If you really need to communicate with the device during tear down, then you are supposed to write your own .remove, in which you control where the hid_hw_stop() happens.

We might have overlooked one or two, but I think we are on a good basis for now.

You have to be _very_ careful there. For example, we can take a look at hid-elan.c. If you notice, it uses devm_led_classdev_register() to create "mute" led and it needs to talk over HID to control it;s brightness/state. So the driver has custom remove() and calls hid_hw_stop() from it. But the LED will be unregistered much later (in the depth of the driver core) so users of LED subsystem are free to send requests through and the driver will try to talk to the device even after hid_hw_stop() is called and the io_started/driver_input_lock is reset/released.

I am sure there are more such examples.

Yep, this is problematic. There is no guard in elan_mute_led_set_brigtness() which tells us that the bus has been stopped, so we likely have an issue here.

Note that a .remove() that just calls hid_hw_stop() should be removed, as hid core can do it for us.

...

...

io_started/driver_input_lock is broken on removal as well as we release the lock when driver may very well be still talking to the device in devm teardown actions.

Again, this is not supposed to happen. Once hid_hw_stop() is called, we do not have access to the transport, so drivers can't talk to the device. So releasing/clearing the locks is supposed to be safe now.

Except that it is hard to enforce once you throw in devm.

...

...

I think we have similar kind of issues in other buses as well (i2c, spi, etc). For example, in i2c we remove the device from power domain before we actually complete devm unwinding.

I agree that this looks bad.

I would need to have a better look at it on Monday. Time to go on week end (this jet lag doesn't help me to go to sleep...)

I wonder if every bus should open a new devm group for device and manually release it after calling ->remove(). That would ensure that all devm resouces allocated by drivers will be freed before we start executing bus-specific code.

That would be indeed useful. There is no reasons I can think of for a resource to be created during the .probe() of a device that should stick around after its .remove().

In the Elan case above, it won't solve all of the issues, as there will still be a tiny window where the resource will get access to the bus when it has been stopped. Maybe adding an other group when we call hid_hw_start() that will be freed by hid_hw_stop() before the actual stop to the bus could come to the rescue....

Cheers, Benjamin

On Fri, Oct 11, 2019 at 02:02:30PM -0700, Andrey Smirnov wrote:

On Fri, Oct 11, 2019 at 1:33 PM Dmitry Torokhov dmitry.torokhov@gmail.com wrote:

...

On Fri, Oct 11, 2019 at 09:25:52PM +0200, Benjamin Tissoires wrote:

...

On Fri, Oct 11, 2019 at 8:26 PM Dmitry Torokhov dmitry.torokhov@gmail.com wrote:

...

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@gmail.com 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@gmail.com Cc: Jiri Kosina jikos@kernel.org Cc: Benjamin Tissoires benjamin.tissoires@redhat.com Cc: Henrik Rydberg rydberg@bitmath.org Cc: Sam Bazely sambazley@fastmail.com Cc: Pierre-Loup A. Griffais pgriffais@valvesoftware.com Cc: Austin Palmer austinp@valvesoftware.com Cc: linux-input@vger.kernel.org Cc: linux-kernel@vger.kernel.org Cc: stable@vger.kernel.org

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.

Yeah, that stealing of the pointer is not good :) But OTOH, it helps

...

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

The problem I had when doing the review was that there is no easy way to have a "devm_input_ff_create_()", because the way it's built is already "devres-compatible": the destroy gets called by input core.

I do not think we want devm_input_ff_create() explicitly, I think the fact that you can "build up" an input device by allocating it, then adding slots, poller, ff support, etc, and input core cleans it up is all good. It is just the ownership if the driver-private data block is not very obvious and is not compatible with allocating via devm.

What do you think of creating input_ff_create_with_private() which would take a pointer to private memory and set a new "borrowed_private" flag in struct ff_device which input_ff_destroy() can check to determine if it needs to free ->private or not?

From my quick check we only have 3 users of ff->private in the tree:

dtor@dtor-ws:~/kernel/master (next)$ git grep -l "[^a-z]ff->private" -- drivers/ drivers/hid/hid-logitech-hidpp.c drivers/hid/usbhid/hid-pidff.c drivers/input/ff-core.c drivers/input/ff-memless.c

and I really wonder if we actually need to have this pointer and maintain it, given that logitech driver appears to not needing it that much...

Thanks.

On Fri, Oct 11, 2019 at 12:26 PM Benjamin Tissoires benjamin.tissoires@redhat.com wrote:

On Fri, Oct 11, 2019 at 8:26 PM Dmitry Torokhov dmitry.torokhov@gmail.com wrote:

...

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@gmail.com 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@gmail.com Cc: Jiri Kosina jikos@kernel.org Cc: Benjamin Tissoires benjamin.tissoires@redhat.com Cc: Henrik Rydberg rydberg@bitmath.org Cc: Sam Bazely sambazley@fastmail.com Cc: Pierre-Loup A. Griffais pgriffais@valvesoftware.com Cc: Austin Palmer austinp@valvesoftware.com Cc: linux-input@vger.kernel.org Cc: linux-kernel@vger.kernel.org Cc: stable@vger.kernel.org

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.

Yeah, that stealing of the pointer is not good :) But OTOH, it helps

...

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

The problem I had when doing the review was that there is no easy way to have a "devm_input_ff_create_()", because the way it's built is already "devres-compatible": the destroy gets called by input core.

So I don't have a good answer to simplify in a transparent manner without breaking the API.

...

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.

Sounds like a good idea. However, it seems there might be a race when removing the workqueue: the workqueue gets deleted in hidpp_remove, when the input node will be freed by devres, so after the call of hidpp_remove.

So we should probably keep hidpp_ff_destroy() to clean up the ff bits, and instead move the content of hidpp_ff_deinit() into hidpp_ff_destroy() so we ensure proper ordering.

Andrey, note that ensuring the workqueue gets freed after the call of input_destroy_device is something that should definitively go into stable as this is a potential race problem.

Sure, I'll add this to the series as a separate patch.

Thanks, Andrey Smirnov

On Fri, Oct 11, 2019 at 7:56 AM Benjamin Tissoires benjamin.tissoires@redhat.com wrote:

On Mon, Oct 7, 2019 at 7:13 AM Andrey Smirnov andrew.smirnov@gmail.com wrote:

...

G920 device only advertises REPORT_ID_HIDPP_LONG and REPORT_ID_HIDPP_VERY_LONG in its HID report descriptor, so querying for REPORT_ID_HIDPP_SHORT with optional=false will always fail and prevent G920 to be recognized as a valid HID++ device.

Modify hidpp_validate_device() to check only REPORT_ID_HIDPP_LONG with optional=false on G920 to fix this.

Fixes: fe3ee1ec007b ("HID: logitech-hidpp: allow non HID++ devices to be handled by this module") Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=204191 Reported-by: Sam Bazely sambazley@fastmail.com Signed-off-by: Andrey Smirnov andrew.smirnov@gmail.com Cc: Jiri Kosina jikos@kernel.org Cc: Benjamin Tissoires benjamin.tissoires@redhat.com Cc: Henrik Rydberg rydberg@bitmath.org Cc: Sam Bazely sambazley@fastmail.com Cc: Pierre-Loup A. Griffais pgriffais@valvesoftware.com Cc: Austin Palmer austinp@valvesoftware.com Cc: linux-input@vger.kernel.org Cc: linux-kernel@vger.kernel.org Cc: stable@vger.kernel.org

---

drivers/hid/hid-logitech-hidpp.c | 6 ++++++ 1 file changed, 6 insertions(+)

diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c index cadf36d6c6f3..f415bf398e17 100644 --- a/drivers/hid/hid-logitech-hidpp.c +++ b/drivers/hid/hid-logitech-hidpp.c @@ -3511,6 +3511,12 @@ static bool hidpp_validate_report(struct hid_device *hdev, int id,

static bool hidpp_validate_device(struct hid_device *hdev) {

•   struct hidpp_device *hidpp = hid_get_drvdata(hdev);
  

•   if (hidpp->quirks & HIDPP_QUIRK_CLASS_G920)
  

•           return hidpp_validate_report(hdev, REPORT_ID_HIDPP_LONG,
  

•                                        HIDPP_REPORT_SHORT_LENGTH, false);
  

with https://patchwork.kernel.org/patch/11184749/ we also have a need for such a trick for BLE mice.

I wonder if we should not have a more common way of validating the devices

What about just checking for:

hidpp_validate_report(REPORT_ID_HIDPP_SHORT, HIDPP_REPORT_SHORT_LENGTH, true) || hidpp_validate_report(hdev, REPORT_ID_HIDPP_LONG, HIDPP_REPORT_LONG_LENGTH, true);

and probably dropping the "optional" argument for hidpp_validate_report()? Original code allows there to be devices supporting shorts reports only, but it seems that devices that support only long reports are legitimate too, so maybe the only "invalid" combination is if both are invalid length or missing?

Thanks, Andrey Smirnov

On Fri, Oct 11, 2019 at 3:33 PM Benjamin Tissoires benjamin.tissoires@redhat.com wrote:

On Fri, Oct 11, 2019 at 9:39 PM Andrey Smirnov andrew.smirnov@gmail.com wrote:

...

On Fri, Oct 11, 2019 at 7:56 AM Benjamin Tissoires benjamin.tissoires@redhat.com wrote:

...

On Mon, Oct 7, 2019 at 7:13 AM Andrey Smirnov andrew.smirnov@gmail.com wrote:

...

G920 device only advertises REPORT_ID_HIDPP_LONG and REPORT_ID_HIDPP_VERY_LONG in its HID report descriptor, so querying for REPORT_ID_HIDPP_SHORT with optional=false will always fail and prevent G920 to be recognized as a valid HID++ device.

Modify hidpp_validate_device() to check only REPORT_ID_HIDPP_LONG with optional=false on G920 to fix this.

Fixes: fe3ee1ec007b ("HID: logitech-hidpp: allow non HID++ devices to be handled by this module") Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=204191 Reported-by: Sam Bazely sambazley@fastmail.com Signed-off-by: Andrey Smirnov andrew.smirnov@gmail.com Cc: Jiri Kosina jikos@kernel.org Cc: Benjamin Tissoires benjamin.tissoires@redhat.com Cc: Henrik Rydberg rydberg@bitmath.org Cc: Sam Bazely sambazley@fastmail.com Cc: Pierre-Loup A. Griffais pgriffais@valvesoftware.com Cc: Austin Palmer austinp@valvesoftware.com Cc: linux-input@vger.kernel.org Cc: linux-kernel@vger.kernel.org Cc: stable@vger.kernel.org

---

drivers/hid/hid-logitech-hidpp.c | 6 ++++++ 1 file changed, 6 insertions(+)

diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c index cadf36d6c6f3..f415bf398e17 100644 --- a/drivers/hid/hid-logitech-hidpp.c +++ b/drivers/hid/hid-logitech-hidpp.c @@ -3511,6 +3511,12 @@ static bool hidpp_validate_report(struct hid_device *hdev, int id,

static bool hidpp_validate_device(struct hid_device *hdev) {

•   struct hidpp_device *hidpp = hid_get_drvdata(hdev);
  

•   if (hidpp->quirks & HIDPP_QUIRK_CLASS_G920)
  

•           return hidpp_validate_report(hdev, REPORT_ID_HIDPP_LONG,
  

•                                        HIDPP_REPORT_SHORT_LENGTH, false);
  

with https://patchwork.kernel.org/patch/11184749/ we also have a need for such a trick for BLE mice.

I wonder if we should not have a more common way of validating the devices

What about just checking for:

hidpp_validate_report(REPORT_ID_HIDPP_SHORT, HIDPP_REPORT_SHORT_LENGTH, true) || hidpp_validate_report(hdev, REPORT_ID_HIDPP_LONG, HIDPP_REPORT_LONG_LENGTH, true);

and probably dropping the "optional" argument for hidpp_validate_report()? Original code allows there to be devices supporting shorts reports only, but it seems that devices that support only long reports are legitimate too, so maybe the only "invalid" combination is if both are invalid length or missing?

Well, the problem is we also want to detect 2 things:

• devices that do not have any of the HID++ collections, and handle

them as generic ones (the second mouse/keyboard collection in the gaming mice should still be exported by the driver, or this will kill the macros / rebinding capabilities

• malicious devices that pretends to have a HID++ collection but want

to trigger a buffer overflow by having a shorter than expected report length

Point 2 above should still be fine, but point 1 is why we have the enforcement of the HID++ short report in the first place.

It sounds like the result of hidpp_validate_report() can't really be contained in a bool. If we modify it to return -EINVAL for bogus report length, -ENOTSUPP if report ID is not supported and 0 if everything is valid we should be able to capture all valid permutation by checking for with

int id_short = hidpp_validate_report(ID_SHORT); int id_long = hidpp_validate_report(ID_LONG);

return (!id_short && !id_long) || (id_short == -ENOTSUPP && !id_long) || (id_long == -ENOTSUPP && !id_short)

no?

Thanks, Andrey Smirnov