Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
MIME-Version: 1.0
References: <20220415130005.85879-1-andrea.merello@gmail.com>
 <20220415130005.85879-13-andrea.merello@gmail.com> <20220415174808.3b81baa4@jic23-huawei>
In-Reply-To: <20220415174808.3b81baa4@jic23-huawei>
Reply-To: andrea.merello@gmail.com
From:   Andrea Merello <andrea.merello@gmail.com>
Date:   Wed, 20 Apr 2022 09:22:07 +0200
Message-ID: <CAN8YU5Pfc6t=3_Yj7v5PF36Ut=1_Vr86Li60z=+SENKRyn+iTw@mail.gmail.com>
Subject: Re: [v4 12/14] iio: imu: add BNO055 serdev driver
To:     Jonathan Cameron <jic23@kernel.org>
Cc:     Mauro Carvalho Chehab <mchehab+huawei@kernel.org>,
        linux-iio <linux-iio@vger.kernel.org>,
        linux-kernel <linux-kernel@vger.kernel.org>,
        devicetree <devicetree@vger.kernel.org>,
        Lars-Peter Clausen <lars@metafoo.de>,
        Rob Herring <robh+dt@kernel.org>,
        Andy Shevchenko <andy.shevchenko@gmail.com>,
        Matt Ranostay <matt.ranostay@konsulko.com>,
        Alexandru Ardelean <ardeleanalex@gmail.com>,
        Jacopo Mondi <jacopo@jmondi.org>,
        Andrea Merello <andrea.merello@iit.it>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Il giorno ven 15 apr 2022 alle ore 18:40 Jonathan Cameron
<jic23@kernel.org> ha scritto:
>
> On Fri, 15 Apr 2022 15:00:03 +0200
> Andrea Merello <andrea.merello@gmail.com> wrote:
>
> > From: Andrea Merello <andrea.merello@iit.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@iit.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);
> > +}
> > +
> > +/*