| 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: <CAPk366Qo916k_UggtMog875J98s5PkagiReJbO8s7eNpGZrr=g@mail.gmail.com>
Date: Thu, 7 Nov 2019 10:27:19 +0100
From: Mateusz Holenko <mholenko@...micro.com>
To: Rob Herring <robh@...nel.org>
Cc: Mark Rutland <mark.rutland@....com>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Jiri Slaby <jslaby@...e.com>, devicetree@...r.kernel.org,
"open list:SERIAL DRIVERS" <linux-serial@...r.kernel.org>,
Stafford Horne <shorne@...il.com>,
Karol Gugala <kgugala@...micro.com>,
Mauro Carvalho Chehab <mchehab+samsung@...nel.org>,
"David S. Miller" <davem@...emloft.net>,
"Paul E. McKenney" <paulmck@...ux.ibm.com>,
Filip Kokosinski <fkokosinski@...ernships.antmicro.com>,
Joel Stanley <joel@....id.au>,
Jonathan Cameron <Jonathan.Cameron@...wei.com>,
Maxime Ripard <mripard@...nel.org>,
Shawn Guo <shawnguo@...nel.org>,
Heiko Stuebner <heiko@...ech.de>,
Sam Ravnborg <sam@...nborg.org>,
Icenowy Zheng <icenowy@...c.io>,
Laurent Pinchart <laurent.pinchart@...asonboard.com>,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 2/4] litex: add common LiteX header
sob., 26 paź 2019 o 02:03 Rob Herring <robh@...nel.org> napisał(a):
>
> On Wed, Oct 23, 2019 at 11:47:04AM +0200, Mateusz Holenko wrote:
> > It provides helper CSR access functions used by all
> > LiteX drivers.
> >
> > Signed-off-by: Mateusz Holenko <mholenko@...micro.com>
> > ---
> > This commit has been introduced in v2 of the patchset.
> >
> > MAINTAINERS | 6 +++++
> > include/linux/litex.h | 59 +++++++++++++++++++++++++++++++++++++++++++
> > 2 files changed, 65 insertions(+)
> > create mode 100644 include/linux/litex.h
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 296de2b51c83..eaa51209bfb2 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -9493,6 +9493,12 @@ F: Documentation/misc-devices/lis3lv02d.rst
> > F: drivers/misc/lis3lv02d/
> > F: drivers/platform/x86/hp_accel.c
> >
> > +LITEX PLATFORM
> > +M: Karol Gugala <kgugala@...micro.com>
> > +M: Mateusz Holenko <mholenko@...micro.com>
> > +S: Maintained
> > +F: include/linux/litex.h
> > +
> > LIVE PATCHING
> > M: Josh Poimboeuf <jpoimboe@...hat.com>
> > M: Jiri Kosina <jikos@...nel.org>
> > diff --git a/include/linux/litex.h b/include/linux/litex.h
> > new file mode 100644
> > index 000000000000..e793d2d7c881
> > --- /dev/null
> > +++ b/include/linux/litex.h
> > @@ -0,0 +1,59 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/*
> > + * Common LiteX header providing
> > + * helper functions for accessing CSRs.
> > + *
> > + * Copyright (C) 2019 Antmicro <www.antmicro.com>
> > + */
> > +
> > +#ifndef _LINUX_LITEX_H
> > +#define _LINUX_LITEX_H
> > +
> > +#include <linux/io.h>
> > +#include <linux/types.h>
> > +#include <linux/compiler_types.h>
> > +
> > +#define LITEX_REG_SIZE 0x4
> > +#define LITEX_SUBREG_SIZE 0x1
> > +#define LITEX_SUBREG_SIZE_BIT (LITEX_SUBREG_SIZE * 8)
> > +
> > +#ifdef __LITTLE_ENDIAN
> > +# define LITEX_READ_REG(addr) ioread32(addr)
> > +# define LITEX_READ_REG_OFF(addr, off) ioread32(addr + off)
> > +# define LITEX_WRITE_REG(val, addr) iowrite32(val, addr)
> > +# define LITEX_WRITE_REG_OFF(val, addr, off) iowrite32(val, addr + off)
> > +#else
> > +# define LITEX_READ_REG(addr) ioread32be(addr)
> > +# define LITEX_READ_REG_OFF(addr, off) ioread32be(addr + off)
> > +# define LITEX_WRITE_REG(val, addr) iowrite32be(val, addr)
> > +# define LITEX_WRITE_REG_OFF(val, addr, off) iowrite32be(val, addr + off)
>
> Defining custom accessors makes it harder for others to understand
> the code. The __raw_readl/writel accessors are native endian, so use
> those. One difference though is they don't have a memory barrier, but
> based on the below functions, you may want to do your own barrier
> anyways. And if DMA is not involved you don't need the barriers either.
LiteX CSRs are always little endian (even when combined with a big endian CPU),
so using just __raw_readl/writel won't work in all cases. This is the reason why
we proposed a custom accessors defined based on host CPU endianness.
Would adding a comment explaining this be enough or do you have other ideas?
> The _OFF variants don't add anything. LITEX_READ_REG(addr + off) is just
> as easy to read if not easier than LITEX_READ_REG_OFF(addr, off).
I agree, LITEX_READ_REG/LITEX_WRITE_REG is enough.
> > +#endif
> > +
> > +/* Helper functions for manipulating LiteX registers */
> > +
> > +static inline void litex_set_reg(void __iomem *reg, u32 reg_size, u32 val)
> > +{
> > + u32 shifted_data, shift, i;
> > +
> > + for (i = 0; i < reg_size; ++i) {
> > + shift = ((reg_size - i - 1) * LITEX_SUBREG_SIZE_BIT);
> > + shifted_data = val >> shift;
> > + LITEX_WRITE_REG(shifted_data, reg + (LITEX_REG_SIZE * i));
> > + }
> > +}
>
> The problem with this is it hides whether you need to do any locking.
> Normally, you would assume a register access is atomic when it's not
> here. You could add locking in this function, but then you're doing
> locking even when not needed.
>
> It doesn't look like you actually use this in your series, so maybe
> remove until you do.
That's right. I added those functions in advance, having in mind
further drivers,
but maybe it will be better to drop them for now and re-introduce later.
> > +
> > +static inline u32 litex_get_reg(void __iomem *reg, u32 reg_size)
> > +{
> > + u32 shifted_data, shift, i;
> > + u32 result = 0;
> > +
> > + for (i = 0; i < reg_size; ++i) {
> > + shifted_data = LITEX_READ_REG(reg + (LITEX_REG_SIZE * i));
> > + shift = ((reg_size - i - 1) * LITEX_SUBREG_SIZE_BIT);
> > + result |= (shifted_data << shift);
> > + }
> > +
> > + return result;
> > +}
> > +
> > +#endif /* _LINUX_LITEX_H */
> > --
> > 2.23.0
--
Mateusz Holenko
Antmicro Ltd | www.antmicro.com
Roosevelta 22, 60-829 Poznan, Poland
Powered by blists - more mailing lists