Bug#553024: phylib: Support phy module autoloading
 

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.

Bug#553024: phylib: Support phy module autoloading
 

On Thu, 2010-04-01 at 05:34 +0100, Ben Hutchings wrote:
> 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.

It shouldn't be _that_ hard.

You could contrive a macro which you use inside the driver definition
and which takes the phy_id and phy_id_mask as arguments, and has the
side-effect of setting up the MODULE_DEVICE_TABLE data.

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

I'll tolerate the silly __u32 crap if I must for consistency, but
normally I prefer to write in C.

I did think about 'mdio:' for the module alias, but I decided that
'phy:' probably made more sense since these are PHY driver modules and
the number is the phy_id.

Kernel-doc is good though.

--
David Woodhouse Open Source Technology Centre
David.Woodhouse@intel.com Intel Corporation




--
To UNSUBSCRIBE, email to debian-kernel-REQUEST@lists.debian.org
with a subject of "unsubscribe". Trouble? Contact listmaster@lists.debian.org
Archive: 1270141428.3101.399.camel@macbook.infradead.org">h ttp://lists.debian.org/1270141428.3101.399.camel@macbook.infradead.org

Bug#553024: phylib: Support phy module autoloading
 

On Thu, Apr 01, 2010 at 06:03:48PM +0100, David Woodhouse wrote:
> On Thu, 2010-04-01 at 05:34 +0100, Ben Hutchings wrote:
[...]
> > 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/.
>
> I'll tolerate the silly __u32 crap if I must for consistency, but
> normally I prefer to write in C.
>
> I did think about 'mdio:' for the module alias, but I decided that
> 'phy:' probably made more sense since these are PHY driver modules and
> the number is the phy_id.
[...]

Many multi-layered communication standards have distinct PHY devices,
and they presumably have their own ID spaces. phylib deals only with
management of Ethernet PHYs over an MDIO bus, identified using MDIO
ID registers.

Ben.

--
Ben Hutchings
We get into the habit of living before acquiring the habit of thinking.
- Albert Camus



--
To UNSUBSCRIBE, email to debian-kernel-REQUEST@lists.debian.org
with a subject of "unsubscribe". Trouble? Contact listmaster@lists.debian.org
Archive: 20100401180511.GQ16821@decadent.org.uk">http://lists.debian.org/20100401180511.GQ16821@decadent.org.uk

Bug#553024: phylib: Support phy module autoloading
 

From: Ben Hutchings <ben@decadent.org.uk>
Date: Thu, 1 Apr 2010 19:05:12 +0100

> On Thu, Apr 01, 2010 at 06:03:48PM +0100, David Woodhouse wrote:
>> On Thu, 2010-04-01 at 05:34 +0100, Ben Hutchings wrote:
> [...]
>> > 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/.
>>
>> I'll tolerate the silly __u32 crap if I must for consistency, but
>> normally I prefer to write in C.
>>
>> I did think about 'mdio:' for the module alias, but I decided that
>> 'phy:' probably made more sense since these are PHY driver modules and
>> the number is the phy_id.
> [...]
>
> Many multi-layered communication standards have distinct PHY devices,
> and they presumably have their own ID spaces. phylib deals only with
> management of Ethernet PHYs over an MDIO bus, identified using MDIO
> ID registers.

Agreed, PHYs exist on so many different kinds of topologies, the ones
here are definitely specific to Ethernet and MDIO and therefore that
is probably the more useful basis for naming.

David can you freshen things up in this area and integrate whatever
you deem useful and immediate from Ben's patch? Whatever you submit
next I'd like to toss into net-next-2.6 so it can cook for a while
and maybe we'll backport it so that this bug can be fixed for good
upstream and then perhaps even in -stable.

Thanks!



--
To UNSUBSCRIBE, email to debian-kernel-REQUEST@lists.debian.org
with a subject of "unsubscribe". Trouble? Contact listmaster@lists.debian.org
Archive: 20100401.193810.172601735.davem@davemloft.net">htt p://lists.debian.org/20100401.193810.172601735.davem@davemloft.net

Bug#553024: phylib: Support phy module autoloading
 

On Thu, 2010-04-01 at 05:34 +0100, Ben Hutchings wrote:
> --- 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);

You forgot to increase the size of the 'modid' storage there, and it
needed to grow by one character. But that's OK. I forgot that
request_module() takes printf-style arguments. So now I've killed the
temporary 'modid' buffer altogether, and I just call
request_module(MDIO_MODULE_PREFIX ...). I dropped the ifdef, too.

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

Still tempted to add a %b format to the kernel's printf... some runtimes
have it.

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

That last sentence is a lie; I removed it. file2alias.c just ignores the
last entry in the array regardless of what it contains. I have no idea
why we use a 'terminator' in these arrays. Legacy, perhaps?

> static int do_phy_entry(const char *filename,
> - struct phy_device_id *id, char *alias)
> + struct mdio_device_id *id, char *alias)

I made that 'do_mdio_entry()' for cosmetic reasons.

> - sizeof(struct phy_device_id), "phy",
> + sizeof(struct mdio_device_id), "phy",

And that "mdio", not "phy", so that it works.

--
dwmw2




--
To UNSUBSCRIBE, email to debian-kernel-REQUEST@lists.debian.org
with a subject of "unsubscribe". Trouble? Contact listmaster@lists.debian.org
Archive: 1270204703.3101.2368.camel@macbook.infradead.org"> http://lists.debian.org/1270204703.3101.2368.camel@macbook.infradead.org

Bug#553024: phylib: Support phy module autoloading
 

On Thu, 2010-04-01 at 19:38 -0700, David Miller wrote:
> Whatever you submit
> next I'd like to toss into net-next-2.6 so it can cook for a while
> and maybe we'll backport it so that this bug can be fixed for good
> upstream and then perhaps even in -stable.

When backporting, note that you'll need s/PHY_ID_BCMAC131/0x0143bc70/ in
the patch to broadcom.c, because 2.6.32 didn't have a definition for
that.

There's also trivial change in phy_device_create() which will make the
patch fail to apply, but git-cherry-pick copes.

--
David Woodhouse Open Source Technology Centre
David.Woodhouse@intel.com Intel Corporation




--
To UNSUBSCRIBE, email to debian-kernel-REQUEST@lists.debian.org
with a subject of "unsubscribe". Trouble? Contact listmaster@lists.debian.org
Archive: 1270206895.3101.2467.camel@macbook.infradead.org"> http://lists.debian.org/1270206895.3101.2467.camel@macbook.infradead.org

Bug#553024: phylib: Support phy module autoloading
 

On Fri, 2010-04-02 at 12:14 +0100, David Woodhouse wrote:
> On Thu, 2010-04-01 at 19:38 -0700, David Miller wrote:
> > Whatever you submit
> > next I'd like to toss into net-next-2.6 so it can cook for a while
> > and maybe we'll backport it so that this bug can be fixed for good
> > upstream and then perhaps even in -stable.
>
> When backporting, note that you'll need s/PHY_ID_BCMAC131/0x0143bc70/ in
> the patch to broadcom.c, because 2.6.32 didn't have a definition for
> that.
>
> There's also trivial change in phy_device_create() which will make the
> patch fail to apply, but git-cherry-pick copes.

Thanks again, David.

Ben.

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