linux-kernel - Re: [PATCH 1/3] iio: imu: adis: add doc-string for 'adis' struct

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <20200221161428.188e1532@archlinux>
Date:   Fri, 21 Feb 2020 16:14:28 +0000
From:   Jonathan Cameron <jic23@...nel.org>
To:     "Ardelean, Alexandru" <alexandru.Ardelean@...log.com>
Cc:     "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-iio@...r.kernel.org" <linux-iio@...r.kernel.org>
Subject: Re: [PATCH 1/3] iio: imu: adis: add doc-string for 'adis' struct

On Fri, 21 Feb 2020 15:59:35 +0000
"Ardelean, Alexandru" <alexandru.Ardelean@...log.com> wrote:

> On Fri, 2020-02-21 at 13:11 +0000, Jonathan Cameron wrote:
> > On Fri, 21 Feb 2020 13:49:41 +0200
> > Alexandru Ardelean <alexandru.ardelean@...log.com> wrote:
> >   
> > > This change adds a doc-string for the 'adis' struct. It details the fields
> > > and their roles.
> > > 
> > > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@...log.com>
> > > ---
> > >  include/linux/iio/imu/adis.h | 14 ++++++++++++++
> > >  1 file changed, 14 insertions(+)
> > > 
> > > diff --git a/include/linux/iio/imu/adis.h b/include/linux/iio/imu/adis.h
> > > index ac7cfd073804..0787a3aabd05 100644
> > > --- a/include/linux/iio/imu/adis.h
> > > +++ b/include/linux/iio/imu/adis.h
> > > @@ -73,6 +73,20 @@ struct adis_data {
> > >  	bool has_paging;
> > >  };
> > >  
> > > +/**
> > > + * struct adis - ADIS device instance data
> > > + * @spi: Reference to SPI device which owns this ADIS IIO device
> > > + * @trig: IIO trigger object data
> > > + * @data: ADIS chip variant specific data
> > > + * @burst: ADIS burst transfer information
> > > + * @state_lock: Lock used by the device to protect state
> > > + * @msg: SPI message object
> > > + * @xfer: SPI transfer objects to be used for a @msg
> > > + * @current_page: Some ADIS devices have registers, this selects current
> > > page
> > > + * @buffer: Data buffer for information read from the device
> > > + * @tx: Cacheline aligned TX buffer for SPI transfers
> > > + * @rx: Cacheline aligned RX buffer for SPI transfers  
> > 
> > This last one isn't true..   
> 
> Oh right.
> I noticed this at some point, then forgot about it.
> [ The "joys" of multi-tasking; sorry about it :) ]
> I guess I should also add a patch in this series making it cacheline-aligned.
> I don't see a reason why not-to [unless I am misunderstanging something]
No.  The code is fine.  The purpose of the cacheline stuff is to ensure
no other data ends up in the same cacheline and might be changed in parallel
with the spi dma taking place.   You only need to force it for the first
element when you have 2 buffers like this.

So you need to update the comment in this patch to say something less specific.
Perhaps

@tx: DMA safe TX buffer for SPI transfers
@rx: DMA safe RX buffer for SPI transfers.

Thanks,

Jonathan

> 
> 
> >   
> > > + */
> > >  struct adis {
> > >  	struct spi_device	*spi;
> > >  	struct iio_trigger	*trig;