| 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
| ||
|
Message-ID: <56F9576A.8050007@kernel.org>
Date: Mon, 28 Mar 2016 17:10:18 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: "Tirdea, Irina" <irina.tirdea@...el.com>,
Peter Meerwald-Stadler <pmeerw@...erw.net>
Cc: "linux-iio@...r.kernel.org" <linux-iio@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
Hartmut Knaack <knaack.h@....de>,
Lars-Peter Clausen <lars@...afoo.de>,
"Purdila, Octavian" <octavian.purdila@...el.com>,
Markus Pargmann <mpa@...gutronix.de>,
"Pandruvada, Srinivas" <srinivas.pandruvada@...el.com>
Subject: Re: [PATCH 4/6] iio: accel: bmg160: optimize transfers in trigger
handler
On 28/03/16 17:03, Tirdea, Irina wrote:
>
>
>> -----Original Message-----
>> From: Jonathan Cameron [mailto:jic23@...nel.org]
>> Sent: 28 March, 2016 17:15
>> To: Peter Meerwald-Stadler
>> Cc: Tirdea, Irina; linux-iio@...r.kernel.org; linux-kernel@...r.kernel.org; Hartmut Knaack; Lars-Peter Clausen; Purdila, Octavian;
>> Markus Pargmann; Pandruvada, Srinivas
>> Subject: Re: [PATCH 4/6] iio: accel: bmg160: optimize transfers in trigger handler
>>
>> On 28/03/16 11:09, Peter Meerwald-Stadler wrote:
>>>
>>>>> Some i2c busses (e.g.: Synopsys DesignWare I2C adapter) need to
>>>>> enable/disable the bus at each i2c transfer and must wait for
>>>>> the enable/disable to happen before sending the data.
>>>>>
>>>>> When reading data in the trigger handler, the bmc150 accel driver does
>>>
>>> should refer to bmg160
>> Good spot. It's also a gyroscope, not an accelerometer... I just fixed this
>> up and repushed out testing.
>>>
> Oops... too much copy-paste :)
>
>>>>> one bus transfer for each axis. This has an impact on the frequency
>>>>> of the accelerometer at high sample rates due to additional delays
>>>>> introduced by the bus at each transfer.
>>>>>
>>>>> Reading all axis values in one bus transfer reduces the delays
>>>>> introduced by the bus.
>>>>>
>>>>> Signed-off-by: Irina Tirdea <irina.tirdea@...el.com>
>>>> I forgot to highlight on the earlier driver that there is also 'technically'
>>>> a bit of an ABI change here because we are now exporting as LE rather than CPU
>>>> order. However, I 'hope' anyone actually accessing the buffered data is either
>>>> doing it through a nice library or hasn't hacked the endian unwinding out of
>>>> the generic_buffer example!
>>>
>>> the patch takes away the possibility to do buffered reads on individual
>>> channels (not sure if this is useful per se)
>>>
>>> this optimizes for the common case, ok;
>>>
>>> wondering if adding
>>> .endianness = IIO_LE
>>> is actually an unrelated fix
>> Good point, when I first read the code I assumed we were moving from an i2c_word
>> read to a bulk read, thus necessitating this addition. However, we aren't as it
>> was previously as an i2c_bulk read of 2 bytes...
>>
>> Irina, could you confirm if this was broken before your patches?
>>
>
> Peter is right. I also had in mind the change from i2c_word to bulk read, but
> the regmap API has changed this in the meantime.
>
> Since the driver uses regmap_bulk_read to read 2 bytes for each
> axis, the data read will have the endianness of the device (little endian)
> and we should do endianness conversion or else it will not work on big
> endian platforms.
>
>> I'll leave this as is, perhaps we need an additional fix patch specifying LE to
>> put out as a fix.
>
> There is one more place in both bmc150 and bmg160 drivers where
> regmap_bulk_read is used without endianness conversion (when reading raw axes).
> I will send a separate patch to fix all endianness issues.
>
> While looking at the existent code, I also found another bug in the bmg160 driver
> that this patch fixes as a side effect: the error code returned by regmap_bulk_read
> is saved in the data->buffer instead of the value read. Not sure how to handle this,
> since it is fixed now by this patch. Jonathan, should I send a fix patch for this?
Send a patch against, fixes-togreg, or given timing fixes-togreg-post-rc1 as appropriate
but put a comment in there saying it was also fixed in "adfasfaf" so that the merge
is obvious when the two hit each other - hopefully in Greg's tree...
Jonathan
>
> Thanks,
> Irina
>
>>>
>>>> Again, fingers crossed this doesn't break anything significant.
>>>>
>>>> Applied,
>>>>
>>>> Thanks,
>>>>
>>>> Jonathan
>>>>> ---
>>>>> drivers/iio/gyro/bmg160_core.c | 17 ++++++-----------
>>>>> 1 file changed, 6 insertions(+), 11 deletions(-)
>>>>>
>>>>> diff --git a/drivers/iio/gyro/bmg160_core.c b/drivers/iio/gyro/bmg160_core.c
>>>>> index 8d6e5b1..43570b8 100644
>>>>> --- a/drivers/iio/gyro/bmg160_core.c
>>>>> +++ b/drivers/iio/gyro/bmg160_core.c
>>>>> @@ -734,6 +734,7 @@ static const struct iio_event_spec bmg160_event = {
>>>>> .sign = 's', \
>>>>> .realbits = 16, \
>>>>> .storagebits = 16, \
>>>>> + .endianness = IIO_LE, \
>>>>> }, \
>>>>> .event_spec = &bmg160_event, \
>>>>> .num_event_specs = 1 \
>>>>> @@ -773,20 +774,14 @@ static irqreturn_t bmg160_trigger_handler(int irq, void *p)
>>>>> struct iio_poll_func *pf = p;
>>>>> struct iio_dev *indio_dev = pf->indio_dev;
>>>>> struct bmg160_data *data = iio_priv(indio_dev);
>>>>> - int bit, ret, i = 0;
>>>>> - unsigned int val;
>>>>> + int ret;
>>>>>
>>>>> mutex_lock(&data->mutex);
>>>>> - for (bit = 0; bit < AXIS_MAX; bit++) {
>>>>> - ret = regmap_bulk_read(data->regmap, BMG160_AXIS_TO_REG(bit),
>>>>> - &val, 2);
>>>>> - if (ret < 0) {
>>>>> - mutex_unlock(&data->mutex);
>>>>> - goto err;
>>>>> - }
>>>>> - data->buffer[i++] = ret;
>>>>> - }
>>>>> + ret = regmap_bulk_read(data->regmap, BMG160_REG_XOUT_L,
>>>>> + data->buffer, AXIS_MAX * 2);
>>>>> mutex_unlock(&data->mutex);
>>>>> + if (ret < 0)
>>>>> + goto err;
>>>>>
>>>>> iio_push_to_buffers_with_timestamp(indio_dev, data->buffer,
>>>>> pf->timestamp);
>>>>>
>>>>
>>>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@...r.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
Powered by blists - more mailing lists