netdev - Re: [PATCH 1/4] netdev/phy: Handle IEEE802.3 clause 45 Ethernet PHYs

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 for Android: free password hash cracker in your pocket

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

Message-Id: <20120625.153440.17010814246237639.davem@davemloft.net>
Date:	Mon, 25 Jun 2012 15:34:40 -0700 (PDT)
From:	David Miller <davem@...emloft.net>
To:	ddaney.cavm@...il.com
Cc:	grant.likely@...retlab.ca, rob.herring@...xeda.com,
	devicetree-discuss@...ts.ozlabs.org, netdev@...r.kernel.org,
	linux-kernel@...r.kernel.org, linux-mips@...ux-mips.org,
	afleming@...il.com, david.daney@...ium.com
Subject: Re: [PATCH 1/4] netdev/phy: Handle IEEE802.3 clause 45 Ethernet
 PHYs

From: David Daney <ddaney.cavm@...il.com>
Date: Fri, 22 Jun 2012 17:24:13 -0700

> From: David Daney <david.daney@...ium.com>
> 
> The IEEE802.3 clause 45 MDIO bus protocol allows for directly
> addressing PHY registers using a 21 bit address, and is used by many
> 10G Ethernet PHYS.  Already existing is the ability of MDIO bus
> drivers to use clause 45, with the MII_ADDR_C45 flag.  Here we add
> struct phy_c45_device_ids to hold the device identifier registers
> present in clause 45. struct phy_device gets a couple of new fields:
> c45_ids to hold the identifiers and is_c45 to signal that it is clause
> 45.
> 
> Normally the MII_ADDR_C45 flag is ORed with the register address to
> indicate a clause 45 transaction.  Here we also use this flag in the
> *device* address passed to get_phy_device() to indicate that probing
> should be done with clause 45 transactions.
> 
> EXPORT phy_device_create() so that the follow-on patch to of_mdio.c
> can use it to create phy devices for PHYs, that have non-standard
> device identifier registers, based on the device tree bindings.
> 
> Signed-off-by: David Daney <david.daney@...ium.com>

I see no value in having two ways to say that clause-45 transactions
should be used.

Either make it a PHY device attribute, or specify it in the address
in the register accesses, but not both.

Also your patch is full of coding style errors, I simply couldn't
stomache applying this even if I agreed with the substance of the
changes:

> +	     i < ARRAY_SIZE(c45_ids->device_ids) &&
> +		     c45_ids->devices_in_package == 0;

c45_ids on the second line should line up with the initial 'i'
on the first line.

> +		c45_ids->devices_in_package = (phy_reg & 0xffff) << 16;
> +
> +
> +		reg_addr = MII_ADDR_C45 | i << 16 | 5;

There is not reason in the world to have two empty lines there, it
looks awful.

> +		/*
> +		 * If mostly Fs, there is no device there,
> +		 * let's get out of here.
> +		 */

Format comments:

	/* Like
	 * this.
	 */

Not.

	/*
	 * Like
	 * this.
	 */

> +		c45_ids->device_ids[i] = (phy_reg & 0xffff) << 16;
> +
> +
> +		reg_addr = MII_ADDR_C45 | i << 16 | MII_PHYSID2;

Two empty lines.  This is extremely irritating, it looks like you
had some kind of debugging code here and then were very lazy about
removing it.

> +/*
> + * Or MII_ADDR_C45 into regnum for read/write on mii_bus to enable the
> + * 21 bit IEEE 802.3ae clause 45 addressing mode used by 10GIGE phy
> + * chips.  Also may be ORed into the device address in
> + * get_phy_device().
> + */

Comment formatting.

> +/*
> + * phy_c45_device_ids: 802.3-c45 Device Identifiers
> + *
> + * devices_in_package: Bit vector of devices present.
> + * device_ids: The device identifer for each present device.
> + */

If you're going to list the struct members use the correct kerneldoc
format to do so.

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