Re: [PATCH] module: Add hard dependencies as syntactic sugar

From: Dragan Simic
Date: Thu Jul 25 2024 - 15:32:49 EST

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@xxxxxxxxxxx/