linux-kernel - Re: [PATCH v3 1/2] HID: i2c-hid: Use devm to allocate i2c

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <095738e5-eb31-905b-b496-0d78489e253d@redhat.com>
Date:   Fri, 22 Jun 2018 11:13:07 +0200
From:   Hans de Goede <hdegoede@...hat.com>
To:     Benjamin Tissoires <benjamin.tissoires@...hat.com>,
        Stephen Boyd <swboyd@...omium.org>
Cc:     Jiri Kosina <jikos@...nel.org>,
        lkml <linux-kernel@...r.kernel.org>,
        "open list:HID CORE LAYER" <linux-input@...r.kernel.org>,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        Dmitry Torokhov <dtor@...omium.org>,
        Doug Anderson <dianders@...omium.org>
Subject: Re: [PATCH v3 1/2] HID: i2c-hid: Use devm to allocate i2c_hid struct

Hi,

On 22-06-18 09:16, Benjamin Tissoires wrote:
> On Fri, Jun 22, 2018 at 4:27 AM, Stephen Boyd <swboyd@...omium.org> wrote:
>> 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 <benjamin.tissoires@...hat.com>
>> Cc: Hans de Goede <hdegoede@...hat.com>
>> Cc: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
>> Cc: Dmitry Torokhov <dtor@...omium.org>
>> Cc: Doug Anderson <dianders@...omium.org>
>> Signed-off-by: Stephen Boyd <swboyd@...omium.org>
>> ---
>>   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.

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

Regards,

Hans


> 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).
> 
> Otherwise, for the series:
> Acked-by: Benjamin Tissoires <benjamin.tissoires@...hat.com>
> 
> Cheers,
> Benjamin
> 
> 
> 
>>          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
>>