linux-kernel - Re: [v4 12/14] iio: imu: add BNO055 serdev 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]

Message-ID: <CAN8YU5Pfc6t=3_Yj7v5PF36Ut=1_Vr86Li60z=+SENKRyn+iTw@mail.gmail.com>
Date:   Wed, 20 Apr 2022 09:22:07 +0200
From:   Andrea Merello <andrea.merello@...il.com>
To:     Jonathan Cameron <jic23@...nel.org>
Cc:     Mauro Carvalho Chehab <mchehab+huawei@...nel.org>,
        linux-iio <linux-iio@...r.kernel.org>,
        linux-kernel <linux-kernel@...r.kernel.org>,
        devicetree <devicetree@...r.kernel.org>,
        Lars-Peter Clausen <lars@...afoo.de>,
        Rob Herring <robh+dt@...nel.org>,
        Andy Shevchenko <andy.shevchenko@...il.com>,
        Matt Ranostay <matt.ranostay@...sulko.com>,
        Alexandru Ardelean <ardeleanalex@...il.com>,
        Jacopo Mondi <jacopo@...ndi.org>,
        Andrea Merello <andrea.merello@....it>
Subject: Re: [v4 12/14] iio: imu: add BNO055 serdev driver

Il giorno ven 15 apr 2022 alle ore 18:40 Jonathan Cameron
<jic23@...nel.org> ha scritto:
>
> On Fri, 15 Apr 2022 15:00:03 +0200
> Andrea Merello <andrea.merello@...il.com> wrote:
>
> > From: Andrea Merello <andrea.merello@....it>
> >
> > This path adds a serdev driver for communicating to a BNO055 IMU via
> > serial bus, and it enables the BNO055 core driver to work in this
> > scenario.
> >
> > Signed-off-by: Andrea Merello <andrea.merello@....it>
> Hi Andrea
>
> A few really trivial things in here from me.

Few inline comments below; OK for all indeed.

> > +struct bno055_ser_priv {
> > +     struct serdev_device *serdev;
> > +     struct completion cmd_complete;
> > +     enum {
> > +             CMD_NONE,
> > +             CMD_READ,
> > +             CMD_WRITE,
> > +     } expect_response;
> > +     int expected_data_len;
> > +     u8 *response_buf;
> > +
> > +     /**
> > +      * enum cmd_status - represent the status of a command sent to the HW.
> > +      * @STATUS_CRIT: The command failed: the serial communication failed.
> > +      * @STATUS_OK:   The command executed successfully.
> > +      * @STATUS_FAIL: The command failed: HW responded with an error.
> > +      */
> > +     enum {
> > +             STATUS_CRIT = -1,
> > +             STATUS_OK = 0,
> > +             STATUS_FAIL = 1,
> > +     } cmd_status;
>
> Locks need documentation to say what scope they cover. In this case
> I think it is most but not quite all of this structure.

I admit my comments here are awkward: I've put a couple of comment
that indicate what doesn't need the lock.. I'll change to do the
reverse (comment on what need the lock)

> See comment on completion below.
>
> > +     struct mutex lock;
> > +
> > +     /* Only accessed in RX callback context. No lock needed. */
> > +     struct {
> > +             enum {
> > +                     RX_IDLE,
> > +                     RX_START,
> > +                     RX_DATA,
> > +             } state;
> > +             int databuf_count;
> > +             int expected_len;
> > +             int type;
> > +     } rx;
> > +
> > +     /* Never accessed in behalf of RX callback context. No lock needed */
> > +     bool cmd_stale;
> > +};
>
[...]

> > +             }
> > +             break;
> > +
> > +     case CMD_WRITE:
> > +             priv->cmd_status = status;
> > +             break;
> > +     }
> > +
> > +     priv->expect_response = CMD_NONE;
> > +     complete(&priv->cmd_complete);
>
> I argued with myself a bit on whether the complete() should be inside the lock
> or not - but then concluded it doesn't really matter and moving it out is
> probably premature optimisation... Maybe it's worth moving it out simply
> so that it's clear the lock isn't needed to protect it, or am I missing something?

It should make no real difference to move the complete() out of the lock.

I think I put it inside the lock because of the (paranoid, and
hopefully not really required - would mean we have been too strict
with completion timeout) reinit_completion(). On serdev rx handler
side (i.e. bno055_ser_handle_rx()) we clear expect_response and
complete(), on the other side (bno055_ser_send_cmd()) we set
expect_response and clear spurious completed state, before issuing the
command and waiting for outcome. This looks symmetric, but those two
shouldn't really race in practice.


> > +     mutex_unlock(&priv->lock);
> > +}
> > +
> > +/*