| 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: Thu, 3 Dec 2020 08:15:26 +0000
From: "Ardelean, Alexandru" <alexandru.Ardelean@...log.com>
To: Andy Shevchenko <andy.shevchenko@...il.com>
CC: linux-spi <linux-spi@...r.kernel.org>,
devicetree <devicetree@...r.kernel.org>,
Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
Rob Herring <robh+dt@...nel.org>,
Mark Brown <broonie@...nel.org>,
"Bogdan, Dragos" <Dragos.Bogdan@...log.com>
Subject: RE: [PATCH v3 1/3] spi: uapi: unify SPI modes into a single spi.h
header
> -----Original Message-----
> From: Andy Shevchenko <andy.shevchenko@...il.com>
> Sent: Friday, November 27, 2020 4:13 PM
> To: Ardelean, Alexandru <alexandru.Ardelean@...log.com>
> Cc: linux-spi <linux-spi@...r.kernel.org>; devicetree
> <devicetree@...r.kernel.org>; Linux Kernel Mailing List <linux-
> kernel@...r.kernel.org>; Rob Herring <robh+dt@...nel.org>; Mark Brown
> <broonie@...nel.org>; Bogdan, Dragos <Dragos.Bogdan@...log.com>
> Subject: Re: [PATCH v3 1/3] spi: uapi: unify SPI modes into a single spi.h header
>
> On Fri, Nov 27, 2020 at 3:08 PM Alexandru Ardelean
> <alexandru.ardelean@...log.com> wrote:
> >
> > This change moves all the SPI mode bits into a separate 'spi.h' header
> > in uapi. This is meant to re-use these definitions inside the kernel
> > as well as export them to userspace (via uapi).
>
> uapi -> UAPI (or uAPI) here and everywhere else where it makes sense.
>
> > The SPI mode definitions have usually been duplicated between between
> > 'include/linux/spi/spi.h' and 'include/uapi/linux/spi/spidev.h', so
> > whenever adding a new entry, this would need to be put in both headers.
> >
> > They've been moved from 'include/linux/spi/spi.h', since that seems a
> > bit more complete; the bits have descriptions and there is the
> SPI_MODE_X_MASK.
> >
> > For now, this change does a simple move; no conversions to BIT()
> > macros are being done at this point. This can be done later, as that
> > requires also another header inclusion (the 'const.h' header).
> > The change as-is makes this 'spi.h' header more standalone.
> >
> > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@...log.com>
> > ---
> >
> > Personally, I am not sure whether to convert the bitfield tos _BITUL()
> > macros or not. I feel that not-having these macros makes this uapi
> > spi.h header more standalone.
> > If there's a strong insistence to use those _BITUL() macros, I'll do it.
> > I'm hesitant now, because it requires that this spi.h includes the
> > 'const.h' header.
>
> _BITUL is a part of uAPI, why not to use it?
> In general BIT() type of macros makes values easier to read and less error prone
> (in big numbers it's easy to miss 0).
> It's not a strong opinion, it's just the rationale behind how I see it.
Yeah I understand.
I guess since this is a new header in UAPI, I can use the _BITUL macro.
Sometimes I look at some code and remember how some people tend to use some things.
Like, I would expect some people to maybe just copy this header as-is in some "libspi".
So, then: do we make it easier for those people or not?
But again, since this is a new header, the concern may be reduced by just using _BITUL() from the start.
I sometimes have these arguments with myself whenever going into API code.
Kernel code is easy: all the users of that code in the kernel, and we try not to bother too much with out-of-tree drivers. Those get impossible to care about.
>
> > Changelog v2 -> v3:
> > *
> > https://urldefense.com/v3/__https://lore.kernel.org/linux-spi/20201124
> > 102152.16548-1-
> alexandru.ardelean@...log.com/__;!!A3Ni8CS0y2Y!oNpULNID
> > JsvU1BX8CZgZYoS90mW3rZ2dHrZOwTRS4ndZ-_rLq3eJt22Rj1RQafWyw1-3YA$
> > * dropped 'spi: convert to BIT() all spi_device flags '
> > added 'spi: uapi: unify SPI modes into a single spi.h header'
> >
> > include/linux/spi/spi.h | 22 +--------------
> > include/uapi/linux/spi/spi.h | 47 +++++++++++++++++++++++++++++++++
> > include/uapi/linux/spi/spidev.h | 30 +--------------------
> > 3 files changed, 49 insertions(+), 50 deletions(-) create mode
> > 100644 include/uapi/linux/spi/spi.h
> >
> > diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h index
> > aa09fdc8042d..a4fedb33d34b 100644
> > --- a/include/linux/spi/spi.h
> > +++ b/include/linux/spi/spi.h
> > @@ -14,6 +14,7 @@
> > #include <linux/scatterlist.h>
> > #include <linux/gpio/consumer.h>
> > #include <linux/ptp_clock_kernel.h>
> > +#include <uapi/linux/spi/spi.h>
> >
> > struct dma_chan;
> > struct property_entry;
> > @@ -165,27 +166,6 @@ struct spi_device {
> > u8 bits_per_word;
> > bool rt;
> > u32 mode;
> > -#define SPI_CPHA 0x01 /* clock phase */
> > -#define SPI_CPOL 0x02 /* clock polarity */
> > -#define SPI_MODE_0 (0|0) /* (original MicroWire) */
> > -#define SPI_MODE_1 (0|SPI_CPHA)
> > -#define SPI_MODE_2 (SPI_CPOL|0)
> > -#define SPI_MODE_3 (SPI_CPOL|SPI_CPHA)
> > -#define SPI_MODE_X_MASK (SPI_CPOL|SPI_CPHA)
> > -#define SPI_CS_HIGH 0x04 /* chipselect active high? */
> > -#define SPI_LSB_FIRST 0x08 /* per-word bits-on-wire */
> > -#define SPI_3WIRE 0x10 /* SI/SO signals shared */
> > -#define SPI_LOOP 0x20 /* loopback mode */
> > -#define SPI_NO_CS 0x40 /* 1 dev/bus, no chipselect */
> > -#define SPI_READY 0x80 /* slave pulls low to pause */
> > -#define SPI_TX_DUAL 0x100 /* transmit with 2 wires */
> > -#define SPI_TX_QUAD 0x200 /* transmit with 4 wires */
> > -#define SPI_RX_DUAL 0x400 /* receive with 2 wires */
> > -#define SPI_RX_QUAD 0x800 /* receive with 4 wires */
> > -#define SPI_CS_WORD 0x1000 /* toggle cs after each word */
> > -#define SPI_TX_OCTAL 0x2000 /* transmit with 8 wires */
> > -#define SPI_RX_OCTAL 0x4000 /* receive with 8 wires */
> > -#define SPI_3WIRE_HIZ 0x8000 /* high impedance turnaround
> */
> > int irq;
> > void *controller_state;
> > void *controller_data;
> > diff --git a/include/uapi/linux/spi/spi.h
> > b/include/uapi/linux/spi/spi.h new file mode 100644 index
> > 000000000000..ae028d64c779
> > --- /dev/null
> > +++ b/include/uapi/linux/spi/spi.h
> > @@ -0,0 +1,47 @@
> > +/* SPDX-License-Identifier: GPL-2.0+ WITH Linux-syscall-note */
> > +/*
> > + * include/linux/spi/spi.h
> > + *
> > + * This program is free software; you can redistribute it and/or
> > +modify
> > + * it under the terms of the GNU General Public License as published
> > +by
> > + * the Free Software Foundation; either version 2 of the License, or
> > + * (at your option) any later version.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> > + * GNU General Public License for more details.
> > + *
> > + * You should have received a copy of the GNU General Public License
> > + * along with this program; if not, write to the Free Software
> > + * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
> > + */
>
> Do we still need this boilerplate license header?
I'll remove this.
I'm still a n00b with all this licensing stuff.
>
> > +
> > +#ifndef _UAPI_SPI_H
> > +#define _UAPI_SPI_H
> > +
> > +#define SPI_CPHA 0x01 /* clock phase */
> > +#define SPI_CPOL 0x02 /* clock polarity */
> > +
> > +#define SPI_MODE_0 (0|0) /* (original MicroWire) */
> > +#define SPI_MODE_1 (0|SPI_CPHA)
> > +#define SPI_MODE_2 (SPI_CPOL|0)
> > +#define SPI_MODE_3 (SPI_CPOL|SPI_CPHA)
> > +#define SPI_MODE_X_MASK (SPI_CPOL|SPI_CPHA)
> > +
> > +#define SPI_CS_HIGH 0x04 /* chipselect active high? */
> > +#define SPI_LSB_FIRST 0x08 /* per-word bits-on-wire */
> > +#define SPI_3WIRE 0x10 /* SI/SO signals shared */
> > +#define SPI_LOOP 0x20 /* loopback mode */
> > +#define SPI_NO_CS 0x40 /* 1 dev/bus, no chipselect */
> > +#define SPI_READY 0x80 /* slave pulls low to pause */
> > +#define SPI_TX_DUAL 0x100 /* transmit with 2 wires */
> > +#define SPI_TX_QUAD 0x200 /* transmit with 4 wires */
> > +#define SPI_RX_DUAL 0x400 /* receive with 2 wires */
> > +#define SPI_RX_QUAD 0x800 /* receive with 4 wires */
> > +#define SPI_CS_WORD 0x1000 /* toggle cs after each word */
> > +#define SPI_TX_OCTAL 0x2000 /* transmit with 8 wires */
> > +#define SPI_RX_OCTAL 0x4000 /* receive with 8 wires */
> > +#define SPI_3WIRE_HIZ 0x8000 /* high impedance turnaround
> */
> > +
> > +#endif /* _UAPI_SPI_H */
> > diff --git a/include/uapi/linux/spi/spidev.h
> > b/include/uapi/linux/spi/spidev.h index d56427c0b3e0..0c3da08f2aff
> > 100644
> > --- a/include/uapi/linux/spi/spidev.h
> > +++ b/include/uapi/linux/spi/spidev.h
> > @@ -25,35 +25,7 @@
> >
> > #include <linux/types.h>
> > #include <linux/ioctl.h>
> > -
> > -/* User space versions of kernel symbols for SPI clocking modes,
> > - * matching <linux/spi/spi.h>
> > - */
> > -
> > -#define SPI_CPHA 0x01
> > -#define SPI_CPOL 0x02
> > -
> > -#define SPI_MODE_0 (0|0)
> > -#define SPI_MODE_1 (0|SPI_CPHA)
> > -#define SPI_MODE_2 (SPI_CPOL|0)
> > -#define SPI_MODE_3 (SPI_CPOL|SPI_CPHA)
> > -
> > -#define SPI_CS_HIGH 0x04
> > -#define SPI_LSB_FIRST 0x08
> > -#define SPI_3WIRE 0x10
> > -#define SPI_LOOP 0x20
> > -#define SPI_NO_CS 0x40
> > -#define SPI_READY 0x80
> > -#define SPI_TX_DUAL 0x100
> > -#define SPI_TX_QUAD 0x200
> > -#define SPI_RX_DUAL 0x400
> > -#define SPI_RX_QUAD 0x800
> > -#define SPI_CS_WORD 0x1000
> > -#define SPI_TX_OCTAL 0x2000
> > -#define SPI_RX_OCTAL 0x4000
> > -#define SPI_3WIRE_HIZ 0x8000
> > -
> > -/*-------------------------------------------------------------------
> > --------*/
> > +#include <linux/spi/spi.h>
> >
> > /* IOCTL commands */
> >
> > --
> > 2.27.0
> >
>
>
> --
> With Best Regards,
> Andy Shevchenko
Powered by blists - more mailing lists