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
| ||
|
Date: Wed, 15 Sep 2010 21:19:46 +0900 From: "Masayuki Ohtake" <masa-korg@....okisemi.com> To: "Joe Perches" <joe@...ches.com> Cc: "David S. Miller" <davem@...emloft.net>, "Randy Dunlap" <randy.dunlap@...cle.com>, "John Linn" <john.linn@...inx.com>, "Ralf Baechle" <ralf@...ux-mips.org>, "Kristoffer Glembo" <kristoffer@...sler.com>, "Maxime Bizon" <mbizon@...ebox.fr>, "Greg Rose" <gregory.v.rose@...el.com>, "ML netdev" <netdev@...r.kernel.org>, "LKML" <linux-kernel@...r.kernel.org>, "MeeGo" <meego-dev@...go.com>, "Stephen Hemminger" <shemminger@...tta.com>, "Jili Slaby" <jslaby@...e.cz>, "FUJITA Tomonori" <fujita.tomonori@....ntt.co.jp>, "Wang, Qi" <qi.wang@...el.com>, "Wang, Yong Y" <yong.y.wang@...el.com>, "Andrew" <andrew.chih.howe.khor@...el.com>, "Intel OTC" <joel.clark@...el.com>, "Foster, Margie" <margie.foster@...el.com>, "Arjan" <arjan@...ux.intel.com>, "Toshiharu Okada" <okada533@....okisemi.com>, "Takahiro Shimizu" <shimizu394@....okisemi.com>, "Tomoya Morinaga" <morinaga526@....okisemi.com> Subject: Re: [PATCH v3] Gigabit Ethernet driver of Topcliff PCH Hi Joe Thank you for your comments. My reply comments are included in the following. I will resubmit after modified. Thanks Ohtake ----- Date: Thu, 09 Sep 2010 23:32:07 -0700 From: "Joe Perches" <joe@...ches.com> > > diff --git a/drivers/net/pch_gbe/pch_gbe_ethtool.c b/drivers/net/pch_gbe/pch_gbe_ethtool.c > [] > > +static void pch_gbe_get_regs(struct net_device *netdev, > > + struct ethtool_regs *regs, void *p) > > +{ > > + struct pch_gbe_adapter *adapter = netdev_priv(netdev); > > + struct pch_gbe_hw *hw = &adapter->hw; > > + struct pci_dev *pdev = adapter->pdev; > > + u32 *regs_buff = p; > > + u16 i, reg, tmp; > > + > > + regs->version = pdev->revision; > > + regs->version = 0x1000000 | (regs->version << 16) | pdev->device; > > Might be simpler as: > regs->version = 0x1000000 | (__u32)pdev->revision << 16 | pdev->device; > > The block below is a bit confusing to me. [masa] This will be modified so. > > + memset(p, 0, PCH_GBE_REGS_LEN * (int)sizeof(u32)); > > It seems the memset is unnecessary as it's completely > overwritten below. [masa] This will be deleted. > > > + for (i = 0; i < PCH_GBE_MAC_REGS_LEN; i++) > > + regs_buff[i] = ioread32(&hw->reg->INT_ST + i); > > + /* PHY register */ > > + for (i = PCH_GBE_MAC_REGS_LEN, reg = 0; reg < PCH_GBE_PHY_REGS_LEN; > > + i++, reg++) { > > + pch_gbe_hal_read_phy_reg(&adapter->hw, reg, &tmp); > > + regs_buff[i] = tmp; > > + } > > I think i'd be simpler to do something like: > > for (i = 0; i < PCH_GBE_MAC_REGS_LEN; i++) > *regs_buff++ = ioread32(&hw->reg->INT_ST + i); > > /* PHY register */ > for (i = 0; i < PCH_GBE_PHY_REGS_LEN; i++) { > pch_gbe_hal_read_phy_reg(&adapter->hw, i, &tmp); > *regs_buff++ = tmp; > } [masa] This will be modified so. > +void pch_gbe_mac_mar_set(struct pch_gbe_hw *hw, u8 * addr, u32 index) > > +{ > > + u32 mar_low, mar_high, adrmask; > > + > > + pr_debug("index : 0x%x\n", index); > > + > > + /* > > + * HW expects these in little endian so we reverse the byte order > > + * from network order (big endian) to little endian > > + */ > > + mar_high = ((u32) addr[0] | ((u32) addr[1] << 8) | > > + ((u32) addr[2] << 16) | ((u32) addr[3] << 24)); > > + mar_low = ((u32) addr[4] | ((u32) addr[5] << 8)); > > + /* Stop the MAC Address of index. */ > > + adrmask = ioread32(&hw->reg->ADDR_MASK); > > + iowrite32((adrmask | (0x0001 << index)), &hw->reg->ADDR_MASK); > > + /* wait busy */ > > + while ((ioread32(&hw->reg->ADDR_MASK) & PCH_GBE_BUSY)) { > > + int tmp = 0; > > + udelay(20); > > + tmp++; > > + if (tmp == 1000) { > > + pr_err("Address mask bit is not cleared\n"); > > + break; > > + } > > + } > > You need to move the declaration of tmp out of the while. > > Do these really need to be busy-waits? [masa] The declaration of tmp will be moved. It is necessary to wait for BUSY by the specification of hardware. > > > + /* Set the MAC address to the MAC address 1A/1B register */ > > + iowrite32(mar_high, &hw->reg->mac_adr[index].high); > > + iowrite32(mar_low, &hw->reg->mac_adr[index].low); > > + /* Start the MAC address of index */ > > + iowrite32((adrmask & ~(0x0001 << index)), &hw->reg->ADDR_MASK); > > + /* wait busy */ > > + while ((ioread32(&hw->reg->ADDR_MASK) & PCH_GBE_BUSY)) { > > + int tmp = 0; > > + udelay(20); > > + tmp++; > > + if (tmp == 1000) { > > + pr_err("Address mask bit is not cleared\n"); > > + break; > > + } > > + } > > +} > > Here too. There are more of these too I'm not listing. > Perhaps you could review the logic a bit more. > [masa] This will be modified. -- 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