Linux-IIO Archive on lore.kernel.org
 help / color / Atom feed
From: Gwendal Grignou <gwendal@chromium.org>
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>
Subject: Re: [PATCH v6 01/11] platform: chrome: sensorhub: Add FIFO support
Date: Thu, 26 Mar 2020 01:56:06 -0700
Message-ID: <CAPUE2uvZdbeU0zAgCGErDbDqu-VifVuNcLzvuo6mYY1MwYsaPQ@mail.gmail.com> (raw)
In-Reply-To: <399a282a-e6a6-a1ed-26c4-1999008f242d@collabora.com>

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
>

  reply index

Thread overview: 15+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-03-24 20:27 [PATCH v6 00/11] Cros EC sensor hub " Gwendal Grignou
2020-03-24 20:27 ` [PATCH v6 01/11] platform: chrome: sensorhub: Add " Gwendal Grignou
2020-03-25 16:28   ` Enric Balletbo i Serra
2020-03-26  8:56     ` Gwendal Grignou [this message]
2020-03-24 20:27 ` [PATCH v6 02/11] platform: chrome: sensorhub: Add code to spread timestmap Gwendal Grignou
2020-03-24 20:27 ` [PATCH v6 03/11] platform: chrome: sensorhub: Add median filter Gwendal Grignou
2020-03-24 20:27 ` [PATCH v6 04/11] iio: cros_ec: Move function description to .c file Gwendal Grignou
2020-03-24 20:27 ` [PATCH v6 05/11] iio: expose iio_device_set_clock Gwendal Grignou
2020-03-24 20:27 ` [PATCH v6 06/11] iio: cros_ec: Register to cros_ec_sensorhub when EC supports FIFO Gwendal Grignou
2020-03-24 20:27 ` [PATCH v6 07/11] iio: cros_ec: Remove pm function Gwendal Grignou
2020-03-24 20:27 ` [PATCH v6 08/11] iio: cros_ec: Expose hwfifo_timeout Gwendal Grignou
2020-03-24 20:27 ` [PATCH v6 09/11] iio: cros_ec: Report hwfifo_watermark_max Gwendal Grignou
2020-03-24 20:27 ` [PATCH v6 10/11] iio: cros_ec: Use Hertz as unit for sampling frequency Gwendal Grignou
2020-03-24 20:27 ` [PATCH v6 11/11] iio: cros_ec: flush as hwfifo attribute Gwendal Grignou
2020-03-25 15:59 ` [PATCH v6 00/11] Cros EC sensor hub FIFO support Andy Shevchenko

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CAPUE2uvZdbeU0zAgCGErDbDqu-VifVuNcLzvuo6mYY1MwYsaPQ@mail.gmail.com \
    --to=gwendal@chromium.org \
    --cc=Jonathan.Cameron@huawei.com \
    --cc=bleung@chromium.org \
    --cc=enric.balletbo@collabora.com \
    --cc=linux-iio@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-IIO Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-iio/0 linux-iio/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-iio linux-iio/ https://lore.kernel.org/linux-iio \
		linux-iio@vger.kernel.org
	public-inbox-index linux-iio

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-iio


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git