Message-ID: <202ebbd07a2484573dc7d235d847f228@manjaro.org>
Date: Thu, 25 Jul 2024 21:32:34 +0200
From: Dragan Simic <dsimic@...jaro.org>
To: Lucas De Marchi <lucas.demarchi@...el.com>
Cc: Steven Price <steven.price@....com>, linux-modules@...r.kernel.org,
 mcgrof@...nel.org, linux-kernel@...r.kernel.org, didi.debian@...ow.org,
 Boris Brezillon <boris.brezillon@...labora.com>, Qiang Yu <yuq825@...il.com>
Subject: Re: [PATCH] module: Add hard dependencies as syntactic sugar

Hello Lucas,

On 2024-07-25 19:18, Lucas De Marchi wrote:
> On Thu, Jul 25, 2024 at 04:39:40PM GMT, Steven Price wrote:
>> On 25/07/2024 15:29, Lucas De Marchi wrote:
>>> On Thu, Jul 25, 2024 at 01:37:46PM GMT, Dragan Simic wrote:
>>>> Panfrost and Lima DRM drivers use devfreq to perform DVFS, which is
>>>> supported
>>>> on the associated platforms, while using simple_ondemand devfreq
>>>> governor by
>>>> default.  This makes the simple_ondemand module a hard dependency 
>>>> for
>>>> both
>>>> Panfrost and Lima, because the presence of the simple_ondemand 
>>>> module
>>>> in an
>>>> initial ramdisk allows the initialization of Panfrost or Lima to 
>>>> succeed.
>>>> This is currently expressed using MODULE_SOFTDEP. [1][2]  Please see
>>>> commits
>>>> 80f4e62730a9 ("drm/panfrost: Mark simple_ondemand governor as
>>>> softdep") and
>>>> 0c94f58cef31 ("drm/lima: Mark simple_ondemand governor as softdep") 
>>>> for
>>>> additional background information.
>>>> 
>>>> With the addition of MODULE_WEAKDEP in commit 61842868de13 ("module:
>>>> create
>>>> weak dependecies"), the dependency between Panfrost/Lima and
>>>> simple_ondemand
>>>> can be expressed in a much better way as a weakdep, because that 
>>>> provides
>>>> the required dependency information to the utilities that generate
>>>> initial
>>>> ramdisks, but leaves the actual loading of the required kernel
>>>> module(s) to
>>>> the kernel.  However, being able to actually express this as a hard
>>>> module
>>>> dependency would still be beneficial.
>>>> 
>>>> With all this in mind, let's add MODULE_HARDDEP as some kind of 
>>>> syntactic
>>> 
>>> Sorry, but NACK from me. This only adds to the confusion.
>>> 
>>> hard/normal dependency:
>>>     It's a symbol dependency. If you want it in your module, you
>>>     have to use a symbol. Example:
>>> 
>>>     $ modinfo ksmbd | grep depends
>>>     depends:        ib_core,rdma_cm,nls_ucs2_utils,cifs_arc4
>>> 
>>> 
>>> soft dependency:
>>>     A dependency you declare in configuration or in the module
>>>     info added by the kernel. A "pre" softdep means libkmod/modprobe
>>>     will try to load that dep before the actual module. Example:
>>> 
>>>     $ modinfo ksmbd | grep softdep
>>>     softdep:        pre: crc32
>>>     softdep:        pre: gcm
>>>     softdep:        pre: ccm
>>>     softdep:        pre: aead2
>>>     softdep:        pre: sha512
>>>     softdep:        pre: sha256
>>>     softdep:        pre: cmac
>>>     softdep:        pre: aes
>>>     softdep:        pre: nls
>>>     softdep:        pre: md5
>>>     softdep:        pre: hmac
>>>     softdep:        pre: ecb
>>> 
>>> weak dependency:
>>>     A dependency you declare in configuration or in the module
>>>     info added by the kernel. libkmod/modprobe will not change the
>>>     way it loads the module and it will only used by tools that need
>>>     to make sure the module is there when the kernel does a
>>>     request_module() or somehow tries to load that module.
>>> 
>>> So if you want a hard dependency, just use a symbol from the module. 
>>> If
>>> you want to emulate a hard dependency without calling a symbol, you 
>>> use
>>> a pre softdep, not a weakdep.  You use a weakdep if the kernel 
>>> itself,
>>> somehow may load module in runtime.
>>> 
>>> The problem described in 80f4e62730a9 ("drm/panfrost: Mark
>>> simple_ondemand governor as softdep")
>>> could indeed be solved with a weakdep, so I'm not sure why you'd want 
>>> to
>>> alias it as a "hard dep".
>> 
>> The simple_ondemand dependency sadly isn't visible as a symbol. It's
>> currently 'fixed' by using a softdep, but that has drawbacks and 
>> doesn't
>> actually express the requirement. A "weakdep" works, but has the
>> drawback that it implies that the dependency is optional. This patch 
>> at
>> least means that the driver can express the dependency, even if
>> currently that just gets output as the same weakdep.
>> 
>> I'm not sure what the logic was behind the name "weak" - what we
> 
> borrowed terminology from linker and weak symbols
> 
>> (currently at least) have in panfrost is not a weak dependency by the
>> normal definition of the term - the driver will fail to load if the
>> ondemand governor is unavailable.
> 
> there are 2 options:
> 
> 1) use a softdep and let the module loading logic always load the
> simple_ondemand module before panfrost
> 2) use a weakdep and if/when needed, do a request_module()
> 
> In both cases the tools creating the initramfs should add all
> dependencies to initramfs: weakdep, softdep and dep.

I do appreciate your detailed explanations, but please note that the
way softdeps and weakdeps work is already fully understood.

>> This patch doesn't solve the confusion, but at least means panfrost
>> doesn't need another round of churn in the future. The alternative
>> presumably is don't merge this and panfrost will have to wait until a
>> proper "hard dependency" mechanism is available.
> 
> hard dependency == symbol dependency. I think the mix of terms isn't
> helping. soft doesn't necessarily means "optional". AFAICT this hard 
> dep
> thing is trying to introduce a "mandatory softdep", with a name that is
> already used to denote symbol dependency. And it currently does 
> anything
> other than turning it into a weakdep.

Please, read what I wrote in my earlier response, [1] which includes a
rather elaborate explanation of the intent behind MODULE_HARDDEP being
currently just a proposed alias for MODULE_WEAKDEP.  It also describes
why the "pinned softdeps", as I called the harddeps, are seen as 
important
in the long run, besides expressing the actual dependency better.

[1] 
https://lore.kernel.org/linux-modules/0720a516416a92a8f683053d37ee9481@manjaro.org/

Powered by blists - more mailing lists