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: Thu, 08 Mar 2012 10:27:58 +0100 From: Florian Fainelli <florian@...nwrt.org> To: Roland Stigge <stigge@...com.de> CC: davem@...emloft.net, jeffrey.t.kirsher@...el.com, alexander.h.duyck@...el.com, eilong@...adcom.com, ian.campbell@...rix.com, netdev@...r.kernel.org, w.sang@...gutronix.de, linux-kernel@...r.kernel.org, kevin.wells@....com, linux-arm-kernel@...ts.infradead.org, arnd@...db.de, baruch@...s.co.il, joe@...ches.com, bhutchings@...arflare.com Subject: Re: [PATCH v6] lpc32xx: Added ethernet driver Hi Roland, Le 03/07/12 21:21, Roland Stigge a écrit : > This patch adds an ethernet driver for the LPC32xx ARM SoC. > > Signed-off-by: Roland Stigge<stigge@...com.de> I have a couple of phylib-related comments addressed inline. > > --- > > Applies to v3.3-rc6 > > Changes since v5: > * Indentation and whitespace fixes > * Use __le32 for descriptors > * Use bool for boolean function parameters > * Removed unnecessary (u32) cast > * Added comment regarding future device tree introduction > > Thanks to David Miller, Arnd Bergmann and Joe Perches for > reviewing the last version! > > drivers/net/ethernet/Kconfig | 1 > drivers/net/ethernet/Makefile | 1 > drivers/net/ethernet/nxp/Kconfig | 8 > drivers/net/ethernet/nxp/Makefile | 1 > drivers/net/ethernet/nxp/lpc_eth.c | 1616 +++++++++++++++++++++++++++++++++++++ > 5 files changed, 1627 insertions(+) > > --- linux-2.6.orig/drivers/net/ethernet/Kconfig > +++ linux-2.6/drivers/net/ethernet/Kconfig > + > +static int lpc_mii_probe(struct net_device *ndev) > +{ > + struct netdata_local *pldat = netdev_priv(ndev); > + struct phy_device *phydev = NULL; > + int phy_addr; > + > + /* find the first phy */ > + for (phy_addr = 0; phy_addr< PHY_MAX_ADDR; phy_addr++) { > + if (pldat->mii_bus->phy_map[phy_addr]) { > + phydev = pldat->mii_bus->phy_map[phy_addr]; > + break; > + } > + } Please use phy_find_first() instead of open-coding this. > + > + if (!phydev) { > + netdev_err(ndev, "no PHY found\n"); > + return -ENODEV; > + } > + > + /* Attach to the PHY */ > + if (lpc_phy_interface_mode() == PHY_INTERFACE_MODE_MII) > + netdev_info(ndev, "using MII interface\n"); > + else > + netdev_info(ndev, "using RMII interface\n"); > + phydev = phy_connect(ndev, dev_name(&phydev->dev), > + &lpc_handle_link_change, 0, lpc_phy_interface_mode()); > + > + if (IS_ERR(phydev)) { > + netdev_err(ndev, "Could not attach to PHY\n"); > + return PTR_ERR(phydev); > + } > + > + /* mask with MAC supported features */ > + phydev->supported&= PHY_BASIC_FEATURES; > + > + phydev->advertising = phydev->supported; > + > + pldat->link = 0; > + pldat->speed = 0; > + pldat->duplex = -1; > + pldat->phy_dev = phydev; > + > + return 0; > +} > + > +static int lpc_mii_init(struct netdata_local *pldat) > +{ > + int err = -ENXIO, i; > + > + pldat->mii_bus = mdiobus_alloc(); > + if (!pldat->mii_bus) { > + err = -ENOMEM; > + goto err_out; > + } > + > + /* Setup MII mode */ > + if (lpc_phy_interface_mode() == PHY_INTERFACE_MODE_MII) > + writel(LPC_COMMAND_PASSRUNTFRAME, > + LPC_ENET_COMMAND(pldat->net_base)); > + else { > + writel((LPC_COMMAND_PASSRUNTFRAME | LPC_COMMAND_RMII), > + LPC_ENET_COMMAND(pldat->net_base)); > + writel(LPC_SUPP_RESET_RMII, LPC_ENET_SUPP(pldat->net_base)); > + } > + > + pldat->mii_bus->name = "lpc_mii_bus"; > + pldat->mii_bus->read =&lpc_mdio_read; > + pldat->mii_bus->write =&lpc_mdio_write; > + pldat->mii_bus->reset =&lpc_mdio_reset; > + snprintf(pldat->mii_bus->id, MII_BUS_ID_SIZE, "%x", pldat->pdev->id); No, makes this bus name unique in the system: snprnitf(pldat->mii_bus->id, MII_BUS_ID_SIZE, "%s-%x", pldat->pdev->name, pldat->pdev->id); > + pldat->mii_bus->priv = pldat; > + pldat->mii_bus->parent =&pldat->pdev->dev; > + pldat->mii_bus->phy_mask = 0xFFFFFFF0; I would rather not hardcode the phy_mask here, just leave it to 0, and let phy_find_first() find the PHY devices for you, or make this platform configurable. > + > + pldat->mii_bus->irq = kmalloc(sizeof(int) * PHY_MAX_ADDR, GFP_KERNEL); > + if (!pldat->mii_bus->irq) { > + err = -ENOMEM; > + goto err_out_1; > + } > + > + for (i = 0; i< PHY_MAX_ADDR; i++) > + pldat->mii_bus->irq[i] = PHY_POLL; > + > + platform_set_drvdata(pldat->pdev, pldat->mii_bus); > + > + if (mdiobus_register(pldat->mii_bus)) > + goto err_out_free_mdio_irq; > + > + if (lpc_mii_probe(pldat->ndev) != 0) > + goto err_out_unregister_bus; > + > + return 0; > + > +err_out_unregister_bus: > + mdiobus_unregister(pldat->mii_bus); > +err_out_free_mdio_irq: > + kfree(pldat->mii_bus->irq); > +err_out_1: > + mdiobus_free(pldat->mii_bus); > +err_out: > + return err; > +} > + [snip] > + > +static int lpc_eth_open(struct net_device *ndev) > +{ > + struct netdata_local *pldat = netdev_priv(ndev); > + > + /* if the phy is not yet registered, retry later*/ > + if (!pldat->phy_dev) > + return -EAGAIN; You have already probed the mii bus in the driver's probe function, how can you end up here without a phy being attached here? [snip] > + if (lpc_mii_init(pldat) != 0) > + goto err_out_unregister_netdev; > + > + netdev_info(ndev, "LPC mac at 0x%08x irq %d\n", > + res->start, ndev->irq); > + > + phydev = pldat->phy_dev; > + netdev_info(ndev, > + "attached PHY driver [%s] (mii_bus:phy_addr=%s, irq=%d)\n", > + phydev->drv->name, dev_name(&phydev->dev), phydev->irq); Most drivers put this informational message in their mii probing function, please move it here as well. > + > + device_init_wakeup(&pdev->dev, 1); > + device_set_wakeup_enable(&pdev->dev, 0); > + > + return 0; > + > +err_out_unregister_netdev: > + platform_set_drvdata(pdev, NULL); > + unregister_netdev(ndev); > +err_out_dma_unmap: > + if (!use_iram_for_net() || > + pldat->dma_buff_size> lpc32xx_return_iram_size()) > + dma_free_coherent(&pldat->pdev->dev, pldat->dma_buff_size, > + pldat->dma_buff_base_v, > + pldat->dma_buff_base_p); > +err_out_free_irq: > + free_irq(ndev->irq, ndev); > +err_out_iounmap: > + iounmap(pldat->net_base); > +err_out_disable_clocks: > + clk_disable(pldat->clk); > + clk_put(pldat->clk); > +err_out_free_dev: > + free_netdev(ndev); > +err_exit: > + pr_err("%s: not found (%d).\n", MODNAME, ret); > + return ret; > +} > + > +static int lpc_eth_drv_remove(struct platform_device *pdev) > +{ > + struct net_device *ndev = platform_get_drvdata(pdev); > + struct netdata_local *pldat = netdev_priv(ndev); > + > + unregister_netdev(ndev); > + platform_set_drvdata(pdev, NULL); > + > + if (!use_iram_for_net() || > + pldat->dma_buff_size> lpc32xx_return_iram_size()) > + dma_free_coherent(&pldat->pdev->dev, pldat->dma_buff_size, > + pldat->dma_buff_base_v, > + pldat->dma_buff_base_p); > + free_irq(ndev->irq, ndev); > + iounmap(pldat->net_base); > + clk_disable(pldat->clk); > + clk_put(pldat->clk); > + free_netdev(ndev); You are not freeing the mii bus in the driver's remove path. -- Florian -- 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