[PATCH net-next] net: phylink: improve phylink_sfp_config_phy() error message with empty supported

[PATCH net-next] net: phylink: improve phylink_sfp_config_phy() error message with empty supported

[Vladimir Oltean]

It seems that phylink does not support driving PHYs in SFP modules using
the Generic PHY or Generic Clause 45 PHY driver. I've come to this
conclusion after analyzing these facts:

- sfp_sm_probe_phy(), who is our caller here, first calls
  phy_device_register() and then sfp_add_phy() -> ... ->
  phylink_sfp_connect_phy().

- phydev->supported is populated by phy_probe()

- phy_probe() is usually called synchronously from phy_device_register()
  via phy_bus_match(), if a precise device driver is found for the PHY.
  In that case, phydev->supported has a good chance of being set to a
  non-zero mask.

- There is an exceptional case for the PHYs for which phy_bus_match()
  didn't find a driver. Those devices sit for a while without a driver,
  then phy_attach_direct() force-binds the genphy_c45_driver or
  genphy_driver to them. Again, this triggers phy_probe() and renders
  a good chance of phydev->supported being populated, assuming
  compatibility with genphy_read_abilities() or
  genphy_c45_pma_read_abilities().

- phylink_sfp_config_phy() does not support the exceptional case of
  retrieving phydev->supported from the Generic PHY driver, due to its
  code flow. It expects the phydev->supported mask to already be
  non-empty, because it first calls phylink_validate() on it, and only
  calls phylink_attach_phy() if that succeeds. Thus, phylink_attach_phy()
  -> phy_attach_direct() has no chance of running.

It is not my wish to change the state of affairs by altering the code
flow, but merely to document the limitation rather than have the current
unspecific error:

[   61.800079] mv88e6085 d0032004.mdio-mii:12 sfp: validation with support 00,00000000,00000000,00000000 failed: -EINVAL
[   61.820743] sfp sfp: sfp_add_phy failed: -EINVAL

On the premise that an empty phydev->supported is going to make
phylink_validate() fail anyway, it would be more informative to single
out that case, undercut the phylink_validate() call, and print a more
specific message:

[   64.738270] mv88e6085 d0032004.mdio-mii:12 sfp: PHY i2c:sfp:16 (id 0x01410cc2) supports no link modes. Maybe its specific PHY driver not loaded?
[   64.769731] sfp sfp: sfp_add_phy failed: -EINVAL

Of course, there may be other reasons due to which phydev->supported is
empty, thus the use of the word "maybe", but I think the lack of a
driver would be the most common.

Link: https://lore.kernel.org/netdev/20241113144229.3ff4bgsalvj7spb7@skbuf/
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 drivers/net/phy/phylink.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index b1e828a4286d..efeff8733a52 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -3219,6 +3219,11 @@ static int phylink_sfp_config_phy(struct phylink *pl, u8 mode,
 	int ret;
 
 	linkmode_copy(support, phy->supported);
+	if (linkmode_empty(support)) {
+		phylink_err(pl, "PHY %s (id 0x%.8lx) supports no link modes. Maybe its specific PHY driver not loaded?\n",
+			    phydev_name(phy), (unsigned long)phy->phy_id);
+		return -EINVAL;
+	}
 
 	memset(&config, 0, sizeof(config));
 	linkmode_copy(config.advertising, phy->advertising);
-- 
2.43.0

Re: [PATCH net-next] net: phylink: improve phylink_sfp_config_phy() error message with empty supported

[Andrew Lunn]

> [   64.738270] mv88e6085 d0032004.mdio-mii:12 sfp: PHY i2c:sfp:16 (id 0x01410cc2) supports no link modes. Maybe its specific PHY driver not loaded?
> [   64.769731] sfp sfp: sfp_add_phy failed: -EINVAL
> 
> Of course, there may be other reasons due to which phydev->supported is
> empty, thus the use of the word "maybe", but I think the lack of a
> driver would be the most common.

I think this is useful.

I only have a minor nitpick, maybe in the commit message mention which
PHY drivers are typically used by SFPs, to point somebody who gets
this message in the right direction. The Marvell driver is one. at803x
i think is also used. Are then any others?

	Andrew

Re: [PATCH net-next] net: phylink: improve phylink_sfp_config_phy() error message with empty supported

[Russell King (Oracle)]

On Thu, Nov 14, 2024 at 06:38:13PM +0100, Andrew Lunn wrote:
> > [   64.738270] mv88e6085 d0032004.mdio-mii:12 sfp: PHY i2c:sfp:16 (id 0x01410cc2) supports no link modes. Maybe its specific PHY driver not loaded?
> > [   64.769731] sfp sfp: sfp_add_phy failed: -EINVAL
> > 
> > Of course, there may be other reasons due to which phydev->supported is
> > empty, thus the use of the word "maybe", but I think the lack of a
> > driver would be the most common.
> 
> I think this is useful.
> 
> I only have a minor nitpick, maybe in the commit message mention which
> PHY drivers are typically used by SFPs, to point somebody who gets
> this message in the right direction. The Marvell driver is one. at803x
> i think is also used. Are then any others?

bcm84881 too. Not sure about at803x - the only SFP I know that uses
that PHY doesn't make the PHY available to the host.

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

Re: [PATCH net-next] net: phylink: improve phylink_sfp_config_phy() error message with empty supported

[Andrew Lunn]

On Thu, Nov 14, 2024 at 06:53:48PM +0200, Vladimir Oltean wrote:
> It seems that phylink does not support driving PHYs in SFP modules using
> the Generic PHY or Generic Clause 45 PHY driver.

Somewhat related, i wounder if we should add a phydev_info() message
in genphy_probe() which prints a message that you probably want to
swap to using a PHY driver specific to the hardware?

The less genphy is used, the better.

	Andrew

Re: [PATCH net-next] net: phylink: improve phylink_sfp_config_phy() error message with empty supported

[Vladimir Oltean]

[-- Attachment #1: Type: text/plain, Size: 2311 bytes --]

On Thu, Nov 14, 2024 at 06:33:00PM +0000, Russell King (Oracle) wrote:
> On Thu, Nov 14, 2024 at 06:38:13PM +0100, Andrew Lunn wrote:
> > > [   64.738270] mv88e6085 d0032004.mdio-mii:12 sfp: PHY i2c:sfp:16 (id 0x01410cc2) supports no link modes. Maybe its specific PHY driver not loaded?
> > > [   64.769731] sfp sfp: sfp_add_phy failed: -EINVAL
> > > 
> > > Of course, there may be other reasons due to which phydev->supported is
> > > empty, thus the use of the word "maybe", but I think the lack of a
> > > driver would be the most common.
> > 
> > I think this is useful.
> > 
> > I only have a minor nitpick, maybe in the commit message mention which
> > PHY drivers are typically used by SFPs, to point somebody who gets
> > this message in the right direction. The Marvell driver is one. at803x
> > i think is also used. Are then any others?
> 
> bcm84881 too. Not sure about at803x - the only SFP I know that uses
> that PHY doesn't make the PHY available to the host.

So which Kconfig options should I put down for v2? CONFIG_BCM84881_PHY
and CONFIG_MARVELL_PHY?

To avoid this "Please insert the name of your sound card" situation
reminiscent of the 90s, another thing which might be interesting to
explore would be for each PHY driver to have a stub portion always built
into the kernel, keeping an association between the phy_id/phy_id_mask
and the Kconfig information associated with it (Kconfig option, and
whether it was enabled or not).

This way we could eliminate the guesswork and the kernel would always
know and print to the user which driver, if any, could handle that PHY
ID, rather than rely on the user to know that there is a portion of the
PHY ID which needs to be masked out when searching the kernel sources
for a compatible driver.

Please see the attached patch an inelegant way in which I've actually
implemented this for the Marvell and BCM84881 drivers. My idea is to
move each driver's struct mdio_device_id to a separate stub file.
Ugly but definitely low in complexity. It produces this output:

[   64.515123] mv88e6085 d0032004.mdio-mii:12 sfp: PHY i2c:sfp:16 supports no link modes. Its driver, CONFIG_MARVELL_PHY, is compiled as module.
[   64.532233] sfp sfp: sfp_add_phy failed: -EINVAL

Let me know if something like this would be interesting to submit to
mainline.

[-- Attachment #2: 0001-net-phylink-pretty-print-Kconfig-name-and-status-of-.patch --]
[-- Type: text/x-diff, Size: 12459 bytes --]

From 1f2a514fe6095e586203af5ffd1b0653d72d1513 Mon Sep 17 00:00:00 2001
From: Vladimir Oltean <vladimir.oltean@nxp.com>
Date: Fri, 15 Nov 2024 18:08:56 +0200
Subject: [PATCH] net: phylink: pretty-print Kconfig name and status of PHY
 missing driver

Figuring out, based on the PHY ID, which driver is appropriate is not
always easy. This involves walking the source code and masking the PHY
ID we can get from sysfs or other places with the phy_driver phy_id_mask.

Typically users care about the PHY ID when there isn't a matching driver
already loaded. That's a problem, because exactly then is when the
kernel also has no idea what PHY driver might be adequate for a PHY ID.

My idea is to move each driver's struct mdio_device_id to a separate
stub file, always compiled into phylib. I've implemented this for the
Marvell and BCM84881 drivers, but it will have to scale for all PHY
drivers eventually. The good part is that it can be rolled out gradually,
and not all PHY drivers need to use this infrastructure from the get go.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 drivers/net/phy/Makefile           |  3 +
 drivers/net/phy/bcm84881.c         |  7 +--
 drivers/net/phy/bcm84881_stubs.c   | 10 ++++
 drivers/net/phy/bcm84881_stubs.h   |  3 +
 drivers/net/phy/marvell.c          | 28 +--------
 drivers/net/phy/marvell_stubs.c    | 32 ++++++++++
 drivers/net/phy/marvell_stubs.h    |  3 +
 drivers/net/phy/phy_driver_stubs.c | 96 ++++++++++++++++++++++++++++++
 drivers/net/phy/phy_driver_stubs.h | 17 ++++++
 drivers/net/phy/phylink.c          | 13 +++-
 include/linux/phy.h                |  2 +
 11 files changed, 181 insertions(+), 33 deletions(-)
 create mode 100644 drivers/net/phy/bcm84881_stubs.c
 create mode 100644 drivers/net/phy/bcm84881_stubs.h
 create mode 100644 drivers/net/phy/marvell_stubs.c
 create mode 100644 drivers/net/phy/marvell_stubs.h
 create mode 100644 drivers/net/phy/phy_driver_stubs.c
 create mode 100644 drivers/net/phy/phy_driver_stubs.h

diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile
index e6145153e837..123effc6fac9 100644
--- a/drivers/net/phy/Makefile
+++ b/drivers/net/phy/Makefile
@@ -14,6 +14,9 @@ endif
 # dedicated loadable module, so we bundle them all together into libphy.ko
 ifdef CONFIG_PHYLIB
 libphy-y			+= $(mdio-bus-y)
+libphy-y			+= phy_driver_stubs.o
+libphy-y			+= bcm84881_stubs.o
+libphy-y			+= marvell_stubs.o
 # the stubs are built-in whenever PHYLIB is built-in or module
 obj-y				+= stubs.o
 else
diff --git a/drivers/net/phy/bcm84881.c b/drivers/net/phy/bcm84881.c
index 97da3aee4942..436c028074ca 100644
--- a/drivers/net/phy/bcm84881.c
+++ b/drivers/net/phy/bcm84881.c
@@ -16,6 +16,8 @@
 #include <linux/module.h>
 #include <linux/phy.h>
 
+#include "bcm84881_stubs.h"
+
 enum {
 	MDIO_AN_C22 = 0xffe0,
 };
@@ -251,11 +253,6 @@ static struct phy_driver bcm84881_drivers[] = {
 
 module_phy_driver(bcm84881_drivers);
 
-/* FIXME: module auto-loading for Clause 45 PHYs seems non-functional */
-static struct mdio_device_id __maybe_unused bcm84881_tbl[] = {
-	{ 0xae025150, 0xfffffff0 },
-	{ },
-};
 MODULE_AUTHOR("Russell King");
 MODULE_DESCRIPTION("Broadcom BCM84881 PHY driver");
 MODULE_DEVICE_TABLE(mdio, bcm84881_tbl);
diff --git a/drivers/net/phy/bcm84881_stubs.c b/drivers/net/phy/bcm84881_stubs.c
new file mode 100644
index 000000000000..cb646c8b89fd
--- /dev/null
+++ b/drivers/net/phy/bcm84881_stubs.c
@@ -0,0 +1,10 @@
+#include "bcm84881_stubs.h"
+#include "phy_driver_stubs.h"
+
+/* FIXME: module auto-loading for Clause 45 PHYs seems non-functional */
+struct mdio_device_id __maybe_unused bcm84881_tbl[] = {
+	{ 0xae025150, 0xfffffff0 },
+	{ },
+};
+
+PHY_DRIVER_STUBS(bcm84881_tbl, CONFIG_BCM84881_PHY);
diff --git a/drivers/net/phy/bcm84881_stubs.h b/drivers/net/phy/bcm84881_stubs.h
new file mode 100644
index 000000000000..d28797596720
--- /dev/null
+++ b/drivers/net/phy/bcm84881_stubs.h
@@ -0,0 +1,3 @@
+#include <linux/mod_devicetable.h>
+
+extern struct mdio_device_id __maybe_unused bcm84881_tbl[];
diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c
index cd50cd6a7f75..b3313bb8681b 100644
--- a/drivers/net/phy/marvell.c
+++ b/drivers/net/phy/marvell.c
@@ -38,6 +38,8 @@
 #include <asm/irq.h>
 #include <linux/uaccess.h>
 
+#include "marvell_stubs.h"
+
 #define MII_MARVELL_PHY_PAGE		22
 #define MII_MARVELL_COPPER_PAGE		0x00
 #define MII_MARVELL_FIBER_PAGE		0x01
@@ -4143,30 +4145,4 @@ static struct phy_driver marvell_drivers[] = {
 
 module_phy_driver(marvell_drivers);
 
-static struct mdio_device_id __maybe_unused marvell_tbl[] = {
-	{ MARVELL_PHY_ID_88E1101, MARVELL_PHY_ID_MASK },
-	{ MARVELL_PHY_ID_88E3082, MARVELL_PHY_ID_MASK },
-	{ MARVELL_PHY_ID_88E1112, MARVELL_PHY_ID_MASK },
-	{ MARVELL_PHY_ID_88E1111, MARVELL_PHY_ID_MASK },
-	{ MARVELL_PHY_ID_88E1111_FINISAR, MARVELL_PHY_ID_MASK },
-	{ MARVELL_PHY_ID_88E1118, MARVELL_PHY_ID_MASK },
-	{ MARVELL_PHY_ID_88E1121R, MARVELL_PHY_ID_MASK },
-	{ MARVELL_PHY_ID_88E1145, MARVELL_PHY_ID_MASK },
-	{ MARVELL_PHY_ID_88E1149R, MARVELL_PHY_ID_MASK },
-	{ MARVELL_PHY_ID_88E1240, MARVELL_PHY_ID_MASK },
-	{ MARVELL_PHY_ID_88E1318S, MARVELL_PHY_ID_MASK },
-	{ MARVELL_PHY_ID_88E1116R, MARVELL_PHY_ID_MASK },
-	{ MARVELL_PHY_ID_88E1510, MARVELL_PHY_ID_MASK },
-	{ MARVELL_PHY_ID_88E1540, MARVELL_PHY_ID_MASK },
-	{ MARVELL_PHY_ID_88E1545, MARVELL_PHY_ID_MASK },
-	{ MARVELL_PHY_ID_88E3016, MARVELL_PHY_ID_MASK },
-	{ MARVELL_PHY_ID_88E6250_FAMILY, MARVELL_PHY_ID_MASK },
-	{ MARVELL_PHY_ID_88E6341_FAMILY, MARVELL_PHY_ID_MASK },
-	{ MARVELL_PHY_ID_88E6390_FAMILY, MARVELL_PHY_ID_MASK },
-	{ MARVELL_PHY_ID_88E6393_FAMILY, MARVELL_PHY_ID_MASK },
-	{ MARVELL_PHY_ID_88E1340S, MARVELL_PHY_ID_MASK },
-	{ MARVELL_PHY_ID_88E1548P, MARVELL_PHY_ID_MASK },
-	{ }
-};
-
 MODULE_DEVICE_TABLE(mdio, marvell_tbl);
diff --git a/drivers/net/phy/marvell_stubs.c b/drivers/net/phy/marvell_stubs.c
new file mode 100644
index 000000000000..f4770c9b38f7
--- /dev/null
+++ b/drivers/net/phy/marvell_stubs.c
@@ -0,0 +1,32 @@
+#include <linux/marvell_phy.h>
+
+#include "marvell_stubs.h"
+#include "phy_driver_stubs.h"
+
+struct mdio_device_id __maybe_unused marvell_tbl[] = {
+	{ MARVELL_PHY_ID_88E1101, MARVELL_PHY_ID_MASK },
+	{ MARVELL_PHY_ID_88E3082, MARVELL_PHY_ID_MASK },
+	{ MARVELL_PHY_ID_88E1112, MARVELL_PHY_ID_MASK },
+	{ MARVELL_PHY_ID_88E1111, MARVELL_PHY_ID_MASK },
+	{ MARVELL_PHY_ID_88E1111_FINISAR, MARVELL_PHY_ID_MASK },
+	{ MARVELL_PHY_ID_88E1118, MARVELL_PHY_ID_MASK },
+	{ MARVELL_PHY_ID_88E1121R, MARVELL_PHY_ID_MASK },
+	{ MARVELL_PHY_ID_88E1145, MARVELL_PHY_ID_MASK },
+	{ MARVELL_PHY_ID_88E1149R, MARVELL_PHY_ID_MASK },
+	{ MARVELL_PHY_ID_88E1240, MARVELL_PHY_ID_MASK },
+	{ MARVELL_PHY_ID_88E1318S, MARVELL_PHY_ID_MASK },
+	{ MARVELL_PHY_ID_88E1116R, MARVELL_PHY_ID_MASK },
+	{ MARVELL_PHY_ID_88E1510, MARVELL_PHY_ID_MASK },
+	{ MARVELL_PHY_ID_88E1540, MARVELL_PHY_ID_MASK },
+	{ MARVELL_PHY_ID_88E1545, MARVELL_PHY_ID_MASK },
+	{ MARVELL_PHY_ID_88E3016, MARVELL_PHY_ID_MASK },
+	{ MARVELL_PHY_ID_88E6250_FAMILY, MARVELL_PHY_ID_MASK },
+	{ MARVELL_PHY_ID_88E6341_FAMILY, MARVELL_PHY_ID_MASK },
+	{ MARVELL_PHY_ID_88E6390_FAMILY, MARVELL_PHY_ID_MASK },
+	{ MARVELL_PHY_ID_88E6393_FAMILY, MARVELL_PHY_ID_MASK },
+	{ MARVELL_PHY_ID_88E1340S, MARVELL_PHY_ID_MASK },
+	{ MARVELL_PHY_ID_88E1548P, MARVELL_PHY_ID_MASK },
+	{ }
+};
+
+PHY_DRIVER_STUBS(marvell_tbl, CONFIG_MARVELL_PHY);
diff --git a/drivers/net/phy/marvell_stubs.h b/drivers/net/phy/marvell_stubs.h
new file mode 100644
index 000000000000..2ba1f5af526f
--- /dev/null
+++ b/drivers/net/phy/marvell_stubs.h
@@ -0,0 +1,3 @@
+#include <linux/mod_devicetable.h>
+
+extern struct mdio_device_id __maybe_unused marvell_tbl[];
diff --git a/drivers/net/phy/phy_driver_stubs.c b/drivers/net/phy/phy_driver_stubs.c
new file mode 100644
index 000000000000..bffb9788d2b9
--- /dev/null
+++ b/drivers/net/phy/phy_driver_stubs.c
@@ -0,0 +1,96 @@
+#include <linux/phy.h>
+
+#include "phy_driver_stubs.h"
+
+struct phy_driver_stub {
+	u32 phy_id;
+	u32 phy_id_mask;
+	const char *kconfig_name;
+	const char *kconfig_status;
+	struct list_head list;
+};
+
+static LIST_HEAD(phy_driver_stubs);
+static DEFINE_MUTEX(phy_driver_stubs_lock);
+
+int phy_driver_stubs_register(const struct mdio_device_id *device_tbl,
+			      const char *kconfig_name,
+			      const char *kconfig_status)
+{
+	size_t i;
+
+	mutex_lock(&phy_driver_stubs_lock);
+
+	for (i = 0; device_tbl[i].phy_id_mask; i++) {
+		struct phy_driver_stub *stub;
+
+		stub = kzalloc(sizeof(*stub), GFP_KERNEL);
+		if (!stub) {
+			while (--i >= 0) {
+				stub = list_last_entry(&phy_driver_stubs,
+						       struct phy_driver_stub,
+						       list);
+				list_del(&stub->list);
+				kfree(stub);
+			}
+			mutex_unlock(&phy_driver_stubs_lock);
+			return -ENOMEM;
+		}
+
+		stub->phy_id = device_tbl[i].phy_id;
+		stub->phy_id_mask = device_tbl[i].phy_id_mask;
+		stub->kconfig_name = kconfig_name;
+		stub->kconfig_status = kconfig_status;
+		list_add_tail(&stub->list, &phy_driver_stubs);
+	}
+
+	mutex_unlock(&phy_driver_stubs_lock);
+
+	return 0;
+}
+
+const char *phy_library_driver_kconfig_name(u32 phy_id)
+{
+	const char *kconfig_name = NULL;
+	struct phy_driver_stub *stub;
+
+	mutex_lock(&phy_driver_stubs_lock);
+
+	list_for_each_entry(stub, &phy_driver_stubs, list) {
+		if ((phy_id & stub->phy_id_mask) != stub->phy_id)
+			continue;
+
+		/* Safe to return this pointer after unlocking the mutex,
+		 * because we never remove the stubs from memory
+		 */
+		kconfig_name = stub->kconfig_name;
+		break;
+	}
+
+	mutex_unlock(&phy_driver_stubs_lock);
+
+	return kconfig_name;
+}
+
+const char *phy_library_driver_kconfig_status(u32 phy_id)
+{
+	const char *kconfig_status = NULL;
+	struct phy_driver_stub *stub;
+
+	mutex_lock(&phy_driver_stubs_lock);
+
+	list_for_each_entry(stub, &phy_driver_stubs, list) {
+		if ((phy_id & stub->phy_id_mask) != stub->phy_id)
+			continue;
+
+		/* Safe to return this pointer after unlocking the mutex,
+		 * because we never remove the stubs from memory
+		 */
+		kconfig_status = stub->kconfig_status;
+		break;
+	}
+
+	mutex_unlock(&phy_driver_stubs_lock);
+
+	return kconfig_status;
+}
diff --git a/drivers/net/phy/phy_driver_stubs.h b/drivers/net/phy/phy_driver_stubs.h
new file mode 100644
index 000000000000..66b1165a658b
--- /dev/null
+++ b/drivers/net/phy/phy_driver_stubs.h
@@ -0,0 +1,17 @@
+#include <linux/mod_devicetable.h>
+#include <linux/module.h>
+
+int phy_driver_stubs_register(const struct mdio_device_id *device_tbl,
+			      const char *kconfig_name,
+			      const char *kconfig_status);
+
+#define PHY_DRIVER_STUBS(device_id_tbl, kconfig_name)			\
+static int __init phy_driver_register_stubs(void)			\
+{									\
+	return phy_driver_stubs_register(device_id_tbl,			\
+					 __stringify(kconfig_name),	\
+					 IS_MODULE(kconfig_name) ? "compiled as module" : \
+					 IS_BUILTIN(kconfig_name) ? "built into the kernel" : \
+					 "disabled");			\
+}									\
+module_init(phy_driver_register_stubs)
diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index efeff8733a52..ea8f8a47d11a 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -3220,8 +3220,17 @@ static int phylink_sfp_config_phy(struct phylink *pl, u8 mode,
 
 	linkmode_copy(support, phy->supported);
 	if (linkmode_empty(support)) {
-		phylink_err(pl, "PHY %s (id 0x%.8lx) supports no link modes. Maybe its specific PHY driver not loaded?\n",
-			    phydev_name(phy), (unsigned long)phy->phy_id);
+		const char *kconfig_name;
+
+		kconfig_name = phy_library_driver_kconfig_name(phy->phy_id);
+		if (kconfig_name) {
+			phylink_err(pl, "PHY %s supports no link modes. Its driver, %s, is %s.\n",
+				    phydev_name(phy), kconfig_name,
+				    phy_library_driver_kconfig_status(phy->phy_id));
+		} else {
+			phylink_err(pl, "PHY %s (id 0x%.8lx) supports no link modes, and appears to have no known driver\n",
+				    phydev_name(phy), (unsigned long)phy->phy_id);
+		}
 		return -EINVAL;
 	}
 
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 1e4127c495c0..c4ab5a1d225e 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -2214,5 +2214,7 @@ module_exit(phy_module_exit)
 
 bool phy_driver_is_genphy(struct phy_device *phydev);
 bool phy_driver_is_genphy_10g(struct phy_device *phydev);
+const char *phy_library_driver_kconfig_name(u32 phy_id);
+const char *phy_library_driver_kconfig_status(u32 phy_id);
 
 #endif /* __PHY_H */
-- 
2.43.0

Re: [PATCH net-next] net: phylink: improve phylink_sfp_config_phy() error message with empty supported

[Andrew Lunn]

On Fri, Nov 15, 2024 at 06:14:01PM +0200, Vladimir Oltean wrote:
> On Thu, Nov 14, 2024 at 06:33:00PM +0000, Russell King (Oracle) wrote:
> > On Thu, Nov 14, 2024 at 06:38:13PM +0100, Andrew Lunn wrote:
> > > > [   64.738270] mv88e6085 d0032004.mdio-mii:12 sfp: PHY i2c:sfp:16 (id 0x01410cc2) supports no link modes. Maybe its specific PHY driver not loaded?
> > > > [   64.769731] sfp sfp: sfp_add_phy failed: -EINVAL
> > > > 
> > > > Of course, there may be other reasons due to which phydev->supported is
> > > > empty, thus the use of the word "maybe", but I think the lack of a
> > > > driver would be the most common.
> > > 
> > > I think this is useful.
> > > 
> > > I only have a minor nitpick, maybe in the commit message mention which
> > > PHY drivers are typically used by SFPs, to point somebody who gets
> > > this message in the right direction. The Marvell driver is one. at803x
> > > i think is also used. Are then any others?
> > 
> > bcm84881 too. Not sure about at803x - the only SFP I know that uses
> > that PHY doesn't make the PHY available to the host.
> 
> So which Kconfig options should I put down for v2? CONFIG_BCM84881_PHY
> and CONFIG_MARVELL_PHY?
> 
> To avoid this "Please insert the name of your sound card" situation
> reminiscent of the 90s, another thing which might be interesting to
> explore would be for each PHY driver to have a stub portion always built
> into the kernel, keeping an association between the phy_id/phy_id_mask
> and the Kconfig information associated with it (Kconfig option, and
> whether it was enabled or not).

This might be useful in other ways, if we can make it work for every
driver. genphy somewhat breaks the usual device model, and that causes
us pain at times. fw_devlink gets confused by genphy, and users as
well. We have the issue of not knowing if genphy is to be used, or we
should wait around longer for the correct driver to load.

So i can see three use cases:

1) There is a driver for this hardware, it is just not being built

2) There is a driver for this hardware, it is being built, it has not
loaded yet.

3) There is no driver for this hardware, genphy is the fallback.

I would actually say 1) is not something we should solve at the PHY
driver layer, it is a generic problem for all drivers. We want some
Makefile support for extracting the MODULE_DEVICE_TABLE() for modules
which are not enabled, and some way to create a modules.disabled.alias
which module loading can look at and issue a warning. 2) i also think
is a generic problem. 3) is probably PHY specific, because i don't
know of any other case where there is a fallback driver.

	Andrew

Re: [PATCH net-next] net: phylink: improve phylink_sfp_config_phy() error message with empty supported

[Vladimir Oltean]

On Fri, Nov 15, 2024 at 06:51:00PM +0100, Andrew Lunn wrote:
> We want some Makefile support for extracting the MODULE_DEVICE_TABLE()
> for modules which are not enabled, and some way to create a
> modules.disabled.alias which module loading can look at and issue a
> warning.

Sadly, for me, automatically extracting MODULE_DEVICE_TABLE() from files
which are not compiled in C is science fiction.

Also, I need to sleep on the idea that creating an association between
device and missing driver might be useful kernel-wide. I'm not prepared
to handle the possibility where we make that happen, but it gets push
back and dropped.