netdev - Re: [PATCH 3/3] drivers/net/ipg.c: fix horrible mdio_read and

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]

Date:	Tue, 8 Jan 2008 21:47:44 +0100
From:	Francois Romieu <romieu@...zoreil.com>
To:	linux@...izon.com
Cc:	netdev@...r.kernel.org, akpm@...ux-foundation.org,
	davem@...emloft.net
Subject: Re: [PATCH 3/3] drivers/net/ipg.c: fix horrible mdio_read and
	_write

linux@...izon.com <linux@...izon.com> :
[...]
> diff --git a/drivers/net/ipg.c b/drivers/net/ipg.c
> index 3860fcd..b3d3fc8 100644
> --- a/drivers/net/ipg.c
> +++ b/drivers/net/ipg.c
> @@ -202,12 +202,31 @@ static u16 read_phy_bit(void __iomem * ioaddr, u8 phyctrlpolarity)
>  }
>  
>  /*
> + * Transmit the given bits, MSB-first, through the MgmtData bit (bit 1)
> + * of the PhyCtrl register. 1 <= len <= 32.  "ioaddr" is the register
> + * address, and "otherbits" are the values of the other bits.
> + */
> +static void mdio_write_bits(void __iomem *ioaddr, u8 otherbits, u32 data, unsigned len)
> +{
> +	otherbits |= IPG_PC_MGMTDIR;
> +	do {
> +		/* IPG_PC_MGMTDATA is a power of 2; compiler knows to shift */
> +		u8 d = ((data >> --len) & 1) * IPG_PC_MGMTDATA;
> +		/* + rather than | lets compiler microoptimize better */
> +		ipg_drive_phy_ctl_low_high(ioaddr, d + otherbits);
> +	} while (len);

Imho something is not quite right when the code needs a comment every line
and I am mildly convinced that we really want to honk an "optimizing mdio
methods is ok" signal around.

"while (len--) {" is probably more akpm-ish btw.

[...]
>  static int mdio_read(struct net_device * dev, int phy_id, int phy_reg)
>  {
>  	void __iomem *ioaddr = ipg_ioaddr(dev);
> +	u8 const polarity = ipg_r8(PHY_CTRL) &
> +			(IPG_PC_DUPLEX_POLARITY | IPG_PC_LINK_POLARITY);

(IPG_PC_DUPLEX_POLARITY | IPG_PC_LINK_POLARITY) appears twice. I would not
mind a #define for it.

> @@ -221,75 +240,30 @@ static int mdio_read(struct net_device * dev, int phy_id, int phy_reg)
[...]
> -	for (i = 0; i < p[6].len; i++) {
> -		p[6].field |=
> -		    (read_phy_bit(ioaddr, polarity) << (p[6].len - 1 - i));
> -	}
> +	send_three_state(ioaddr, polarity);	/* TA first bit */
> +	(void)read_phy_bit(ioaddr, polarity);		/* TA second bit */
> +
> +	for (i = 0; i < 16; i++)
> +		data += data + read_phy_bit(ioaddr, polarity);
                ^^^^^^^^^^^^
Huh ?

> @@ -299,11 +273,13 @@ static int mdio_read(struct net_device * dev, int phy_id, int phy_reg)
>  static void mdio_write(struct net_device *dev, int phy_id, int phy_reg, int val)
[...]
> +	mdio_write_bits(ioaddr, polarity, GMII_PREAMBLE, 32);
> +	mdio_write_bits(ioaddr, polarity, GMII_ST << 14 | GMII_WRITE << 12 |
> +	                                  phy_id << 7 | phy_reg << 2 |
> +					  0x2, 16);

Use the 80 cols luke:
	                                  phy_id << 7 | phy_reg << 2 | 0x2, 16);

It is a nice improvement otherwise.

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