| 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: <eb4b9a30-0527-4fa0-b3eb-c886da31cc80@redhat.com>
Date: Thu, 17 Apr 2025 17:12:35 +0200
From: Ivan Vecera <ivecera@...hat.com>
To: Andrew Lunn <andrew@...n.ch>
Cc: netdev@...r.kernel.org, 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 v3 net-next 3/8] mfd: Add Microchip ZL3073x support
On 17. 04. 25 4:50 odp., Ivan Vecera wrote:
>
>
> On 17. 04. 25 3:13 odp., Andrew Lunn wrote:
>> On Wed, Apr 16, 2025 at 08:19:25PM +0200, Ivan Vecera wrote:
>>> On Wed, Apr 16, 2025 at 7:11 PM Andrew Lunn <andrew@...n.ch> wrote:
>>>
>>> > +++ b/include/linux/mfd/zl3073x_regs.h
>>> > @@ -0,0 +1,105 @@
>>> > +/* SPDX-License-Identifier: GPL-2.0-only */
>>> > +
>>> > +#ifndef __LINUX_MFD_ZL3073X_REGS_H
>>> > +#define __LINUX_MFD_ZL3073X_REGS_H
>>> > +
>>> > +#include <asm/byteorder.h>
>>> > +#include <linux/lockdep.h>
>>>
>>> lockdep?
>>>
>>>
>>> lockdep_assert*() is used in later introduced helpers.
>>
>> nitpicking, but you generally add headers as they are needed.
>
> +1
>
>
>>> > +#include <linux/mfd/zl3073x.h>
>>> > +#include <linux/regmap.h>
>>> > +#include <linux/types.h>
>>> > +#include <linux/unaligned.h>
>>> > +
>>> > +/* Registers are mapped at offset 0x100 */
>>> > +#define ZL_RANGE_OFF 0x100
>>> > +#define ZL_PAGE_SIZE 0x80
>>> > +#define ZL_REG_ADDR(_pg, _off) (ZL_RANGE_OFF + (_pg) *
>>> ZL_PAGE_SIZE +
>>> (_off))
>>> > +
>>> > +/**************************
>>> > + * Register Page 0, General
>>> > + **************************/
>>> > +
>>> > +/*
>>> > + * Register 'id'
>>> > + * Page: 0, Offset: 0x01, Size: 16 bits
>>> > + */
>>> > +#define ZL_REG_ID ZL_REG_ADDR(0, 0x01)
>>> > +
>>> > +static inline __maybe_unused int
>>> > +zl3073x_read_id(struct zl3073x_dev *zldev, u16 *value)
>>> > +{
>>> > + __be16 temp;
>>> > + int rc;
>>> > +
>>> > + rc = regmap_bulk_read(zldev->regmap, ZL_REG_ID, &temp,
>>> sizeof
>>> (temp));
>>> > + if (rc)
>>> > + return rc;
>>> > +
>>> > + *value = be16_to_cpu(temp);
>>> > + return rc;
>>> > +}
>>>
>>> It seems odd these are inline functions in a header file.
>>>
>>>
>>> There are going to be used by dpll_zl3073x sub-driver in series part 2.
>>
>> The subdriver needs to know the ID, firmware version, etc?
>
> No
>
>> Anyway, look around. How many other MFD, well actually, any sort of
>> driver at all, have a bunch of low level helpers as inline functions
>> in a header? You are aiming to write a plain boring driver which looks
>> like every other driver in Linux....
>
> Well, I took inline functions approach as this is safer than macro usage
> and each register have own very simple implementation with type and
> range control (in case of indexed registers).
>
> It is safer to use:
> zl3073x_read_ref_config(..., &v);
> ...
> zl3073x_read_ref_config(..., &v);
>
> than:
> zl3073x_read_reg8(..., ZL_REG_REF_CONFIG, &v);
> ...
> zl3073x_read_reg16(..., ZL_REG_REF_CONFIG, &v); /* wrong */
>
> With inline function defined for each register the mistake in the
> example cannot happen and also compiler checks that 'v' has correct type.
>
>> Think about your layering. What does the MFD need to offer to sub
>> drivers so they can work? For lower registers, maybe just
>> zl3073x_read_u8(), zl3073x_read_u16() & zl3073x_read_read_u32(). Write
>> variants as well. Plus the API needed to safely access the mailbox.
>> Export these using one of the EXPORT_SYMBOL_GPL() variants, so the sub
>> drivers can access them. The #defines for the registers numbers can be
>> placed into a shared header file.
>
> Would it be acceptable for you something like this:
>
> int zl3073x_read_reg{8,16,32,48}(..., unsigned int reg, u{8,16,32,64} *v);
> int zl3073x_write_reg{8,16,32,48}(..., unsigned int reg, u{8,16,32,64} v);
> int zl3073x_read_idx_reg{8,16,32,48}(..., unsigned int reg, unsigned int
> idx, unsigned int max_idx, unsigned int stride, u{8,16,32,64} *v);
> int zl3073x_write_idx_reg{8,16,32,48}(..., unsigned int reg, unsigned
> int idx, unsigned int max_idx, unsigned int stride, u{8,16,32,64} v);
>
> /* Simple non-indexed register */
> #define ZL_REG_ID ZL_REG_ADDR(0 /* page */, 0x01 /* offset */
> #define ZL_REG_ID_BITS 8
>
> /* Simple indexed register */
> #define ZL_REG_REF_STATUS ZL_REG_ADDR(2, 0x44)
> #define ZL_REG_REF_STATUS_BITS 16
> #define ZL_REG_REF_STATUS_ITEMS 10
> #define ZL_REG_REF_STATUS_STRIDE 2
>
> /* Read macro for non-indexed register */
> #define ZL3073X_READ_REG(_zldev, _reg, _v) \
> __PASTE(zl3073x_read_reg, _reg##_BITS)(_zldev, _reg, _v)
>
> /* For indexed one */
> #define ZL3073X_READ_IDX_REG(_zldev, _reg, _idx, _v) \
> __PASTE(zl3073x_read_idx_reg, _reg##_BITS)(_zldev, _reg, _v,
> _idx, \
> _reg##_ITEMS, \
> _reg##_STRIDE, _v)
>
> This would allow to call simply
> ZL3073X_READ_REG(zldev, ZL_REG_ID, &val);
> or
> ZL3073X_READ_IDX_REG(zldev, ZL_REG_REF_STATUS, 4);
>
> and caller does not need to know register size and range constraints.
>
> WDYT?
Or another approach could be:
struct zl_reg {
unsigned int addr;
unsigned int bits;
unsigned int items;
unsigned int stride;
};
#define ZL_DEF_IDX_REG(_name, _addr, _bits, _items, _stride) \
static const struct zl_reg _name = {
.addr = _addr,
.bits = _bits,
.items = _items,
.stride = _stride,
}
#define ZL_DEF_REG(_name, _addr, _bits) \
ZL_DEF_IDX_REG(_name, _addr, _bits, 1, 0)
static inline int zl_read_reg(..., const struct zl_reg reg, void *value)
{
int rc;
/* Check that all fields in reg are known constant at build time */
BUILD_BUG_ON(!const_true(reg.addr));
BUILD_BUG_ON(reg.items != 1);
switch (reg.bits) {
case 8:
rc = zl_read_reg8(zldev, reg.addr, value);
...
case 48:
rc = zl_read_reg48(zldev, reg.addr, value);
}
return rc;
}
static inline int zl_read_idx_reg(..., const struct zl_reg reg,
unsigned int idx, void *value)
{
unsigned int addr;
int rc;
/* Check that all fields in reg are known constant at build time */
BUILD_BUG_ON(!const_true(reg.addr));
BUILD_BUG_ON(reg.items > 1);
if (idx >= reg.items)
return -EINVAL;
addr = reg.addr + (idx * reg.stride);
switch (reg.bits) {
case 8:
rc = zl_read_reg8(zldev, addr, value);
...
case 48:
rc = zl_read_reg48(zldev, addr, value);
}
return rc;
}
// page 0, offset 0x01, bits 8
ZL_DEF_REG(id, ZL_REG_ADDR(0, 0x01, 8);
// page 2, offset 0x55, bits 16, 10 items, 2 bytes between them
ZL_DEF_IDX_REG(ref_config, ZL_REG_ADDR(2, 0x55, 16, 10, 2))
Caller:
rc = zl_read_reg(zldev, id, &val);
rc = zl_read_idx_reg(zldev, ref_config, 5, &val);
Passing constant structures ensures that all 'reg' fields are known
during compilation and zl_read_*reg() functions are rendered into tiny
assembler output.
WDYT?
Ivan
Powered by blists - more mailing lists