| Message ID | 20230720205324.58702-4-andriy.shevchenko@linux.intel.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | iio: core: A few code cleanups and documentation fixes | expand |
On Thu, 2023-07-20 at 23:53 +0300, Andy Shevchenko wrote: > Let the krealloc_array() copy the original data and > check for a multiplication overflow. > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > --- > drivers/iio/industrialio-core.c | 20 +++++++------------- > 1 file changed, 7 insertions(+), 13 deletions(-) > > diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c > index 90e59223b178..be154879983e 100644 > --- a/drivers/iio/industrialio-core.c > +++ b/drivers/iio/industrialio-core.c > @@ -1465,7 +1465,7 @@ int iio_device_register_sysfs_group(struct iio_dev > *indio_dev, > const struct attribute_group **new, **old = iio_dev_opaque->groups; > unsigned int cnt = iio_dev_opaque->groupcounter; > > - new = krealloc(old, sizeof(*new) * (cnt + 2), GFP_KERNEL); > + new = krealloc_array(old, cnt + 2, sizeof(*new), GFP_KERNEL); > if (!new) > return -ENOMEM; > > @@ -1483,13 +1483,13 @@ static int iio_device_register_sysfs(struct iio_dev > *indio_dev) > { > struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev); > int i, ret = 0, attrcount, attrn, attrcount_orig = 0; > + struct attribute **attrs, **attr, *clk = NULL; > struct iio_dev_attr *p; > - struct attribute **attr, *clk = NULL; > > /* First count elements in any existing group */ > - if (indio_dev->info->attrs) { > - attr = indio_dev->info->attrs->attrs; > - while (*attr++ != NULL) > + attrs = indio_dev->info->attrs ? indio_dev->info->attrs->attrs : NULL; > + if (attrs) { > + for (attr = attrs; *attr; attr++) > attrcount_orig++; not really related with the change... maybe just mention it in the commit? > } > attrcount = attrcount_orig; > @@ -1521,20 +1521,14 @@ static int iio_device_register_sysfs(struct iio_dev > *indio_dev) > if (clk) > attrcount++; > > + /* Copy across original attributes, and point to original binary > attributes */ > iio_dev_opaque->chan_attr_group.attrs = > - kcalloc(attrcount + 1, > - sizeof(iio_dev_opaque->chan_attr_group.attrs[0]), > - GFP_KERNEL); > + krealloc_array(attrs, attrcount + 1, sizeof(*attrs), > GFP_KERNEL); > if (iio_dev_opaque->chan_attr_group.attrs == NULL) { since you're here and you also already did some style cleanups above, maybe change it to 'if (!iio_dev_opaque->chan_attr_group.attrs)'? - Nuno Sá
On Fri, Jul 21, 2023 at 09:59:37AM +0200, Nuno Sá wrote: > On Thu, 2023-07-20 at 23:53 +0300, Andy Shevchenko wrote: ... > > + struct attribute **attrs, **attr, *clk = NULL; > > struct iio_dev_attr *p; > > - struct attribute **attr, *clk = NULL; > > > > /* First count elements in any existing group */ > > - if (indio_dev->info->attrs) { > > - attr = indio_dev->info->attrs->attrs; > > - while (*attr++ != NULL) > > + attrs = indio_dev->info->attrs ? indio_dev->info->attrs->attrs : NULL; > > + if (attrs) { > > + for (attr = attrs; *attr; attr++) > > attrcount_orig++; > not really related with the change... maybe just mention it in the commit? Hmm... It's related to make krealloc_array() to work as expected. > > } ... > > iio_dev_opaque->chan_attr_group.attrs = > > - kcalloc(attrcount + 1, > > - sizeof(iio_dev_opaque->chan_attr_group.attrs[0]), > > - GFP_KERNEL); > > + krealloc_array(attrs, attrcount + 1, sizeof(*attrs), > > GFP_KERNEL); > > if (iio_dev_opaque->chan_attr_group.attrs == NULL) { > > since you're here and you also already did some style cleanups above, maybe > change it to 'if (!iio_dev_opaque->chan_attr_group.attrs)'? I don't think it's related (but you can tell that this check related to the allocator, and since we touch it, we may touch this), if Jonathan wants this, I definitely do. ... Thank you for the review!
On Fri, 2023-07-21 at 13:14 +0300, Andy Shevchenko wrote: > On Fri, Jul 21, 2023 at 09:59:37AM +0200, Nuno Sá wrote: > > On Thu, 2023-07-20 at 23:53 +0300, Andy Shevchenko wrote: > > ... > > > > + struct attribute **attrs, **attr, *clk = NULL; > > > struct iio_dev_attr *p; > > > - struct attribute **attr, *clk = NULL; > > > > > > /* First count elements in any existing group */ > > > - if (indio_dev->info->attrs) { > > > - attr = indio_dev->info->attrs->attrs; > > > - while (*attr++ != NULL) > > > + attrs = indio_dev->info->attrs ? indio_dev->info->attrs->attrs : > > > NULL; > > > + if (attrs) { > > > + for (attr = attrs; *attr; attr++) > > > attrcount_orig++; > > > not really related with the change... maybe just mention it in the commit? > > Hmm... It's related to make krealloc_array() to work as expected. > Hmm, I think it's arguable :). while() -> for() it's not really needed unless I'm missing something. You could even initialize 'attrs' to NULL at declaration and keep the above diff minimum. That said, I actually prefer this style (even though some people don't like much the ternary operator). > > > } > > ... > > > > iio_dev_opaque->chan_attr_group.attrs = > > > - kcalloc(attrcount + 1, > > > - sizeof(iio_dev_opaque->chan_attr_group.attrs[0]), > > > - GFP_KERNEL); > > > + krealloc_array(attrs, attrcount + 1, sizeof(*attrs), > > > GFP_KERNEL); > > > if (iio_dev_opaque->chan_attr_group.attrs == NULL) { > > > > since you're here and you also already did some style cleanups above, maybe > > change it to 'if (!iio_dev_opaque->chan_attr_group.attrs)'? > > I don't think it's related (but you can tell that this check related to > the allocator, and since we touch it, we may touch this), if Jonathan > wants this, I definitely do. Fair enough... - Nuno Sá
On Fri, Jul 21, 2023 at 12:53:53PM +0200, Nuno Sá wrote: > On Fri, 2023-07-21 at 13:14 +0300, Andy Shevchenko wrote: > > On Fri, Jul 21, 2023 at 09:59:37AM +0200, Nuno Sá wrote: > > > On Thu, 2023-07-20 at 23:53 +0300, Andy Shevchenko wrote: ... > > > > + struct attribute **attrs, **attr, *clk = NULL; > > > > struct iio_dev_attr *p; > > > > - struct attribute **attr, *clk = NULL; > > > > > > > > /* First count elements in any existing group */ > > > > - if (indio_dev->info->attrs) { > > > > - attr = indio_dev->info->attrs->attrs; > > > > - while (*attr++ != NULL) > > > > + attrs = indio_dev->info->attrs ? indio_dev->info->attrs->attrs : > > > > NULL; > > > > + if (attrs) { > > > > + for (attr = attrs; *attr; attr++) > > > > attrcount_orig++; > > > > > not really related with the change... maybe just mention it in the commit? > > > > Hmm... It's related to make krealloc_array() to work as expected. > > > > Hmm, I think it's arguable :). while() -> for() it's not really needed unless > I'm missing something. You could even initialize 'attrs' to NULL at declaration > and keep the above diff minimum. I'm not a fan of the assignments in the declarations when it potentially can be disrupted by a chunk of code and reading the code itself may be harder due to an interruption for checking the initial value. Hence, having + attr = attrs; while (... != NULL) seems enough to be replaced with one liner for-loop. > That said, I actually prefer this style (even though some people don't like much > the ternary operator). Thanks! > > > > }
On Fri, Jul 21, 2023 at 02:28:36PM +0300, Andy Shevchenko wrote: > On Fri, Jul 21, 2023 at 12:53:53PM +0200, Nuno Sá wrote: > > On Fri, 2023-07-21 at 13:14 +0300, Andy Shevchenko wrote: > > > On Fri, Jul 21, 2023 at 09:59:37AM +0200, Nuno Sá wrote: > > > > On Thu, 2023-07-20 at 23:53 +0300, Andy Shevchenko wrote: ... > > > > > + struct attribute **attrs, **attr, *clk = NULL; > > > > > struct iio_dev_attr *p; > > > > > - struct attribute **attr, *clk = NULL; > > > > > > > > > > /* First count elements in any existing group */ > > > > > - if (indio_dev->info->attrs) { > > > > > - attr = indio_dev->info->attrs->attrs; > > > > > - while (*attr++ != NULL) > > > > > + attrs = indio_dev->info->attrs ? indio_dev->info->attrs->attrs : > > > > > NULL; > > > > > + if (attrs) { > > > > > + for (attr = attrs; *attr; attr++) > > > > > attrcount_orig++; > > > > > > > not really related with the change... maybe just mention it in the commit? > > > > > > Hmm... It's related to make krealloc_array() to work as expected. > > > > > > > Hmm, I think it's arguable :). while() -> for() it's not really needed unless > > I'm missing something. You could even initialize 'attrs' to NULL at declaration > > and keep the above diff minimum. > > I'm not a fan of the assignments in the declarations when it potentially can be > disrupted by a chunk of code and reading the code itself may be harder due to > an interruption for checking the initial value. Hence, having > > + attr = attrs; > while (... != NULL) > > seems enough to be replaced with one liner for-loop. Note that attrs is reused later, so the above assignment makes it cleaner that some value is assigned to it. With the original code it's not so obvious. > > That said, I actually prefer this style (even though some people don't like much > > the ternary operator). > > Thanks! > > > > > > }
diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c index 90e59223b178..be154879983e 100644 --- a/drivers/iio/industrialio-core.c +++ b/drivers/iio/industrialio-core.c @@ -1465,7 +1465,7 @@ int iio_device_register_sysfs_group(struct iio_dev *indio_dev, const struct attribute_group **new, **old = iio_dev_opaque->groups; unsigned int cnt = iio_dev_opaque->groupcounter; - new = krealloc(old, sizeof(*new) * (cnt + 2), GFP_KERNEL); + new = krealloc_array(old, cnt + 2, sizeof(*new), GFP_KERNEL); if (!new) return -ENOMEM; @@ -1483,13 +1483,13 @@ static int iio_device_register_sysfs(struct iio_dev *indio_dev) { struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev); int i, ret = 0, attrcount, attrn, attrcount_orig = 0; + struct attribute **attrs, **attr, *clk = NULL; struct iio_dev_attr *p; - struct attribute **attr, *clk = NULL; /* First count elements in any existing group */ - if (indio_dev->info->attrs) { - attr = indio_dev->info->attrs->attrs; - while (*attr++ != NULL) + attrs = indio_dev->info->attrs ? indio_dev->info->attrs->attrs : NULL; + if (attrs) { + for (attr = attrs; *attr; attr++) attrcount_orig++; } attrcount = attrcount_orig; @@ -1521,20 +1521,14 @@ static int iio_device_register_sysfs(struct iio_dev *indio_dev) if (clk) attrcount++; + /* Copy across original attributes, and point to original binary attributes */ iio_dev_opaque->chan_attr_group.attrs = - kcalloc(attrcount + 1, - sizeof(iio_dev_opaque->chan_attr_group.attrs[0]), - GFP_KERNEL); + krealloc_array(attrs, attrcount + 1, sizeof(*attrs), GFP_KERNEL); if (iio_dev_opaque->chan_attr_group.attrs == NULL) { ret = -ENOMEM; goto error_clear_attrs; } - /* Copy across original attributes, and point to original binary attributes */ if (indio_dev->info->attrs) { - memcpy(iio_dev_opaque->chan_attr_group.attrs, - indio_dev->info->attrs->attrs, - sizeof(iio_dev_opaque->chan_attr_group.attrs[0]) - *attrcount_orig); iio_dev_opaque->chan_attr_group.is_visible = indio_dev->info->attrs->is_visible; iio_dev_opaque->chan_attr_group.bin_attrs =
Let the krealloc_array() copy the original data and check for a multiplication overflow. Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> --- drivers/iio/industrialio-core.c | 20 +++++++------------- 1 file changed, 7 insertions(+), 13 deletions(-)