| 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
| ||
|
Message-ID: <20080108233349.21615.qmail@science.horizon.com>
Date: 8 Jan 2008 18:33:49 -0500
From: linux@...izon.com
To: linux@...izon.com, romieu@...zoreil.com
Cc: akpm@...ux-foundation.org, davem@...emloft.net,
netdev@...r.kernel.org
Subject: Re: [PATCH 3/3] drivers/net/ipg.c: fix horrible mdio_read and _write
>> + 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.
Oh, but those are SPACE-saving optimiztions. :-)
I know it's not time-critical; it's really pure hack value, but is it
that evil?
> "while (len--) {" is probably more akpm-ish btw.
Well spotted.
[...]
>> 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.
I'm hardly going to go to war over over the matter, but actually I disagree.
There's a non-zero mental cost to keeping track of an additional name,
and when it's only used two times, and is pretty simple, I think reducing
the number of layers of #defines to understand is a positive advantage.
The above reads "the two polarity bits from the PHY_CTRL register"
to a person who's never read ipg.h. Adding IPG_PC_POLARITY_BITS just
requires mentally dereferencing another layer of pointers.
Think of it as a function small enough that it can be inlined.
>> @@ -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 ?
Okay, I guess you prefer
+ data = 2*data + read_phy_bit(ioaddr, polarity);
That's only one character longer and easier to understand.
Or even four characters:
+ data = (data<<1) + read_phy_bit(ioaddr, polarity);
That's just the synonym that happened to come out of my fingers at the
time. There's no particular meaning to it.
>> @@ -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);
Good spotting, thanks.
Here's a revised patch:
drivers/net/ipg.c: Fixed style problems that AKPM noticed.
(And a few more while at it. Including an actual bug in enabling multicast
due to & vs. && confusion.)
diff --git a/drivers/net/ipg.c b/drivers/net/ipg.c
index 3860fcd..fb69374 100644
--- a/drivers/net/ipg.c
+++ b/drivers/net/ipg.c
@@ -188,9 +188,9 @@ static void send_end(void __iomem *ioaddr, u8 phyctrlpolarity)
phyctrlpolarity) & IPG_PC_RSVD_MASK, PHY_CTRL);
}
-static u16 read_phy_bit(void __iomem * ioaddr, u8 phyctrlpolarity)
+static unsigned read_phy_bit(void __iomem * ioaddr, u8 phyctrlpolarity)
{
- u16 bit_data;
+ unsigned bit_data;
ipg_write_phy_ctl(ioaddr, IPG_PC_MGMTCLK_LO | phyctrlpolarity);
@@ -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;
+ while (len--) {
+ /* IPG_PC_MGMTDATA is a power of 2; compiler knows to shift */
+ u8 d = ((data >> len) & 1) * IPG_PC_MGMTDATA;
+ /* + rather than | allows slight code size microoptimization */
+ ipg_drive_phy_ctl_low_high(ioaddr, d + otherbits);
+ }
+}
+
+/*
* Read a register from the Physical Layer device located
* on the IPG NIC, using the IPG PHYCTRL register.
*/
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);
+ unsigned i, data = 0;
/*
* The GMII mangement frame structure for a read is as follows:
*
@@ -218,78 +237,34 @@ static int mdio_read(struct net_device * dev, int phy_id, int phy_reg)
* A = bit of Physical Layer device address (MSB first)
* R = bit of register address (MSB first)
* z = High impedance state
- * D = bit of read data (MSB first)
+ * 0 = preamble bit sent by PHY
+ * D = bit of read data (MSB first), sent by PHY
*
* Transmission order is 'Preamble' field first, bits transmitted
- * left to right (first to last).
+ * left to right (msbit-first).
*/
- struct {
- u32 field;
- unsigned int len;
- } p[] = {
- { GMII_PREAMBLE, 32 }, /* Preamble */
- { GMII_ST, 2 }, /* ST */
- { GMII_READ, 2 }, /* OP */
- { phy_id, 5 }, /* PHYAD */
- { phy_reg, 5 }, /* REGAD */
- { 0x0000, 2 }, /* TA */
- { 0x0000, 16 }, /* DATA */
- { 0x0000, 1 } /* IDLE */
- };
- unsigned int i, j;
- u8 polarity, data;
-
- polarity = ipg_r8(PHY_CTRL);
- polarity &= (IPG_PC_DUPLEX_POLARITY | IPG_PC_LINK_POLARITY);
-
- /* Create the Preamble, ST, OP, PHYAD, and REGAD field. */
- for (j = 0; j < 5; j++) {
- for (i = 0; i < p[j].len; i++) {
- /* For each variable length field, the MSB must be
- * transmitted first. Rotate through the field bits,
- * starting with the MSB, and move each bit into the
- * the 1st (2^1) bit position (this is the bit position
- * corresponding to the MgmtData bit of the PhyCtrl
- * register for the IPG).
- *
- * Example: ST = 01;
- *
- * First write a '0' to bit 1 of the PhyCtrl
- * register, then write a '1' to bit 1 of the
- * PhyCtrl register.
- *
- * To do this, right shift the MSB of ST by the value:
- * [field length - 1 - #ST bits already written]
- * then left shift this result by 1.
- */
- data = (p[j].field >> (p[j].len - 1 - i)) << 1;
- data &= IPG_PC_MGMTDATA;
- data |= polarity | IPG_PC_MGMTDIR;
-
- ipg_drive_phy_ctl_low_high(ioaddr, data);
- }
- }
-
- send_three_state(ioaddr, polarity);
-
- read_phy_bit(ioaddr, polarity);
+ mdio_write_bits(ioaddr, polarity, GMII_PREAMBLE, 32);
+ mdio_write_bits(ioaddr, polarity, GMII_ST<<12 | GMII_READ << 10 |
+ phy_id << 5 | phy_reg, 14);
/*
* For a read cycle, the bits for the next two fields (TA and
* DATA) are driven by the PHY (the IPG reads these bits).
*/
- 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 = 2*data + read_phy_bit(ioaddr, polarity);
+ /* Trailing idle */
send_three_state(ioaddr, polarity);
send_three_state(ioaddr, polarity);
send_three_state(ioaddr, polarity);
send_end(ioaddr, polarity);
/* Return the value of the DATA field. */
- return p[6].field;
+ return data;
}
/*
@@ -299,11 +274,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)
{
void __iomem *ioaddr = ipg_ioaddr(dev);
+ u8 const polarity = ipg_r8(PHY_CTRL) &
+ (IPG_PC_DUPLEX_POLARITY | IPG_PC_LINK_POLARITY);
/*
- * The GMII mangement frame structure for a read is as follows:
+ * The GMII mangement frame structure for a write is as follows:
*
* |Preamble|st|op|phyad|regad|ta| data |idle|
- * |< 32 1s>|01|10|AAAAA|RRRRR|z0|DDDDDDDDDDDDDDDD|z |
+ * |< 32 1s>|01|01|AAAAA|RRRRR|10|DDDDDDDDDDDDDDDD|z |
*
* <32 1s> = 32 consecutive logic 1 values
* A = bit of Physical Layer device address (MSB first)
@@ -312,66 +289,17 @@ static void mdio_write(struct net_device *dev, int phy_id, int phy_reg, int val)
* D = bit of write data (MSB first)
*
* Transmission order is 'Preamble' field first, bits transmitted
- * left to right (first to last).
+ * left to right (msbit-first).
*/
- struct {
- u32 field;
- unsigned int len;
- } p[] = {
- { GMII_PREAMBLE, 32 }, /* Preamble */
- { GMII_ST, 2 }, /* ST */
- { GMII_WRITE, 2 }, /* OP */
- { phy_id, 5 }, /* PHYAD */
- { phy_reg, 5 }, /* REGAD */
- { 0x0002, 2 }, /* TA */
- { val & 0xffff, 16 }, /* DATA */
- { 0x0000, 1 } /* IDLE */
- };
- unsigned int i, j;
- u8 polarity, data;
-
- polarity = ipg_r8(PHY_CTRL);
- polarity &= (IPG_PC_DUPLEX_POLARITY | IPG_PC_LINK_POLARITY);
-
- /* Create the Preamble, ST, OP, PHYAD, and REGAD field. */
- for (j = 0; j < 7; j++) {
- for (i = 0; i < p[j].len; i++) {
- /* For each variable length field, the MSB must be
- * transmitted first. Rotate through the field bits,
- * starting with the MSB, and move each bit into the
- * the 1st (2^1) bit position (this is the bit position
- * corresponding to the MgmtData bit of the PhyCtrl
- * register for the IPG).
- *
- * Example: ST = 01;
- *
- * First write a '0' to bit 1 of the PhyCtrl
- * register, then write a '1' to bit 1 of the
- * PhyCtrl register.
- *
- * To do this, right shift the MSB of ST by the value:
- * [field length - 1 - #ST bits already written]
- * then left shift this result by 1.
- */
- data = (p[j].field >> (p[j].len - 1 - i)) << 1;
- data &= IPG_PC_MGMTDATA;
- data |= polarity | IPG_PC_MGMTDIR;
-
- ipg_drive_phy_ctl_low_high(ioaddr, data);
- }
- }
+ 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);
+ mdio_write_bits(ioaddr, polarity, val, 16); /* DATA */
/* The last cycle is a tri-state, so read from the PHY. */
- for (j = 7; j < 8; j++) {
- for (i = 0; i < p[j].len; i++) {
- ipg_write_phy_ctl(ioaddr, IPG_PC_MGMTCLK_LO | polarity);
-
- p[j].field |= ((ipg_r8(PHY_CTRL) &
- IPG_PC_MGMTDATA) >> 1) << (p[j].len - 1 - i);
-
- ipg_write_phy_ctl(ioaddr, IPG_PC_MGMTCLK_HI | polarity);
- }
- }
+ ipg_write_phy_ctl(ioaddr, IPG_PC_MGMTCLK_LO | polarity);
+ (void)ipg_r8(PHY_CTRL);
+ ipg_write_phy_ctl(ioaddr, IPG_PC_MGMTCLK_HI | polarity);
}
/* Set LED_Mode JES20040127EEPROM */
@@ -379,18 +307,17 @@ static void ipg_set_led_mode(struct net_device *dev)
{
struct ipg_nic_private *sp = netdev_priv(dev);
void __iomem *ioaddr = sp->ioaddr;
- u32 mode;
+ u32 mode = ipg_r32(ASIC_CTRL);
- mode = ipg_r32(ASIC_CTRL);
mode &= ~(IPG_AC_LED_MODE_BIT_1 | IPG_AC_LED_MODE | IPG_AC_LED_SPEED);
- if ((sp->LED_Mode & 0x03) > 1)
- mode |= IPG_AC_LED_MODE_BIT_1; /* Write Asic Control Bit 29 */
-
- if ((sp->LED_Mode & 0x01) == 1)
+ if (sp->LED_Mode & 0x01)
mode |= IPG_AC_LED_MODE; /* Write Asic Control Bit 14 */
- if ((sp->LED_Mode & 0x08) == 8)
+ if (sp->LED_Mode & 0x02)
+ mode |= IPG_AC_LED_MODE_BIT_1; /* Write Asic Control Bit 29 */
+
+ if (sp->LED_Mode & 0x08)
mode |= IPG_AC_LED_SPEED; /* Write Asic Control Bit 27 */
ipg_w32(mode, ASIC_CTRL);
@@ -401,11 +328,13 @@ static void ipg_set_phy_set(struct net_device *dev)
{
struct ipg_nic_private *sp = netdev_priv(dev);
void __iomem *ioaddr = sp->ioaddr;
- int physet;
+ u8 physet = ipg_r8(PHY_SET);
+ u8 newset = sp->LED_Mode >> 4;
- physet = ipg_r8(PHY_SET);
- physet &= ~(IPG_PS_MEM_LENB9B | IPG_PS_MEM_LEN9 | IPG_PS_NON_COMPDET);
- physet |= ((sp->LED_Mode & 0x70) >> 4);
+ /* Change three bits of physet to values in newset */
+ newset ^= physet;
+ newset &= (IPG_PS_MEM_LENB9B | IPG_PS_MEM_LEN9 | IPG_PS_NON_COMPDET);
+ physet ^= newset;
ipg_w8(physet, PHY_SET);
}
@@ -444,7 +373,7 @@ static int ipg_find_phyaddr(struct net_device *dev)
unsigned int phyaddr, i;
for (i = 0; i < 32; i++) {
- u32 status;
+ unsigned status;
/* Search for the correct PHY address among 32 possible. */
phyaddr = (IPG_NIC_PHY_ADDRESS + i) % 32;
@@ -470,10 +399,7 @@ static int ipg_config_autoneg(struct net_device *dev)
{
struct ipg_nic_private *sp = netdev_priv(dev);
void __iomem *ioaddr = sp->ioaddr;
- unsigned int txflowcontrol;
- unsigned int rxflowcontrol;
- unsigned int fullduplex;
- unsigned int gig;
+ bool txflowcontrol, rxflowcontrol, fullduplex, gig;
u32 mac_ctrl_val;
u32 asicctrl;
u8 phyctrl;
@@ -487,10 +413,10 @@ static int ipg_config_autoneg(struct net_device *dev)
/* Set flags for use in resolving auto-negotation, assuming
* non-1000Mbps, half duplex, no flow control.
*/
- fullduplex = 0;
- txflowcontrol = 0;
- rxflowcontrol = 0;
- gig = 0;
+ fullduplex = false;
+ txflowcontrol = false;
+ rxflowcontrol = false;
+ gig = false;
/* To accomodate a problem in 10Mbps operation,
* set a global flag if PHY running in 10Mbps mode.
@@ -512,7 +438,7 @@ static int ipg_config_autoneg(struct net_device *dev)
break;
case IPG_PC_LINK_SPEED_1000MBPS:
printk("1000Mbps.\n");
- gig = 1;
+ gig = true;
break;
default:
printk("undefined!\n");
@@ -520,19 +446,20 @@ static int ipg_config_autoneg(struct net_device *dev)
}
if (phyctrl & IPG_PC_DUPLEX_STATUS) {
- fullduplex = 1;
- txflowcontrol = 1;
- rxflowcontrol = 1;
+ fullduplex = true;
+ txflowcontrol = true;
+ rxflowcontrol = true;
}
/* Configure full duplex, and flow control. */
- if (fullduplex == 1) {
+ if (fullduplex) {
/* Configure IPG for full duplex operation. */
printk(KERN_INFO "%s: setting full duplex, ", dev->name);
mac_ctrl_val |= IPG_MC_DUPLEX_SELECT_FD;
- if (txflowcontrol == 1) {
+ /* FIXME: Does this variable always equal fullduplex? */
+ if (txflowcontrol) {
printk("TX flow control");
mac_ctrl_val |= IPG_MC_TX_FLOW_CONTROL_ENABLE;
} else {
@@ -540,7 +467,7 @@ static int ipg_config_autoneg(struct net_device *dev)
mac_ctrl_val &= ~IPG_MC_TX_FLOW_CONTROL_ENABLE;
}
- if (rxflowcontrol == 1) {
+ if (rxflowcontrol) {
printk(", RX flow control.");
mac_ctrl_val |= IPG_MC_RX_FLOW_CONTROL_ENABLE;
} else {
@@ -568,9 +495,8 @@ static int ipg_config_autoneg(struct net_device *dev)
static void ipg_nic_set_multicast_list(struct net_device *dev)
{
void __iomem *ioaddr = ipg_ioaddr(dev);
- struct dev_mc_list *mc_list_ptr;
- unsigned int hashindex;
- u32 hashtable[2];
+ struct dev_mc_list *mc;
+ u32 hashtable[2] = { 0, 0 };
u8 receivemode;
IPG_DEBUG_MSG("_nic_set_multicast_list\n");
@@ -581,52 +507,34 @@ static void ipg_nic_set_multicast_list(struct net_device *dev)
/* NIC to be configured in promiscuous mode. */
receivemode = IPG_RM_RECEIVEALLFRAMES;
} else if ((dev->flags & IFF_ALLMULTI) ||
- (dev->flags & IFF_MULTICAST &
- (dev->mc_count > IPG_MULTICAST_HASHTABLE_SIZE))) {
+ (dev->flags & IFF_MULTICAST &&
+ (dev->mc_count > 2*IPG_MULTICAST_HASHTABLE_SIZE))) {
/* NIC to be configured to receive all multicast
* frames. */
receivemode |= IPG_RM_RECEIVEMULTICAST;
- } else if (dev->flags & IFF_MULTICAST & (dev->mc_count > 0)) {
+ } else if (dev->flags & IFF_MULTICAST && (dev->mc_count > 0)) {
/* NIC to be configured to receive selected
* multicast addresses. */
receivemode |= IPG_RM_RECEIVEMULTICASTHASH;
- }
-
- /* Calculate the bits to set for the 64 bit, IPG HASHTABLE.
- * The IPG applies a cyclic-redundancy-check (the same CRC
- * used to calculate the frame data FCS) to the destination
- * address all incoming multicast frames whose destination
- * address has the multicast bit set. The least significant
- * 6 bits of the CRC result are used as an addressing index
- * into the hash table. If the value of the bit addressed by
- * this index is a 1, the frame is passed to the host system.
- */
-
- /* Clear hashtable. */
- hashtable[0] = 0x00000000;
- hashtable[1] = 0x00000000;
-
- /* Cycle through all multicast addresses to filter. */
- for (mc_list_ptr = dev->mc_list;
- mc_list_ptr != NULL; mc_list_ptr = mc_list_ptr->next) {
- /* Calculate CRC result for each multicast address. */
- hashindex = crc32_le(0xffffffff, mc_list_ptr->dmi_addr,
- ETH_ALEN);
- /* Use only the least significant 6 bits. */
- hashindex = hashindex & 0x3F;
-
- /* Within "hashtable", set bit number "hashindex"
- * to a logic 1.
+ /*
+ * The IPG uses the standard hash table technique to filter
+ * incoming multicast packets. It computes the CRC of the
+ * incoming MAC address, and uses the low 6 bits of the
+ * result to select a hash table bit. If set (and the address
+ * is a multicast address), the packet is received.
*/
- set_bit(hashindex, (void *)hashtable);
- }
+ for (mc = dev->mc_list; mc; mv = mc->next) {
+ /* Calculate CRC result for each multicast address. */
+ u32 hashindex = crc32_le(0xffffffff, mc->dmi_addr,
+ ETH_ALEN);
- /* Write the value of the hashtable, to the 4, 16 bit
- * HASHTABLE IPG registers.
- */
- ipg_w32(hashtable[0], HASHTABLE_0);
- ipg_w32(hashtable[1], HASHTABLE_1);
+ /* Use low 6 bits to select hash table bit */
+ set_bit(hashindex & 63, (void *)hashtable);
+ }
+ ipg_w32(hashtable[0], HASHTABLE_0);
+ ipg_w32(hashtable[1], HASHTABLE_1);
+ }
ipg_w8(IPG_RM_RSVD_MASK & receivemode, RECEIVE_MODE);
--
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
Powered by blists - more mailing lists