| 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, 16 Feb 2011 16:04:44 +0900
From: Nobuhiro Iwamatsu <nobuhiro.iwamatsu.yj@...esas.com>
To: Ben Hutchings <bhutchings@...arflare.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
2011/1/12 Ben Hutchings <bhutchings@...arflare.com>:
> 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.
I see. I removed 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?!
Oh, This was not need 100ms.
I changed to 1 ms.
>
>> + /* 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?
>
Hmm. this has bad function name.
This function does not linkup. This enable recv / send function of the
hardware.
I changed a function name from sh_eth_linkup to sh_eth_rcv_send_enable.
> [...]
>> +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.
I forgot this.
I am going to include msglevel stuff.
>
> [...]
>> @@ -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.
I revised this.
>
>> @@ -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>.
Thanks, I replaced to NETIF_MSG_*.
>
>> 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!
Oh, I split to other patch.
>
> Ben.
>
Best regards,
Nobuhiro
--
Nobuhiro Iwamatsu
--
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