On Fri, Jun 22, 2018 at 2:13 AM Hans de Goede [off-list ref] wrote:

Hi,

On 22-06-18 09:16, Benjamin Tissoires wrote:

quoted

On Fri, Jun 22, 2018 at 4:27 AM, Stephen Boyd [off-list ref] wrote:

quoted

Use devm here to save some lines and prepare for bulk regulator usage in
this driver. Otherwise, when we devm bulk get regulators we'll free the
containing i2c_hid structure and try to put regulator pointers from
freed memory.

Cc: Benjamin Tissoires <redacted>
Cc: Hans de Goede <redacted>
Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: Dmitry Torokhov <redacted>
Cc: Doug Anderson <dianders@chromium.org>
Signed-off-by: Stephen Boyd <redacted>
---
  drivers/hid/i2c-hid/i2c-hid.c | 9 +++------
  1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c
index c1652bb7bd15..c7d6738dc524 100644
--- a/drivers/hid/i2c-hid/i2c-hid.c
+++ b/drivers/hid/i2c-hid/i2c-hid.c

@@ -1002,18 +1002,18 @@ static int i2c_hid_probe(struct i2c_client *client,
                 return client->irq;
         }

-       ihid = kzalloc(sizeof(struct i2c_hid), GFP_KERNEL);
+       ihid = devm_kzalloc(&client->dev, sizeof(*ihid), GFP_KERNEL);

IIRC, I never made the switch towards devm for i2c_hid because at the
time there was a "all or nothing" rule regarding devm.

I'm not aware of any such rule. Sure ideally everything should use
devm, because it makes life just so much easier. But I've seen mixed
use in plenty of cases.

When mixing managed and unmanaged resources you need to be extremely
careful to make sure they are released in proper order. There were
many naive conversions that would convert just part of resources to
devm and end up, let's say, powering down rails of a device or turning
off clocks while interrupts are still registered, which causes errors
if interrupt fires, and so on. I usually request people to do either
full conversion or leave the driver alone.

That said, using devm to allocate the "main" driver structure is usually safe.

With that said converting fully to devm is not necessarily a bad
idea.

Regards,

Hans

quoted

But given that the regulator already has a devm inside, I think we are
screwed here and we should probably try to devm-ize the module.

I seem to remember that someone posted a devm version of
hid_allocate_device/hid-add_device, but I don't think this ever went
upstream (because no use).

There is always devm_add_action_or_reset() to inject custom actions
into devm stack to work around lacking devm APIs.

quoted

Otherwise, for the series:
Acked-by: Benjamin Tissoires <redacted>

Cheers,
Benjamin

quoted

         if (!ihid)
                 return -ENOMEM;

         if (client->dev.of_node) {
                 ret = i2c_hid_of_probe(client, &ihid->pdata);
                 if (ret)
-                       goto err;
+                       return ret;
         } else if (!platform_data) {
                 ret = i2c_hid_acpi_pdata(client, &ihid->pdata);
                 if (ret)
-                       goto err;
+                       return ret;
         } else {
                 ihid->pdata = *platform_data;
         }

@@ -1126,7 +1126,6 @@ static int i2c_hid_probe(struct i2c_client *client,

  err:
         i2c_hid_free_buffers(ihid);
-       kfree(ihid);
         return ret;
  }

@@ -1150,8 +1149,6 @@ static int i2c_hid_remove(struct i2c_client *client)

         regulator_disable(ihid->pdata.supply);

-       kfree(ihid);
-
         return 0;
  }

--

Sent by a computer through tubes