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

Date: Tue, 02 Jul 2024 11:58:40 -0700
From: Stephen Boyd <sboyd@...nel.org>
To: Jerome Brunet <jbrunet@...libre.com>
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

Quoting Jerome Brunet (2024-06-10 03:10:55)
> On Wed 29 May 2024 at 18:01, Stephen Boyd <sboyd@...nel.org> wrote:
> 
> >
> > 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.

Yes.

We use auxiliary devices so that the clk and reset drivers are
decoupled. Calling reset_controller_register() directly must return
success, whereas creating a device _should_ only fail if the device
couldn't be created/registered. It's less likely to fail with an
auxiliary device. This also allows the clk driver to be a module and the
reset driver builtin if, for example, the reset framework doesn't
support modules. Another benefit would be moving the probe context to
another thread if it can be async.

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

It seems better to put the device creation in the same file as the
driver so that it's further decoupled from the clk driver and
consolidated in the reset directory. This way, a single API is the only
thing the clk driver uses to create the reset device, instead of putting
the matching string for the device and driver in the clk and reset
drivers and having to know that the auxiliary bus is used.

The downside I see is that the clk driver can't be builtin if the reset
driver is a module. I'm not sure that's really a problem though. If it
is then we can make the registration API into another C file that still
lives in drivers/reset that has another kconfig symbol.

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

Cool.