| 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
| ||
|
Date: Fri, 28 Jun 2024 19:57:52 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: Guillaume Stols <gstols@...libre.com>
Cc: Lars-Peter Clausen <lars@...afoo.de>, Michael Hennerich
<Michael.Hennerich@...log.com>, linux-iio@...r.kernel.org,
linux-kernel@...r.kernel.org, jstephan@...libre.com, dlechner@...libre.com
Subject: Re: [PATCH v2] iio: adc: ad7606: remove frstdata check for serial
mode
On Mon, 24 Jun 2024 15:33:01 +0200
Guillaume Stols <gstols@...libre.com> wrote:
> On 6/23/24 17:54, Jonathan Cameron wrote:
> > On Tue, 18 Jun 2024 13:45:00 +0000
> > Guillaume Stols <gstols@...libre.com> wrote:
> >
> >> Frstdata pin is set high during the first sample's transmission and then
> >> set low. This code chunk attempts to recover from an eventual glitch in
> >> the clock by checking frstdata state after reading the first channel's
> >> sample. Currently, in serial mode, this check happens AFTER the 16th
> >> pulse, and if frstdata is not set it resets the device and returns EINVAL.
> >> According to the datasheet, "The FRSTDATA output returns to a logic low
> >> following the 16th SCLK falling edge.", thus after the 16th pulse, the
> >> check will always be true, and the driver will not work as expected. Thus
> >> it must be removed for serial mode.
> > when you say will not work as expected, is this is normal circumstances, or
> > when dealing with a clock glitch? i.e. should this have a fixes tag and
> > got upstream asap or is it just cleaning up a corner case and can wait for
> > now?
> >
> > One trivial comment inline.
> >
> > Jonathan
>
> It completely prevents the driver to work when adi, first-data
> (optional) is defined in the DT.
>
> So I guess anyone having the driver working with the serial interface
> right now did not define frstdata in the DT.
>
> However, for someone new that sets adi, first-data in the DT, it is not
> very straightforward to spot where the issue is.
Thanks for the info. Could you send a v2 with that included in the
patch description and a suitable fixes tag. Plus tidy up the extra
linebreak noted below.
Thanks,
Jonathan
>
> >
> >> Signed-off-by: Guillaume Stols <gstols@...libre.com>
> >> ---
> >> Changes in v2:
> >> - Remove frstdata check only for the serial interface as suggested by
> >> Michael Hennerich.
> >> - Link to v1: https://lore.kernel.org/r/20240417-cleanup-ad7606-v1-1-5c2a29662c0a@baylibre.com
> >> ---
> >> drivers/iio/adc/ad7606.c | 28 ++-----------------------
> >> drivers/iio/adc/ad7606.h | 2 ++
> >> drivers/iio/adc/ad7606_par.c | 49 +++++++++++++++++++++++++++++++++++++++++---
> >> 3 files changed, 50 insertions(+), 29 deletions(-)
> >>
> >> diff --git a/drivers/iio/adc/ad7606.c b/drivers/iio/adc/ad7606.c
> >> index 3a417595294f..c321c6ef48df 100644
> >> --- a/drivers/iio/adc/ad7606.c
> >> +++ b/drivers/iio/adc/ad7606.c
> >> @@ -49,7 +49,7 @@ static const unsigned int ad7616_oversampling_avail[8] = {
> >> 1, 2, 4, 8, 16, 32, 64, 128,
> >> };
> >>
> >> -static int ad7606_reset(struct ad7606_state *st)
> >> +int ad7606_reset(struct ad7606_state *st)
> >> {
> >> if (st->gpio_reset) {
> >> gpiod_set_value(st->gpio_reset, 1);
> >> @@ -60,6 +60,7 @@ static int ad7606_reset(struct ad7606_state *st)
> >>
> >> return -ENODEV;
> >> }
> >> +EXPORT_SYMBOL_NS_GPL(ad7606_reset, IIO_AD7606);
> >>
> >> static int ad7606_reg_access(struct iio_dev *indio_dev,
> >> unsigned int reg,
> >> @@ -88,31 +89,6 @@ static int ad7606_read_samples(struct ad7606_state *st)
> >> {
> >> unsigned int num = st->chip_info->num_channels - 1;
> >> u16 *data = st->data;
> >> - int ret;
> >> -
> >> - /*
> >> - * The frstdata signal is set to high while and after reading the sample
> >> - * of the first channel and low for all other channels. This can be used
> >> - * to check that the incoming data is correctly aligned. During normal
> >> - * operation the data should never become unaligned, but some glitch or
> >> - * electrostatic discharge might cause an extra read or clock cycle.
> >> - * Monitoring the frstdata signal allows to recover from such failure
> >> - * situations.
> >> - */
> >> -
> >> - if (st->gpio_frstdata) {
> >> - ret = st->bops->read_block(st->dev, 1, data);
> >> - if (ret)
> >> - return ret;
> >> -
> >> - if (!gpiod_get_value(st->gpio_frstdata)) {
> >> - ad7606_reset(st);
> >> - return -EIO;
> >> - }
> >> -
> >> - data++;
> >> - num--;
> >> - }
> >>
> >> return st->bops->read_block(st->dev, num, data);
> >> }
> >> diff --git a/drivers/iio/adc/ad7606.h b/drivers/iio/adc/ad7606.h
> >> index 0c6a88cc4695..6649e84d25de 100644
> >> --- a/drivers/iio/adc/ad7606.h
> >> +++ b/drivers/iio/adc/ad7606.h
> >> @@ -151,6 +151,8 @@ int ad7606_probe(struct device *dev, int irq, void __iomem *base_address,
> >> const char *name, unsigned int id,
> >> const struct ad7606_bus_ops *bops);
> >>
> >> +int ad7606_reset(struct ad7606_state *st);
> >> +
> >> enum ad7606_supported_device_ids {
> >> ID_AD7605_4,
> >> ID_AD7606_8,
> >> diff --git a/drivers/iio/adc/ad7606_par.c b/drivers/iio/adc/ad7606_par.c
> >> index d8408052262e..1f7050297b64 100644
> >> --- a/drivers/iio/adc/ad7606_par.c
> >> +++ b/drivers/iio/adc/ad7606_par.c
> >> @@ -7,6 +7,7 @@
> >>
> >> #include <linux/mod_devicetable.h>
> >> #include <linux/module.h>
> >> +#include <linux/gpio/consumer.h>
> >> #include <linux/platform_device.h>
> >> #include <linux/types.h>
> >> #include <linux/err.h>
> >> @@ -21,8 +22,30 @@ static int ad7606_par16_read_block(struct device *dev,
> >> struct iio_dev *indio_dev = dev_get_drvdata(dev);
> >> struct ad7606_state *st = iio_priv(indio_dev);
> >>
> >> - insw((unsigned long)st->base_address, buf, count);
> >>
> >> + /*
> >> + * On the parallel interface, the frstdata signal is set to high while
> >> + * and after reading the sample of the first channel and low for all
> >> + * other channels. This can be used to check that the incoming data is
> >> + * correctly aligned. During normal operation the data should never
> >> + * become unaligned, but some glitch or electrostatic discharge might
> >> + * cause an extra read or clock cycle. Monitoring the frstdata signal
> >> + * allows to recover from such failure situations.
> >> + */
> >> + int num = count;
> >> + u16 *_buf = buf;
> >> +
> >> + if (st->gpio_frstdata) {
> >> + insw((unsigned long)st->base_address, _buf, 1);
> >> + if (!gpiod_get_value(st->gpio_frstdata)) {
> >> + ad7606_reset(st);
> >> + return -EIO;
> >> + }
> >> + _buf++;
> >> + num--;
> >> + }
> >> + insw((unsigned long)st->base_address, _buf, num)
> >> +;
> > Seems this slipped onto the next line.
> > Make sure to run checkpatch which I would have thought would catch this.
> >
> >> return 0;
> >> }
> >>
> >> @@ -35,8 +58,28 @@ static int ad7606_par8_read_block(struct device *dev,
> >> {
> >> struct iio_dev *indio_dev = dev_get_drvdata(dev);
> >> struct ad7606_state *st = iio_priv(indio_dev);
> >> -
> >> - insb((unsigned long)st->base_address, buf, count * 2);
> >> + /*
> >> + * On the parallel interface, the frstdata signal is set to high while
> >> + * and after reading the sample of the first channel and low for all
> >> + * other channels. This can be used to check that the incoming data is
> >> + * correctly aligned. During normal operation the data should never
> >> + * become unaligned, but some glitch or electrostatic discharge might
> >> + * cause an extra read or clock cycle. Monitoring the frstdata signal
> >> + * allows to recover from such failure situations.
> >> + */
> >> + int num = count;
> >> + u16 *_buf = buf;
> >> +
> >> + if (st->gpio_frstdata) {
> >> + insb((unsigned long)st->base_address, _buf, 2);
> >> + if (!gpiod_get_value(st->gpio_frstdata)) {
> >> + ad7606_reset(st);
> >> + return -EIO;
> >> + }
> >> + _buf++;
> >> + num--;
> >> + }
> >> + insb((unsigned long)st->base_address, _buf, num * 2);
> >>
> >> return 0;
> >> }
> >>
> >> ---
> >> base-commit: 07d4d0bb4a8ddcc463ed599b22f510d5926c2495
> >> change-id: 20240416-cleanup-ad7606-161e2ed9818b
> >>
> >> Best regards,
Powered by blists - more mailing lists