Message-ID: <1393cce9-bd64-469d-aa2b-0fb768bf9e87@arm.com>
Date: Thu, 25 Jul 2024 16:39:40 +0100
From: Steven Price <steven.price@....com>
To: Lucas De Marchi <lucas.demarchi@...el.com>,
 Dragan Simic <dsimic@...jaro.org>
Cc: 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 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
(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.

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.

Steve

Powered by blists - more mailing lists