All of lore.kernel.org
 help / color / mirror / Atom feed
From: zhuyinbo <zhuyinbo@loongson.cn>
To: Andrew Lunn <andrew@lunn.ch>,
	"Russell King (Oracle)" <linux@armlinux.org.uk>
Cc: Heiner Kallweit <hkallweit1@gmail.com>,
	netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
	linux-kbuild@vger.kernel.org, zhuyinbo@loongson.cn,
	"David S. Miller" <davem@davemloft.net>,
	Jakub Kicinski <kuba@kernel.org>,
	Masahiro Yamada <masahiroy@kernel.org>,
	Michal Marek <michal.lkml@markovi.net>,
	Nick Desaulniers <ndesaulniers@google.com>,
	netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
	linux-kbuild@vger.kernel.org
Subject: Re: [PATCH v2 1/2] modpost: file2alias: fixup mdio alias garbled code in modules.alias
Date: Tue, 7 Dec 2021 17:41:51 +0800	[thread overview]
Message-ID: <f91f4fff-8bdf-663b-68f5-b8ccbd0c187a@loongson.cn> (raw)
In-Reply-To: <YabEHd+Z5SPAhAT5@lunn.ch>



在 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"




  parent reply	other threads:[~2021-12-07  9:42 UTC|newest]

Thread overview: 17+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-11-26  9:45 [PATCH v2 1/2] modpost: file2alias: fixup mdio alias garbled code in modules.alias Yinbo Zhu
2021-11-26  9:45 ` [PATCH v2 2/2] net: mdio: fixup ethernet phy module auto-load function Yinbo Zhu
2021-11-26 10:13   ` Russell King (Oracle)
2021-11-26 10:21 ` [PATCH v2 1/2] modpost: file2alias: fixup mdio alias garbled code in modules.alias Heiner Kallweit
2021-11-30  8:45   ` zhuyinbo
2021-11-30 11:46   ` Russell King (Oracle)
2021-12-01  0:38     ` Andrew Lunn
2021-12-01  9:02       ` Russell King (Oracle)
2021-12-06  2:27       ` zhuyinbo
2021-12-07  9:41       ` zhuyinbo [this message]
2021-12-07 11:18         ` Russell King (Oracle)
2022-01-04 13:11         ` zhuyinbo
2022-01-04 13:25           ` Russell King (Oracle)
2022-01-05  3:33           ` zhuyinbo
2022-01-05  9:42             ` Russell King (Oracle)
2022-01-06  3:56             ` zhuyinbo
2022-01-06 12:46               ` Andrew Lunn

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=f91f4fff-8bdf-663b-68f5-b8ccbd0c187a@loongson.cn \
    --to=zhuyinbo@loongson.cn \
    --cc=andrew@lunn.ch \
    --cc=davem@davemloft.net \
    --cc=hkallweit1@gmail.com \
    --cc=kuba@kernel.org \
    --cc=linux-kbuild@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux@armlinux.org.uk \
    --cc=masahiroy@kernel.org \
    --cc=michal.lkml@markovi.net \
    --cc=ndesaulniers@google.com \
    --cc=netdev@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.