[PATCH v3 1/2] modpost: file2alias: make mdio alias configure match mdio uevent

[PATCH v3 1/2] modpost: file2alias: make mdio alias configure match mdio uevent

[Yinbo Zhu]

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@loongson.cn>
---
Change in v3:
		Rework the patch commit log information.

 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 v3 2/2] net: mdio: rework mdio_uevent for mdio ethernet phy device

[Yinbo Zhu]

The of_device_uevent_modalias is service for 'of' type platform driver
, which ask the first args must be 'of' that use MODULE_DEVICE_TABLE
when driver was exported, but ethernet phy is a kind of 'mdio' type
device and it is inappropriate if driver use 'of' type for exporting,
in fact, most mainstream ethernet phy driver hasn't used 'of' type,
even though phy driver was exported use 'of' type and it's irrelevant
with mdio_uevent, at this time, platform_uevent was responsible for
reporting uevent to match modules.alias configure, so, whatever that
of_device_uevent_modalias was unnecessary, this patch was to remove it
and add phy_id as modio uevent then ethernet phy module auto load
function will work well.

Signed-off-by: Yinbo Zhu <zhuyinbo@loongson.cn>
---
Change in v3:
		Rework the patch commit log information.

 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 v3 2/2] net: mdio: rework mdio_uevent for mdio ethernet phy device

[Russell King (Oracle)]

On Tue, Nov 30, 2021 at 04:21:57PM +0800, Yinbo Zhu wrote:
> The of_device_uevent_modalias is service for 'of' type platform driver
> , which ask the first args must be 'of' that use MODULE_DEVICE_TABLE
> when driver was exported, but ethernet phy is a kind of 'mdio' type
> device and it is inappropriate if driver use 'of' type for exporting,
> in fact, most mainstream ethernet phy driver hasn't used 'of' type,
> even though phy driver was exported use 'of' type and it's irrelevant
> with mdio_uevent, at this time, platform_uevent was responsible for
> reporting uevent to match modules.alias configure, so, whatever that
> of_device_uevent_modalias was unnecessary, this patch was to remove it
> and add phy_id as modio uevent then ethernet phy module auto load
> function will work well.

NAK.

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

Re: [PATCH v3 1/2] modpost: file2alias: make mdio alias configure match mdio uevent

[Russell King (Oracle)]

On Tue, Nov 30, 2021 at 04:21:56PM +0800, Yinbo Zhu wrote:
> 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.

This is getting rediculous. You don't appear to be listening to the
technical feedback on your patches, and are just reposting the same
patches. I don't see any point in giving the same feedback, so I'll
keep this brief for both patches:

NAK.

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

Re: [PATCH v3 1/2] modpost: file2alias: make mdio alias configure match mdio uevent

[Russell King (Oracle)]

On Sat, Dec 04, 2021 at 05:02:51PM +0800, zhuyinbo wrote:
> 
> 在 2021/11/30 下午5:13, Russell King (Oracle) 写道:
> > On Tue, Nov 30, 2021 at 04:21:56PM +0800, Yinbo Zhu wrote:
> > > 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.
> > This is getting rediculous. You don't appear to be listening to the
> > technical feedback on your patches, and are just reposting the same
> > patches. I don't see any point in giving the same feedback, so I'll
> > keep this brief for both patches:
> > 
> > NAK.
> 
> 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,

I get your email. What I don't see is any change in the resulting code
as a result of my feedback.

I have told you several times to separate out the change to the name
used in bus_type. You have completely ignored that. I will quite simply
NAK that patch every time you post it as long as you have not made that
change.

I have told you that your patch will cause regressions. You continue
to repost it. I will NAK your patch as long as it contains a known
regression because causing a regression is totally _unacceptable_.

> 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

This shows that there's a problem with understanding right here, and
I'm quite sure Qualcomm would be very disappointed to hear you think
that their PHYs are made by their competitor, Marvell.

Marvell PHY driver entries use a mask of 0xfffffff0.
Atheros PHY driver entries use a mask of 0xffffffef.

The point I was making is that different PHYs can have different masks,
and this information needs to be encoded in the PHYs alias in the driver
module. We do this by forcing bits that are zero in the mask to be
encoded with a wildcard "?". Bits that are set in the mask are "0" or
"1" depending on their bit value in the driver's ID.

So:
Marvell 88E1510 has an ID of: mdio:0000000101000001000011011101????
Atheros AR8035 has an ID of:  mdio:000000000100110111010000011?0010

The wildcard "?" is in a different position because the mask is
different.

> look other phy,e.g. motorcomm phy, that phy mask is 0x00000fff or 0x0000ffff or ther, for different phy silicon,

... which is exactly my point.  The PHY mask depends on the PHY, and
this is known to the PHY driver.

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

The mask never comes from the PHY. It only _ever_ comes from the driver.
The operation for matching a PHY driver to a PHY device is:

  phy_id - read from device
  phydrv->phy_id - from the driver
  phydrv->phy_mask - from the driver

  match = (phy_id & phydrv->phy_mask) == (phydrv->phy_id & phydrv->phy_mask);

This is exactly the same operation that we use when matching a PHY
driver module to the PHY ID that the kernel has. The "phy_id" comes
from the uevent. The two phydrv fields come from the alias string
encoded with its wildcards in the module. Matching using these
wildcards achieves the above matching 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.

I think you have a fundamental misunderstanding of how this matching
works - and it does work. The uevent modalias string will be the
full ID read from the PHY hardware - there is no mask at this stage.

The mask is encoded in the module using "?" as a wildcard.

> 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

This is where you appear to be incorrect. It is _not_ full matching,
it never has been. Other schemes use other _patterns_ to match, and
by that I mean character groups (inside [] brackets).

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

So now we finally know you have a revision of the Marvell 88E1510 PHY,
after we've asked you many times for that information.

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

Re: [PATCH v3 1/2] modpost: file2alias: make mdio alias configure match mdio uevent

[Russell King (Oracle)]

On Sat, Dec 04, 2021 at 09:38:03AM +0000, Russell King (Oracle) wrote:
> I have told you several times to separate out the change to the name
> used in bus_type. You have completely ignored that. I will quite simply
> NAK that patch every time you post it as long as you have not made that
> change.
> 
> I have told you that your patch will cause regressions. You continue
> to repost it. I will NAK your patch as long as it contains a known
> regression because causing a regression is totally _unacceptable_.

Here is a patch that stands a far better chance of being acceptable
and resolving your issue. Please test this patch and check whether
it solves your issue.

If it does not solve your issue, then please post a _full_ kernel
boot log and provide your kernel configuration (the .config file).

Thanks.

8<====
From: "Russell King (Oracle)" <rmk+kernel@armlinux.org.uk>
Subject: [PATCH net-next] 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 9b6f2df07211..68cf59ce7343 100644
--- a/drivers/net/phy/mdio_bus.c
+++ b/drivers/net/phy/mdio_bus.c
@@ -994,8 +994,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 74d8e1dc125f..208ef5342798 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -565,6 +565,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)
@@ -584,6 +597,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 9f3587a61e14..fb97cbf65ec7 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 v3 1/2] modpost: file2alias: make mdio alias configure match mdio uevent

[Andrew Lunn]

> 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

Hi Russell

You patch looks good for the straight forward cases.

What happens in the case of

        ethernet-phy@0 {
            compatible = "ethernet-phy-ieee802.3-c45";


Does this get appended to the end, or does it overwrite?

     Andrew

Re: [PATCH v3 1/2] modpost: file2alias: make mdio alias configure match mdio uevent

[Russell King (Oracle)]

On Sun, Dec 05, 2021 at 04:56:44PM +0100, Andrew Lunn wrote:
> > This patch changes the MODALIAS entry for only PHY devices from:
> > 	MODALIAS=of:Nethernet-phyT(null)
> > to:
> > 	MODALIAS=mdio:00000000001000100001010100010011
> 
> Hi Russell
> 
> You patch looks good for the straight forward cases.
> 
> What happens in the case of
> 
>         ethernet-phy@0 {
>             compatible = "ethernet-phy-ieee802.3-c45";
> 
> 
> Does this get appended to the end, or does it overwrite?

Hmm, good point, I'd forgotten about clause 45 PHYs - we need to dig
the first existing ID out of the clause 45 ID array and use that
instead. That said, we don't publish the ID through the "phy_id"
sysfs file either for clause 45.

I don't believe it's possible to publish multiple modalias strings.

This gives a problem for clause 45 PHYs which can have a different ID
for each MMD, and the driver might match on only one of those. 88x3310
is an example where different MMDs have different IDs. The mechanism
we have in phy_device_create() gets around that, because we call
phy_request_driver_module() for every ID there is in the hope that one
of those will load the appropriate driver, but as I say, I don't
believe that's a possibility via the udev approach.

So I think we may have to say that clause 45 PHYs can't reliably use
the conventional udev mechanism.

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