linux-kernel - Re: [PATCH v5 3/3] drivers: iio: adc: add support for ad777x family

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CAMknhBFyydCJeAazDYMkkH=rKU2DbJGy=Kpb0242Vn81MHn0mQ@mail.gmail.com>
Date: Mon, 23 Sep 2024 14:51:28 +0200
From: David Lechner <dlechner@...libre.com>
To: "Nechita, Ramona" <Ramona.Nechita@...log.com>
Cc: Jonathan Cameron <jic23@...nel.org>, Lars-Peter Clausen <lars@...afoo.de>, 
	"Tanislav, Cosmin" <Cosmin.Tanislav@...log.com>, 
	"Hennerich, Michael" <Michael.Hennerich@...log.com>, Rob Herring <robh@...nel.org>, 
	Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>, 
	"Sa, Nuno" <Nuno.Sa@...log.com>, Andy Shevchenko <andy@...nel.org>, 
	"Schmitt, Marcelo" <Marcelo.Schmitt@...log.com>, Olivier Moysan <olivier.moysan@...s.st.com>, 
	Dumitru Ceclan <mitrutzceclan@...il.com>, Matteo Martelli <matteomartelli3@...il.com>, 
	AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>, 
	Alisa-Dariana Roman <alisadariana@...il.com>, Ivan Mikhaylov <fr0st61te@...il.com>, 
	Mike Looijmans <mike.looijmans@...ic.nl>, 
	"linux-iio@...r.kernel.org" <linux-iio@...r.kernel.org>, 
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>, 
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>
Subject: Re: [PATCH v5 3/3] drivers: iio: adc: add support for ad777x family

On Fri, Sep 20, 2024 at 3:24 PM Nechita, Ramona
<Ramona.Nechita@...log.com> wrote:
>
> Hello all,
>
> Just a minor question
> ...
> >
> >> +
> >> +static irqreturn_t ad7779_trigger_handler(int irq, void *p) {
> >> +    struct iio_poll_func *pf = p;
> >> +    struct iio_dev *indio_dev = pf->indio_dev;
> >> +    struct ad7779_state *st = iio_priv(indio_dev);
> >> +    int ret;
> >> +    int bit;
> >> +    int k = 0;
> >> +    /*
> >> +     * Each channel shifts out HEADER + 24 bits of data therefore 8 * u32
> >> +     * for the data and 64 bits for the timestamp
> >> +     */
> >> +    u32 tmp[10];
> >> +
> >> +    struct spi_transfer sd_readback_tr[] = {
> >> +            {
> >> +                    .rx_buf = st->spidata_rx,
> >> +                    .tx_buf = st->spidata_tx,
> >> +                    .len = AD7779_NUM_CHANNELS * AD7779_CHAN_DATA_SIZE,
> >> +            }
> >> +    };
> >> +
> >> +    if (!iio_buffer_enabled(indio_dev))
> >> +            goto exit_handler;
> >
> >If buffers aren't enabled, the push to buffers won't do anything. So this race shouldn't matter.  If it does, what happens?
> >I'm curious because I'd expect any races that cause trouble in this case to be pretty universal across drivers.
>
> I added that condition rather because the DRDY pulse will keep on being generated even when the buffers are not active,
> and it would be better to exit the function sooner. I tested it and it does not break to remove the condition, I just
> thought it made more sense like this. Should I delete it?
>
> >....
>
> Best regards,
> Ramona Nechita
>
>

Perhaps a better way to handle this would be to move

    disable_irq(st->spi->irq);

to the buffer predisable callback instead of doing it in the buffer
postdisable callback. Then we will be sure to not get any more DRDY
interrupts after the buffer is disabled.

(And to keep things balanced, moved the corresponding irq_enable() to
the buffer postenable callback.)

Since ad7779_trigger_handler is the IIO trigger interrupt handler and
not the DRDY interrupt handler though, it is already not possible for
this interrupt handler to be called while the IIO buffer is enabled.
So it should be safe to remove the if
(!iio_buffere_enabled(indio_dev)) even without the other changes I
suggested.