Message-ID: <ybluy4bqgow5qurzfame6kxx2sflsh5trmnlyaifrlurasid3e@73kpadpk5d3p>
Date: Thu, 25 Jul 2024 01:50:51 -0500
From: Lucas De Marchi <lucas.demarchi@...el.com>
To: Florian Fainelli <f.fainelli@...il.com>
CC: Andrew Lunn <andrew@...n.ch>, Jose Ignacio Tornos Martinez
	<jtornosm@...hat.com>, <UNGLinuxDriver@...rochip.com>, <davem@...emloft.net>,
	<edumazet@...gle.com>, <gregkh@...uxfoundation.org>, <kuba@...nel.org>,
	<linux-kernel@...r.kernel.org>, <linux-usb@...r.kernel.org>,
	<mcgrof@...nel.org>, <netdev@...r.kernel.org>, <pabeni@...hat.com>,
	<woojung.huh@...rochip.com>, Masahiro Yamada <masahiroy@...nel.org>,
	<linux-kbuild@...r.kernel.org>
Subject: Re: [PATCH] net: usb: lan78xx: add weak dependency with micrel phy
 module

On Wed, Jul 24, 2024 at 09:42:48PM GMT, Florian Fainelli wrote:
>
>
>On 7/24/2024 9:25 PM, Lucas De Marchi wrote:
>>On Thu, Jul 25, 2024 at 12:57:05AM GMT, Andrew Lunn wrote:
>>>>For the commented case, I have included only one phy because it 
>>>>is the hardware
>>>>that I have, but other phy devices (modules) are possible and 
>>>>they can be some.
>>>
>>>So this the whole whacker a mole problem. It works for you but fails
>>>for 99% of users. How is this helping us?
>>
>>no, this is the first instance that was found/added.
>>
>>if you declare a softdep what happens is that the dep is loaded first
>>(needed or not) and your module is loaded after that
>>
>>if you declare a weakdep, you are just instructing the tools that the
>>module may or may not be needed.  Any module today that does a
>>request_module("foo") could be a candidate to migrate from
>>MODULE_SOFTDEP("pre: foo") to the new weakdep, as long as it handles
>>properly the module being loaded ondemand as opposed to using
>>request_module() to just synchronize the module being loaded.
>>
>>>
>>>Maybe a better solution is to first build an initramfs with
>>>everything, plus the kitchen sink. Boot it, and then look at what has
>>>been loaded in order to get the rootfs mounted. Then update the
>>>initramfs with just what is needed? That should be pretty generic,
>>>with throw out networking ig NFS root is not used, just load JFFS2 and
>>>a NAND driver if it was used for the rootfs, etc.
>>
>>that works for development systems or if you are fine tuning it for each
>>system you have. It doesn't work for a generic distro with the kitchen
>>sink of modules and still trying to minimize the initrd without end user
>>intervention. So it works for 99% of users.
>
>OK, but 'config USB_LAN78XX' does have a number of 'select' meaning 
>those are hard functional dependencies, and so those should be more 
>than a hint that these modules are necessary. Why should we encode 
>that information twice: once in Kconfig and another time within the 

selecting a config currently has nothing to do with how module
dependency is calculated. You can select whatever in kconfig for
whatever reason. And a selecting a kconfig is often used to select
things other than modules.

"hard" dependencies are calculated by depmod based purely on the symbols
the module uses. So if module A calls (a exported) symbol from module
B, there is a "hard" dependency. modules.dep will have a line like

kernel/drivers/gpu/drm/xe/xe.ko.zst: kernel/drivers/gpu/drm/drm_gpuvm.ko.zst kernel/drivers/gpu/drm/drm_exec.ko.zst kernel/drivers/gpu/drm/scheduler/gpu-sched.ko.zst kernel/drivers/gpu/drm/drm_buddy.ko.zst kernel/drivers/gpu/drm/drm_suballoc_helper.ko.zst kernel/drivers/gpu/drm/drm_ttm_helper.ko.zst kernel/drivers/gpu/drm/ttm/ttm.ko.zst kernel/drivers/gpu/drm/display/drm_display_helper.ko.zst kernel/drivers/media/cec/core/cec.ko.zst kernel/drivers/media/rc/rc-core.ko.zst kernel/drivers/i2c/algos/i2c-algo-bit.ko.zst kernel/drivers/acpi/video.ko.zst kernel/drivers/platform/x86/wmi.ko.zst

that means the xe module uses symbols from (some of) these modules and
these modules uses symbols from the other ones in this line. This is the
expanded dependency chain.

On the other hand, there are certain situations in which you don't use
a symbol directly from the other module, but having it around provides
additional functionality by other means (apparently the situation here
in this patch). It's common to record that dependency with
MODULE_SOFTDEP("pre: ...")

$ grep lan78xx /lib/modules/$(uname -r)/modules.dep
kernel/drivers/net/usb/lan78xx.ko.zst:

So you don't use any symbol from other modules (the above doesn't list
things that are builtin in my kernel config)...

$ grep -e 'CONFIG_USB_LAN78XX[= ]' \
	-e 'CONFIG_MII[= ]' \
	-e 'CONFIG_PHYLIB[= ]' \
	-e 'CONFIG_MICROCHIP_PHY[= ]' \
	-e 'CONFIG_FIXED_PHY[= ]' \
	-e 'CONFIG_CRC32[= ]' \
	/boot/config-$(uname -r)
CONFIG_MII=m
CONFIG_PHYLIB=y
CONFIG_FIXED_PHY=y
CONFIG_MICROCHIP_PHY=m
CONFIG_USB_LAN78XX=m
CONFIG_CRC32=y

>module .c file itself? Cannot we have better tooling to help build an 
>initramfs which does include everything that has been selected?

if you are saying that the build system should automatically convert
this:

	config USB_LAN78XX
		tristate "Microchip LAN78XX Based USB Ethernet Adapters"
		select MII
		select PHYLIB
		select MICROCHIP_PHY
		select FIXED_PHY
		select CRC32

into (for my config):

	MODULE_WEAKDEP("mii");
	MODULE_WEAKDEP("microchip");

then humn... why is CONFIG_MICREL (being added in this patch) not there?
It seems even if we automatically derive that information it wouldn't
fix the problem Jose is trying to solve.

Note that the MODULE_WEAKDEP() should be added only if that's selecting
a module and if if there isn't already a hard dependency. Then I think
there would be quite some work to do. It was not how MODULE_SOFTDEP()
was handled in the past. Cc'ing Masahiro and linux-kbuild to know if
they have an idea how feasible that would be to add in modpost.

Lucas De Marchi

>-- 
>Florian

Powered by blists - more mailing lists