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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <1270204703.3101.2368.camel@macbook.infradead.org>
Date:	Fri, 02 Apr 2010 11:38:23 +0100
From:	David Woodhouse <dwmw2@...radead.org>
To:	Ben Hutchings <ben@...adent.org.uk>
Cc:	davem@...emloft.net, netdev@...r.kernel.org, 553024@...s.debian.org
Subject: Re: [PATCH 1/2] 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 from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html