Date:   Sat,  4 Dec 2021 17:13:27 +0800
From:   Yinbo Zhu <zhuyinbo@...ngson.cn>
To:     Andrew Lunn <andrew@...n.ch>,
        Heiner Kallweit <hkallweit1@...il.com>,
        Russell King <linux@...linux.org.uk>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Masahiro Yamada <masahiroy@...nel.org>,
        Michal Marek <michal.lkml@...kovi.net>,
        Nick Desaulniers <ndesaulniers@...gle.com>,
        netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-kbuild@...r.kernel.org
Cc:     zhuyinbo@...ngson.cn
Subject: [PATCH v4 1/2] modpost: file2alias: make mdio alias configure match mdio uevent

The do_mdio_entry was responsible for generating a phy alias configure
that according to the phy driver's mdio_device_id, before apply this
patch, which alias configure is like "alias mdio:000000010100000100001
1011101????", it doesn't match the phy_id of mdio_uevent, because of
the phy_id was a hexadecimal digit and the mido uevent is consisit of
phy_id with the char 'p', the uevent string is different from alias.
Add this patch that mdio alias configure will can match mdio uevent.

Signed-off-by: Yinbo Zhu <zhuyinbo@...ngson.cn>
---
Change in v4:
		Add following explain information.

Hi Russell King ,


I had given you  feedback lots of times, but it may be mail list server issue, you don't accept my mail,

and I don't get your mail, then I add that what i want explain in v1 patch for your v1 patch comment, 

you can check. then for v3 patch that is for rework commit inforation accorting , just , I notice you

have a comment in v2, but i dont' accept your mail. and I give you explain as follows:


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

On my platform, I can find following, stmmac-xx that is is mac name, it represent this platform has two mac 
controller, it isn't phy, but it's sub dir has phy id it is phy silicon id, and devices name is set by mdio bus,
then, you said that "where we match the entire ID except bit 4." I think marvell it is special, and you can have 
look other phy,e.g. motorcomm phy, that phy mask is 0x00000fff or 0x0000ffff or ther, for different phy silicon, 
that phy maskit is not same, and that phy mask it isn't device property, you dont't get it from register, and mdio
 bus for phy register a device then mdiobus_scan will get phy id that phy id it is include all value, and don't has 
mask operation. then phy uevent must use phy_id that from phy register and that uevent doesn't include phy mask, if
uevent add phy mask, then you need  define a phy mask. if you have mature consideration, you will find that definition
phy mask it isn't appropriate, if you define it in mac driver, because  mac can select different phy, if you define it
 in dts, then phy driver is "of" type, mdio_uevent will doesn't be called. if you ask phy_id & phy_mask as a phy uevent,
 I think it is no make sense, e.g. 88e1512 and 88e1510 that has different "phy revision" so that phy silicon has difference, 
and uevent should be unique. If you have no other opinion on the above view, Back to this patch, that phy id of uevent
need match phy alias configure, so alis configure must use phy id all value.

In addition, Even if you hadn't  consided what I said above, you need to know one thing, uevent match alias that must be full
 matching. not Partial matching. I said it a long time ago.  why you think Binary digit and "?" can match dev uevent,   
according my code analysis and test analysis that  any platform and any mdio phy device is all fail that be matched if that
 phy driver module and phy dev was use uevent-alias mechanism

[root@...alhost ~]# cat /sys/bus/mdio/devices/stmmac-19\:00/phy_id 
0x01410dd1
[root@...alhost ~]# 
[root@...alhost ~]# cat /sys/bus/mdio/devices/stmmac-18\:00/phy_id 
0x01410dd1
[root@...alhost ~]# 

Thanks,
BRs,
Yinbo Zhu.


 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

Powered by blists - more mailing lists