linux-kernel - Re: [PATCH] drivers/soc/litex: remove 8-bit subregister option

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]

Date:   Sat, 22 May 2021 08:44:35 +0900
From:   Stafford Horne <shorne@...il.com>
To:     Gabriel Somlo <gsomlo@...il.com>
Cc:     linux-kernel@...r.kernel.org, kgugala@...micro.com,
        mholenko@...micro.com, pczarnecki@...ernships.antmicro.com,
        davidgow@...gle.com, florent@...oy-digital.fr, joel@....id.au
Subject: Re: [PATCH] drivers/soc/litex: remove 8-bit subregister option

On Fri, May 21, 2021 at 02:36:21PM -0400, Gabriel Somlo wrote:
> Since upstream LiteX recommends that Linux support be limited to
> designs configured with 32-bit CSR subregisters (see commit a2b71fde
> in upstream LiteX, https://github.com/enjoy-digital/litex), remove
> the option to select 8-bit subregisters, significantly reducing the
> complexity of LiteX CSR (MMIO register) accessor methods.
> 
> NOTE: for details on the underlying mechanics of LiteX CSR registers,
> see https://github.com/enjoy-digital/litex/wiki/CSR-Bus or the original
> LiteX accessors (litex/soc/software/include/hw/common.h in the upstream
> repository).
> 
> Signed-off-by: Gabriel Somlo <gsomlo@...il.com>
> Cc: Stafford Horne <shorne@...il.com>
> Cc: Florent Kermarrec <florent@...oy-digital.fr>
> Cc: Mateusz Holenko <mholenko@...micro.com>
> Cc: Joel Stanley <joel@....id.au>
> 
> ---
>  drivers/soc/litex/Kconfig |  12 -----
>  include/linux/litex.h     | 100 +++++++-------------------------------
>  2 files changed, 17 insertions(+), 95 deletions(-)

...

>  static inline void litex_write64(void __iomem *reg, u64 val)
>  {
> -	_litex_set_reg(reg, sizeof(u64), val);
> +	_write_litex_subregister(val >> LITEX_SUBREG_SIZE_BIT, reg);
> +	_write_litex_subregister(val, reg + LITEX_SUBREG_ALIGN);
>  }

I wonder if it would be more clear to remove the macros and just write as:

static inline void litex_write64(void __iomem *reg, u64 val)
{
	_litex_set_reg(reg, sizeof(u64), val);
	_write_litex_subregister(val >> 32, reg);
	_write_litex_subregister(val, reg + 0x4);
}

>  static inline u8 litex_read8(void __iomem *reg)
>  {
> -	return _litex_get_reg(reg, sizeof(u8));
> +	return _read_litex_subregister(reg);
>  }
>  
>  static inline u16 litex_read16(void __iomem *reg)
>  {
> -	return _litex_get_reg(reg, sizeof(u16));
> +	return _read_litex_subregister(reg);
>  }
>  
>  static inline u32 litex_read32(void __iomem *reg)
>  {
> -	return _litex_get_reg(reg, sizeof(u32));
> +	return _read_litex_subregister(reg);
>  }
>  
>  static inline u64 litex_read64(void __iomem *reg)
>  {
> -	return _litex_get_reg(reg, sizeof(u64));
> +	return ((u64)_read_litex_subregister(reg) << LITEX_SUBREG_SIZE_BIT) |
> +		_read_litex_subregister(reg + LITEX_SUBREG_ALIGN);
>  }

Same here.

This all looks good to me.  Just a bit of style preference/question for
discussion, for me it's easier to read without the macro's but it just may be
me.  The macro's make sense when they could change, but now it's just something
to double check when reading the code.

Though they are used here in the init code which we could remove too now:

        pr_info("LiteX SoC Controller driver initialized: subreg:%d, align:%d",
                LITEX_SUBREG_SIZE, LITEX_SUBREG_ALIGN);


-Stafford

>  #endif /* _LINUX_LITEX_H */
> -- 
> 2.31.1
>