linux-kernel - Re: [v5 08/14] iio: imu: add Bosch Sensortec BNO055 core 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:   Mon, 2 May 2022 11:50:47 +0200
From:   Andrea Merello <andrea.merello@...il.com>
To:     Andy Shevchenko <andy.shevchenko@...il.com>
Cc:     Jonathan Cameron <jic23@...nel.org>,
        Mauro Carvalho Chehab <mchehab+huawei@...nel.org>,
        linux-iio <linux-iio@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        devicetree <devicetree@...r.kernel.org>,
        Lars-Peter Clausen <lars@...afoo.de>,
        Rob Herring <robh+dt@...nel.org>,
        Matt Ranostay <matt.ranostay@...sulko.com>,
        Alexandru Ardelean <ardeleanalex@...il.com>,
        jmondi <jacopo@...ndi.org>,
        Andrea Merello <andrea.merello@....it>
Subject: Re: [v5 08/14] iio: imu: add Bosch Sensortec BNO055 core driver

Il giorno mer 27 apr 2022 alle ore 15:23 Andy Shevchenko
<andy.shevchenko@...il.com> ha scritto:
>

As usual, some inline comments. OK for the rest.

[...]

>
> > +#define BNO055_ATTR_VALS(...)          \
> > +       .vals = (int[]){ __VA_ARGS__},  \
> > +       .len = ARRAY_SIZE(((int[]){__VA_ARGS__}))
>
> Not sure this adds any readability to the code. Can we simply have an
> array of int for each case with the explicit ARRAY_SIZE() calls?

Do you mean moving the vals array out of the structs? Something like:

static int bno055_gyr_scale_vals[] = {125, 1877467, 250, 1877467,
        500, 1877467, 1000, 1877467, 2000, 1877467};

static struct bno055_sysfs_attr_aux_data bno055_gyr_scale_aux = {
        .fusion_vals = (int[]){1, 900},
        .hw_xlate = (int[]){4, 3, 2, 1, 0},
        .type = IIO_VAL_FRACTIONAL

?
But then I'd make also something like:

#define bno055_sysfs_attr_avail(priv, attr, vals, len) \
        _bno055_sysfs_attr_avail(priv, attr##_vals,
ARRAY_SIZE(attr##_vals), attr##_aux, vals, len)

And the same for all other users of those structs.

My point is not about readability, but about avoiding as much as
possible bugs caused by mismatched attr_vals, attr_aux and
ARRAY_SIZE() arg. e.g:
bno055_sysfs_attr_avail(priv, bno_foo_vals, ARRAY_SIZE(bno_bar_vals),
bno_foobar_aux, vals, len)

I used to make quite a lot of mess until I grouped all the stuff in
one struct :/

[...]

>
> > +       msleep(20);
>
> Perhaps a comment why so long sleep is needed.

DS says that switching mode can last from 7mS up to 19mS depending on
the case, but I don't know _why_ it takes so long. I may add a comment
that just states that it's a sensor requirement.

[...]

>
> > +       for (i = 0; i < bno055_acc_range.len; i++)
> > +               len += sysfs_emit_at(buf, len, "%d%c", bno055_acc_range.vals[i],
> > +                                    (i == bno055_acc_range.len - 1) ? '\n' : ' ');
>
> You may move the condition out of the loop.

May you elaborate, please? Do you mean something like: loop one time
less, and then call sysfs_emit_at() once more outside the loop,
getting rid of the conditional ternary operator at all?

[...]

> > +       if (indio_dev->active_scan_mask &&
> > +           !bitmap_empty(indio_dev->active_scan_mask, _BNO055_SCAN_MAX))
> > +               return -EBUSY;
> > +
> > +       if (sysfs_streq(buf, "0")) {
> > +               ret = bno055_operation_mode_set(priv, BNO055_OPR_MODE_AMG);
>
> return bno055_operation_mode_set(...);

Why? bno055_operation_mode_set() returns an error code, while here we
need to return the len, or propagate the error code only when it's the
case

> > +       } else {
>
> ...and drop this with the following decreasing indentation.

if you want to drop this, then I can just duplicate if(ret) return
ret; i.e. add it after bno055_operation_mode_set(priv,
BNO055_OPR_MODE_AMG); and get rid of the else branch (see above)

[...]

>
> Can be removed to group all related checks together.

I'm not sure what you mean here, but see below

> > +       if (ret)
> > +               dev_notice(dev, "Calibration file load failed. See instruction in kernel Documentation/iio/bno055.rst");
> > +
> > +       if (caldata) {
> > +               caldata_data = caldata->data;
> > +               caldata_size = caldata->size;
> > +       }
> > +       ret = bno055_init(priv, caldata_data, caldata_size);
>
> > +       if (caldata)
> > +               release_firmware(caldata);
>
> > +       if (ret)
> > +               return ret;
>
> Can be rewritten in a form of
>
> if (caldata) {
>  ret = bno055_init();
>  release_firmware(...);
> } else {
>  ret = bno055_init();
> }
> if (ret)
>   return ret;
>
> ?

Indeed I'd say it could be rewritten as:

        if (ret)
                ret = request_firmware(&caldata, BNO055_FW_GENERIC_NAME, dev);
        if (ret) {
                dev_notice(dev, "Calibration file load failed. See
instruction in kernel Documentation/iio/bno055.rst");
                ret = bno055_init(priv, NULL, 0);
        } else {
                ret = bno055_init(priv, caldata->data, caldata->size);
                release_firmware(caldata);
        }
        if (ret)
                return ret;