linux-kernel - Re: [RFC PATCH 8/9] clk: meson: add auxiliary reset helper driver

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: Mon, 10 Jun 2024 12:10:55 +0200
From: Jerome Brunet <jbrunet@...libre.com>
To: Stephen Boyd <sboyd@...nel.org>
Cc: Neil Armstrong <neil.armstrong@...aro.org>,  Philipp Zabel
 <p.zabel@...gutronix.de>,  Jan Dakinevich
 <jan.dakinevich@...utedevices.com>,  linux-kernel@...r.kernel.org,
  linux-amlogic@...ts.infradead.org,  linux-clk@...r.kernel.org
Subject: Re: [RFC PATCH 8/9] clk: meson: add auxiliary reset helper driver

On Wed 29 May 2024 at 18:01, Stephen Boyd <sboyd@...nel.org> wrote:

> Quoting Jerome Brunet (2024-05-16 08:08:38)
>> Add an helper module to register auxiliary reset drivers from
>> Amlogic clock controller.
>> 
>> Signed-off-by: Jerome Brunet <jbrunet@...libre.com>
>> ---
>>  drivers/clk/meson/Kconfig             |  5 ++
>>  drivers/clk/meson/Makefile            |  1 +
>>  drivers/clk/meson/meson-clk-rst-aux.c | 84 +++++++++++++++++++++++++++
>>  drivers/clk/meson/meson-clk-rst-aux.h | 14 +++++
>>  4 files changed, 104 insertions(+)
>>  create mode 100644 drivers/clk/meson/meson-clk-rst-aux.c
>>  create mode 100644 drivers/clk/meson/meson-clk-rst-aux.h
>> 
>> diff --git a/drivers/clk/meson/meson-clk-rst-aux.h b/drivers/clk/meson/meson-clk-rst-aux.h
>> new file mode 100644
>> index 000000000000..386a55a36cd9
>> --- /dev/null
>> +++ b/drivers/clk/meson/meson-clk-rst-aux.h
>> @@ -0,0 +1,14 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +/*
>> + * Copyright (c) 2024 BayLibre, SAS.
>> + * Author: Jerome Brunet <jbrunet@...libre.com>
>> + */
>> +
>> +#ifndef __MESON_CLK_RST_AUX_H
>> +#define __MESON_CLK_RST_AUX_H
>> +
>> +int devm_meson_clk_rst_aux_register(struct device *dev,
>> +                                   struct regmap *map,
>> +                                   const char *adev_name);
>
> I'd prefer we move the device creation and registration logic to
> drivers/reset as well. See commit 098c290a490d ("clock, reset:
> microchip: move all mpfs reset code to the reset subsystem") for some
> inspiration.

Ok but if it lives in reset I don't really get the purpose served by the
auxiliary devices in that case. Why not export a function that directly
calls reset_controller_register() in that case ? 

I thought the point was to properly decouple both sides.

I don't have strong opinion about it, TBH. It is just how it made sense
to me. If you are sure about this, I don't mind changing

>
> One thing I haven't really thought about too much is if they're two
> different modules. One for clk and one for reset. If the device
> registration API is a symbol the clk module depends on then maybe that
> is better because it means both modules are loaded, avoiding a
> round-trip through modprobe. It also makes sure that the drivers are
> either both builtin or both modular.

I have checked with the current implementation, if the reset driver is
missing, the clock part does not fail. Registering the aux device
succeeds in clock but the device never comes up (duh). So it does
not crash, the consumers of the aux reset device will just defer.

Said differently, the '#if IS_ENABLED(CONFIG_RESET_CONTROLLER)' in
clk-mpfs.c was not necessary ... it was removed in the changed you
linked anyway.

(Sorry Stephen, you got it twice ... missed the Reply-all the first time
around)

-- 
Jerome