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 PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Mon, 28 May 2007 23:13:28 -0400 From: Jeff Garzik <jeff@...zik.org> To: Marc St-Jean <Marc_St-Jean@...-sierra.com> CC: akpm@...ux-foundation.org, linux-mips@...ux-mips.org, netdev@...r.kernel.org Subject: Re: [PATCH 10/12] drivers: PMC MSP71xx ethernet driver Marc St-Jean wrote: > Jeff Garzik wrote: >> Marc St-Jean wrote: >> > + res = platform_get_resource(pldev, IORESOURCE_MEM, 0); >> > + if (!res) { >> > + printk(KERN_ERR "MSPETH(probe) %s: " >> > + "IOMEM resource not found for eth%d\n", >> > + dev->name, unit); >> > + goto out_netdev; >> > + } >> > + >> > + /* reserve the memory region */ >> > + if (!request_mem_region(res->start, res->end - res->start + 1, >> > + cardname)) { >> > + printk(KERN_ERR "MSPETH(probe) %s: unable to " >> > + "get memory/io address region 0x08%lx\n", >> > + dev->name, dev->base_addr); >> > + goto out_netdev; >> > + } >> > + >> > + /* remap the memory */ >> > + mapaddr = ioremap_nocache(res->start, res->end - res->start + 1); >> > + if (!mapaddr) { >> > + printk(KERN_WARNING "MSPETH(probe) %s: " >> > + "unable to ioremap address 0x%08x\n", >> > + dev->name, res->start); >> > + goto out_unreserve; >> > + } >> > + >> > + lp->mapaddr = mapaddr; >> > + dev->base_addr = res->start; >> > + dev->irq = platform_get_irq(pldev, 0); >> > + >> > + /* remap the system reset registers */ >> > + lp->rstaddr = ioremap_nocache(MSP_RST_BASE, MSP_RST_SIZE); >> > + if (!lp->rstaddr) { >> > + printk(KERN_ERR "MSPETH(probe) %s: unable to " >> > + "ioremap address 0x%08x\n", >> > + dev->name, MSP_RST_BASE); >> > + goto out_unmap; >> > + } >> > + >> > + /* set the logical and hardware units */ >> > + lp->unit = unit; >> > + lp->hwunit = hwunit; >> > + >> > + /* probe for PHYS attached to this MACs MDIO interface */ >> > + if (mspeth_phyprobe(dev)) >> > + goto out_unmap; >> > + >> > + /* parse the environment and command line */ >> > + mspeth_init_cmdline(dev); >> > + mspeth_init_phyaddr(dev); >> > + >> > + /* MAC address */ >> > + dev->addr_len = ETH_ALEN; >> > + for (i = 0; i < dev->addr_len; i++) >> > + dev->dev_addr[i] = macaddr[i]; >> > + >> > + /* register the /proc entry */ >> > + snprintf(tmp_str, 128, "pmcmspeth%d", unit); >> > + create_proc_read_entry(tmp_str, 0644, proc_net, >> > + mspeth_proc_info, dev); >> >> use sysfs > > I thought sysfs was only required if the file was to be writable? > Is this no longer the case? That was never the case. And these days, additions to procfs are insta-deprecated. New code should start out with sysfs. >> > >> +/************************************************************************** >> >> > + * Probe the hardware and fill out the array of PHY control elements >> > + */ >> > +static int >> > +mspeth_phyprobe(struct net_device *dev) >> > +{ >> > + struct mspeth_priv *lp = netdev_priv(dev); >> > + u32 reg1; >> > + int phyaddr; >> > + struct mspeth_phy tmp_phy; >> > + struct mspeth_phy **tail_pp; >> > + >> > + tmp_phy.next_phy = NULL; >> > + tmp_phy.hwunit = lp->hwunit; >> > + tmp_phy.phyaddr = 0; >> > + tmp_phy.memaddr = lp->mapaddr + MSPETH_MD_DATA; >> > + tmp_phy.assigned = false; >> > + tmp_phy.linkup = false; >> > + spin_lock_init(&tmp_phy.lock); >> > + >> > + /* find the tail of the phy list */ >> > + for (tail_pp = &root_phy_dev; *tail_pp != NULL; >> > + tail_pp = &(*tail_pp)->next_phy) {;} >> > + >> > + /* probe the phys and add to list */ >> > + for (phyaddr = 0; phyaddr < MD_MAX_PHY; phyaddr++) { >> >> for standard MII, you should start at PHY ID 1, since PHY ID 0 is often >> a ghost. normal loop looks like >> >> for (phy = 1; phy <= 32 && phy_idx < MII_CNT; phy++) { >> int phyx = phy & 0x1f; > > I'm not sure what you mean by ghost, an alias for ID 1? An alias for a non-zero PHY ID. > We have several boards which access either stand-alone or switch/phy-combos > at ID 0. We don't have access to all boards (some are customers) and can't > risk breaking them at this point. Did you read my example code? It clearly scans PHY ID 0. What would break? > I've only looked at mii.c briefly but it appears that it would involve a complete > re-write of your mspeth_phy struct and all code accessing it. That is risky > for a driver that has been solid for the last couple years and is already in > production. > > Is this mandatory? We are not inclined to merge Yet Another Duplicate of mii.c, which means twice the maintenance headache over the lifetime of the Linux kernel. >> > + * cleared afterwards, although we don't change the pointers so >> > + * they don't need to be reallocated. >> > + */ >> > +static void >> > +mspeth_mac_reset(struct net_device *dev) >> > +{ >> > + struct mspeth_priv *lp = netdev_priv(dev); >> > + int i; >> > + u32 rstpat; >> > + >> > + /* hardware reset */ >> > + switch (lp->hwunit) { >> > + case 0: >> > + rstpat = MSP_EA_RST; >> > + break; >> > + case 1: >> > + rstpat = MSP_EB_RST; >> > + break; >> > + case 2: >> > + rstpat = MSP_EC_RST; >> > + break; >> > + default: >> > + printk(KERN_WARNING >> > + "MSPETH(mac_reset) %s: Unsupported hwunit %d\n", >> > + dev->name, lp->hwunit); >> > + return; >> > + } >> > + >> > + __raw_writel(rstpat, lp->rstaddr + MSPRST_SET); >> > + mdelay(100); >> > + __raw_writel(rstpat, lp->rstaddr + MSPRST_CLR); >> >> host bus posting bug? > > Not sure what you mean here? The above lines reset one of the MACs within the > SoC device. Are writes delayed or posted in any way? If yes, then the timing of mdelay() is not guaranteed. Normally you must issue a read, following a write, before your delay. That guarantees the full delay occurs after the write reaches the MAC. >> > + /* load station address to ARC */ >> > + mspeth_set_arc_entry(dev, ARC_ENTRY_SOURCE, dev->dev_addr); >> > + >> > + /* Enable ARC (broadcast and unicast) */ >> > + msp_write(lp, MSPETH_ARC_Ena, ARC_Ena_Bit(ARC_ENTRY_SOURCE)); >> > + msp_write(lp, MSPETH_ARC_Ctl, ARC_CompEn | ARC_BroadAcc); >> > + >> > + /* configure DMA */ >> > + msp_write(lp, MSPETH_DMA_Ctl, DMA_CTL_CMD); >> > + >> > + /* configure the RX/TX mac */ >> > + msp_write(lp, MSPETH_RxFragSize, 0); >> > + msp_write(lp, MSPETH_TxPollCtr, TX_POLL_CNT); >> > + msp_write(lp, MSPETH_TxThrsh, TX_THRESHOLD); >> > + >> > + /* zero and enable the interrupts */ >> > + lp->fatal_icnt = 0; >> > + msp_write(lp, MSPETH_Int_En, INT_EN_CMD); >> > + >> > + /* >> > + * Set queues >> > + * >> > + * hammtrev, 2005-11-25: >> > + * Using the formula used in the old driver, which gives a >> > + * little bit less than (RX_BUF_NUM - 1) << 5, allowing for more >> > + * buffer descriptors attached to a frame descriptor. >> > + */ >> > + msp_write(lp, MSPETH_FDA_Bas, (u32)lp->rxfd_base); >> > + msp_write(lp, MSPETH_FDA_Lim, (RX_BUF_NUM - 1) << 5); >> >> you should be using DMA mapping functions (if only silly wrappers), not >> direct casts > > A bit of background to clarify the drivers use... > This driver is for a proprietary MAC sitting across a proprietary bus. It is > also device and arch specific, it will never be on a PCI card. > > By mapping the frame and buffer descriptors to a uncached memory segment and > allowing the DMA to make use of those addresses, we avoid a lot of extra > code/cycles when manipulating those structures throughout the driver. > > In order to accomplish this the MAC must be given the uncached address above. See the "if only silly wrappers" comment. You should create a DMA-mapping API, even if it intentionally degenerates inside the compiler's optimizer to a direct memory access and/or no-ops. The gives the driver well-defined access points for accessing memory with unusual attributes such as uncached memory segments. >> > + * Open/initialize the board. This is called (in the current kernel) >> > + * sometime after booting when the 'ifconfig' program is run. >> > + * >> > + * This routine should set everything up anew at each open, even >> > + * registers that "should" only need to be set once at boot, so that >> > + * there is non-reboot way to recover if something goes wrong. >> > + */ >> > +static int >> > +mspeth_open(struct net_device *dev) >> > +{ >> > + struct mspeth_priv *lp = netdev_priv(dev); >> > + int err = -EBUSY; >> > + >> > + /* reset the hardware, disabling/clearing all interrupts */ >> > + mspeth_mac_reset(dev); >> > + mspeth_phy_reset(dev); >> > + >> > + /* determine preset speed and duplex settings */ >> > + if (lp->option & MSP_OPT_10M) >> > + lp->speed = 10; >> > + else >> > + lp->speed = 100; >> > + >> > + if (lp->option & MSP_OPT_FDUP) >> > + lp->fullduplex = true; >> > + else >> > + lp->fullduplex = false; >> > + >> > + /* initialize the queues and the hardware */ >> > + if (!mspeth_init_queues(dev)) { >> > + printk(KERN_ERR "MSPETH(open) %s: " >> > + "Unable to allocate queues\n", dev->name); >> > + goto out_err; >> > + } >> > + >> > + /* allocate and initialize the tasklets */ >> > +#ifndef CONFIG_MSPETH_NAPI >> > + tasklet_init(&lp->rx_tasklet, mspeth_rx, (u32)dev); >> > + tasklet_init(&lp->tx_tasklet, mspeth_txdone, (u32)dev); >> > +#endif >> >> with tasklets you don't need NAPI. also, the casts are bogus > > Some of our customers prefer NAPI over tasklets, that is why we have > a configuration options so each one can choose the preferred method. This again revisits the issue of /not/ duplicating code that exists elsewhere. The CONFIG_MSPETH_NAPI choice is essentially between "NAPI" and "NAPI implemented solely within our driver". Drivers that permit a NAPI compilation choice allow the user to choose between no software mitigation (no-NAPI) or software mitigation. That is a reasonable choice. But including your own software mitigation scheme is wasteful, and different from the NAPI/no-NAPI configurations that normally get accepted. Jeff - 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