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

[PATCH v1 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>
---
 scripts/mod/file2alias.c | 17 +----------------
 1 file changed, 1 insertion(+), 16 deletions(-)

diff --git a/scripts/mod/file2alias.c b/scripts/mod/file2alias.c
index 49aba86..5ba1039 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 != 0, phy_id);
 	return 1;
 }
 
-- 
1.8.3.1

[PATCH v1 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 v1 1/2] modpost: file2alias: fixup mdio alias garbled code in modules.alias

[Andrew Lunn]

On Mon, Nov 22, 2021 at 08:14:57PM +0800, 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.

What PHY do you have problems with? What is the PHY id and which
driver should be loaded.

This code has existed a long time, so suddenly saying it is wrong and
changing it needs a good explanation why it is wrong. Being ugly is
not a good reason.

    Andrew

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

[Russell King (Oracle)]

On Mon, Nov 22, 2021 at 08:14:58PM +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;

The MDIO bus contains more than just PHYs. This completely breaks
anything that isn't a PHY device - likely by performing an
out-of-bounds access.

This change also _totally_ breaks any MDIO devices that rely on
matching via the "of:" mechanism using the compatible specified in
DT. An example of that is the B53 DSA switch.

Sorry, but we've already learnt this lesson from a similar case with
SPI. Once one particular way of dealing with MODALIAS has been
established for auto-loading modules for a subsystem, it is very
difficult to change it without causing regressions.

We need a very clear description of the problem that these patches are
attempting to address, and then we need to see that effort has been
put in to verify that changing the auto-loading mechanism is safe to
do - such as auditing every single driver that use the MDIO subsystem.

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

This looks like an unrelated user-interface breaking change. This
changes the path of all MDIO devices and drivers in /sys/bus/mdio_bus/*

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 v1 1/2] modpost: file2alias: fixup mdio alias garbled code in modules.alias

[zhuyinbo]

在 2021/11/22 下午10:07, Andrew Lunn 写道:
> On Mon, Nov 22, 2021 at 08:14:57PM +0800, 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.
> What PHY do you have problems with? What is the PHY id and which
> driver should be loaded.

     about that phy id,  phy dev read it from  PHY Identifier 1 and 
Identifier 2 register, phy driver will call MODULE_DEVICE_TABLE to 
configure

     phy id to mdio_device_id, phy id was used to do a match phy driver 
with phy device.  that phy problems is phy driver was select 'M' then it

     doesn't be auto load.
> This code has existed a long time, so suddenly saying it is wrong and
> changing it needs a good explanation why it is wrong. Being ugly is
> not a good reason.
>
>      Andrew

Hi Andrew,

     Use default mdio configure, After module compilation, mdio alias 
configure is "alias mdio:0000000101000001000011111001???? marvell"

     and it doesn't match  the match phy dev(mdio dev)  uevent, because 
the mdio alias configure "0000000101000001000011111001????"

    include "?" and  "binary number",   as general, uevent it include 
one string or some string that string consist of one character and one 
hexadecimal digit ,

     which uevent is reported by mdio when mdio register a device for 
ethernet phy device, only uevent from phy dev match alias configure from

     phy driver that phy driver will can be auto-load when phy driver 
was selected  'M'.


BRs

Yinbo Zhu.

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

[Andrew Lunn]

On Tue, Nov 23, 2021 at 09:33:03AM +0800, zhuyinbo wrote:
> 
> 在 2021/11/22 下午10:07, Andrew Lunn 写道:
> 
>     On Mon, Nov 22, 2021 at 08:14:57PM +0800, 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.
> 
>     What PHY do you have problems with? What is the PHY id and which
>     driver should be loaded.
> 
>     This code has existed a long time, so suddenly saying it is wrong and
>     changing it needs a good explanation why it is wrong. Being ugly is
>     not a good reason.
> 
>         Andrew
> 
> Hi Andrew,
> 
>     Use default mdio configure, After module compilation, mdio alias configure
> is following and it doesn't match
> 
>     the match phy dev(mdio dev)  uevent, because the mdio alias configure 
> "0000000101000001000011111001????"  include "?" and

A PHY ID generally break up into 3 parts.

The OUI of the manufacture.
The device.
The revision

The ? means these bits don't matter. Those correspond to the
revision. Generally, a driver can driver any revision of the PHY,
which is why those bits don't matter.

So when a driver probes with the id 00000001010000010000111110010110
we expect user space to find the best match, performing wildcard
expansion. So the ? will match anything.

Since this is worked for a long time, do you have an example where it
is broken? If so, which PHY driver? If it is broken, no driver is
loaded, or the wrong driver is loaded, i expect it is a bug in a
specific driver. And we should fix that bug in the specific driver.

     Andrew

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

[zhuyinbo]

在 2021/11/23 下午12:12, Andrew Lunn 写道:
> On Tue, Nov 23, 2021 at 09:33:03AM +0800, zhuyinbo wrote:
>> 在 2021/11/22 下午10:07, Andrew Lunn 写道:
>>
>>      On Mon, Nov 22, 2021 at 08:14:57PM +0800, 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.
>>
>>      What PHY do you have problems with? What is the PHY id and which
>>      driver should be loaded.
>>
>>      This code has existed a long time, so suddenly saying it is wrong and
>>      changing it needs a good explanation why it is wrong. Being ugly is
>>      not a good reason.
>>
>>          Andrew
>>
>> Hi Andrew,
>>
>>      Use default mdio configure, After module compilation, mdio alias configure
>> is following and it doesn't match
>>
>>      the match phy dev(mdio dev)  uevent, because the mdio alias configure
>> "0000000101000001000011111001????"  include "?" and
> A PHY ID generally break up into 3 parts.
>
> The OUI of the manufacture.
> The device.
> The revision
>
> The ? means these bits don't matter. Those correspond to the
> revision. Generally, a driver can driver any revision of the PHY,
> which is why those bits don't matter.
>
> So when a driver probes with the id 00000001010000010000111110010110
> we expect user space to find the best match, performing wildcard
> expansion. So the ? will match anything.
>
> Since this is worked for a long time, do you have an example where it
> is broken? If so, which PHY driver? If it is broken, no driver is
> loaded, or the wrong driver is loaded, i expect it is a bug in a
> specific driver. And we should fix that bug in the specific driver.
>
>       Andrew

Hi Andrew,

The string like "0000000101000001000011111001????" dont't match any mdio driver, and i said it include "? that "?" doesn't match any driver, in addition that include Binary digit
like "0000000101000001000011111001", that binary digit doesn't match any driver, that should use Hexadecimal for phy id, and I test on some platform, not only a platform, it isn't some
specifi driver issue, it is gerneral issue. please you note.  that phy driver match phy device must use whole string "MODALIAS=xxxyyzz", not partial match.

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

[Andrew Lunn]

> > > Hi Andrew,
> > > 
> > >      Use default mdio configure, After module compilation, mdio alias configure
> > > is following and it doesn't match
> > > 
> > >      the match phy dev(mdio dev)  uevent, because the mdio alias configure
> > > "0000000101000001000011111001????"  include "?" and
> > A PHY ID generally break up into 3 parts.
> > 
> > The OUI of the manufacture.
> > The device.
> > The revision
> > 
> > The ? means these bits don't matter. Those correspond to the
> > revision. Generally, a driver can driver any revision of the PHY,
> > which is why those bits don't matter.
> > 
> > So when a driver probes with the id 00000001010000010000111110010110
> > we expect user space to find the best match, performing wildcard
> > expansion. So the ? will match anything.
> > 
> > Since this is worked for a long time, do you have an example where it
> > is broken? If so, which PHY driver? If it is broken, no driver is
> > loaded, or the wrong driver is loaded, i expect it is a bug in a
> > specific driver. And we should fix that bug in the specific driver.
> > 
> >       Andrew
> 
> Hi Andrew,
> 
> The string like "0000000101000001000011111001????" dont't match any mdio driver, and i said it include "? that "?" doesn't match any driver, in addition that include Binary digit
> like "0000000101000001000011111001", that binary digit doesn't match any driver, that should use Hexadecimal for phy id, and I test on some platform, not only a platform, it isn't some
> specifi driver issue, it is gerneral issue. please you note.  that phy driver match phy device must use whole string "MODALIAS=xxxyyzz", not partial match.

Please give a concrete example. Show us udev logs of it not working,
it failing to find a match.

	Andrew

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

[zhuyinbo]

在 2021/11/23 下午9:54, Andrew Lunn 写道:
>>>> Hi Andrew,
>>>>
>>>>       Use default mdio configure, After module compilation, mdio alias configure
>>>> is following and it doesn't match
>>>>
>>>>       the match phy dev(mdio dev)  uevent, because the mdio alias configure
>>>> "0000000101000001000011111001????"  include "?" and
>>> A PHY ID generally break up into 3 parts.
>>>
>>> The OUI of the manufacture.
>>> The device.
>>> The revision
>>>
>>> The ? means these bits don't matter. Those correspond to the
>>> revision. Generally, a driver can driver any revision of the PHY,
>>> which is why those bits don't matter.
>>>
>>> So when a driver probes with the id 00000001010000010000111110010110
>>> we expect user space to find the best match, performing wildcard
>>> expansion. So the ? will match anything.
>>>
>>> Since this is worked for a long time, do you have an example where it
>>> is broken? If so, which PHY driver? If it is broken, no driver is
>>> loaded, or the wrong driver is loaded, i expect it is a bug in a
>>> specific driver. And we should fix that bug in the specific driver.
>>>
>>>        Andrew
>> Hi Andrew,
>>
>> The string like "0000000101000001000011111001????" dont't match any mdio driver, and i said it include "? that "?" doesn't match any driver, in addition that include Binary digit
>> like "0000000101000001000011111001", that binary digit doesn't match any driver, that should use Hexadecimal for phy id, and I test on some platform, not only a platform, it isn't some
>> specifi driver issue, it is gerneral issue. please you note.  that phy driver match phy device must use whole string "MODALIAS=xxxyyzz", not partial match.
> Please give a concrete example. Show us udev logs of it not working,
> it failing to find a match.
>
> 	Andrew

Hi Andrew,


     I don't get udev log, but I can find that phy module wether be load 
by lsmod ways,  and you can try

     it in any a phy deice and in any arch platform.   in addition,  I 
will send v2 version patch that need consider

     some phy device doesn't follow IEEE802.3 protocol strictly and 
doesn't read phy id from phy register successfully,

     please review.


Brs,

Yinbo Zhu.

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

[Russell King (Oracle)]

On Fri, Nov 26, 2021 at 05:34:36PM +0800, zhuyinbo wrote:
> Hi Andrew,
> 
> 
>     I don't get udev log, but I can find that phy module wether be load by
> lsmod ways,  and you can try
> 
>     it in any a phy deice and in any arch platform.   in addition,  I will
> send v2 version patch that need consider
> 
>     some phy device doesn't follow IEEE802.3 protocol strictly and doesn't
> read phy id from phy register successfully,

I'm not sure I understand what you've said above correctly.

You seem to be saying that the module has been loaded (it appears in
lsmod) but it doesn't get bound to the PHY device? If that is correct,
there isn't a problem with the modalias, but there's something wrong
in the driver matching.

The last sentence seems to point at a completely different problem -
one concerning an inability to correctly read the PHY ID from the PHY
itself. If that's the case, then that isn't a problem with modalias,
it's a problem with reading the PHY ID.


The existing modalias scheme doesn't care about the format of the ID.
Normally the least significant four bits are the PHY revision, but that
doesn't always hold. Let's take a couple of examples:

The PHY ID for a Marvell PHY is 0x01410dd1. This will expand to:

mdio:00000001010000010000110111010001

The kernel modalias tables will be generated from an ID of 0x01410dd0
with a mask of 0xfffffff0. The mask means "ignore the bottom four bits".
So, we end up with this in the module's alias table:

mdio:0000000101000001000011011101????

udev knows that "?" is a wildcard. Consequently the above matches.

On an Atheros PHY, this has an ID value of 0x004dd072. The driver also
has a value of 0x004dd072 and a mask of 0xffffffef, meaning bit 4 is
ignored:

mdio:00000000010011011101000001110010 <= PHY ID
mdio:000000000100110111010000011?0010 <= module alias table

This will also match a PHY with id 0x004dd062.

mdio:00000000010011011101000001100010 <= PHY ID

The current modalias approach is flexible. Publishing the raw hex ID
and having an exact match is too inflexible for phylib and will lead to
users reporting regressions.

Please show us:

1) the contents of the phy_id file for the PHY you are having problems
   with. This can be found in /sys/bus/mdio_bus/devices/.../phy_id

2) which driver in the kernel is a problem.

Thanks.

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