Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <20200324202736.243314-1-gwendal@chromium.org> <20200324202736.243314-2-gwendal@chromium.org>
 <399a282a-e6a6-a1ed-26c4-1999008f242d@collabora.com>
In-Reply-To: <399a282a-e6a6-a1ed-26c4-1999008f242d@collabora.com>
From:   Gwendal Grignou <gwendal@chromium.org>
Date:   Thu, 26 Mar 2020 01:56:06 -0700
Message-ID: <CAPUE2uvZdbeU0zAgCGErDbDqu-VifVuNcLzvuo6mYY1MwYsaPQ@mail.gmail.com>
Subject: Re: [PATCH v6 01/11] platform: chrome: sensorhub: Add FIFO support
To:     Enric Balletbo i Serra <enric.balletbo@collabora.com>
Cc:     Benson Leung <bleung@chromium.org>,
        Jonathan Cameron <Jonathan.Cameron@huawei.com>,
        linux-iio <linux-iio@vger.kernel.org>,
        linux-kernel <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Wed, Mar 25, 2020 at 9:28 AM Enric Balletbo i Serra
<enric.balletbo@collabora.com> wrote:
>
> Hi Gwendal,
>
> Many thanks for sending this series upstream. Just one comment, other than that
> looks good to me.
>
> On 24/3/20 21:27, Gwendal Grignou wrote:
> > cros_ec_sensorhub registers a listener and query motion sense FIFO,
> > spread to iio sensors registers.
> >
> > To test, we can use libiio:
> > iiod&
> > iio_readdev -u ip:localhost -T 10000 -s 25 -b 16 cros-ec-gyro | od -x
> >
> > Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > Signed-off-by: Gwendal Grignou <gwendal@chromium.org>
>
> [snip]
>
> > +/**
> > + * cros_ec_sensorhub_ring_handler() - The trigger handler function
> > + *
> > + * @sensorhub: Sensor Hub object.
> > + *
> > + * Called by the notifier, process the EC sensor FIFO queue.
> > + */
> > +static void cros_ec_sensorhub_ring_handler(struct cros_ec_sensorhub *sensorhub)
> > +{
> > +     struct cros_ec_fifo_info *fifo_info = &sensorhub->fifo_info;
> > +     struct cros_ec_dev *ec = sensorhub->ec;
> > +     ktime_t fifo_timestamp, current_timestamp;
> > +     int i, j, number_data, ret;
> > +     unsigned long sensor_mask = 0;
> > +     struct ec_response_motion_sensor_data *in;
> > +     struct cros_ec_sensors_ring_sample *out, *last_out;
> > +
> > +     mutex_lock(&sensorhub->cmd_lock);
> > +
> > +     /* Get FIFO information if there are lost vectors. */
> > +     if (fifo_info->info.total_lost) {
> > +             /* Need to retrieve the number of lost vectors per sensor */
> > +             sensorhub->params->cmd = MOTIONSENSE_CMD_FIFO_INFO;
> > +             sensorhub->msg->outsize = 1;
> > +             sensorhub->msg->insize =
> > +                     sizeof(struct ec_response_motion_sense_fifo_info) +
> > +                     sizeof(u16) * CROS_EC_SENSOR_MAX;
> > +
> > +             if (cros_ec_cmd_xfer_status(ec->ec_dev, sensorhub->msg) < 0)
> > +                     goto error;
> > +
> > +             memcpy(fifo_info, &sensorhub->resp->fifo_info,
> > +                    sizeof(*fifo_info));
> > +
>
> Smatch is reporting:
Which version of smatch are you using? I am using
v0.5.0-6279-g2f013029 and the problem is not reported.
>
> cros_ec_sensorhub_ring_handler() error: memcpy() '&sensorhub->resp->fifo_info'
> too small (10 vs 42)
>
> Is it fine and safe to copy always the 42 bytes? I suspect that we should only
> copy the number of lost events, total_lost , not always the maximum (16). Or the
> EC is always sending the full array (16 bytes)?
EC will not fill the 42 bytes if it has less than 16 sensors. It is
safe because we are not looking at the bytes we don't need, but it is
not clean.
Working on a new patch set where I remove the CROS_EC_SENSOR_MAX
constant and dynamically allocate the data I need based on the number
of sensors.
>
> Thanks,
> Enric
>