Re: [PATCH] net: usb: lan78xx: add weak dependency with micrel phy module

Re: [PATCH] net: usb: lan78xx: add weak dependency with micrel phy module

[Lucas De Marchi]

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

Re: [PATCH] net: usb: lan78xx: add weak dependency with micrel phy module

[Paolo Abeni]

On 7/25/24 08:50, Lucas De Marchi wrote:
> 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.

I hoped that the 'weak dependency' towards mii and microchip could be 
inferred greping for 'request_module()' in the relevant code, but 
apparently it's not the case.

The MODULE_WEAKDEP() construct usage makes sense to me, but this patch 
will need at least for MODULE_WEAKDEP() to land into net-next, and to 
grasp more consensus in the phy land.

Cheers,

Paolo

Re: [PATCH] net: usb: lan78xx: add weak dependency with micrel phy module

[Andrew Lunn]

On Thu, Jul 25, 2024 at 11:53:54AM +0200, Paolo Abeni wrote:
> On 7/25/24 08:50, Lucas De Marchi wrote:
> > 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.
> 
> I hoped that the 'weak dependency' towards mii and microchip could be
> inferred greping for 'request_module()' in the relevant code, but apparently
> it's not the case.

Nope. The module is not explicitly loaded by this driver. The PHY core
will look at ID registers in the PHY to determine what it is, and then
load a module which says it drives that ID.

There are also pin compatible PHYs, so it is possible a different
version of the LAN78xx USB dongle could need a different PHY
driver. So you might need multiple of these week dependencies.

And there are many boards using for example the FEC, with many
different PHYs in use, since Freescale was never really a PHY vendor,
its not really paired with a Freescale PHY.

As i said, whacker a mole.

So you cannot use this to determine what PHY driver goes into the
initramfs.

What this does appear to do is differentiate between 'pre' which will
load the kernel module before it is requested. Since there is no 'pre'
for this, it seems pointless whacking this mole.

What to me make more sense it to look at all the existing 'pre'
drivers and determine if they can be converted to use this macro.

	Andrew

Re: [PATCH] net: usb: lan78xx: add weak dependency with micrel phy module

[Jose Ignacio Tornos Martinez]

Hello Andrew,

> What this does appear to do is differentiate between 'pre' which will
> load the kernel module before it is requested. Since there is no 'pre'
> for this, it seems pointless whacking this mole.
Precisely, we need to fix the lan78xx case with micrel phy (and other
possible phy modules) too, due to the commented issue generating initramfs
in order to include the phy module. 

> What to me make more sense it to look at all the existing 'pre'
> drivers and determine if they can be converted to use this macro.
Of course, now that we have the possibility we can do this with other cases
that have been already detected (and fixed with a softdep pre) and others
still not detected (if anyone apart from lan78xx).

Thanks

Best regards
José Ignacio

Re: [PATCH] net: usb: lan78xx: add weak dependency with micrel phy module

[Andrew Lunn]

On Fri, Jul 26, 2024 at 02:15:26PM +0200, Jose Ignacio Tornos Martinez wrote:
> Hello Andrew,
> 
> > What this does appear to do is differentiate between 'pre' which will
> > load the kernel module before it is requested. Since there is no 'pre'
> > for this, it seems pointless whacking this mole.
> Precisely, we need to fix the lan78xx case with micrel phy (and other
> possible phy modules) too, due to the commented issue generating initramfs
> in order to include the phy module. 

I still don't see how this solves any issues with generating the
initramfs.

There are more than 200 Ethernet drivers, and around 75 PHY
drivers. If this patch is merged, you have one MAC driver indicating
it needs one PHY driver. There is nothing much you can do with that
information. You need to wait until 99% of the MAC drivers indicate
which PHY drivers are needed. Then you can use this information leave
out any PHY which is not needed, and hope you only break a small
number of devices. But even if you wait 20 years i doubt you will get
99% of the MAC drivers indicating what PHY drivers you need.  Because
nothing really uses this information today.

So as far as i see, this has nothing to do with building the
initramfs.

	Andrew

Re: [PATCH] net: usb: lan78xx: add weak dependency with micrel phy module

[Dragan Simic]

Hello,

On 2024-07-26 22:59, Andrew Lunn wrote:
> On Fri, Jul 26, 2024 at 02:15:26PM +0200, Jose Ignacio Tornos Martinez 
> wrote:
>> Hello Andrew,
>> 
>> > What this does appear to do is differentiate between 'pre' which will
>> > load the kernel module before it is requested. Since there is no 'pre'
>> > for this, it seems pointless whacking this mole.
>> 
>> Precisely, we need to fix the lan78xx case with micrel phy (and other
>> possible phy modules) too, due to the commented issue generating 
>> initramfs
>> in order to include the phy module.
> 
> I still don't see how this solves any issues with generating the
> initramfs.
> 
> There are more than 200 Ethernet drivers, and around 75 PHY
> drivers. If this patch is merged, you have one MAC driver indicating
> it needs one PHY driver. There is nothing much you can do with that
> information. You need to wait until 99% of the MAC drivers indicate
> which PHY drivers are needed. Then you can use this information leave
> out any PHY which is not needed, and hope you only break a small
> number of devices. But even if you wait 20 years i doubt you will get
> 99% of the MAC drivers indicating what PHY drivers you need.  Because
> nothing really uses this information today.
> 
> So as far as i see, this has nothing to do with building the
> initramfs.

Before going into explaining my viewpoint, could someone, please, 
clarify
which LAN78xx USB-to-Ethernet bridge does this apply to?  I already had
a look at a few LAN78xx datasheets, and I'm not sure how the external 
PHY
becomes exposed over the USB interface, so it needs a driver.

Re: [PATCH] net: usb: lan78xx: add weak dependency with micrel phy module

[Andrew Lunn]

> Before going into explaining my viewpoint, could someone, please, clarify
> which LAN78xx USB-to-Ethernet bridge does this apply to?  I already had
> a look at a few LAN78xx datasheets, and I'm not sure how the external PHY
> becomes exposed over the USB interface, so it needs a driver.

https://elixir.bootlin.com/linux/v6.10/source/drivers/net/usb/lan78xx.c#L2049

This is creating an MDIO bus device. The MDIO bus will be scanned and
PHYs on the bus found. There are then a few calls to phy_find_first()
which will get the PHY.

The code itself looks pretty broken, it is directly accessing PHY
registers, which a MAC driver should not do. That is a layering
violation.

	Andrew

Re: [PATCH] net: usb: lan78xx: add weak dependency with micrel phy module

[Masahiro Yamada]

On Fri, Jul 26, 2024 at 9:15 PM Jose Ignacio Tornos Martinez
<jtornosm@redhat.com> wrote:
>
> Hello Andrew,
>
> > What this does appear to do is differentiate between 'pre' which will
> > load the kernel module before it is requested. Since there is no 'pre'
> > for this, it seems pointless whacking this mole.
> Precisely, we need to fix the lan78xx case with micrel phy (and other
> possible phy modules) too, due to the commented issue generating initramfs
> in order to include the phy module.
>
> > What to me make more sense it to look at all the existing 'pre'
> > drivers and determine if they can be converted to use this macro.
> Of course, now that we have the possibility we can do this with other cases
> that have been already detected (and fixed with a softdep pre) and others
> still not detected (if anyone apart from lan78xx).
>
> Thanks
>
> Best regards
> José Ignacio
>



I am not familiar with MAC/PHY interface, but perhaps the
situation might be different on internal/external PHYs?

I do not know if "micrel" is an internal or an external PHY, though.


[1] internal PHY

Commit e57cf3639c323eeed05d3725fd82f91b349adca8 moved the
internal PHY code from drivers/net/usb/lan78xx.c
to drivers/net/phy/microchip.c

So, lan78xx.ko is likely to use microchip.ko

Perhaps, is the following useful?

  MODULE_WEAKDEP("microchip");    /* internal PHY */

Or, is this the case for MODULE_SOFTDEP()?



[2] external PHY

When an external PHY device is connected, the MAC/PHY combination is
pretty much board-specific. We may end up with
a bunch of MODULE_WEAKDEP().





The second question is, is it so important to enable network
at the initramfs time? Personally, I am fine with having network
drivers in the root file system.

Is this useful when the root file system is nfs or something?



-- 
Best Regards
Masahiro Yamada

Re: [PATCH] net: usb: lan78xx: add weak dependency with micrel phy module

[Dragan Simic]

Hello Masahiro,

On 2024-07-28 09:37, Masahiro Yamada wrote:
> On Fri, Jul 26, 2024 at 9:15 PM Jose Ignacio Tornos Martinez
> <jtornosm@redhat.com> wrote:
>> > What this does appear to do is differentiate between 'pre' which will
>> > load the kernel module before it is requested. Since there is no 'pre'
>> > for this, it seems pointless whacking this mole.
>> Precisely, we need to fix the lan78xx case with micrel phy (and other
>> possible phy modules) too, due to the commented issue generating 
>> initramfs
>> in order to include the phy module.
>> 
>> > What to me make more sense it to look at all the existing 'pre'
>> > drivers and determine if they can be converted to use this macro.
>> Of course, now that we have the possibility we can do this with other 
>> cases
>> that have been already detected (and fixed with a softdep pre) and 
>> others
>> still not detected (if anyone apart from lan78xx).
> 
> I am not familiar with MAC/PHY interface, but perhaps the
> situation might be different on internal/external PHYs?
> 
> I do not know if "micrel" is an internal or an external PHY, though.
> 
> [1] internal PHY
> 
> Commit e57cf3639c323eeed05d3725fd82f91b349adca8 moved the
> internal PHY code from drivers/net/usb/lan78xx.c
> to drivers/net/phy/microchip.c
> 
> So, lan78xx.ko is likely to use microchip.ko
> 
> Perhaps, is the following useful?
> 
>   MODULE_WEAKDEP("microchip");    /* internal PHY */
> 
> Or, is this the case for MODULE_SOFTDEP()?
> 
> [2] external PHY
> 
> When an external PHY device is connected, the MAC/PHY combination is
> pretty much board-specific. We may end up with
> a bunch of MODULE_WEAKDEP().
> 
> The second question is, is it so important to enable network
> at the initramfs time? Personally, I am fine with having network
> drivers in the root file system.
> 
> Is this useful when the root file system is nfs or something?

The troubles happen when the driver is probed during the initial
ramdisk stage, e.g. when a machine is rebooted with a USB adapter
plugged in.  If the required dependent PHY driver module isn't also
found in the initial ramdisk, probing the main driver may actually
fail or (hopefully not) end up in some strange state.

If you have time, I'd suggest that you go through the following
related discussions, which should provide further clarification
and additional examples of such issues with initial ramdisks and
additional driver modules:

- 
https://lore.kernel.org/linux-modules/04e0676b0e77c5eb69df6972f41d77cdf061265a.1721906745.git.dsimic@manjaro.org/
- 
https://lore.kernel.org/dri-devel/4e1e00422a14db4e2a80870afb704405da16fd1b.1718655077.git.dsimic@manjaro.org/T/#u
- 
https://lore.kernel.org/dri-devel/fdaf2e41bb6a0c5118ff9cc21f4f62583208d885.1718655070.git.dsimic@manjaro.org/T/#u

Re: [PATCH] net: usb: lan78xx: add weak dependency with micrel phy module

[Dragan Simic]

Hello Andrew,

On 2024-07-28 01:29, Andrew Lunn wrote:
>> Before going into explaining my viewpoint, could someone, please, 
>> clarify
>> which LAN78xx USB-to-Ethernet bridge does this apply to?  I already 
>> had
>> a look at a few LAN78xx datasheets, and I'm not sure how the external 
>> PHY
>> becomes exposed over the USB interface, so it needs a driver.
> 
> https://elixir.bootlin.com/linux/v6.10/source/drivers/net/usb/lan78xx.c#L2049
> 
> This is creating an MDIO bus device. The MDIO bus will be scanned and
> PHYs on the bus found. There are then a few calls to phy_find_first()
> which will get the PHY.
> 
> The code itself looks pretty broken, it is directly accessing PHY
> registers, which a MAC driver should not do. That is a layering
> violation.

Thanks for the clarification.

Basically, the way I see it, weakdeps are the right solution for the
problem at hand, i.e. for the generation of the initial ramdisk with
all the possible PHY driver modules.  However, I don't think that some
automagical generation of the associated MODULE_WEAKDEP() statements
is the way to go.  Instead, all those statements should be added by
hand to the lan78xx driver, making sure that each PHY is tested and
validated beforehand.

Re: [PATCH] net: usb: lan78xx: add weak dependency with micrel phy module

[Andrew Lunn]

> Basically, the way I see it, weakdeps are the right solution for the
> problem at hand, i.e. for the generation of the initial ramdisk with
> all the possible PHY driver modules.

Sorry, but i still don't see how this works.

Say you get this one patch merged. What then? You throw all other PHY
drivers which don't have a weakdep out of the initramfs? That
potentially breaks around 200 MAC drivers who need those PHYs you have
discarded.

	Andrew

Re: [PATCH] net: usb: lan78xx: add weak dependency with micrel phy module

[Dragan Simic]

On 2024-07-28 21:45, Andrew Lunn wrote:
>> Basically, the way I see it, weakdeps are the right solution for the
>> problem at hand, i.e. for the generation of the initial ramdisk with
>> all the possible PHY driver modules.
> 
> Sorry, but i still don't see how this works.
> 
> Say you get this one patch merged. What then? You throw all other PHY
> drivers which don't have a weakdep out of the initramfs? That
> potentially breaks around 200 MAC drivers who need those PHYs you have
> discarded.

Actually, no PHY drivers can be thrown out of an initial ramdisk by this
patch, [1] simply because no PHY drivers not needed for a specific 
system
shouldn't be included there before, unless they were added specifically
by the utilities that created the initial ramdisk, but that's beyond the
dependencies that the kernel provides in /lib/modules, and beyond what
can be obtained by automatic detection using /sys/devices on a 
particular
system.  That's all a result of this specific module dependency being
unknown before, at the kernel level.

In other words, this patch doesn't subtract anything.  Instead, it just
adds a weakdep link between the lan78xx and micrel modules, so the 
kernel
itself can report that dependency, which may actually result in one more
PHY driver added to a generated initial ramdisk.

[1] 
https://lore.kernel.org/netdev/20240724102349.430078-1-jtornosm@redhat.com/

Re: [PATCH] net: usb: lan78xx: add weak dependency with micrel phy module

[Andrew Lunn]

> In other words, this patch doesn't subtract anything.  Instead, it just
> adds a weakdep link between the lan78xx and micrel modules, so the kernel
> itself can report that dependency, which may actually result in one more
> PHY driver added to a generated initial ramdisk.

So at the moment, does the initramfs contain all PHY modules? I guess
it does, because you have no knowledge which are actually needed. And
this does not help you in any way, as you said, it does not subtract
anything.

So to me, this is pointless.

	Andrew

Re: [PATCH] net: usb: lan78xx: add weak dependency with micrel phy module

[Dragan Simic]

On 2024-07-28 22:57, Andrew Lunn wrote:
>> In other words, this patch doesn't subtract anything.  Instead, it 
>> just
>> adds a weakdep link between the lan78xx and micrel modules, so the 
>> kernel
>> itself can report that dependency, which may actually result in one 
>> more
>> PHY driver added to a generated initial ramdisk.
> 
> So at the moment, does the initramfs contain all PHY modules? I guess
> it does, because you have no knowledge which are actually needed. And
> this does not help you in any way, as you said, it does not subtract
> anything.

Basically, an initial ramdisk shouldn't contain any PHY modules that
aren't automatically detected as needed on a particular system, for
which the initial ramdisk is built.  That's how selecting modules
while building the initial ramdisks works.  On the other hand, if it's
some initial ramdisk built by a Linux distribution and intended to
support multiple systems or boards, it may contain whatever the
distribution sees fit.

Having weakdeps defined actually does help here.  For example, a Linux
distribution mentioned above no longer needs to hand-craft the rules
for initial ramdisk generation for the PHY modules that should be put
into an initial ramdisk together with the lan78xx driver, if the Linux
distribution chooses to include the lax78xx driver.  Having weakdep(s)
defined makes the kernel do that instead.  Also, there's no point in
including every single PHY driver module, because not all of them are
needed for a particular selection of MAC drivers, which comes from the
intended purpose of the initial ramdisk built by a Linux distribution,
i.e. the target architecture, supported board category, etc.

Let's also keep in mind that including all PHY modules into an initial
ramdisk inevitably makes it larger, which often isn't an option for
resource-constrained embedded systems.

Re: [PATCH] net: usb: lan78xx: add weak dependency with micrel phy module

[Greg KH]

On Mon, Jul 29, 2024 at 06:43:40AM +0200, Dragan Simic wrote:
> On 2024-07-28 22:57, Andrew Lunn wrote:
> > > In other words, this patch doesn't subtract anything.  Instead, it
> > > just
> > > adds a weakdep link between the lan78xx and micrel modules, so the
> > > kernel
> > > itself can report that dependency, which may actually result in one
> > > more
> > > PHY driver added to a generated initial ramdisk.
> > 
> > So at the moment, does the initramfs contain all PHY modules? I guess
> > it does, because you have no knowledge which are actually needed. And
> > this does not help you in any way, as you said, it does not subtract
> > anything.
> 
> Basically, an initial ramdisk shouldn't contain any PHY modules that
> aren't automatically detected as needed on a particular system, for
> which the initial ramdisk is built.  That's how selecting modules
> while building the initial ramdisks works.  On the other hand, if it's
> some initial ramdisk built by a Linux distribution and intended to
> support multiple systems or boards, it may contain whatever the
> distribution sees fit.
> 
> Having weakdeps defined actually does help here.  For example, a Linux
> distribution mentioned above no longer needs to hand-craft the rules
> for initial ramdisk generation for the PHY modules that should be put
> into an initial ramdisk together with the lan78xx driver, if the Linux
> distribution chooses to include the lax78xx driver.  Having weakdep(s)
> defined makes the kernel do that instead.  Also, there's no point in
> including every single PHY driver module, because not all of them are
> needed for a particular selection of MAC drivers, which comes from the
> intended purpose of the initial ramdisk built by a Linux distribution,
> i.e. the target architecture, supported board category, etc.
> 
> Let's also keep in mind that including all PHY modules into an initial
> ramdisk inevitably makes it larger, which often isn't an option for
> resource-constrained embedded systems.
> 

resource-constrained embedded systems know their dependancies and their
hardware configurations, so I don't see how the weak-deps help at all
here.

You are arguing two different things it seems, neither of which this
change helps out at all with, so I will provide a:

  Nacked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

here until it gets straightened out.

thanks,

greg k-h

Re: [PATCH] net: usb: lan78xx: add weak dependency with micrel phy module

[Dragan Simic]

Hello Greg, Jose and Luis,

On 2024-07-29 08:13, Greg KH wrote:
> On Mon, Jul 29, 2024 at 06:43:40AM +0200, Dragan Simic wrote:
>> On 2024-07-28 22:57, Andrew Lunn wrote:
>> > > In other words, this patch doesn't subtract anything.  Instead, it
>> > > just
>> > > adds a weakdep link between the lan78xx and micrel modules, so the
>> > > kernel
>> > > itself can report that dependency, which may actually result in one
>> > > more
>> > > PHY driver added to a generated initial ramdisk.
>> >
>> > So at the moment, does the initramfs contain all PHY modules? I guess
>> > it does, because you have no knowledge which are actually needed. And
>> > this does not help you in any way, as you said, it does not subtract
>> > anything.
>> 
>> Basically, an initial ramdisk shouldn't contain any PHY modules that
>> aren't automatically detected as needed on a particular system, for
>> which the initial ramdisk is built.  That's how selecting modules
>> while building the initial ramdisks works.  On the other hand, if it's
>> some initial ramdisk built by a Linux distribution and intended to
>> support multiple systems or boards, it may contain whatever the
>> distribution sees fit.
>> 
>> Having weakdeps defined actually does help here.  For example, a Linux
>> distribution mentioned above no longer needs to hand-craft the rules
>> for initial ramdisk generation for the PHY modules that should be put
>> into an initial ramdisk together with the lan78xx driver, if the Linux
>> distribution chooses to include the lax78xx driver.  Having weakdep(s)
>> defined makes the kernel do that instead.  Also, there's no point in
>> including every single PHY driver module, because not all of them are
>> needed for a particular selection of MAC drivers, which comes from the
>> intended purpose of the initial ramdisk built by a Linux distribution,
>> i.e. the target architecture, supported board category, etc.
>> 
>> Let's also keep in mind that including all PHY modules into an initial
>> ramdisk inevitably makes it larger, which often isn't an option for
>> resource-constrained embedded systems.
> 
> resource-constrained embedded systems know their dependancies and their
> hardware configurations, so I don't see how the weak-deps help at all
> here.
> 
> You are arguing two different things it seems, neither of which this
> change helps out at all with, so I will provide a:
> 
>   Nacked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> 
> here until it gets straightened out.

Quite frankly, all this makes me wonder why weakdeps were merged into
the mainline kernel [1] with no real consumers?  Perhaps this is good
time for Jose and Luis to chime in.

[1] 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/include/linux/module.h?id=61842868de13aa7fd7391c626e889f4d6f1450bf

Re: [PATCH] net: usb: lan78xx: add weak dependency with micrel phy module

[Jose Ignacio Tornos Martinez]

Hello Dragan,

> Quite frankly, all this makes me wonder why weakdeps were merged into
> the mainline kernel [1] with no real consumers?  Perhaps this is good
> time for Jose and Luis to chime in.
Well, I requested this commenting as an example the case of lan78xx and
the possible phy modules,  becasue it is clearly failing when initramfs
is generated due to the dynamic phy module loading process.
In my opinion this example was enough good because I found it difficult get
an automatic way to get this information in advance for all the cases and
becasue I need to fix this initramfs issue.

But with a first glance, I also found several examples (not phy related),
in which it seems the suitable softdep was added to solve the initramfs
missing module issue:
80f4e62730a9 drm/panfrost: Mark simple_ondemand governor as softdep
0c94f58cef31 drm/lima: Mark simple_ondemand governor as softdep
2ebe16155dc8 scsi: ufs: core: Add soft dependency on governor_simpleondemand
dfe085d8dcd0 crypto: xts - Add softdep on ecb
...

Therefore, I requested to provide this  kind of new dependency (weakdep)
first in general, becasue I thought it could be useful for a lot of cases
not only for the unkown (for initramfs) phy modules (i.e. lan78xx).
That is, in spite of the initial usage has been rejected, I think it can
still be considered by the other commented examples (or new ones).
I would like to confirm some example(s) to have some usage, but this will
need to be from September after my holidays.

Thanks

Best regards
José Ignacio

Re: [PATCH] net: usb: lan78xx: add weak dependency with micrel phy module

[Dragan Simic]

Hello Jose,

On 2024-07-29 10:34, Jose Ignacio Tornos Martinez wrote:
>> Quite frankly, all this makes me wonder why weakdeps were merged into
>> the mainline kernel [1] with no real consumers?  Perhaps this is good
>> time for Jose and Luis to chime in.
> 
> Well, I requested this commenting as an example the case of lan78xx and
> the possible phy modules,  becasue it is clearly failing when initramfs
> is generated due to the dynamic phy module loading process.
> In my opinion this example was enough good because I found it difficult 
> get
> an automatic way to get this information in advance for all the cases 
> and
> becasue I need to fix this initramfs issue.

I see and agree, but please note that other people highly disagree about
that being an issue at all.  Thus, I'd suggest that you provide a 
detailed
explanation of why and how that presents an issue that weakdeps solve.

> But with a first glance, I also found several examples (not phy 
> related),
> in which it seems the suitable softdep was added to solve the initramfs
> missing module issue:
> 80f4e62730a9 drm/panfrost: Mark simple_ondemand governor as softdep
> 0c94f58cef31 drm/lima: Mark simple_ondemand governor as softdep
> 2ebe16155dc8 scsi: ufs: core: Add soft dependency on 
> governor_simpleondemand
> dfe085d8dcd0 crypto: xts - Add softdep on ecb

Regarding Lima and Panfrost, I agree that weakdeps are a better solution
than softdeps, but please see also harddeps. [1]  I'd appreciate if 
you'd
provide your opinion about the proposed harddeps.

[1] 
https://lore.kernel.org/linux-modules/04e0676b0e77c5eb69df6972f41d77cdf061265a.1721906745.git.dsimic@manjaro.org/T/#u

> Therefore, I requested to provide this  kind of new dependency 
> (weakdep)
> first in general, becasue I thought it could be useful for a lot of 
> cases
> not only for the unkown (for initramfs) phy modules (i.e. lan78xx).
> That is, in spite of the initial usage has been rejected, I think it 
> can
> still be considered by the other commented examples (or new ones).
> I would like to confirm some example(s) to have some usage, but this 
> will
> need to be from September after my holidays.

Maybe we can have Lima and Panfrost as the first consumers, but I'd 
prefer
to have them use harddeps, as "earmarks" for the future (please see the
discussion linked above, and the two additional discussions linked from
the patch description in that thread).

Re: [PATCH] net: usb: lan78xx: add weak dependency with micrel phy module

[Jose Ignacio Tornos Martinez]

Hello Dragan and others,

> I see and agree, but please note that other people highly disagree about
> that being an issue at all.  Thus, I'd suggest that you provide a 
> detailed
> explanation of why and how that presents an issue that weakdeps solve.
I think that the problem that I am trying to fix related to initramfs
generation is understood. At least what I tried to explain at the beginning
of this thread with my messages and the help of Lucas. But maybe you are  
right, so let me provide a more specific explanation.

The only thing that I could repeat and/remark is that I am not modifying
anything in the current kernel behavior, and specifically for this case 
(lan78xx) with a network driver and related phy modules: I am just trying
to add a flag (and nothing else) to complete the information of the
necessary modules to be collected by the tools that build the initramfs.

And if this information about the necessary modules is not correctly
collected, the kernel is not going to work from initramfs, Especially if
the network drivers are not working (because the phy module is not found),
some initial and necessary resources could not be available before and
after initramfs stage, because unless the network driver is unloaded and
loaded again after initramfs stage (then the phy modules would be available
from rootfs), it is going to be in the same situation, that is, not
correctly initialized and not working.

Including in the initramfs all the phy modules is an option but I think it
would be better to include only the necessary stuff (this is the default
behavior for the tools that are used to build the initramfs). This is valid
for embedded and not-embedded systems.

If this patch, to only add the related flag of the network driver to inform
about the possible phy modules, is rejected because a more general solution
is preferred, I would like to dig into it, at least to know if it is possible
to do better.
Maybe Andrew in other part of the thread, after the interesting comments
from Jakub, can help and provide some new (for me) inputs.

> Regarding Lima and Panfrost, I agree that weakdeps are a better solution
> than softdeps, but please see also harddeps. [1]  I'd appreciate if 
> you'd provide your opinion about the proposed harddeps.
> [1] 
> https://lore.kernel.org/linux-modules/04e0676b0e77c5eb69df6972f41d77cdf061265a.1721906745.git.dsimic@manjaro.org/T/#u
Ok, I will think more about it.
After a quick first look I agree with Lucas, but let's go little by little
(at least I don't have a lot of time before my holidays). 

Thanks

Best regards
José Ignacio

Re: [PATCH] net: usb: lan78xx: add weak dependency with micrel phy module

[Andrew Lunn]

> Including in the initramfs all the phy modules is an option but I think it
> would be better to include only the necessary stuff (this is the default
> behavior for the tools that are used to build the initramfs). This is valid
> for embedded and not-embedded systems.

So are you saying current initramfs are broken, because they don't
include the needed PHY modules? You can fix one example of the lan78xx
USB dongle, but are going to leave everything else broken?

	Andrew

Re: [PATCH] net: usb: lan78xx: add weak dependency with micrel phy module

[Jose Ignacio Tornos Martinez]

Hello Andrew,

> So are you saying current initramfs are broken, because they don't
> include the needed PHY modules?
I am just saying that the default initramfs including the current lan78xx
driver is broken because in this case there is no information to collect
the possible phy modules. And as I commented, after the complete boot, the
only solution is to unload and load lan78xx to get the phy module from
rootfs.
 
> You can fix one example of the lan78xx
> USB dongle, but are going to leave everything else broken?
My intention was to fix the case for lan78xx because it is the one that I
have detected that does not work. Others are already working, for example
r8169, by means of a softdep with realtek phy. And my idea was to do the
same for the other detected/needed, if any (I am not aware of other similar
reported issues).
I see that you prefer to fix all the cases and always including all the phy
modules would solve the problem for lan78xx and for other possible ones.
But take into account that we should also try to avoid creating large
initramfs if not necessary, at least, if there is anyway to solve this.  
Indeed, if I am not wrong, only some phy modules are possible for
a driver and these are known.
Anyway, as it was suggested, we can explore some automatic procedure to
identify the hardware and with that, select the phy module or at least,
reduce the number of phy modules to introduce.

Thanks

Best regards
José Ignacio