| 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: Tue, 11 Jan 2011 15:57:06 +0000
From: Ben Hutchings <bhutchings@...arflare.com>
To: nobuhiro.iwamatsu.yj@...esas.com
Cc: netdev@...r.kernel.org, linux-sh@...r.kernel.org,
yoshihiro.shimoda.uh@...esas.com
Subject: Re: [PATCH v2] sh: sh_eth: Add support ethtool
On Tue, 2011-01-11 at 20:58 +0900, nobuhiro.iwamatsu.yj@...esas.com
wrote:
> From: Nobuhiro Iwamatsu <nobuhiro.iwamatsu.yj@...esas.com>
>
> This commit supports following functions.
> - get_drvinfo
> - get_settings
> - set_settings
> - nway_reset
> - get_msglevel
> - set_msglevel
> - get_link
> - get_strings
> - get_ethtool_stats
> - get_sset_count
>
> About other function, the device does not support.
>
> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@...esas.com>
> Signed-off-by: Nobuhiro Iwamatsu <nobuhiro.iwamatsu.yj@...esas.com>
> ---
> v2: reverted one part of the checks of checkpatch.pl.
> foo *bar -> foo * bar.
> changed function copying of net_device_stats from *for* to memcopy.
>
> drivers/net/sh_eth.c | 186 ++++++++++++++++++++++++++++++++++++++++++++++---
> 1 files changed, 174 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/net/sh_eth.c b/drivers/net/sh_eth.c
> index 819c175..0b2cb7d 100644
> --- a/drivers/net/sh_eth.c
> +++ b/drivers/net/sh_eth.c
[...]
> @@ -1063,6 +1074,154 @@ static int sh_eth_phy_start(struct net_device *ndev)
> return 0;
> }
>
> +static void sh_eth_get_drvinfo(struct net_device *ndev,
> + struct ethtool_drvinfo *info)
> +{
> + strncpy(info->driver, "sh_eth", sizeof(info->driver) - 1);
> + strcpy(info->version, "N/A");
> + strcpy(info->fw_version, "N/A");
> + strlcpy(info->bus_info, dev_name(ndev->dev.parent),
> + sizeof(info->bus_info));
> +}
This is redundant; the default implementation already does this.
[...]
> +static int sh_eth_set_settings(struct net_device *ndev,
> + struct ethtool_cmd *ecmd)
> +{
> + struct sh_eth_private *mdp = netdev_priv(ndev);
> + unsigned long flags;
> + int ret;
> + u32 ioaddr = ndev->base_addr;
> +
> + spin_lock_irqsave(&mdp->lock, flags);
> +
> + /* disable tx and rx */
> + sh_eth_linkdown(ioaddr);
> +
> + ret = phy_ethtool_sset(mdp->phydev, ecmd);
> + if (ret)
> + goto error_exit;
> +
> + if (ecmd->duplex == DUPLEX_FULL)
> + mdp->duplex = 1;
> + else
> + mdp->duplex = 0;
> +
> + if (mdp->cd->set_duplex)
> + mdp->cd->set_duplex(ndev);
> +
> +error_exit:
> + mdelay(100);
Ugh, 100 ms holding a spinlock?!
> + /* enable tx and rx */
> + sh_eth_linkup(ioaddr);
How do you know the link is up? Shouldn't this be left to the link
polling function?
[...]
> +static u32 sh_eth_get_msglevel(struct net_device *ndev)
> +{
> + struct sh_eth_private *mdp = netdev_priv(ndev);
> + return mdp->msg_enable;
> +}
> +
> +static void sh_eth_set_msglevel(struct net_device *ndev, u32 value)
> +{
> + struct sh_eth_private *mdp = netdev_priv(ndev);
> + mdp->msg_enable = value;
> +}
This would be more useful if msg_enable was actually used anywhere in
the driver.
[...]
> @@ -1073,8 +1232,8 @@ static int sh_eth_open(struct net_device *ndev)
>
> ret = request_irq(ndev->irq, sh_eth_interrupt,
> #if defined(CONFIG_CPU_SUBTYPE_SH7763) || \
> - defined(CONFIG_CPU_SUBTYPE_SH7764) || \
> - defined(CONFIG_CPU_SUBTYPE_SH7757)
> + defined(CONFIG_CPU_SUBTYPE_SH7764) || \
> + defined(CONFIG_CPU_SUBTYPE_SH7757)
> IRQF_SHARED,
> #else
> 0,
> @@ -1232,11 +1391,11 @@ static int sh_eth_close(struct net_device *ndev)
> sh_eth_ring_free(ndev);
>
> /* free DMA buffer */
> - ringsize = sizeof(struct sh_eth_rxdesc) * RX_RING_SIZE;
> + ringsize = sizeof(struct sh_eth_rxdesc) *RX_RING_SIZE;
> dma_free_coherent(NULL, ringsize, mdp->rx_ring, mdp->rx_desc_dma);
>
> /* free DMA buffer */
> - ringsize = sizeof(struct sh_eth_txdesc) * TX_RING_SIZE;
> + ringsize = sizeof(struct sh_eth_txdesc) *TX_RING_SIZE;
> dma_free_coherent(NULL, ringsize, mdp->tx_ring, mdp->tx_desc_dma);
>
> pm_runtime_put_sync(&mdp->pdev->dev);
Please do not include these space changes.
> @@ -1497,8 +1656,11 @@ static int sh_eth_drv_probe(struct platform_device *pdev)
>
> /* set function */
> ndev->netdev_ops = &sh_eth_netdev_ops;
> + SET_ETHTOOL_OPS(ndev, &sh_eth_ethtool_ops);
> ndev->watchdog_timeo = TX_TIMEOUT;
>
> + /* debug message level */
> + mdp->msg_enable = (1 << 3) - 1;
If you're actually going to *use* msg_enable, its value should be
initialised in terms of the NETIF_MSG_* flags defined in
<linux/netdevice.h>.
> mdp->post_rx = POST_RX >> (devno << 1);
> mdp->post_fw = POST_FW >> (devno << 1);
>
> @@ -1572,7 +1734,7 @@ static int sh_eth_runtime_nop(struct device *dev)
> return 0;
> }
>
> -static struct dev_pm_ops sh_eth_dev_pm_ops = {
> +static const struct dev_pm_ops sh_eth_dev_pm_ops = {
> .runtime_suspend = sh_eth_runtime_nop,
> .runtime_resume = sh_eth_runtime_nop,
> };
This is worthwhile but unrelated to ethtool!
Ben.
--
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.
--
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