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

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

[Yinbo Zhu]

After module compilation, module alias mechanism will generate a ugly
mdio modules alias configure if ethernet phy was selected, this patch
is to fixup mdio alias garbled code.

In addition, that ugly alias configure will cause ethernet phy module
doens't match udev, phy module auto-load is fail, but add this patch
that it is well mdio driver alias configure match phy device uevent.

Signed-off-by: Yinbo Zhu <zhuyinbo@loongson.cn>
---
Change in v2:
		Add a MDIO_ANY_ID for considering some special phy device 
		which phy id doesn't be read from phy register.


 include/linux/mod_devicetable.h |  2 ++
 scripts/mod/file2alias.c        | 17 +----------------
 2 files changed, 3 insertions(+), 16 deletions(-)

diff --git a/include/linux/mod_devicetable.h b/include/linux/mod_devicetable.h
index ae2e75d..7bd23bf 100644
--- a/include/linux/mod_devicetable.h
+++ b/include/linux/mod_devicetable.h
@@ -595,6 +595,8 @@ struct platform_device_id {
 	kernel_ulong_t driver_data;
 };
 
+#define MDIO_ANY_ID (~0)
+
 #define MDIO_NAME_SIZE		32
 #define MDIO_MODULE_PREFIX	"mdio:"
 
diff --git a/scripts/mod/file2alias.c b/scripts/mod/file2alias.c
index 49aba86..63f3149 100644
--- a/scripts/mod/file2alias.c
+++ b/scripts/mod/file2alias.c
@@ -1027,24 +1027,9 @@ static int do_platform_entry(const char *filename,
 static int do_mdio_entry(const char *filename,
 			 void *symval, char *alias)
 {
-	int i;
 	DEF_FIELD(symval, mdio_device_id, phy_id);
-	DEF_FIELD(symval, mdio_device_id, phy_id_mask);
-
 	alias += sprintf(alias, MDIO_MODULE_PREFIX);
-
-	for (i = 0; i < 32; i++) {
-		if (!((phy_id_mask >> (31-i)) & 1))
-			*(alias++) = '?';
-		else if ((phy_id >> (31-i)) & 1)
-			*(alias++) = '1';
-		else
-			*(alias++) = '0';
-	}
-
-	/* Terminate the string */
-	*alias = 0;
-
+	ADD(alias, "p", phy_id != MDIO_ANY_ID, phy_id);
 	return 1;
 }
 
-- 
1.8.3.1

[PATCH v2 2/2] net: mdio: fixup ethernet phy module auto-load function

[Yinbo Zhu]

the phy_id is only phy identifier, that phy module auto-load function
should according the phy_id event rather than other information, this
patch is remove other unnecessary information and add phy_id event in
mdio_uevent function and ethernet phy module auto-load function will
work well.

Signed-off-by: Yinbo Zhu <zhuyinbo@loongson.cn>
---
 drivers/net/phy/mdio_bus.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
index 6865d93..999f0d4 100644
--- a/drivers/net/phy/mdio_bus.c
+++ b/drivers/net/phy/mdio_bus.c
@@ -962,12 +962,12 @@ static int mdio_bus_match(struct device *dev, struct device_driver *drv)
 
 static int mdio_uevent(struct device *dev, struct kobj_uevent_env *env)
 {
-	int rc;
+	struct phy_device *pdev;
 
-	/* Some devices have extra OF data and an OF-style MODALIAS */
-	rc = of_device_uevent_modalias(dev, env);
-	if (rc != -ENODEV)
-		return rc;
+	pdev = to_phy_device(dev);
+
+	if (add_uevent_var(env, "MODALIAS=mdio:p%08X", pdev->phy_id))
+		return -ENOMEM;
 
 	return 0;
 }
@@ -991,7 +991,7 @@ static int mdio_uevent(struct device *dev, struct kobj_uevent_env *env)
 };
 
 struct bus_type mdio_bus_type = {
-	.name		= "mdio_bus",
+	.name		= "mdio",
 	.dev_groups	= mdio_bus_dev_groups,
 	.match		= mdio_bus_match,
 	.uevent		= mdio_uevent,
-- 
1.8.3.1

Re: [PATCH v2 2/2] net: mdio: fixup ethernet phy module auto-load function

[Russell King (Oracle)]

On Fri, Nov 26, 2021 at 05:45:57PM +0800, Yinbo Zhu wrote:
> the phy_id is only phy identifier, that phy module auto-load function
> should according the phy_id event rather than other information, this
> patch is remove other unnecessary information and add phy_id event in
> mdio_uevent function and ethernet phy module auto-load function will
> work well.
> 
> Signed-off-by: Yinbo Zhu <zhuyinbo@loongson.cn>
> ---
>  drivers/net/phy/mdio_bus.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
> index 6865d93..999f0d4 100644
> --- a/drivers/net/phy/mdio_bus.c
> +++ b/drivers/net/phy/mdio_bus.c
> @@ -962,12 +962,12 @@ static int mdio_bus_match(struct device *dev, struct device_driver *drv)
>  
>  static int mdio_uevent(struct device *dev, struct kobj_uevent_env *env)
>  {
> -	int rc;
> +	struct phy_device *pdev;
>  
> -	/* Some devices have extra OF data and an OF-style MODALIAS */
> -	rc = of_device_uevent_modalias(dev, env);
> -	if (rc != -ENODEV)
> -		return rc;
> +	pdev = to_phy_device(dev);
> +
> +	if (add_uevent_var(env, "MODALIAS=mdio:p%08X", pdev->phy_id))
> +		return -ENOMEM;
>  
>  	return 0;
>  }

No. I think we've been over the reasons already. It _will_ break
existing module loading.

If I look at the PHY IDs on my Clearfog board:

/sys/bus/mdio_bus/devices/f1072004.mdio-mii:00/phy_id:0x01410dd1
/sys/bus/mdio_bus/devices/mv88e6xxx-0:00/phy_id:0x01410eb1
/sys/bus/mdio_bus/devices/mv88e6xxx-0:01/phy_id:0x01410eb1
/sys/bus/mdio_bus/devices/mv88e6xxx-0:02/phy_id:0x01410eb1
/sys/bus/mdio_bus/devices/mv88e6xxx-0:03/phy_id:0x01410eb1
/sys/bus/mdio_bus/devices/mv88e6xxx-0:04/phy_id:0x01410eb1
/sys/bus/mdio_bus/devices/mv88e6xxx-0:0f/phy_id:0x01410ea1

and then look at the PHY IDs that the kernel uses in the drivers, and
thus will be used in the module's alias tables.

#define MARVELL_PHY_ID_88E1510          0x01410dd0
#define MARVELL_PHY_ID_88E1540          0x01410eb0
#define MARVELL_PHY_ID_88E1545          0x01410ea0

These numbers are different. This is not just one board. The last nibble
of the PHY ID is generally the PHY revision, but that is not universal.
See Atheros PHYs, where we match the entire ID except bit 4.

You can not "simplify" the "ugly" matching like this. It's the way it is
for good reason. Using the whole ID will _not_ cause a match, and your
change will cause a regression.

> @@ -991,7 +991,7 @@ static int mdio_uevent(struct device *dev, struct kobj_uevent_env *env)
>  };
>  
>  struct bus_type mdio_bus_type = {
> -	.name		= "mdio_bus",
> +	.name		= "mdio",
>  	.dev_groups	= mdio_bus_dev_groups,
>  	.match		= mdio_bus_match,
>  	.uevent		= mdio_uevent,

Definitely no, this won't be accepted, and is in any case a separate
change that is unrelated to the first hunk of the patch.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

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

[Heiner Kallweit]

On 26.11.2021 10:45, Yinbo Zhu wrote:
> After module compilation, module alias mechanism will generate a ugly
> mdio modules alias configure if ethernet phy was selected, this patch
> is to fixup mdio alias garbled code.
> 
> In addition, that ugly alias configure will cause ethernet phy module
> doens't match udev, phy module auto-load is fail, but add this patch
> that it is well mdio driver alias configure match phy device uevent.
> 
I think Andrew asked you for an example already.
For which PHY's the driver isn't auto-loaded?

In addition your commit descriptions are hard to read, especially the
one for patch 2. Could you please try to change them to proper English?
Not being a native speaker myself ..

> Signed-off-by: Yinbo Zhu <zhuyinbo@loongson.cn>
> ---
> Change in v2:
> 		Add a MDIO_ANY_ID for considering some special phy device 
> 		which phy id doesn't be read from phy register.
> 
> 
>  include/linux/mod_devicetable.h |  2 ++
>  scripts/mod/file2alias.c        | 17 +----------------
>  2 files changed, 3 insertions(+), 16 deletions(-)
> 
> diff --git a/include/linux/mod_devicetable.h b/include/linux/mod_devicetable.h
> index ae2e75d..7bd23bf 100644
> --- a/include/linux/mod_devicetable.h
> +++ b/include/linux/mod_devicetable.h
> @@ -595,6 +595,8 @@ struct platform_device_id {
>  	kernel_ulong_t driver_data;
>  };
>  
> +#define MDIO_ANY_ID (~0)
> +
>  #define MDIO_NAME_SIZE		32
>  #define MDIO_MODULE_PREFIX	"mdio:"
>  
> diff --git a/scripts/mod/file2alias.c b/scripts/mod/file2alias.c
> index 49aba86..63f3149 100644
> --- a/scripts/mod/file2alias.c
> +++ b/scripts/mod/file2alias.c
> @@ -1027,24 +1027,9 @@ static int do_platform_entry(const char *filename,
>  static int do_mdio_entry(const char *filename,
>  			 void *symval, char *alias)
>  {
> -	int i;
>  	DEF_FIELD(symval, mdio_device_id, phy_id);
> -	DEF_FIELD(symval, mdio_device_id, phy_id_mask);
> -
>  	alias += sprintf(alias, MDIO_MODULE_PREFIX);
> -
> -	for (i = 0; i < 32; i++) {
> -		if (!((phy_id_mask >> (31-i)) & 1))
> -			*(alias++) = '?';
> -		else if ((phy_id >> (31-i)) & 1)
> -			*(alias++) = '1';
> -		else
> -			*(alias++) = '0';
> -	}
> -
> -	/* Terminate the string */
> -	*alias = 0;
> -
> +	ADD(alias, "p", phy_id != MDIO_ANY_ID, phy_id);
>  	return 1;
>  }
>  
> 

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

[zhuyinbo]

在 2021/11/26 下午6:21, Heiner Kallweit 写道:
> On 26.11.2021 10:45, Yinbo Zhu wrote:
>> After module compilation, module alias mechanism will generate a ugly
>> mdio modules alias configure if ethernet phy was selected, this patch
>> is to fixup mdio alias garbled code.
>>
>> In addition, that ugly alias configure will cause ethernet phy module
>> doens't match udev, phy module auto-load is fail, but add this patch
>> that it is well mdio driver alias configure match phy device uevent.
>>
> I think Andrew asked you for an example already.
> For which PHY's the driver isn't auto-loaded?

I test that use marvell phy, another colleague use motorcomm phy,  which 
auto load function was all fail.

and I need to emphasize one thing that the mdio auto load issue is 
generally issue, not special phy issue.


>
> In addition your commit descriptions are hard to read, especially the
> one for patch 2. Could you please try to change them to proper English?
> Not being a native speaker myself ..
I had changed commit information as v3 version, please you check.
>> Signed-off-by: Yinbo Zhu <zhuyinbo@loongson.cn>
>> ---
>> Change in v2:
>> 		Add a MDIO_ANY_ID for considering some special phy device
>> 		which phy id doesn't be read from phy register.
>>
>>
>>   include/linux/mod_devicetable.h |  2 ++
>>   scripts/mod/file2alias.c        | 17 +----------------
>>   2 files changed, 3 insertions(+), 16 deletions(-)
>>
>> diff --git a/include/linux/mod_devicetable.h b/include/linux/mod_devicetable.h
>> index ae2e75d..7bd23bf 100644
>> --- a/include/linux/mod_devicetable.h
>> +++ b/include/linux/mod_devicetable.h
>> @@ -595,6 +595,8 @@ struct platform_device_id {
>>   	kernel_ulong_t driver_data;
>>   };
>>   
>> +#define MDIO_ANY_ID (~0)
>> +
>>   #define MDIO_NAME_SIZE		32
>>   #define MDIO_MODULE_PREFIX	"mdio:"
>>   
>> diff --git a/scripts/mod/file2alias.c b/scripts/mod/file2alias.c
>> index 49aba86..63f3149 100644
>> --- a/scripts/mod/file2alias.c
>> +++ b/scripts/mod/file2alias.c
>> @@ -1027,24 +1027,9 @@ static int do_platform_entry(const char *filename,
>>   static int do_mdio_entry(const char *filename,
>>   			 void *symval, char *alias)
>>   {
>> -	int i;
>>   	DEF_FIELD(symval, mdio_device_id, phy_id);
>> -	DEF_FIELD(symval, mdio_device_id, phy_id_mask);
>> -
>>   	alias += sprintf(alias, MDIO_MODULE_PREFIX);
>> -
>> -	for (i = 0; i < 32; i++) {
>> -		if (!((phy_id_mask >> (31-i)) & 1))
>> -			*(alias++) = '?';
>> -		else if ((phy_id >> (31-i)) & 1)
>> -			*(alias++) = '1';
>> -		else
>> -			*(alias++) = '0';
>> -	}
>> -
>> -	/* Terminate the string */
>> -	*alias = 0;
>> -
>> +	ADD(alias, "p", phy_id != MDIO_ANY_ID, phy_id);
>>   	return 1;
>>   }
>>   
>>

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

[Russell King (Oracle)]

On Fri, Nov 26, 2021 at 11:21:03AM +0100, Heiner Kallweit wrote:
> On 26.11.2021 10:45, Yinbo Zhu wrote:
> > After module compilation, module alias mechanism will generate a ugly
> > mdio modules alias configure if ethernet phy was selected, this patch
> > is to fixup mdio alias garbled code.
> > 
> > In addition, that ugly alias configure will cause ethernet phy module
> > doens't match udev, phy module auto-load is fail, but add this patch
> > that it is well mdio driver alias configure match phy device uevent.
> > 
> I think Andrew asked you for an example already.
> For which PHY's the driver isn't auto-loaded?
> 
> In addition your commit descriptions are hard to read, especially the
> one for patch 2. Could you please try to change them to proper English?
> Not being a native speaker myself ..

Let's clear this up. PHY module loading is quite different - it does
_not_ use MODALIAS nor does it use the usual udev approach.

The modalias strings use aliases of the form "mdio:<semi-binary-string>"
with "?" used as a wildcard for each bit not in the mask. This is an
entirely appropriate scheme to use, as it allows matching an ID with
an arbitary mask. There is nothing wrong with this format - it may be
a bit on the long side, but it is an entirely valid solution.

The kernel has never generated a MODALIAS of this form, which is fine,
because we don't use MODALIAS or the uevent/udev approach to loading
the modules.

We instead use phy_request_driver_module() at PHY device creation time
to explicitly request modprobe to load a module of the form
"mdio:<binary-id>" which we know works (I have had the marvell10g and
bcm84881 modules autoloaded as a result of inserting SFPs.)

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.

I believe this is the root cause of Yinbo Zhu's issue.

However, changing the modalias format that we use is not a solution -
it _will_ cause DSA module loading to break. We've been here with the
SPI subsystem, where a patch was merged to change the modalias format
allegedly to fix loading of one or two modules, resulting in the
spi-nor driver failing to load (as it had done for years) - and the
resulting change was reverted and the revert backported to all the
stable trees. It created quite a mess. Linus has always been very clear
that if fixing one issue causes regressions, then the fix is wrong and
needs to be reverted. This is exactly what happened in the case of SPI.

This teaches us a lesson: changes to any modalias scheme that has been
in use for years need _extremely_ careful consideration and thorough
testing as they risk causing regressions. Without that, such changes
can result in difficult decisions where no matter what decision is
made, some breakage occurs as a result of sorting out the resulting
mess from not having considered the change carefully enough. It is far
better to avoid boxing oneself into a corner.

We can see that Yinbo Zhu's changes to fix his issue will cause
regressions with DSA, so it is simply an unacceptable fix. Reposting
the same code will _never_ change that fact. So please, Yinbo Zhu, stop
reposting your change. It is provably regression-creating and as such
will never be accepted. You also seem to be resistant to feedback -
I've asked you to separate out the "mdio_bus" change but you still have
not in your version 3 posted today. Therefore, I will assume that you
won't read this email, and in future if I see those patches again, I
will reply with a one-line "NAK" and a reference to this email.

We instead need a different approach to solving this issue. What that
approach is, I'm not sure right now - the tooling is setup to only
permit one MODALIAS published by the kernel, so we can't publish both
a DT based modalias and a mdio: based modalias together. It's one or
the other.

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.

The alternative is we simply declare that udev based module auto-loading
of PHY drivers required for PHYs in DT is simply not supported, and is
something we are unable to support. For something like root-NFS or IP
autoconfiguration by the kernel, that is already the case - the PHY
driver modules _must_ be built-in to the kernel in just the same way as
the network driver modules must be.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

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

[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.

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

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

[Russell King (Oracle)]

On Wed, Dec 01, 2021 at 01:38:53AM +0100, Andrew Lunn wrote:
> > 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.

Yes, because the module loading is performed by phy_device_create() when
it calls phy_request_driver_module(), which will happen when either the
MDIO bus is scanned or the DT is parsed for the PHY nodes.

> > I believe this is the root cause of Yinbo Zhu's issue.
> 
> 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.

s/firmware/module/ and it could also be the MDIO bus driver that is
built in.

> Yinbo Zhu would like udev to try again later when the modules are
> available.

I think so - it's speculation because it seems quite difficult to find
out detailed information.

> > 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.

Well, this suggests we have no instances - if none of our modules
contain a DT table to match a PHY-driver, then we should be pretty
safe.

$ grep phy_driver drivers/net -rl | xargs grep 'MODULE_ALIAS\|MODULE_DEVICE.*of'
drivers/net/phy/xilinx_gmii2rgmii.c:MODULE_DEVICE_TABLE(of, xgmiitorgmii_of_match);
drivers/net/mdio/mdio-moxart.c:MODULE_DEVICE_TABLE(of, moxart_mdio_dt_ids);
drivers/net/dsa/mt7530.c:MODULE_DEVICE_TABLE(of, mt7530_of_match);

All three look to be false hits - none are phy drivers themselves, they
just reference "phy_driver". So, I think we can say that we have no
instances of PHY driver being matched using DT in net-next in
drivers/net. Hopefully, there aren't any PHY drivers elsewhere in the
kernel tree.

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.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

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.
> 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

Hi Andrew, Russell King,


mdio phy device use DT that it isn't appropriate, because phy device was 
depend on mac and was set by mac use mdio.

even though, you use DT to descripte phy device and phy driver must use 
"of" type export, and in mainstrem phy driver,

most of phy driver was use "mdio",  not use DT, please you note! 
perphaps you can learn about do_of_table, and the key

point is that uevent wheter match alias configure for module auto load 
issue.

for more detailed information, please review v4 patch some explain:

[v4,1/2] modpost: file2alias: make mdio alias configure match mdio uevent.

https://patchwork.kernel.org/project/netdevbpf/patch/1638609208-10339-1-git-send-email-zhuyinbo@loongson.cn/


BRs,

Yinbo Zhu.

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"

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

[Russell King (Oracle)]

On Tue, Dec 07, 2021 at 05:41:51PM +0800, zhuyinbo wrote:
> 在 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.

Please can you report the contents of /proc/sys/kernel/modprobe, and
the kernel configuration of CONFIG_MODPROBE_PATH. I wonder if your
userspace has that module loading mechanism disabled, or your kernel
has CONFIG_MODPROBE_PATH as an empty string.

If the module is not present by the time this call is made, then
even if you load the appropriate driver module later, that module
will not be used - the PHY will end up being driven by the generic
clause 22 driver.

> > 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,

xilinx_gmii2rgmii is not a platform driver. It is a mdio driver:

static struct mdio_driver xgmiitorgmii_driver = {
              ^^^^^^^^^^^

Therefore, platform_uevent() is irrelevant since this will never match
a platform device. It will only match mdio devices, and the uevent
generation for that is via mdio_uevent() which is the function you
are changing.

> 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:

Please report back what the following command produces on your
problem system:

/sbin/modprobe -vn mdio:00000001010000010000110111010001

Thanks.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

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

[zhuyinbo]

在 2021/12/7 下午5:41, 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"
> 
> 
> 

 > > > > 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.

 > Please can you report the contents of /proc/sys/kernel/modprobe, and
 > the kernel configuration of CONFIG_MODPROBE_PATH. I wonder if your
 > userspace has that module loading mechanism disabled, or your kernel
 > has CONFIG_MODPROBE_PATH as an empty string.

 > If the module is not present by the time this call is made, then
 > even if you load the appropriate driver module later, that module
 > will not be used - the PHY will end up being driven by the generic
 > clause 22 driver.

 > > > 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,

 > xilinx_gmii2rgmii is not a platform driver. It is a mdio driver:

 > static struct mdio_driver xgmiitorgmii_driver = {
               ^^^^^^^^^^^

 > Therefore, platform_uevent() is irrelevant since this will never match
 > a platform device. It will only match mdio devices, and the uevent
 > generation for that is via mdio_uevent() which is the function you
 > are changing.


static const struct of_device_id xgmiitorgmii_of_match[] = {
         { .compatible = "xlnx,gmii-to-rgmii-1.0" },
         {},
};
MODULE_DEVICE_TABLE(of, xgmiitorgmii_of_match);

static struct mdio_driver xgmiitorgmii_driver = {
         .probe  = xgmiitorgmii_probe,
         .mdiodrv.driver = {
                 .name = "xgmiitorgmii",
                 .of_match_table = xgmiitorgmii_of_match,
         },
};
 From the present point of view, no matter what the situation, my 
supplement can cover udev or request_module for auto load module.

if that phy driver isn't platform driver my patch cover it I think there 
is no doubt, if phy driver is platform driver and platform driver udev 
will cover it. My only requestion is the request_module not work well.

about xgmiitorgmii_of_match that it belongs to platform driver load, 
please you note. and about your doubt usepace whether disable module 
load that module load function is okay becuase other device driver auto 
load is okay.

 > > 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:

 > Please report back what the following command produces on your
 > problem system:

 > /sbin/modprobe -vn mdio:00000001010000010000110111010001

 > Thanks.

[root@localhost ~]# lsmod | grep marvell
[root@localhost ~]# ls 
/lib/modules/4.19.190+/kernel/drivers/net/phy/marvell.ko
/lib/modules/4.19.190+/kernel/drivers/net/phy/marvell.ko
[root@localhost ~]# /sbin/modprobe -vn mdio:00000001010000010000110111010001
insmod /lib/modules/4.19.190+/kernel/drivers/net/phy/marvell.ko
insmod /lib/modules/4.19.190+/kernel/drivers/net/phy/marvell.ko
[root@localhost ~]#
[root@localhost ~]# cat /proc/sys/kernel/modprobe
/sbin/modprobe

BRs,
Yinbo

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

[Russell King (Oracle)]

On Tue, Jan 04, 2022 at 09:11:56PM +0800, zhuyinbo wrote:
> From the present point of view, no matter what the situation, my supplement
> can cover udev or request_module for auto load module.
> 
> if that phy driver isn't platform driver my patch cover it I think there is
> no doubt, if phy driver is platform driver and platform driver udev will
> cover it. My only requestion is the request_module not work well.
> 
> about xgmiitorgmii_of_match that it belongs to platform driver load, please
> you note. and about your doubt usepace whether disable module load that
> module load function is okay becuase other device driver auto load is okay.

xgmiitorgmii is *not* a platform driver.

> > Please report back what the following command produces on your
> > problem system:
> >
> > /sbin/modprobe -vn mdio:00000001010000010000110111010001
> >
> > Thanks.
> 
> [root@localhost ~]# lsmod | grep marvell
> [root@localhost ~]# ls
> /lib/modules/4.19.190+/kernel/drivers/net/phy/marvell.ko
> /lib/modules/4.19.190+/kernel/drivers/net/phy/marvell.ko
> [root@localhost ~]# /sbin/modprobe -vn mdio:00000001010000010000110111010001
> insmod /lib/modules/4.19.190+/kernel/drivers/net/phy/marvell.ko
> insmod /lib/modules/4.19.190+/kernel/drivers/net/phy/marvell.ko
> [root@localhost ~]#
> [root@localhost ~]# cat /proc/sys/kernel/modprobe
> /sbin/modprobe

Great, so the current scheme using "mdio:<binary digits>" works
perfectly for you. What is missing is having that modalias in the
uevent file.

So, my patch on the 4th December should cause the marvell module to
be loaded at boot time. Please test that patch ASAP, which I have
already asked you to do. I'll include it again in this email so you
don't have to hunt for it.

8<===
From: "Russell King (Oracle)" <rmk+kernel@armlinux.org.uk>
Subject: [PATCH] net: phy: generate PHY mdio modalias

The modalias string provided in the uevent sysfs file does not conform
to the format used in PHY driver modules. One of the reasons is that
udev loading of PHY driver modules has not been an expected use case.

This patch changes the MODALIAS entry for only PHY devices from:
        MODALIAS=of:Nethernet-phyT(null)
to:
        MODALIAS=mdio:00000000001000100001010100010011

Other MDIO devices (such as DSA) remain as before.

However, having udev automatically load the module has the advantage
of making use of existing functionality to have the module loaded
before the device is bound to the driver, thus taking advantage of
multithreaded boot systems, potentially decreasing the boot time.

However, this patch will not solve any issues with the driver module
not being loaded prior to the network device needing to use the PHY.
This is something that is completely out of control of any patch to
change the uevent mechanism.

Reported-by: Yinbo Zhu <zhuyinbo@loongson.cn>
Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 drivers/net/phy/mdio_bus.c   |  8 ++++++++
 drivers/net/phy/phy_device.c | 14 ++++++++++++++
 include/linux/mdio.h         |  2 ++
 3 files changed, 24 insertions(+)

diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
index 4638d7375943..663bd98760fb 100644
--- a/drivers/net/phy/mdio_bus.c
+++ b/drivers/net/phy/mdio_bus.c
@@ -1010,8 +1010,16 @@ static int mdio_bus_match(struct device *dev, struct device_driver *drv)
 
 static int mdio_uevent(struct device *dev, struct kobj_uevent_env *env)
 {
+	struct mdio_device *mdio = to_mdio_device(dev);
 	int rc;
 
+	/* Use the device-specific uevent if specified */
+	if (mdio->bus_uevent) {
+		rc = mdio->bus_uevent(mdio, env);
+		if (rc != -ENODEV)
+			return rc;
+	}
+
 	/* Some devices have extra OF data and an OF-style MODALIAS */
 	rc = of_device_uevent_modalias(dev, env);
 	if (rc != -ENODEV)
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 23667658b9c6..f4c2057f0202 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -563,6 +563,19 @@ static int phy_request_driver_module(struct phy_device *dev, u32 phy_id)
 	return 0;
 }
 
+static int phy_bus_uevent(struct mdio_device *mdiodev,
+			  struct kobj_uevent_env *env)
+{
+	struct phy_device *phydev;
+
+	phydev = container_of(mdiodev, struct phy_device, mdio);
+
+	add_uevent_var(env, "MODALIAS=" MDIO_MODULE_PREFIX MDIO_ID_FMT,
+		       MDIO_ID_ARGS(phydev->phy_id));
+
+	return 0;
+}
+
 struct phy_device *phy_device_create(struct mii_bus *bus, int addr, u32 phy_id,
 				     bool is_c45,
 				     struct phy_c45_device_ids *c45_ids)
@@ -582,6 +595,7 @@ struct phy_device *phy_device_create(struct mii_bus *bus, int addr, u32 phy_id,
 	mdiodev->dev.type = &mdio_bus_phy_type;
 	mdiodev->bus = bus;
 	mdiodev->bus_match = phy_bus_match;
+	mdiodev->bus_uevent = phy_bus_uevent;
 	mdiodev->addr = addr;
 	mdiodev->flags = MDIO_DEVICE_FLAG_PHY;
 	mdiodev->device_free = phy_mdio_device_free;
diff --git a/include/linux/mdio.h b/include/linux/mdio.h
index df9c96e56907..5c6676d3de23 100644
--- a/include/linux/mdio.h
+++ b/include/linux/mdio.h
@@ -38,6 +38,8 @@ struct mdio_device {
 	char modalias[MDIO_NAME_SIZE];
 
 	int (*bus_match)(struct device *dev, struct device_driver *drv);
+	int (*bus_uevent)(struct mdio_device *mdiodev,
+			  struct kobj_uevent_env *env);
 	void (*device_free)(struct mdio_device *mdiodev);
 	void (*device_remove)(struct mdio_device *mdiodev);
 
-- 
2.30.2


-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

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

[zhuyinbo]

在 2022/1/4 下午9:11, zhuyinbo 写道:
> 
> 
> 在 2021/12/7 下午5:41, 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"
>>
>>
>>
> 
>  > > > > 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.
> 
>  > Please can you report the contents of /proc/sys/kernel/modprobe, and
>  > the kernel configuration of CONFIG_MODPROBE_PATH. I wonder if your
>  > userspace has that module loading mechanism disabled, or your kernel
>  > has CONFIG_MODPROBE_PATH as an empty string.
> 
>  > If the module is not present by the time this call is made, then
>  > even if you load the appropriate driver module later, that module
>  > will not be used - the PHY will end up being driven by the generic
>  > clause 22 driver.
> 
>  > > > 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,
> 
>  > xilinx_gmii2rgmii is not a platform driver. It is a mdio driver:
> 
>  > static struct mdio_driver xgmiitorgmii_driver = {
>                ^^^^^^^^^^^
> 
>  > Therefore, platform_uevent() is irrelevant since this will never match
>  > a platform device. It will only match mdio devices, and the uevent
>  > generation for that is via mdio_uevent() which is the function you
>  > are changing.
> 
> 
> static const struct of_device_id xgmiitorgmii_of_match[] = {
>          { .compatible = "xlnx,gmii-to-rgmii-1.0" },
>          {},
> };
> MODULE_DEVICE_TABLE(of, xgmiitorgmii_of_match);
> 
> static struct mdio_driver xgmiitorgmii_driver = {
>          .probe  = xgmiitorgmii_probe,
>          .mdiodrv.driver = {
>                  .name = "xgmiitorgmii",
>                  .of_match_table = xgmiitorgmii_of_match,
>          },
> };
>  From the present point of view, no matter what the situation, my 
> supplement can cover udev or request_module for auto load module.
> 
> if that phy driver isn't platform driver my patch cover it I think there 
> is no doubt, if phy driver is platform driver and platform driver udev 
> will cover it. My only requestion is the request_module not work well.
> 
> about xgmiitorgmii_of_match that it belongs to platform driver load, 
> please you note. and about your doubt usepace whether disable module 
> load that module load function is okay becuase other device driver auto 
> load is okay.
> 
>  > > 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:
> 
>  > Please report back what the following command produces on your
>  > problem system:
> 
>  > /sbin/modprobe -vn mdio:00000001010000010000110111010001
> 
>  > Thanks.
> 
> [root@localhost ~]# lsmod | grep marvell
> [root@localhost ~]# ls 
> /lib/modules/4.19.190+/kernel/drivers/net/phy/marvell.ko
> /lib/modules/4.19.190+/kernel/drivers/net/phy/marvell.ko
> [root@localhost ~]# /sbin/modprobe -vn 
> mdio:00000001010000010000110111010001
> insmod /lib/modules/4.19.190+/kernel/drivers/net/phy/marvell.ko
> insmod /lib/modules/4.19.190+/kernel/drivers/net/phy/marvell.ko
> [root@localhost ~]#
> [root@localhost ~]# cat /proc/sys/kernel/modprobe
> /sbin/modprobe
> 
> BRs,
> Yinbo

 > On Tue, Jan 04, 2022 at 09:11:56PM +0800, zhuyinbo wrote:
 > > From the present point of view, no matter what the situation, my 
supplement
 > > can cover udev or request_module for auto load module.
 > >
 > > if that phy driver isn't platform driver my patch cover it I think 
there is
 > > no doubt, if phy driver is platform driver and platform driver udev 
will
 > > cover it. My only requestion is the request_module not work well.
 > >
 > > about xgmiitorgmii_of_match that it belongs to platform driver 
load, please
 > > you note. and about your doubt usepace whether disable module load that
 > > module load function is okay becuase other device driver auto load 
is okay.

 > xgmiitorgmii is *not* a platform driver.

For the module loading function, you need to focus on the first args 
"of" in function MODULE_ DEVICE_TABLE, not the definition type of this 
driver.  for "of" type that must platform covert it !

 > > > Please report back what the following command produces on your
 > > > problem system:
 > > >
 > > > /sbin/modprobe -vn mdio:00000001010000010000110111010001
 > > >
 > > > Thanks.
 > >
 > > [root@localhost ~]# lsmod | grep marvell
 > > [root@localhost ~]# ls
 > > /lib/modules/4.19.190+/kernel/drivers/net/phy/marvell.ko
 > > /lib/modules/4.19.190+/kernel/drivers/net/phy/marvell.ko
 > > [root@localhost ~]# /sbin/modprobe -vn 
 >mdio:00000001010000010000110111010001
 > > insmod /lib/modules/4.19.190+/kernel/drivers/net/phy/marvell.ko
 > > insmod /lib/modules/4.19.190+/kernel/drivers/net/phy/marvell.ko
 > > [root@localhost ~]#
 > > [root@localhost ~]# cat /proc/sys/kernel/modprobe
 > > /sbin/modprobe

 > Great, so the current scheme using "mdio:<binary digits>" works
 > perfectly for you. What is missing is having that modalias in the
 > uevent file.
No, "lsmod | grep marvel" is NULL, so "mdio:<binary digits>" doesn't 
work well. and that lost information is string that match do_mdio_entry 
Hexadecimal string or binary digits and I add my patch use hexadecimal 
and change do_mdio_entry to fix issue was to consider that different 
"Revision Number" represent that phy hardware may be has some 
difference, so I think we should not blindly load the corresponding PHY 
driver. It is appropriate to match the PHY ID exactly. for 
example,following code is marvell phy for 88e1510, which include it's 
initial function for phy. and my platform hardware use 88e1512, if 
doesn't change do_mdio_entry code, use "?" to match it, which represent
88e1512 hardware to match 881510 driver. of course if 88e1510 driver can 
compatible with 88e1512 phy, it is okay, but for all kinds of ethernet 
phy hc and hcd you can ensure it always has a good compatible for that?
Like 88e1510 driver, it is compatible with 88e1512 and has no problem. 
In fact, I'm not sure if there is a problem with the 88e1512 loaded 
88e1510 driver. So I modified do_mdio_entry is used for full matching, 
and it can cover above all issue.

                 .phy_id = MARVELL_PHY_ID_88E1510,
                 .phy_id_mask = MARVELL_PHY_ID_MASK,
                 .name = "Marvell 88E1510",
                 .features = PHY_GBIT_FEATURES | SUPPORTED_FIBRE,
                 .flags = PHY_HAS_INTERRUPT,
                 .probe = &m88e1510_probe,
                 .config_init = &m88e1510_config_init,
                 .config_aneg = &m88e1510_config_aneg,
                 .read_status = &marvell_read_status,


 > So, my patch on the 4th December should cause the marvell module to
 > be loaded at boot time. Please test that patch ASAP, which I have
 > already asked you to do. I'll include it again in this email so you
 > don't have to hunt for it.

 > 8<===
 > From: "Russell King (Oracle)" <rmk+kernel@armlinux.org.uk>
 > Subject: [PATCH] net: phy: generate PHY mdio modalias

 > The modalias string provided in the uevent sysfs file does not conform
 > to the format used in PHY driver modules. One of the reasons is that
 > udev loading of PHY driver modules has not been an expected use case.

 > This patch changes the MODALIAS entry for only PHY devices from:
 >         MODALIAS=of:Nethernet-phyT(null)
 > to:
 >         MODALIAS=mdio:00000000001000100001010100010011

 > Other MDIO devices (such as DSA) remain as before.

 > However, having udev automatically load the module has the advantage
 > of making use of existing functionality to have the module loaded
 > before the device is bound to the driver, thus taking advantage of
 > multithreaded boot systems, potentially decreasing the boot time.

 > However, this patch will not solve any issues with the driver module
 > not being loaded prior to the network device needing to use the PHY.
 > This is something that is completely out of control of any patch to
 > change the uevent mechanism.

 > Reported-by: Yinbo Zhu <zhuyinbo@loongson.cn>
 > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
 > ---
 > drivers/net/phy/mdio_bus.c   |  8 ++++++++
 > drivers/net/phy/phy_device.c | 14 ++++++++++++++
 > include/linux/mdio.h         |  2 ++
 > 3 files changed, 24 insertions(+)

 > diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
 > index 4638d7375943..663bd98760fb 100644
 > --- a/drivers/net/phy/mdio_bus.c
 > +++ b/drivers/net/phy/mdio_bus.c
 > @@ -1010,8 +1010,16 @@ static int mdio_bus_match(struct device *dev, 
 > > struct device_driver *drv)

 >  static int mdio_uevent(struct device *dev, struct kobj_uevent_env *env)
  > {
 > +	struct mdio_device *mdio = to_mdio_device(dev);
 > 	int rc;
 >
 > +	/* Use the device-specific uevent if specified */
 > +	if (mdio->bus_uevent) {
 > +		rc = mdio->bus_uevent(mdio, env);
 > +		if (rc != -ENODEV)
 > +			return rc;
 > +	}
 > +
 > 	/* Some devices have extra OF data and an OF-style MODALIAS */
 >  	rc = of_device_uevent_modalias(dev, env);
 > 	if (rc != -ENODEV)
 > diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
 > index 23667658b9c6..f4c2057f0202 100644
 > --- a/drivers/net/phy/phy_device.c
 > +++ b/drivers/net/phy/phy_device.c
 > @@ -563,6 +563,19 @@ static int phy_request_driver_module(struct 
phy_device *dev, u32 phy_id)
 >  	return 0;
 >  }

 > +static int phy_bus_uevent(struct mdio_device *mdiodev,
 > +			  struct kobj_uevent_env *env)
 > +{
 > +	struct phy_device *phydev;
 > +
 > +	phydev = container_of(mdiodev, struct phy_device, mdio);
 > +
 > +	add_uevent_var(env, "MODALIAS=" MDIO_MODULE_PREFIX MDIO_ID_FMT,
 > +		       MDIO_ID_ARGS(phydev->phy_id));
 > +
 > +	return 0;
 > +}
 > +
 >  struct phy_device *phy_device_create(struct mii_bus *bus, int addr, 
u32 phy_id,
 >  				     bool is_c45,
 >  				     struct phy_c45_device_ids *c45_ids)
 > @@ -582,6 +595,7 @@ struct phy_device *phy_device_create(struct 
mii_bus *bus, int addr, u32 phy_id,
 >  	mdiodev->dev.type = &mdio_bus_phy_type;
 >  	mdiodev->bus = bus;
 > 	mdiodev->bus_match = phy_bus_match;
 > +	mdiodev->bus_uevent = phy_bus_uevent;
 > 	mdiodev->addr = addr;
 > 	mdiodev->flags = MDIO_DEVICE_FLAG_PHY;
 > 	mdiodev->device_free = phy_mdio_device_free;
 > diff --git a/include/linux/mdio.h b/include/linux/mdio.h
 > index df9c96e56907..5c6676d3de23 100644
 > --- a/include/linux/mdio.h
 > +++ b/include/linux/mdio.h
 > @@ -38,6 +38,8 @@ struct mdio_device {
 >  	char modalias[MDIO_NAME_SIZE];

 >  	int (*bus_match)(struct device *dev, struct device_driver *drv);
 > +	int (*bus_uevent)(struct mdio_device *mdiodev,
 > +			  struct kobj_uevent_env *env);
 > 	void (*device_free)(struct mdio_device *mdiodev);
 > 	void (*device_remove)(struct mdio_device *mdiodev);
your patch I have a try and it can make marvel driver auto-load. 
However, you need to evaluate the above compatibility issues !
in addition, if phy id register work bad or other case, you dont' read 
phy id from phy.  your patch will not work well. so you shoud definition 
a any_phy_id, of course, The most critical issue is the above driver 
compatibility, please you note.

in additon, I have never received your email before. I have to check 
patchwork every time, so if you have a advice that could you send a mail 
to zhuyinbo@loongson.cn .

Thanks,

BRs,
Yinbo Zhu.

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

[Russell King (Oracle)]

On Wed, Jan 05, 2022 at 11:33:52AM +0800, zhuyinbo wrote:
> 在 2022/1/4 下午9:11, zhuyinbo 写道:
> > 在 2021/12/7 下午5:41, 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"
> > > 
> > > 
> > > 
> > 
> >  > > > > 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.
> > 
> >  > Please can you report the contents of /proc/sys/kernel/modprobe, and
> >  > the kernel configuration of CONFIG_MODPROBE_PATH. I wonder if your
> >  > userspace has that module loading mechanism disabled, or your kernel
> >  > has CONFIG_MODPROBE_PATH as an empty string.
> > 
> >  > If the module is not present by the time this call is made, then
> >  > even if you load the appropriate driver module later, that module
> >  > will not be used - the PHY will end up being driven by the generic
> >  > clause 22 driver.
> > 
> >  > > > 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,
> > 
> >  > xilinx_gmii2rgmii is not a platform driver. It is a mdio driver:
> > 
> >  > static struct mdio_driver xgmiitorgmii_driver = {
> >                ^^^^^^^^^^^
> > 
> >  > Therefore, platform_uevent() is irrelevant since this will never match
> >  > a platform device. It will only match mdio devices, and the uevent
> >  > generation for that is via mdio_uevent() which is the function you
> >  > are changing.
> > 
> > 
> > static const struct of_device_id xgmiitorgmii_of_match[] = {
> >          { .compatible = "xlnx,gmii-to-rgmii-1.0" },
> >          {},
> > };
> > MODULE_DEVICE_TABLE(of, xgmiitorgmii_of_match);
> > 
> > static struct mdio_driver xgmiitorgmii_driver = {
> >          .probe  = xgmiitorgmii_probe,
> >          .mdiodrv.driver = {
> >                  .name = "xgmiitorgmii",
> >                  .of_match_table = xgmiitorgmii_of_match,
> >          },
> > };
> >  From the present point of view, no matter what the situation, my
> > supplement can cover udev or request_module for auto load module.
> > 
> > if that phy driver isn't platform driver my patch cover it I think there
> > is no doubt, if phy driver is platform driver and platform driver udev
> > will cover it. My only requestion is the request_module not work well.
> > 
> > about xgmiitorgmii_of_match that it belongs to platform driver load,
> > please you note. and about your doubt usepace whether disable module
> > load that module load function is okay becuase other device driver auto
> > load is okay.
> > 
> >  > > 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:
> > 
> >  > Please report back what the following command produces on your
> >  > problem system:
> > 
> >  > /sbin/modprobe -vn mdio:00000001010000010000110111010001
> > 
> >  > Thanks.
> > 
> > [root@localhost ~]# lsmod | grep marvell
> > [root@localhost ~]# ls
> > /lib/modules/4.19.190+/kernel/drivers/net/phy/marvell.ko
> > /lib/modules/4.19.190+/kernel/drivers/net/phy/marvell.ko
> > [root@localhost ~]# /sbin/modprobe -vn
> > mdio:00000001010000010000110111010001
> > insmod /lib/modules/4.19.190+/kernel/drivers/net/phy/marvell.ko
> > insmod /lib/modules/4.19.190+/kernel/drivers/net/phy/marvell.ko
> > [root@localhost ~]#
> > [root@localhost ~]# cat /proc/sys/kernel/modprobe
> > /sbin/modprobe
> > 
> > BRs,
> > Yinbo
> 
> > On Tue, Jan 04, 2022 at 09:11:56PM +0800, zhuyinbo wrote:
> > > From the present point of view, no matter what the situation, my
> supplement
> > > can cover udev or request_module for auto load module.
> > >
> > > if that phy driver isn't platform driver my patch cover it I think there
> is
> > > no doubt, if phy driver is platform driver and platform driver udev will
> > > cover it. My only requestion is the request_module not work well.
> > >
> > > about xgmiitorgmii_of_match that it belongs to platform driver load,
> please
> > > you note. and about your doubt usepace whether disable module load that
> > > module load function is okay becuase other device driver auto load is
> okay.
> 
> > xgmiitorgmii is *not* a platform driver.
> 
> For the module loading function, you need to focus on the first args "of" in
> function MODULE_ DEVICE_TABLE, not the definition type of this driver.  for
> "of" type that must platform covert it !

Total garbage. You have no idea.

> > > [root@localhost ~]# lsmod | grep marvell
> > > [root@localhost ~]# ls
> > > /lib/modules/4.19.190+/kernel/drivers/net/phy/marvell.ko
> > > /lib/modules/4.19.190+/kernel/drivers/net/phy/marvell.ko
> > > [root@localhost ~]# /sbin/modprobe -vn
> >mdio:00000001010000010000110111010001
> > > insmod /lib/modules/4.19.190+/kernel/drivers/net/phy/marvell.ko
> > > insmod /lib/modules/4.19.190+/kernel/drivers/net/phy/marvell.ko
> > > [root@localhost ~]#
> > > [root@localhost ~]# cat /proc/sys/kernel/modprobe
> > > /sbin/modprobe
> 
> > Great, so the current scheme using "mdio:<binary digits>" works
> > perfectly for you. What is missing is having that modalias in the
> > uevent file.
> No, "lsmod | grep marvel" is NULL, so "mdio:<binary digits>" doesn't work
> well.

[removed the rest of the irrelevant claptrap]

Wrong. Your test proved that module loading using the current scheme
_works_ _for_ _you_.

What doesn't work for you is udev based loading, which is an entirely
different issue. There is NOTHING WRONG WITH THE CURRENT SCHEME OF
USING BINARY DIGITS. THERE IS NO NEED TO CHANGE THIS.

So you can stop posting your stupid patches.

> > So, my patch on the 4th December should cause the marvell module to
> > be loaded at boot time. Please test that patch ASAP, which I have
> > already asked you to do. I'll include it again in this email so you
> > don't have to hunt for it.
> 
> > 8<===
> > From: "Russell King (Oracle)" <rmk+kernel@armlinux.org.uk>
> > Subject: [PATCH] net: phy: generate PHY mdio modalias
> 
> > The modalias string provided in the uevent sysfs file does not conform
> > to the format used in PHY driver modules. One of the reasons is that
> > udev loading of PHY driver modules has not been an expected use case.
> 
> > This patch changes the MODALIAS entry for only PHY devices from:
> >         MODALIAS=of:Nethernet-phyT(null)
> > to:
> >         MODALIAS=mdio:00000000001000100001010100010011
> 
> > Other MDIO devices (such as DSA) remain as before.
> 
> > However, having udev automatically load the module has the advantage
> > of making use of existing functionality to have the module loaded
> > before the device is bound to the driver, thus taking advantage of
> > multithreaded boot systems, potentially decreasing the boot time.
> 
> > However, this patch will not solve any issues with the driver module
> > not being loaded prior to the network device needing to use the PHY.
> > This is something that is completely out of control of any patch to
> > change the uevent mechanism.
> 
> > Reported-by: Yinbo Zhu <zhuyinbo@loongson.cn>
> > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> > ---
> > drivers/net/phy/mdio_bus.c   |  8 ++++++++
> > drivers/net/phy/phy_device.c | 14 ++++++++++++++
> > include/linux/mdio.h         |  2 ++
> > 3 files changed, 24 insertions(+)
> 
> > diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
> > index 4638d7375943..663bd98760fb 100644
> > --- a/drivers/net/phy/mdio_bus.c
> > +++ b/drivers/net/phy/mdio_bus.c
> > @@ -1010,8 +1010,16 @@ static int mdio_bus_match(struct device *dev, > >
> struct device_driver *drv)
> 
> >  static int mdio_uevent(struct device *dev, struct kobj_uevent_env *env)
>  > {
> > +	struct mdio_device *mdio = to_mdio_device(dev);
> > 	int rc;
> >
> > +	/* Use the device-specific uevent if specified */
> > +	if (mdio->bus_uevent) {
> > +		rc = mdio->bus_uevent(mdio, env);
> > +		if (rc != -ENODEV)
> > +			return rc;
> > +	}
> > +
> > 	/* Some devices have extra OF data and an OF-style MODALIAS */
> >  	rc = of_device_uevent_modalias(dev, env);
> > 	if (rc != -ENODEV)
> > diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> > index 23667658b9c6..f4c2057f0202 100644
> > --- a/drivers/net/phy/phy_device.c
> > +++ b/drivers/net/phy/phy_device.c
> > @@ -563,6 +563,19 @@ static int phy_request_driver_module(struct
> phy_device *dev, u32 phy_id)
> >  	return 0;
> >  }
> 
> > +static int phy_bus_uevent(struct mdio_device *mdiodev,
> > +			  struct kobj_uevent_env *env)
> > +{
> > +	struct phy_device *phydev;
> > +
> > +	phydev = container_of(mdiodev, struct phy_device, mdio);
> > +
> > +	add_uevent_var(env, "MODALIAS=" MDIO_MODULE_PREFIX MDIO_ID_FMT,
> > +		       MDIO_ID_ARGS(phydev->phy_id));
> > +
> > +	return 0;
> > +}
> > +
> >  struct phy_device *phy_device_create(struct mii_bus *bus, int addr, u32
> phy_id,
> >  				     bool is_c45,
> >  				     struct phy_c45_device_ids *c45_ids)
> > @@ -582,6 +595,7 @@ struct phy_device *phy_device_create(struct mii_bus
> *bus, int addr, u32 phy_id,
> >  	mdiodev->dev.type = &mdio_bus_phy_type;
> >  	mdiodev->bus = bus;
> > 	mdiodev->bus_match = phy_bus_match;
> > +	mdiodev->bus_uevent = phy_bus_uevent;
> > 	mdiodev->addr = addr;
> > 	mdiodev->flags = MDIO_DEVICE_FLAG_PHY;
> > 	mdiodev->device_free = phy_mdio_device_free;
> > diff --git a/include/linux/mdio.h b/include/linux/mdio.h
> > index df9c96e56907..5c6676d3de23 100644
> > --- a/include/linux/mdio.h
> > +++ b/include/linux/mdio.h
> > @@ -38,6 +38,8 @@ struct mdio_device {
> >  	char modalias[MDIO_NAME_SIZE];
> 
> >  	int (*bus_match)(struct device *dev, struct device_driver *drv);
> > +	int (*bus_uevent)(struct mdio_device *mdiodev,
> > +			  struct kobj_uevent_env *env);
> > 	void (*device_free)(struct mdio_device *mdiodev);
> > 	void (*device_remove)(struct mdio_device *mdiodev);
> your patch I have a try and it can make marvel driver auto-load. However,
> you need to evaluate the above compatibility issues !
> in addition, if phy id register work bad or other case, you dont' read phy
> id from phy.  your patch will not work well. so you shoud definition a
> any_phy_id, of course, The most critical issue is the above driver
> compatibility, please you note.

You don't make sense, sorry.

> in additon, I have never received your email before. I have to check
> patchwork every time, so if you have a advice that could you send a mail to
> zhuyinbo@loongson.cn .

It was sent to you. Lore has it.

https://lore.kernel.org/all/YavYM2cs0RuY0JdM@shell.armlinux.org.uk/

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

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

[zhuyinbo]

在 2022/1/5 上午11:33, zhuyinbo 写道:
> 
> 
> 在 2022/1/4 下午9:11, zhuyinbo 写道:
>>
>>
>> 在 2021/12/7 下午5:41, 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"
>>>
>>>
>>>
>>
>>  > > > > 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.
>>
>>  > Please can you report the contents of /proc/sys/kernel/modprobe, and
>>  > the kernel configuration of CONFIG_MODPROBE_PATH. I wonder if your
>>  > userspace has that module loading mechanism disabled, or your kernel
>>  > has CONFIG_MODPROBE_PATH as an empty string.
>>
>>  > If the module is not present by the time this call is made, then
>>  > even if you load the appropriate driver module later, that module
>>  > will not be used - the PHY will end up being driven by the generic
>>  > clause 22 driver.
>>
>>  > > > 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,
>>
>>  > xilinx_gmii2rgmii is not a platform driver. It is a mdio driver:
>>
>>  > static struct mdio_driver xgmiitorgmii_driver = {
>>                ^^^^^^^^^^^
>>
>>  > Therefore, platform_uevent() is irrelevant since this will never match
>>  > a platform device. It will only match mdio devices, and the uevent
>>  > generation for that is via mdio_uevent() which is the function you
>>  > are changing.
>>
>>
>> static const struct of_device_id xgmiitorgmii_of_match[] = {
>>          { .compatible = "xlnx,gmii-to-rgmii-1.0" },
>>          {},
>> };
>> MODULE_DEVICE_TABLE(of, xgmiitorgmii_of_match);
>>
>> static struct mdio_driver xgmiitorgmii_driver = {
>>          .probe  = xgmiitorgmii_probe,
>>          .mdiodrv.driver = {
>>                  .name = "xgmiitorgmii",
>>                  .of_match_table = xgmiitorgmii_of_match,
>>          },
>> };
>>  From the present point of view, no matter what the situation, my 
>> supplement can cover udev or request_module for auto load module.
>>
>> if that phy driver isn't platform driver my patch cover it I think 
>> there is no doubt, if phy driver is platform driver and platform 
>> driver udev will cover it. My only requestion is the request_module 
>> not work well.
>>
>> about xgmiitorgmii_of_match that it belongs to platform driver load, 
>> please you note. and about your doubt usepace whether disable module 
>> load that module load function is okay becuase other device driver 
>> auto load is okay.
>>
>>  > > 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:
>>
>>  > Please report back what the following command produces on your
>>  > problem system:
>>
>>  > /sbin/modprobe -vn mdio:00000001010000010000110111010001
>>
>>  > Thanks.
>>
>> [root@localhost ~]# lsmod | grep marvell
>> [root@localhost ~]# ls 
>> /lib/modules/4.19.190+/kernel/drivers/net/phy/marvell.ko
>> /lib/modules/4.19.190+/kernel/drivers/net/phy/marvell.ko
>> [root@localhost ~]# /sbin/modprobe -vn 
>> mdio:00000001010000010000110111010001
>> insmod /lib/modules/4.19.190+/kernel/drivers/net/phy/marvell.ko
>> insmod /lib/modules/4.19.190+/kernel/drivers/net/phy/marvell.ko
>> [root@localhost ~]#
>> [root@localhost ~]# cat /proc/sys/kernel/modprobe
>> /sbin/modprobe
>>
>> BRs,
>> Yinbo
> 
>  > On Tue, Jan 04, 2022 at 09:11:56PM +0800, zhuyinbo wrote:
>  > > From the present point of view, no matter what the situation, my 
> supplement
>  > > can cover udev or request_module for auto load module.
>  > >
>  > > if that phy driver isn't platform driver my patch cover it I think 
> there is
>  > > no doubt, if phy driver is platform driver and platform driver udev 
> will
>  > > cover it. My only requestion is the request_module not work well.
>  > >
>  > > about xgmiitorgmii_of_match that it belongs to platform driver 
> load, please
>  > > you note. and about your doubt usepace whether disable module load 
> that
>  > > module load function is okay becuase other device driver auto load 
> is okay.
> 
>  > xgmiitorgmii is *not* a platform driver.
> 
> For the module loading function, you need to focus on the first args 
> "of" in function MODULE_ DEVICE_TABLE, not the definition type of this 
> driver.  for "of" type that must platform covert it !
> 
>  > > > Please report back what the following command produces on your
>  > > > problem system:
>  > > >
>  > > > /sbin/modprobe -vn mdio:00000001010000010000110111010001
>  > > >
>  > > > Thanks.
>  > >
>  > > [root@localhost ~]# lsmod | grep marvell
>  > > [root@localhost ~]# ls
>  > > /lib/modules/4.19.190+/kernel/drivers/net/phy/marvell.ko
>  > > /lib/modules/4.19.190+/kernel/drivers/net/phy/marvell.ko
>  > > [root@localhost ~]# /sbin/modprobe -vn 
>  >mdio:00000001010000010000110111010001
>  > > insmod /lib/modules/4.19.190+/kernel/drivers/net/phy/marvell.ko
>  > > insmod /lib/modules/4.19.190+/kernel/drivers/net/phy/marvell.ko
>  > > [root@localhost ~]#
>  > > [root@localhost ~]# cat /proc/sys/kernel/modprobe
>  > > /sbin/modprobe
> 
>  > Great, so the current scheme using "mdio:<binary digits>" works
>  > perfectly for you. What is missing is having that modalias in the
>  > uevent file.
> No, "lsmod | grep marvel" is NULL, so "mdio:<binary digits>" doesn't 
> work well. and that lost information is string that match do_mdio_entry 
> Hexadecimal string or binary digits and I add my patch use hexadecimal 
> and change do_mdio_entry to fix issue was to consider that different 
> "Revision Number" represent that phy hardware may be has some 
> difference, so I think we should not blindly load the corresponding PHY 
> driver. It is appropriate to match the PHY ID exactly. for 
> example,following code is marvell phy for 88e1510, which include it's 
> initial function for phy. and my platform hardware use 88e1512, if 
> doesn't change do_mdio_entry code, use "?" to match it, which represent
> 88e1512 hardware to match 881510 driver. of course if 88e1510 driver can 
> compatible with 88e1512 phy, it is okay, but for all kinds of ethernet 
> phy hc and hcd you can ensure it always has a good compatible for that?
> Like 88e1510 driver, it is compatible with 88e1512 and has no problem. 
> In fact, I'm not sure if there is a problem with the 88e1512 loaded 
> 88e1510 driver. So I modified do_mdio_entry is used for full matching, 
> and it can cover above all issue.
> 
>                  .phy_id = MARVELL_PHY_ID_88E1510,
>                  .phy_id_mask = MARVELL_PHY_ID_MASK,
>                  .name = "Marvell 88E1510",
>                  .features = PHY_GBIT_FEATURES | SUPPORTED_FIBRE,
>                  .flags = PHY_HAS_INTERRUPT,
>                  .probe = &m88e1510_probe,
>                  .config_init = &m88e1510_config_init,
>                  .config_aneg = &m88e1510_config_aneg,
>                  .read_status = &marvell_read_status,
> 
> 
>  > So, my patch on the 4th December should cause the marvell module to
>  > be loaded at boot time. Please test that patch ASAP, which I have
>  > already asked you to do. I'll include it again in this email so you
>  > don't have to hunt for it.
> 
>  > 8<===
>  > From: "Russell King (Oracle)" <rmk+kernel@armlinux.org.uk>
>  > Subject: [PATCH] net: phy: generate PHY mdio modalias
> 
>  > The modalias string provided in the uevent sysfs file does not conform
>  > to the format used in PHY driver modules. One of the reasons is that
>  > udev loading of PHY driver modules has not been an expected use case.
> 
>  > This patch changes the MODALIAS entry for only PHY devices from:
>  >         MODALIAS=of:Nethernet-phyT(null)
>  > to:
>  >         MODALIAS=mdio:00000000001000100001010100010011
> 
>  > Other MDIO devices (such as DSA) remain as before.
> 
>  > However, having udev automatically load the module has the advantage
>  > of making use of existing functionality to have the module loaded
>  > before the device is bound to the driver, thus taking advantage of
>  > multithreaded boot systems, potentially decreasing the boot time.
> 
>  > However, this patch will not solve any issues with the driver module
>  > not being loaded prior to the network device needing to use the PHY.
>  > This is something that is completely out of control of any patch to
>  > change the uevent mechanism.
> 
>  > Reported-by: Yinbo Zhu <zhuyinbo@loongson.cn>
>  > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
>  > ---
>  > drivers/net/phy/mdio_bus.c   |  8 ++++++++
>  > drivers/net/phy/phy_device.c | 14 ++++++++++++++
>  > include/linux/mdio.h         |  2 ++
>  > 3 files changed, 24 insertions(+)
> 
>  > diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
>  > index 4638d7375943..663bd98760fb 100644
>  > --- a/drivers/net/phy/mdio_bus.c
>  > +++ b/drivers/net/phy/mdio_bus.c
>  > @@ -1010,8 +1010,16 @@ static int mdio_bus_match(struct device *dev, 
>  > > struct device_driver *drv)
> 
>  >  static int mdio_uevent(struct device *dev, struct kobj_uevent_env *env)
>   > {
>  > +    struct mdio_device *mdio = to_mdio_device(dev);
>  >     int rc;
>  >
>  > +    /* Use the device-specific uevent if specified */
>  > +    if (mdio->bus_uevent) {
>  > +        rc = mdio->bus_uevent(mdio, env);
>  > +        if (rc != -ENODEV)
>  > +            return rc;
>  > +    }
>  > +
>  >     /* Some devices have extra OF data and an OF-style MODALIAS */
>  >      rc = of_device_uevent_modalias(dev, env);
>  >     if (rc != -ENODEV)
>  > diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
>  > index 23667658b9c6..f4c2057f0202 100644
>  > --- a/drivers/net/phy/phy_device.c
>  > +++ b/drivers/net/phy/phy_device.c
>  > @@ -563,6 +563,19 @@ static int phy_request_driver_module(struct 
> phy_device *dev, u32 phy_id)
>  >      return 0;
>  >  }
> 
>  > +static int phy_bus_uevent(struct mdio_device *mdiodev,
>  > +              struct kobj_uevent_env *env)
>  > +{
>  > +    struct phy_device *phydev;
>  > +
>  > +    phydev = container_of(mdiodev, struct phy_device, mdio);
>  > +
>  > +    add_uevent_var(env, "MODALIAS=" MDIO_MODULE_PREFIX MDIO_ID_FMT,
>  > +               MDIO_ID_ARGS(phydev->phy_id));
>  > +
>  > +    return 0;
>  > +}
>  > +
>  >  struct phy_device *phy_device_create(struct mii_bus *bus, int addr, 
> u32 phy_id,
>  >                       bool is_c45,
>  >                       struct phy_c45_device_ids *c45_ids)
>  > @@ -582,6 +595,7 @@ struct phy_device *phy_device_create(struct 
> mii_bus *bus, int addr, u32 phy_id,
>  >      mdiodev->dev.type = &mdio_bus_phy_type;
>  >      mdiodev->bus = bus;
>  >     mdiodev->bus_match = phy_bus_match;
>  > +    mdiodev->bus_uevent = phy_bus_uevent;
>  >     mdiodev->addr = addr;
>  >     mdiodev->flags = MDIO_DEVICE_FLAG_PHY;
>  >     mdiodev->device_free = phy_mdio_device_free;
>  > diff --git a/include/linux/mdio.h b/include/linux/mdio.h
>  > index df9c96e56907..5c6676d3de23 100644
>  > --- a/include/linux/mdio.h
>  > +++ b/include/linux/mdio.h
>  > @@ -38,6 +38,8 @@ struct mdio_device {
>  >      char modalias[MDIO_NAME_SIZE];
> 
>  >      int (*bus_match)(struct device *dev, struct device_driver *drv);
>  > +    int (*bus_uevent)(struct mdio_device *mdiodev,
>  > +              struct kobj_uevent_env *env);
>  >     void (*device_free)(struct mdio_device *mdiodev);
>  >     void (*device_remove)(struct mdio_device *mdiodev);
> your patch I have a try and it can make marvel driver auto-load. 
> However, you need to evaluate the above compatibility issues !
> in addition, if phy id register work bad or other case, you dont' read 
> phy id from phy.  your patch will not work well. so you shoud definition 
> a any_phy_id, of course, The most critical issue is the above driver 
> compatibility, please you note.
> 
> in additon, I have never received your email before. I have to check 
> patchwork every time, so if you have a advice that could you send a mail 
> to zhuyinbo@loongson.cn .
> 
> Thanks,
> 
> BRs,
> Yinbo Zhu.
Hi phy maintainer,

What's your viewpoint?

Thanks
BRs
Yinbo Zhu.

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

[Andrew Lunn]

> Hi phy maintainer,
> 
> What's your viewpoint?

Russell is a PHY Maintainer.

I suggest you stop arguing with him. Test what Russell proposes, and
let him know if it solves the problem you were seeing.

	Andrew