Messages in this thread | | | Subject | Re: [PATCH v4 4/4] i2c: Add multi-instantiate pseudo driver | From | Peter Rosin <> | Date | Wed, 8 Aug 2018 12:26:54 +0200 |
| |
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
| |