Re: [PATCH 1/2] phylib: Support phy module autoloading

[Ben Hutchings <ben@decadent.org.uk>]

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

On Wed, 2010-03-31 at 02:18 +0100, David Woodhouse wrote:
> We don't use the normal hotplug mechanism because it doesn't work. It will
> load the module some time after the device appears, but that's not good
> enough for us -- we need the driver loaded _immediately_ because otherwise
> the NIC driver may just abort and then the phy 'device' goes away.
> 
> Instead, we just issue a request_module() directly in phy_device_create().
[...]

Thanks for doing this, David.  I had a stab at it earlier when this
problem was reported in Debian <http://bugs.debian.org/553024>.  I
didn't complete this because (a) I didn't understand all the details of
adding new device table type, and (b) I tried to avoid duplicating
information, which turns out to be rather difficult in modules with
multiple drivers.

Since you've dealt with (a), and (b) is not really as important, I would
just like to suggest some minor changes to your patch 1 (see below).
Feel free to fold them in.  Your patch 2 would then need the
substitutions s/phy_device_id/mdio_device_id/; s/TABLE(phy/TABLE(mdio/.

Ben.

From: Ben Hutchings <ben@decadent.org.uk>
Date: Thu, 1 Apr 2010 05:03:02 +0100
Subject: [PATCH] phylib: Minor cleanup of phylib autoloading

Refer to MDIO, consistent with other module aliases using bus names.
Change type names to __u32, consistent with the rest of the file.
Add kernel-doc comment to struct mdio_device_id.

Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
---
 drivers/net/phy/phy_device.c    |    2 +-
 include/linux/mod_devicetable.h |   22 ++++++++++++++--------
 scripts/mod/file2alias.c        |    8 ++++----
 3 files changed, 19 insertions(+), 13 deletions(-)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index b35ec7e..b0e54b4 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -188,7 +188,7 @@ struct phy_device* phy_device_create(struct mii_bus *bus, int addr, int phy_id)
 	   around for long enough for the driver to get loaded. With
 	   MDIO, the NIC driver will get bored and give up as soon
 	   as it finds that there's no driver _already_ loaded. */
-	sprintf(modid, "phy:" PHYID_FMT, PHYID_ARGS(phy_id));
+	sprintf(modid, MDIO_MODULE_PREFIX MDIO_ID_FMT, MDIO_ID_ARGS(phy_id));
 	request_module(modid);
 #endif
 
diff --git a/include/linux/mod_devicetable.h b/include/linux/mod_devicetable.h
index 0c3e300..55f1f9c 100644
--- a/include/linux/mod_devicetable.h
+++ b/include/linux/mod_devicetable.h
@@ -474,10 +474,10 @@ struct platform_device_id {
 			__attribute__((aligned(sizeof(kernel_ulong_t))));
 };
 
-#define PHY_MODULE_PREFIX	"phy:"
+#define MDIO_MODULE_PREFIX	"mdio:"
 
-#define PHYID_FMT "%d%d%d%d%d%d%d%d%d%d%d%d%d%d%d%d%d%d%d%d%d%d%d%d%d%d%d%d%d%d%d%d"
-#define PHYID_ARGS(_id) \
+#define MDIO_ID_FMT "%d%d%d%d%d%d%d%d%d%d%d%d%d%d%d%d%d%d%d%d%d%d%d%d%d%d%d%d%d%d%d%d"
+#define MDIO_ID_ARGS(_id) \
 	(_id)>>31, ((_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, \
@@ -487,11 +487,17 @@ struct platform_device_id {
 	((_id)>>7) & 1, ((_id)>>6) & 1, ((_id)>>5) & 1, ((_id)>>4) & 1, \
 	((_id)>>3) & 1, ((_id)>>2) & 1, ((_id)>>1) & 1, (_id) & 1
 
-
-
-struct phy_device_id {
-	uint32_t phy_id;
-	uint32_t phy_id_mask;
+/**
+ * struct mdio_device_id - identifies PHY devices on an MDIO/MII bus
+ * @phy_id: The result of
+ *     (mdio_read(&MII_PHYSID1) << 16 | mdio_read(&PHYSID2)) & @phy_id_mask
+ *     for this PHY type
+ * @phy_id_mask: Defines the significant bits of @phy_id.  A value of 0
+ *     is used to terminate an array of struct mdio_device_id.
+ */
+struct mdio_device_id {
+	__u32 phy_id;
+	__u32 phy_id_mask;
 };
 
 #endif /* LINUX_MOD_DEVICETABLE_H */
diff --git a/scripts/mod/file2alias.c b/scripts/mod/file2alias.c
index b412185..0e08b8b 100644
--- a/scripts/mod/file2alias.c
+++ b/scripts/mod/file2alias.c
@@ -797,7 +797,7 @@ static int do_platform_entry(const char *filename,
 }
 
 static int do_phy_entry(const char *filename,
-			struct phy_device_id *id, char *alias)
+			struct mdio_device_id *id, char *alias)
 {
 	char str[33];
 	int i;
@@ -813,7 +813,7 @@ static int do_phy_entry(const char *filename,
 			str[i] = '0';
 	}
 
-	sprintf(alias, PHY_MODULE_PREFIX "%s", str);
+	sprintf(alias, MDIO_MODULE_PREFIX "%s", str);
 	return 1;
 }
 
@@ -964,9 +964,9 @@ void handle_moddevtable(struct module *mod, struct elf_info *info,
 		do_table(symval, sym->st_size,
 			 sizeof(struct platform_device_id), "platform",
 			 do_platform_entry, mod);
-	else if (sym_is(symname, "__mod_phy_device_table"))
+	else if (sym_is(symname, "__mod_mdio_device_table"))
 		do_table(symval, sym->st_size,
-			 sizeof(struct phy_device_id), "phy",
+			 sizeof(struct mdio_device_id), "phy",
 			 do_phy_entry, mod);
 	free(zeros);
 }
-- 
1.7.0.3


-- 
Ben Hutchings
Once a job is fouled up, anything done to improve it makes it worse.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]