linux-kernel - Re: [PATCH v2 06/14] mfd: zl3073x: Add macros for device registers access

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: <3e12b213-db36-4a76-9a58-c62dc8b1b2ce@kernel.org>
Date: Thu, 10 Apr 2025 09:17:10 +0200
From: Krzysztof Kozlowski <krzk@...nel.org>
To: Ivan Vecera <ivecera@...hat.com>, netdev@...r.kernel.org
Cc: Vadim Fedorenko <vadim.fedorenko@...ux.dev>,
 Arkadiusz Kubalewski <arkadiusz.kubalewski@...el.com>,
 Jiri Pirko <jiri@...nulli.us>, Rob Herring <robh@...nel.org>,
 Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley
 <conor+dt@...nel.org>, Prathosh Satish <Prathosh.Satish@...rochip.com>,
 Lee Jones <lee@...nel.org>, Kees Cook <kees@...nel.org>,
 Andy Shevchenko <andy@...nel.org>, Andrew Morton
 <akpm@...ux-foundation.org>, Michal Schmidt <mschmidt@...hat.com>,
 devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
 linux-hardening@...r.kernel.org
Subject: Re: [PATCH v2 06/14] mfd: zl3073x: Add macros for device registers
 access

On 09/04/2025 16:42, Ivan Vecera wrote:
> Add several macros to access device registers. These macros
> defines a couple of static inline functions to ease an access
> device registers. There are two types of registers, the 1st type
> is a simple one that is defined by an address and size and the 2nd
> type is indexed register that is defined by base address, type,
> number of register instances and address stride between instances.
> 
> Examples:
> __ZL3073X_REG_DEF(reg1, 0x1234, 4, u32);
> __ZL3073X_REG_IDX_DEF(idx_reg2, 0x1234, 2, u16, 4, 0x10);

Why can't you use standard FIELD_ macros? Why inventing the wheel again?

> 
> this defines the following functions:
> int zl3073x_read_reg1(struct zl3073x_dev *dev, u32 *value);
> int zl3073x_write_reg1(struct zl3073x_dev *dev, u32 value);
> int zl3073x_read_idx_reg2(struct zl3073x_dev *dev, unsigned int idx,
>                           u32 *value);
> int zl3073x_write_idx_reg2(struct zl3073x_dev *dev, unsigned int idx,
>                            u32 value);

Do not copy code into commit msg. I asked about this last time. Explain
why do you need it, why existing API is not good.

> 
> There are also several shortcut macros to define registers with
> certain bit widths: 8, 16, 32 and 48 bits.
> 
> Signed-off-by: Ivan Vecera <ivecera@...hat.com>
> ---


...

> + *
> + * Note that these functions have to be called with the device lock
> + * taken.
> + */
> +#define __ZL3073X_REG_IDX_DEF(_name, _addr, _len, _type, _num, _stride)	\
> +typedef _type zl3073x_##_name##_t;					\
> +static inline __maybe_unused						\
> +int zl3073x_read_##_name(struct zl3073x_dev *zldev, unsigned int idx,	\
> +			 _type * value)					\
> +{									\
> +	WARN_ON(idx >= (_num));						\

No need to cause panic reboots. Either review your code so this does not
happen or properly handle.

Best regards,
Krzysztof