Re: [PATCH v2 1/2] modpost: file2alias: fixup mdio alias garbled code in modules.alias

[zhuyinbo]

在 2021/12/1 上午8:38, Andrew Lunn 写道:
> > However, this won't work for PHY devices created _before_ the kernel
> > has mounted the rootfs, whether or not they end up being used. So,
> > every PHY mentioned in DT will be created before the rootfs is mounted,
> > and none of these PHYs will have their modules loaded.
>
> Hi Russell
>
> I think what you are saying here is, if the MAC or MDIO bus driver is
> built in, the PHY driver also needs to be built in?
>
> If the MAC or MDIO bus driver is a module, it means the rootfs has
> already been mounted in order to get these modules. And so the PHY
> driver as a module will also work.
>
> > I believe this is the root cause of Yinbo Zhu's issue.

I think you should be right and I had did lots of test but use 
rquest_module it doesn't load marvell module, and dts does't include any 
phy node. even though I was use "marvell" as input's args of request_module.
> You are speculating that in Yinbo Zhu case, the MAC driver is built
> in, the PHY is a module. The initial request for the firmware fails.
> Yinbo Zhu would like udev to try again later when the modules are
> available.
>
> > What we _could_ do is review all device trees and PHY drivers to see
> > whether DT modaliases are ever used for module loading. If they aren't,
> > then we _could_ make the modalias published by the kernel conditional
> > on the type of mdio device - continue with the DT approach for non-PHY
> > devices, and switch to the mdio: scheme for PHY devices. I repeat, this
> > can only happen if no PHY drivers match using the DT scheme, otherwise
> > making this change _will_ cause a regression.

> Take a look at
> drivers/net/mdio/of_mdio.c:whitelist_phys[] and the comment above it.
>
> So there are some DT blobs out there with compatible strings for
> PHYs. I've no idea if they actually load that way, or the standard PHY
> mechanism is used.
>
> 	Andrew


> That is not true universally for all MDIO though - as
> xilinx_gmii2rgmii.c clearly shows. That is a MDIO driver which uses DT
> the compatible string to do the module load. So, we have proof there
> that Yinbo Zhu's change will definitely cause a regression which we
> can not allow.

I don't understand that what you said about regression.  My patch 
doesn't cause  xilinx_gmii2rgmii.c driver load fail, in this time that 
do_of_table and platform_uevent will be responsible "of" type driver 
auto load and my patch was responsible for "mdio" type driver auto load,
In default code. There are request_module to load phy driver, but as 
Russell King said that request_module doesn't garantee auto load will 
always work well, but udev mechanism can garantee it. and udev mechaism 
is more mainstream, otherwise mdio_uevent is useless. if use udev 
mechanism that my patch was needed. and if apply my patch it doesn't 
cause request_module mechaism work bad because I will add following change:



-       ret = request_module(MDIO_MODULE_PREFIX MDIO_ID_FMT,
-                            MDIO_ID_ARGS(phy_id));
+       ret = request_module(MDIO_MODULE_PREFIX MDIO_ID_FMT, phy_id);
        /* We only check for failures in executing the usermode binary,
         * not whether a PHY driver module exists for the PHY ID.
         * Accept -ENOENT because this may occur in case no initramfs 
exists,
diff --git a/include/linux/mod_devicetable.h 
b/include/linux/mod_devicetable.h
index 7bd23bf..bc6ea0d 100644
--- a/include/linux/mod_devicetable.h
+++ b/include/linux/mod_devicetable.h
@@ -600,16 +600,7 @@ struct platform_device_id {
 #define MDIO_NAME_SIZE         32
 #define MDIO_MODULE_PREFIX     "mdio:"

-#define MDIO_ID_FMT 
"%u%u%u%u%u%u%u%u%u%u%u%u%u%u%u%u%u%u%u%u%u%u%u%u%u%u%u%u%u%u%u%u"
-#define MDIO_ID_ARGS(_id) \
-       ((_id)>>31) & 1, ((_id)>>30) & 1, ((_id)>>29) & 1, ((_id)>>28) & 
1, \
-       ((_id)>>27) & 1, ((_id)>>26) & 1, ((_id)>>25) & 1, ((_id)>>24) & 
1, \
-       ((_id)>>23) & 1, ((_id)>>22) & 1, ((_id)>>21) & 1, ((_id)>>20) & 
1, \
-       ((_id)>>19) & 1, ((_id)>>18) & 1, ((_id)>>17) & 1, ((_id)>>16) & 
1, \
-       ((_id)>>15) & 1, ((_id)>>14) & 1, ((_id)>>13) & 1, ((_id)>>12) & 
1, \
-       ((_id)>>11) & 1, ((_id)>>10) & 1, ((_id)>>9) & 1, ((_id)>>8) & 1, \
-       ((_id)>>7) & 1, ((_id)>>6) & 1, ((_id)>>5) & 1, ((_id)>>4) & 1, \
-       ((_id)>>3) & 1, ((_id)>>2) & 1, ((_id)>>1) & 1, (_id) & 1
+#define MDIO_ID_FMT "p%08x"