lkml.org 
[lkml]   [2018]   [Aug]   [8]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [PATCH v4 4/4] i2c: Add multi-instantiate pseudo driver
    From
    Date
    On 2018-08-08 12:01, Andy Shevchenko wrote:
    > On Wed, Aug 8, 2018 at 12:47 PM, Peter Rosin <peda@axentia.se> wrote:
    >> On 2018-08-08 11:08, Andy Shevchenko wrote:
    >>> On Wed, Aug 8, 2018 at 11:30 AM, Hans de Goede <hdegoede@redhat.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

    \
     
     \ /
      Last update: 2018-08-08 12:28    [W:4.187 / U:0.000 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site