Re: RFC: Handle hard module dependencies that are not symbol-based (r8169 + realtek)

[Heiner Kallweit]

On 09.04.2020 02:02, Lucas De Marchi wrote:
> On Wed, Apr 01, 2020 at 11:20:20PM +0200, Heiner Kallweit wrote:
>> Currently we have no way to express a hard dependency that is not
>> a symbol-based dependency (symbol defined in module A is used in
>> module B). Use case:
>> Network driver ND uses callbacks in the dedicated PHY driver DP
>> for the integrated PHY (namely read_page() and write_page() in
>> struct phy_driver). If DP can't be loaded (e.g. because ND is in
>> initramfs but DP is not), then phylib will use the generic
>> PHY driver GP. GP doesn't implement certain callbacks that are
>> needed by ND, therefore ND's probe has to bail out with an error
>> once it detects that DP is not loaded.
>> We have this problem with driver r8169 having such a dependency
>> on PHY driver realtek. Some distributions have tools for
>> configuring initramfs that consider hard dependencies based on
>> depmod output. Means so far somebody can add r8169.ko to initramfs,
>> and neither human being nor machine will have an idea that
>> realtek.ko needs to be added too.
> 
> Could you expand on why softdep doesn't solve this problem
> with MODULE_SOFTDEP()
> 
> initramfs tools can already read it and modules can already expose them
> (they end up in /lib/modules/$(uname -r)/modules.softdep and modprobe
> makes use of them)
> 
Thanks for the feedback. I was under the impression that initramfs-tools
is affected, but you're right, it considers softdeps.
Therefore I checked the error reports again, and indeed they are about
Gentoo's "genkernel" tool only. See here:
https://bugzilla.kernel.org/show_bug.cgi?id=204343#c15

If most kernel/initramfs tools consider softdeps, then I don't see
a need for the proposed change. But well, everything is good for
something, and I learnt something about the structure of kmod.
Sorry for the noise.

> Lucas De Marchi
> 
Heiner

>>
>> Attached patch set (two patches for kmod, one for the kernel)
>> allows to express this hard dependency of ND from DP. depmod will
>> read this dependency information and treat it like a symbol-based
>> dependency. As a result tools e.g. populating initramfs can
>> consider the dependency and place DP in initramfs if ND is in
>> initramfs. On my system the patch set does the trick when
>> adding following line to r8169_main.c:
>> MODULE_HARDDEP("realtek");
>>
>> I'm interested in your opinion on the patches, and whether you
>> maybe have a better idea how to solve the problem.
>>
>> Heiner
> 
>> From 290e7dee9f6043d677f08dc06e612e13ee0d2d83 Mon Sep 17 00:00:00 2001
>> From: Heiner Kallweit <hkallweit1@xxxxxxxxx>
>> Date: Tue, 31 Mar 2020 23:02:47 +0200
>> Subject: [PATCH 1/2] depmod: add helper mod_add_dep_unique
>>
>> Create new helper mod_add_dep_unique(), next patch in this series will
>> also make use of it.
>>
>> Signed-off-by: Heiner Kallweit <hkallweit1@xxxxxxxxx>
>> ---
>> tools/depmod.c | 26 +++++++++++++++++++-------
>> 1 file changed, 19 insertions(+), 7 deletions(-)
>>
>> diff --git a/tools/depmod.c b/tools/depmod.c
>> index 875e314..5419d4d 100644
>> --- a/tools/depmod.c
>> +++ b/tools/depmod.c
>> @@ -907,23 +907,35 @@ static void mod_free(struct mod *mod)
>> ÂÂÂÂfree(mod);
>> }
>>
>> -static int mod_add_dependency(struct mod *mod, struct symbol *sym)
>> +static int mod_add_dep_unique(struct mod *mod, struct mod *dep)
>> {
>> ÂÂÂÂint err;
>>
>> -ÂÂÂ DBG("%s depends on %s %s\n", mod->path, sym->name,
>> -ÂÂÂÂÂÂÂ sym->owner != NULL ? sym->owner->path : "(unknown)");
>> -
>> -ÂÂÂ if (sym->owner == NULL)
>> +ÂÂÂ if (dep == NULL)
>> ÂÂÂÂÂÂÂ return 0;
>>
>> -ÂÂÂ err = array_append_unique(&mod->deps, sym->owner);
>> +ÂÂÂ err = array_append_unique(&mod->deps, dep);
>> ÂÂÂÂif (err == -EEXIST)
>> ÂÂÂÂÂÂÂ return 0;
>> ÂÂÂÂif (err < 0)
>> ÂÂÂÂÂÂÂ return err;
>>
>> -ÂÂÂ sym->owner->users++;
>> +ÂÂÂ dep->users++;
>> +
>> +ÂÂÂ return 1;
>> +}
>> +
>> +static int mod_add_dependency(struct mod *mod, struct symbol *sym)
>> +{
>> +ÂÂÂ int err;
>> +
>> +ÂÂÂ DBG("%s depends on %s %s\n", mod->path, sym->name,
>> +ÂÂÂÂÂÂÂ sym->owner != NULL ? sym->owner->path : "(unknown)");
>> +
>> +ÂÂÂ err = mod_add_dep_unique(mod, sym->owner);
>> +ÂÂÂ if (err <= 0)
>> +ÂÂÂÂÂÂÂ return err;
>> +
>> ÂÂÂÂSHOW("%s needs \"%s\": %s\n", mod->path, sym->name, sym->owner->path);
>> ÂÂÂÂreturn 0;
>> }
>> --Â
>> 2.26.0
>>
> 
>> From b12fa0d85b21d84cdf4509c5048c67e17914eb28 Mon Sep 17 00:00:00 2001
>> From: Heiner Kallweit <hkallweit1@xxxxxxxxx>
>> Date: Mon, 30 Mar 2020 17:12:44 +0200
>> Subject: [PATCH] module: add MODULE_HARDDEP
>>
>> Currently we have no way to express a hard dependency that is not a
>> symbol-based dependency (symbol defined in module A is used in
>> module B). Use case:
>> Network driver ND uses callbacks in the dedicated PHY driver DP
>> for the integrated PHY. If DP can't be loaded (e.g. because ND
>> is in initramfs but DP is not), then phylib will load the generic
>> PHY driver GP. GP doesn't implement certain callbacks that are
>> used by ND, therefore ND's probe has to bail out with an error
>> once it detects that DP is not loaded.
>> This patch allows to express this hard dependency of ND from DP.
>> depmod will read this dependency information and treat it like
>> a symbol-based dependency. As a result tools e.g. populating
>> initramfs can consider the dependency and place DP in initramfs
>> if ND is in initramfs.
>>
>> Signed-off-by: Heiner Kallweit <hkallweit1@xxxxxxxxx>
>> ---
>> include/linux/module.h | 5 +++++
>> 1 file changed, 5 insertions(+)
>>
>> diff --git a/include/linux/module.h b/include/linux/module.h
>> index 1ad393e62..f38d4107f 100644
>> --- a/include/linux/module.h
>> +++ b/include/linux/module.h
>> @@ -169,6 +169,11 @@ extern void cleanup_module(void);
>> Â*/
>> #define MODULE_SOFTDEP(_softdep) MODULE_INFO(softdep, _softdep)
>>
>> +/* Hard module dependencies that are not code dependencies
>> + * Example: MODULE_HARDDEP("module-foo module-bar")
>> + */
>> +#define MODULE_HARDDEP(_harddep) MODULE_INFO(harddep, _harddep)
>> +
>> /*
>> Â* MODULE_FILE is used for generating modules.builtin
>> Â* So, make it no-op when this is being built as a module
>> --Â
>> 2.26.0
>>
> 
>> From af3a25833a160e029441eaf5a93f7c8625544296 Mon Sep 17 00:00:00 2001
>> From: Heiner Kallweit <hkallweit1@xxxxxxxxx>
>> Date: Wed, 1 Apr 2020 22:42:55 +0200
>> Subject: [PATCH 2/2] depmod: add depmod_load_harddeps
>>
>> Load explicitly declared hard dependency information from modules and
>> add it to the symbol-derived dependencies. This will allow
>> depmod-based tools to consider hard dependencies that are not code
>> dependencies.
>>
>> Signed-off-by: Heiner Kallweit <hkallweit1@xxxxxxxxx>
>> ---
>> tools/depmod.c | 38 ++++++++++++++++++++++++++++++++++++++
>> 1 file changed, 38 insertions(+)
>>
>> diff --git a/tools/depmod.c b/tools/depmod.c
>> index 5419d4d..5771dc9 100644
>> --- a/tools/depmod.c
>> +++ b/tools/depmod.c
>> @@ -1522,6 +1522,41 @@ static struct symbol *depmod_symbol_find(const struct depmod *depmod,
>> ÂÂÂÂreturn hash_find(depmod->symbols, name);
>> }
>>
>> +static void depmod_load_harddeps(struct depmod *depmod, struct mod *mod)
>> +{
>> +
>> +ÂÂÂ struct kmod_list *l;
>> +
>> +ÂÂÂ kmod_list_foreach(l, mod->info_list) {
>> +ÂÂÂÂÂÂÂ const char *key = kmod_module_info_get_key(l);
>> +ÂÂÂÂÂÂÂ const char *dep_name;
>> +ÂÂÂÂÂÂÂ struct mod *dep;
>> +ÂÂÂÂÂÂÂ char *value;
>> +
>> +ÂÂÂÂÂÂÂ if (!streq(key, "harddep"))
>> +ÂÂÂÂÂÂÂÂÂÂÂ continue;
>> +
>> +ÂÂÂÂÂÂÂ value = strdup(kmod_module_info_get_value(l));
>> +ÂÂÂÂÂÂÂ if (value == NULL)
>> +ÂÂÂÂÂÂÂÂÂÂÂ return;
>> +
>> +ÂÂÂÂÂÂÂ dep_name = strtok(value, " \t");
>> +
>> +ÂÂÂÂÂÂÂ while (dep_name) {
>> +ÂÂÂÂÂÂÂÂÂÂÂ dep = hash_find(depmod->modules_by_name, dep_name);
>> +ÂÂÂÂÂÂÂÂÂÂÂ if (dep)
>> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ mod_add_dep_unique(mod, dep);
>> +ÂÂÂÂÂÂÂÂÂÂÂ else
>> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ WRN("harddep: %s: unknown dependency %s\n",
>> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ mod->modname, dep_name);
>> +
>> +ÂÂÂÂÂÂÂÂÂÂÂ dep_name = strtok(NULL, " \t");
>> +ÂÂÂÂÂÂÂ }
>> +
>> +ÂÂÂÂÂÂÂ free(value);
>> +ÂÂÂ }
>> +}
>> +
>> static int depmod_load_modules(struct depmod *depmod)
>> {
>> ÂÂÂÂstruct mod **itr, **itr_end;
>> @@ -1569,6 +1604,9 @@ static int depmod_load_module_dependencies(struct depmod *depmod, struct mod *mo
>> ÂÂÂÂstruct kmod_list *l;
>>
>> ÂÂÂÂDBG("do dependencies of %s\n", mod->path);
>> +
>> +ÂÂÂ depmod_load_harddeps(depmod, mod);
>> +
>> ÂÂÂÂkmod_list_foreach(l, mod->dep_sym_list) {
>> ÂÂÂÂÂÂÂ const char *name = kmod_module_dependency_symbol_get_symbol(l);
>> ÂÂÂÂÂÂÂ uint64_t crc = kmod_module_dependency_symbol_get_crc(l);
>> --Â
>> 2.26.0
>>
>