linux-kernel - Re: [PATCH] clk: amlogic: axg-audio: select RESET_MESON

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]

Message-ID: <1jplmf1jqa.fsf@starbuckisacylon.baylibre.com>
Date: Thu, 28 Nov 2024 16:06:05 +0100
From: Jerome Brunet <jbrunet@...libre.com>
To: "Arnd Bergmann" <arnd@...db.de>
Cc: "Neil Armstrong" <neil.armstrong@...aro.org>,  "Michael Turquette"
 <mturquette@...libre.com>,  "Stephen Boyd" <sboyd@...nel.org>,  "Kevin
 Hilman" <khilman@...libre.com>,  "Martin Blumenstingl"
 <martin.blumenstingl@...glemail.com>,  linux-amlogic@...ts.infradead.org,
  linux-clk@...r.kernel.org,  linux-arm-kernel@...ts.infradead.org,
  linux-kernel@...r.kernel.org,  "Mark Brown" <broonie@...nel.org>
Subject: Re: [PATCH] clk: amlogic: axg-audio: select RESET_MESON_AUX

On Thu 28 Nov 2024 at 15:51, "Arnd Bergmann" <arnd@...db.de> wrote:

> On Thu, Nov 28, 2024, at 15:39, Jerome Brunet wrote:
>> On Thu 28 Nov 2024 at 15:11, "Arnd Bergmann" <arnd@...db.de> wrote:
>>
>>>> All clk, pinctrl or regulator are used by other driver yes, this one as
>>>> well, sure.
>>>>
>>>> What special about this on is that it is an auxiliary bus driver.
>>>> It is directly instantiated by another driver. That's where
>>>> it differs. The axg-audio clock driver instantiate the auxiliary reset,
>>>> it does not use it, which is why it makes sense for it to select the
>>>> driver.
>>>
>>> Can you explain the logic behind this design? It seems that the
>>> entire problem here is the split into more drivers than it
>>> should be. It's common for clk drivers to also act as a
>>> reset driver, and my impression here is that you were trying
>>> too hard to split out the reset functionality into file
>>> in drivers/reset/ rather than to have it in drivers/clk/.
>>>
>>> Could you perhaps move the contents of
>>> drivers/reset/amlogic/reset-meson-aux.c into
>>> drivers/clk/meson/axg-audio.c, and change the exported
>>> symbol to a static function? This would still require
>>> a dependency on the exported meson_reset_toggle_ops,
>>> but that feels like a more natural interface here,
>>> since it's just a library module.
>>
>> That's what we originally had. Reset implemented in clock.
>> I was specically asked to move the reset part in reset using
>> aux drivers.
>>
>> https://lore.kernel.org/r/e3a85852b911fdf16dd9ae158f42b3ef.sboyd@kernel.org
>>
>> Eventually that will happen for the rest of the reset implemented
>> under drivers/clk/meson.
>>
>> It allows to make some code common between the platform reset
>> drivers and the aux ones. It also ease maintainance for both
>> Stephen and Philipp.
>
> I don't understand how this helps: the entire point of using
> an auxiliary device is to separate the lifetime rules of
> the different bits, but by doing the creation of the device
> in the same file as the implementation, you are not taking
> advantage of that at all, but instead get the complexity of
> a link-time dependency in addition to a lot of extra code
> for dealing with the additional device.

My initial rework had the creation in clock (note: that is why I
initially used 'imply', and forgot to update when the creation moved to
reset).

I was asked to move the creation in reset:
https://lore.kernel.org/r/217a785212d7c1a5b504c6040b3636e6.sboyd@kernel.org

We are deviating a bit from the initial regression reported by Mark.
Is Ok with you to proceed with that fix and then continue this discussion
?

>
>      Arnd

-- 
Jerome