linux-kernel - Re: [PATCH v4 4/4] i2c: Add multi-instantiate pseudo driver

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]

Date:   Wed, 8 Aug 2018 12:26:54 +0200
From:   Peter Rosin <peda@...ntia.se>
To:     Andy Shevchenko <andy.shevchenko@...il.com>
Cc:     Hans de Goede <hdegoede@...hat.com>,
        "Rafael J . Wysocki" <rjw@...ysocki.net>,
        Len Brown <lenb@...nel.org>,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        Mika Westerberg <mika.westerberg@...ux.intel.com>,
        Darren Hart <dvhart@...radead.org>,
        Wolfram Sang <wsa@...-dreams.de>,
        Srinivas Pandruvada <srinivas.pandruvada@...ux.intel.com>,
        ACPI Devel Maling List <linux-acpi@...r.kernel.org>,
        Platform Driver <platform-driver-x86@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Heikki Krogerus <heikki.krogerus@...ux.intel.com>,
        John Garry <john.garry@...wei.com>,
        linux-i2c <linux-i2c@...r.kernel.org>
Subject: Re: [PATCH v4 4/4] i2c: Add multi-instantiate pseudo driver

On 2018-08-08 12:01, Andy Shevchenko wrote:
> On Wed, Aug 8, 2018 at 12:47 PM, Peter Rosin <peda@...ntia.se> wrote:
>> On 2018-08-08 11:08, Andy Shevchenko wrote:
>>> On Wed, Aug 8, 2018 at 11:30 AM, Hans de Goede <hdegoede@...hat.com> wrote:
> 
>>>> +       /* Count number of clients to instantiate */
>>>> +       for (i = 0; inst_data[i].type; i++) {}
>>>> +
>>>> +       multi = devm_kmalloc(dev,
>>>> +                       offsetof(struct i2c_multi_inst_data, clients[i]),
>>>> +                       GFP_KERNEL);
>>>> +       if (!multi)
>>>> +               return -ENOMEM;
>>>
>>> Here I see the following:
>>>  - it's kinda unusual use of offsetof(), perhaps i*sizeof() + sizeof()
>>> would be more understandable
>>>  - there is no guard against i == 0
>>
>> I don't see why a guard is needed?
> 
> Because there is no point to have a module loaded when there is none
> client to serve.
> 
>> *Your* code below needs it, but that
>> issue is not a concern for the original code.
> 
> I can admit that's not a big deal, just making logic slightly more robust.
> 
>> It might however be a
>> good idea to fail the probe if there are no clients to instantiate, but
>> that's a different issue...
> 
> That's what I have in mind.

Ah, but there is no reason what-so-ever for i being zero. The whole point
of the driver is for cases where i > 1. Or to put it bluntly, anyone
defining a struct i2c_inst_data with zero entries deserves to be punished...

>>> multi = devm_kmalloc(sizeof(*multi), GFP_KERNEL);
>>> if (!multi)
>>>  return -ENOMEM;
>>>
>>> multi->clients = devm_kcalloc(i, sizeof(*multi->clients), GFP_KERNEL);
>>> if (ZERO_PTR_OR_NULL(multi->clients))
>>>  return -ENOMEM;
>>>
>>> But I would like to hear your (other's) opinion(s).
>>
>> I think using two allocations is a waste in this case.
> 
> On the other hand it makes code more readable. With offsetof() it is a
> bit hard to get it on the first glance.

The driver is tiny, I think it's good if there is at least one thing that
can be a little bit interesting :-)

>>>> +               if (inst_data[i].irq_idx != -1) {
>>>
>>>> = 0 sounds more robust
>>
>> But not as flexible/future-proof. Why should 0 be the only valid IRQ index?
> 
> Ah, because > is used usually is a quoting character in email you
> missed the point.
> It was written as >= 0.

Ahh, good catch, that explains it.

Cheers,
Peter