Message-ID: <azedppfy2e5vjycn4zpzkj4ek5mjgjmdjsxf63fm347pningpc@uuhkyeiu35yg>
Date: Thu, 25 Jul 2024 12:18:21 -0500
From: Lucas De Marchi <lucas.demarchi@...el.com>
To: Steven Price <steven.price@....com>
CC: Dragan Simic <dsimic@...jaro.org>, <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

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.

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

Lucas De Marchi

Powered by blists - more mailing lists