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: <217a785212d7c1a5b504c6040b3636e6.sboyd@kernel.org>
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.

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ