linux-hardening - Re: [PATCH v3 net-next 3/8] mfd: Add Microchip ZL3073x support

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-next>] [day] [month] [year] [list]

Message-ID: <894d4209-4933-49bf-ae4c-34d6a5b1c9f1@lunn.ch>
Date: Thu, 17 Apr 2025 15:13:13 +0200
From: Andrew Lunn <andrew@...n.ch>
To: Ivan Vecera <ivecera@...hat.com>
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 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.

>     > +#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?

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....

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.

The MFD needs to read the ID, firmware version etc, so it can have
local implementations of these, but if the sub drivers don't need
them, don't make them global scope.

	Andrew