Messages in this thread Patch in this message | | | Subject | Re: [PATCH v2 1/2] modpost: file2alias: fixup mdio alias garbled code in modules.alias | From | zhuyinbo <> | Date | Tue, 7 Dec 2021 17:41:51 +0800 |
| |
在 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"
| |