| Message ID | 20240724102349.430078-1-jtornosm@redhat.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: usb: lan78xx: add weak dependency with micrel phy module | expand |
On Wed, Jul 24, 2024 at 12:23:44PM +0200, Jose Ignacio Tornos Martinez wrote: > The related module for the phy is loaded dynamically depending on the > current hardware. In order to keep this behavior and have the phy modules > available from initramfs, add a 'weak' dependency with the phy modules to > allow user tools, like dracut, get this information. > > Include micrel phy module because it is the hardware that I have. Other > possible phy modules can be added later. > > Signed-off-by: Jose Ignacio Tornos Martinez <jtornosm@redhat.com> > --- > drivers/net/usb/lan78xx.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/net/usb/lan78xx.c b/drivers/net/usb/lan78xx.c > index 8adf77e3557e..c3945aebf94e 100644 > --- a/drivers/net/usb/lan78xx.c > +++ b/drivers/net/usb/lan78xx.c > @@ -5074,3 +5074,4 @@ module_usb_driver(lan78xx_driver); > MODULE_AUTHOR(DRIVER_AUTHOR); > MODULE_DESCRIPTION(DRIVER_DESC); > MODULE_LICENSE("GPL"); > +MODULE_WEAKDEP("micrel"); ~/linux$ grep -r MODULE_WEAKDEP * ~/linux$ Is MODULE_WEAKDEP new? It seems like a "Wack a Mole" solution, which is not going to scale. Does dracut not have a set of configuration files indicating what modules should be included, using wildcards? If you want to have NFS root, you need all the network drivers, and so you need all the PHY drivers? Andrew
On Wed, Jul 24, 2024 at 01:49:14PM +0200, Andrew Lunn wrote: > On Wed, Jul 24, 2024 at 12:23:44PM +0200, Jose Ignacio Tornos Martinez wrote: > > The related module for the phy is loaded dynamically depending on the > > current hardware. In order to keep this behavior and have the phy modules > > available from initramfs, add a 'weak' dependency with the phy modules to > > allow user tools, like dracut, get this information. > > > > Include micrel phy module because it is the hardware that I have. Other > > possible phy modules can be added later. > > > > Signed-off-by: Jose Ignacio Tornos Martinez <jtornosm@redhat.com> > > --- > > drivers/net/usb/lan78xx.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/drivers/net/usb/lan78xx.c b/drivers/net/usb/lan78xx.c > > index 8adf77e3557e..c3945aebf94e 100644 > > --- a/drivers/net/usb/lan78xx.c > > +++ b/drivers/net/usb/lan78xx.c > > @@ -5074,3 +5074,4 @@ module_usb_driver(lan78xx_driver); > > MODULE_AUTHOR(DRIVER_AUTHOR); > > MODULE_DESCRIPTION(DRIVER_DESC); > > MODULE_LICENSE("GPL"); > > +MODULE_WEAKDEP("micrel"); > > ~/linux$ grep -r MODULE_WEAKDEP * > ~/linux$ > > Is MODULE_WEAKDEP new? > > It seems like a "Wack a Mole" solution, which is not going to > scale. Does dracut not have a set of configuration files indicating > what modules should be included, using wildcards? If you want to have > NFS root, you need all the network drivers, and so you need all the > PHY drivers? Agree, this isn't ok, if you have a real dependancy, then show it as a real one please with the tools that we have to show that. thanks, greg k-h
Hello Andrew, > Is MODULE_WEAKDEP new? Yes, and it has been merged into torvalds/linux.git from today: https://git.kernel.org/torvalds/c/f488790059fe7be6b2b059ddee10835b2500b603 Here the commit reference in torvalds/linux.git if you update your repo: https://github.com/torvalds/linux/commit/61842868de13aa7fd7391c626e889f4d6f1450bf I will include more references in case you want to get more information: kmod reference: https://github.com/kmod-project/kmod/commit/05828b4a6e9327a63ef94df544a042b5e9ce4fe7 kmod test-suite has also been completed: https://github.com/kmod-project/kmod/commit/d06712b51404061eef92cb275b8303814fca86ec dracut patch has also been approved: https://github.com/dracut-ng/dracut-ng/commit/8517a6be5e20f4a6d87e55fce35ee3e29e2a1150 > It seems like a "Wack a Mole" solution, which is not going to > scale. Does dracut not have a set of configuration files indicating > what modules should be included, using wildcards? If you want to have > NFS root, you need all the network drivers, and so you need all the > PHY drivers? The intention is to have a general solution not only related to the possible phy modules. That is, it is a solution for any module dependencies that are solved within the kernel but need to be known by user tools to build initramfs. We could use wildcards for some examples but it is not always easy to reference them. In addition, initramfs needs to be as small as possible so we should avoid wildcards and in this case, include the only possible phy modules (indeed not all phy's are compatible with a device). In this way, with the default behavior, initramfs would include only the drivers for the current machine and the only related phy modules. Thanks Best regards José Ignacio
Hello Greg, > Agree, this isn't ok, if you have a real dependancy, then show it as a > real one please with the tools that we have to show that. IMHO, I think it can be very useful. Apart from the comments trying to answer Andrew, let me try to explain better: I am trying to solve dependencies that are not declared in anyway, but without modifying the normal kernel behavior, for special cases in which some modules are automatically loaded when something external is needed or detected. For this cases, user tools like dracut don't have anyway to detect this and if we a use a normal soft dependency, the modules will be always loaded in advance. Yes, it is a real dependency, but for this case, some phy modules are possible and I think it doesn't make sense to load all the phy that could be possible in advance, because there is an internal mechanism to only load the necessary one (the associated phy is read using mdio bus and then the associated phy module is loaded during runtime by means of the function phy_request_driver_module). I think it is better to load only the necessary modules and have only in initramfs the necessary modules. Here you can find the complete/original justification for this: https://github.com/kmod-project/kmod/commit/05828b4a6e9327a63ef94df544a042b5e9ce4fe7 Please, take into account that this is the first usage of this feature, lan78xx can be completed (others possible phy modules can be added) and it can be considered by other modules in the same situation. Let me add in the thread to the other people that have been involved. Thanks Best regards José Ignacio
On 7/24/2024 7:46 AM, Jose Ignacio Tornos Martinez wrote: > Hello Andrew, > >> Is MODULE_WEAKDEP new? > Yes, and it has been merged into torvalds/linux.git from today: > https://git.kernel.org/torvalds/c/f488790059fe7be6b2b059ddee10835b2500b603 > Here the commit reference in torvalds/linux.git if you update your repo: > https://github.com/torvalds/linux/commit/61842868de13aa7fd7391c626e889f4d6f1450bf What is the difference with the existing MODULE_SOFTDEP() which has pre and post qualifiers and seems just as fit? > > I will include more references in case you want to get more information: > kmod reference: > https://github.com/kmod-project/kmod/commit/05828b4a6e9327a63ef94df544a042b5e9ce4fe7 > kmod test-suite has also been completed: > https://github.com/kmod-project/kmod/commit/d06712b51404061eef92cb275b8303814fca86ec > dracut patch has also been approved: > https://github.com/dracut-ng/dracut-ng/commit/8517a6be5e20f4a6d87e55fce35ee3e29e2a1150 > >> It seems like a "Wack a Mole" solution, which is not going to >> scale. Does dracut not have a set of configuration files indicating >> what modules should be included, using wildcards? If you want to have >> NFS root, you need all the network drivers, and so you need all the >> PHY drivers? > The intention is to have a general solution not only related to the > possible phy modules. That is, it is a solution for any module dependencies > that are solved within the kernel but need to be known by user tools to > build initramfs. We could use wildcards for some examples but it is not > always easy to reference them. > In addition, initramfs needs to be as small as possible so we should avoid > wildcards and in this case, include the only possible phy modules (indeed > not all phy's are compatible with a device). In this way, with the default > behavior, initramfs would include only the drivers for the current machine > and the only related phy modules. > > Thanks > > Best regards > José Ignacio > >
Hello Florian, > What is the difference with the existing MODULE_SOFTDEP() which has pre > and post qualifiers and seems just as fit? MODULE_SOFTDEP (with pre and/or post qualifiers as you say) causes the modules to be directly loaded. MODULES_WEAKDEP doesn't cause the modules to be loaded. The load will be done later, if necessary, by other mechanisms in the kernel. For example for the commented case, the associated phy is read using mdio bus and then the associated phy module is loaded during runtime by means of the function phy_request_driver_module. It is just informing to user applications, like dracut, about this in order to be able to prepare the complete and correct initramfs. Keep in mind that these applications have no way of knowing about this situation and if the phy is not included in this case, the driver will not work at initramfs stage. If we used MODULE_SOFTDEP the modules would be loaded in advance even if they were not necessary. 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. Thanks Best regards José Ignacio
> 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? 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. Andrew
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. Lucas De Marchi > > Andrew
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 module .c file itself? Cannot we have better tooling to help build an initramfs which does include everything that has been selected?
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
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
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
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
On Thu, 25 Jul 2024 00:57:05 +0200 Andrew Lunn wrote: > 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. Sorry for a dumb question from the audience but how does this work for PCI devices? We don't worry about what drivers may be needed because there's no "fallback / generic" driver? For MDIO are the ID registers too complicated to expose to user space and let it match the drivers using modinfo (avoiding the need to boot a kitchen sink kernel)?
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
> For MDIO are the ID registers too complicated to expose to user space > and let it match the drivers using modinfo (avoiding the need to boot > a kitchen sink kernel)? That is how it actually works for MDIO. Mostly. We read the ID from register 2 and 3. That gives us a 32 bit value. That gets turned into a binary string. Which is then matched against what is in modules.alias alias mdio:0000000000100010010101010010???? amd alias mdio:0011000111000011000111001011???? aquantia alias mdio:0011000111000011000111000001???? aquantia alias mdio:0011000111000011000111000100???? aquantia The ? means the value of the bit does not matter. The least significant nibble is often the revision of the PHY and the driver can driver any revision. The 'mostly' is because some PHYs need help from the driver to enable clocks etc before you can read register 2 and 3. A chicken/egg problem. So you can put the ID in device tree, and the exact same lookup is performed to load the driver. It gets a bit more complex with C45 devices, because they have multiple ID registers. But the same basic lookup is performed using them one by one until a driver is found. You can also find the C22 ID which matched the driver in /sys/bus/mdio_bus/devices/*/phy_id. Andrew
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.
> 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
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?
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
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.
> 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
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/
> 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
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.
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
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
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
Hello Andrew, I like the idea from Jakub, reading the associated phy from the hardware in some way, although reading your last comments I don't know if it could be possible for all the cases. Let me ask you about this to understand better, if present, always reading /sys/bus/mdio_bus/devices/*/phy_id, could it be enough to get the possible related phy? And after this get the related phy module? Thanks Best regards José Ignacio
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).
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
On Mon, Jul 29, 2024 at 10:37:38AM +0200, Jose Ignacio Tornos Martinez wrote: > Hello Andrew, > > I like the idea from Jakub, reading the associated phy from the hardware > in some way, although reading your last comments I don't know if it could > be possible for all the cases. > Let me ask you about this to understand better, if present, > always reading /sys/bus/mdio_bus/devices/*/phy_id, > could it be enough to get the possible related phy? > And after this get the related phy module? There should be sufficient information in /sys. eg. $ cat /sys/class/net/enp1s0/phydev/phy_id 0x001cc914 So enp1s0 probed a PHY using the ID 0x001cc914. Turn that into binary and you get 0000 0000 0001 1100 1100 1001 0001 0100. Look that up in alias. alias mdio:0000000000011100110010?????????? realtek So you need to realtek PHY driver for my enp1s0. Andrew
> 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
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
diff --git a/drivers/net/usb/lan78xx.c b/drivers/net/usb/lan78xx.c index 8adf77e3557e..c3945aebf94e 100644 --- a/drivers/net/usb/lan78xx.c +++ b/drivers/net/usb/lan78xx.c @@ -5074,3 +5074,4 @@ module_usb_driver(lan78xx_driver); MODULE_AUTHOR(DRIVER_AUTHOR); MODULE_DESCRIPTION(DRIVER_DESC); MODULE_LICENSE("GPL"); +MODULE_WEAKDEP("micrel");
The related module for the phy is loaded dynamically depending on the current hardware. In order to keep this behavior and have the phy modules available from initramfs, add a 'weak' dependency with the phy modules to allow user tools, like dracut, get this information. Include micrel phy module because it is the hardware that I have. Other possible phy modules can be added later. Signed-off-by: Jose Ignacio Tornos Martinez <jtornosm@redhat.com> --- drivers/net/usb/lan78xx.c | 1 + 1 file changed, 1 insertion(+)