linux-kernel - Re: [PATCH 1/2] mfd: syscon: add of_syscon_register

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

Message-ID: <CADrjBPry4Pqk4cKmEccPQB3qJ0uOZ+C1+f=FdZAtzfW4N9Bvcw@mail.gmail.com>
Date: Wed, 19 Jun 2024 09:17:32 +0100
From: Peter Griffin <peter.griffin@...aro.org>
To: Krzysztof Kozlowski <krzk@...nel.org>
Cc: lee@...nel.org, arnd@...db.de, alim.akhtar@...sung.com, 
	linux-arm-kernel@...ts.infradead.org, linux-samsung-soc@...r.kernel.org, 
	linux-kernel@...r.kernel.org, tudor.ambarus@...aro.org, 
	andre.draszik@...aro.org, saravanak@...gle.com, willmcvicker@...gle.com, 
	semen.protsenko@...aro.org, kernel-team@...roid.com
Subject: Re: [PATCH 1/2] mfd: syscon: add of_syscon_register_regmap() API

Hi Krzysztof,

Thanks for your review feedback.

On Wed, 19 Jun 2024 at 07:29, Krzysztof Kozlowski <krzk@...nel.org> wrote:
>
> On 14/06/2024 16:04, Peter Griffin wrote:
> > The of_syscon_register_regmap() API allows an externally created regmap
> > to be registered with syscon. This regmap can then be returned to client
> > drivers using the syscon_regmap_lookup_by_phandle() APIs.
> >
> > The API is used by platforms where mmio access to the syscon registers is
> > not possible, and a underlying soc driver like exynos-pmu provides a SoC
> > specific regmap that can issue a SMC or hypervisor call to write the
> > register.
> >
> > This approach keeps the SoC complexities out of syscon, but allows common
> > drivers such as  syscon-poweroff, syscon-reboot and friends that are used
> > by many SoCs already to be re-used.
> >
> > Signed-off-by: Peter Griffin <peter.griffin@...aro.org>
> > ---
> >  drivers/mfd/syscon.c       | 48 ++++++++++++++++++++++++++++++++++++++
> >  include/linux/mfd/syscon.h |  8 +++++++
> >  2 files changed, 56 insertions(+)
> >
> > diff --git a/drivers/mfd/syscon.c b/drivers/mfd/syscon.c
> > index 7d0e91164cba..44991da3ea23 100644
> > --- a/drivers/mfd/syscon.c
> > +++ b/drivers/mfd/syscon.c
> > @@ -192,6 +192,54 @@ static struct regmap *device_node_get_regmap(struct device_node *np,
> >       return syscon->regmap;
> >  }
> >
> > +/**
> > + * of_syscon_register_regmap() - Register regmap for specified device node
> > + * @np: Device tree node
> > + * @regmap: Pointer to regmap object
> > + *
> > + * Register an externally created regmap object with syscon for the specified
> > + * device tree node. This regmap can then be returned to client drivers using
> > + * the syscon_regmap_lookup_by_phandle() API.
> > + *
> > + * Return: 0 on success, negative error code on failure.
> > + */
> > +int of_syscon_register_regmap(struct device_node *np, struct regmap *regmap)
> > +{
> > +     struct syscon  *entry, *syscon = NULL;
> > +
> > +     if (!np || !regmap)
> > +             return -EINVAL;
> > +
> > +     /* check if syscon entry already exists */
> > +     spin_lock(&syscon_list_slock);
> > +
> > +     list_for_each_entry(entry, &syscon_list, list)
> > +             if (entry->np == np) {
> > +                     syscon = entry;
> > +                     break;
> > +             }
> > +
> > +     spin_unlock(&syscon_list_slock);
> > +
> > +     if (syscon)
> > +             return -EEXIST;
> > +
> > +     syscon = kzalloc(sizeof(*syscon), GFP_KERNEL);
> > +     if (!syscon)
> > +             return -ENOMEM;
> > +
> > +     syscon->regmap = regmap;
> > +     syscon->np = np;
> > +
> > +     /* register the regmap in syscon list */
> > +     spin_lock(&syscon_list_slock);
>
> You still have window between the check for existing syscon and adding
> to the list. This likely is not an issue now, but it might if we have
> more devices using same syscon and we enable asynchronous probing.

Good point, I will update it so that the lock is held throughout for
the check, and also adding it to the list.

Thanks,

Peter.

[..]