Re: [PATCH 11/12] qlcnic: 83xx adpater ethtool
flat view
From: Ben Hutchings <hidden>
Date: 2012-09-06 17:49:48
On Thu, 2012-09-06 at 02:22 -0400, Sony Chacko wrote:
quoted hunk ↗ jump to hunk
From: Sony Chacko <redacted> 83xx ethtool interface routines Signed-off-by: Anirban Chakraborty <redacted> Signed-off-by: Sucheta Chakraborty <redacted> Signed-off-by: Jitendra Kalsaria <redacted> Signed-off-by: Rajesh Borundia <redacted> Signed-off-by: Sritej Velaga <redacted> Signed-off-by: Sony Chacko <redacted> --- .../net/ethernet/qlogic/qlcnic/qlcnic_ethtool.c | 583 +++++++++++++------- 1 files changed, 374 insertions(+), 209 deletions(-)diff --git a/drivers/net/ethernet/qlogic/qlcnic/qlcnic_ethtool.c b/drivers/net/ethernet/qlogic/qlcnic/qlcnic_ethtool.c index 4625253..b0d21ac 100644 --- a/drivers/net/ethernet/qlogic/qlcnic/qlcnic_ethtool.c +++ b/drivers/net/ethernet/qlogic/qlcnic/qlcnic_ethtool.c
[...]
quoted hunk ↗ jump to hunk
@@ -146,6 +188,12 @@ static const u32 diag_registers[] = { QLCNIC_PEG_ALIVE_COUNTER, QLCNIC_PEG_HALT_STATUS1, QLCNIC_PEG_HALT_STATUS2, + -1 +}; + +static const u32 ext_diag_registers[] = { + CRB_XG_STATE_P3P, + ISR_INT_STATE_REG, QLCNIC_CRB_PEG_NET_0+0x3c, QLCNIC_CRB_PEG_NET_1+0x3c, QLCNIC_CRB_PEG_NET_2+0x3c,@@ -154,12 +202,19 @@ static const u32 diag_registers[] = { }; #define QLCNIC_MGMT_API_VERSION 2 -#define QLCNIC_DEV_INFO_SIZE 1 #define QLCNIC_ETHTOOL_REGS_VER 2
I think QLCNIC_ETHTOOL_REGS_VER needs to be changed, as you appear to be dumping more registers from the existing hardware as well.
quoted hunk ↗ jump to hunk
+ static int qlcnic_get_regs_len(struct net_device *dev) { - return sizeof(diag_registers) + QLCNIC_RING_REGS_LEN + - QLCNIC_DEV_INFO_SIZE + 1; + struct qlcnic_adapter *adapter = netdev_priv(dev); + u32 len; + + if (QLCNIC_IS_83XX(adapter)) + len = qlcnic_83xx_get_regs_len(adapter); + else + len = sizeof(ext_diag_registers) + sizeof(diag_registers); + + return QLCNIC_RING_REGS_LEN + len + QLCNIC_DEV_INFO_SIZE + 1; } static int qlcnic_get_eeprom_len(struct net_device *dev)
[...]
quoted hunk ↗ jump to hunk
@@ -509,25 +613,6 @@ qlcnic_set_ringparam(struct net_device *dev, return qlcnic_reset_context(adapter); } -static void qlcnic_get_channels(struct net_device *dev, - struct ethtool_channels *channel) -{ - struct qlcnic_adapter *adapter = netdev_priv(dev); - - channel->max_rx = rounddown_pow_of_two(min_t(int, - adapter->ahw->max_rx_ques, num_online_cpus())); - channel->max_tx = adapter->ahw->max_tx_ques; - - channel->rx_count = adapter->max_sds_rings; - channel->tx_count = adapter->ahw->max_tx_ques; -} - -static int qlcnic_set_channels(struct net_device *dev, - struct ethtool_channels *channel) -{ - return 0; -} - static void qlcnic_get_pauseparam(struct net_device *netdev, struct ethtool_pauseparam *pause)
[...]
quoted hunk ↗ jump to hunk
+static void qlcnic_get_channels(struct net_device *dev, + struct ethtool_channels *channel) +{ + struct qlcnic_adapter *adapter = netdev_priv(dev); + + channel->max_rx = rounddown_pow_of_two(min_t(int, + adapter->ahw->max_rx_ques, + num_online_cpus())); + channel->max_tx = adapter->ahw->max_tx_ques; + + channel->rx_count = adapter->max_sds_rings; + channel->tx_count = adapter->ahw->max_tx_ques; +} + +static int qlcnic_set_channels(struct net_device *dev, + struct ethtool_channels *channel) +{ + struct qlcnic_adapter *adapter = netdev_priv(dev); + int err; + + if (channel->other_count || channel->combined_count || + channel->tx_count != channel->max_tx) + return -EINVAL; + + err = qlcnic_validate_max_rss(channel->max_rx, channel->rx_count); + if (err) + return err; + + err = qlcnic_set_max_rss(adapter, channel->rx_count, 0); + netdev_info(dev, "allocated 0x%x sds rings\n", + adapter->max_sds_rings); + return err; +}
So you removed the body of qlcnic_set_channels() in an earlier patch, and now you're moving it and putting the body back how it was... Please clean up the patch series so it doesn't have noise like this in it. [...]
quoted hunk ↗ jump to hunk
+static int +qlcnic_set_wol(struct net_device *dev, struct ethtool_wolinfo *wol) +{ + int err; + struct qlcnic_adapter *adapter = netdev_priv(dev); + u32 wol_cfg; + + if (QLCNIC_IS_83XX(adapter)) + return -EOPNOTSUPP; + if (wol->wolopts & ~WAKE_MAGIC) + return -EOPNOTSUPP;
Should be -EINVAL in the second error case (the device supports the operation, but not the mode requested).
quoted hunk ↗ jump to hunk
+ wol_cfg = QLCRD32(adapter, QLCNIC_WOL_CONFIG_NV, &err); + if (!(wol_cfg & (1 << adapter->portnum))) + return -EOPNOTSUPP; + + wol_cfg = QLCRD32(adapter, QLCNIC_WOL_CONFIG, &err); + if (wol->wolopts & WAKE_MAGIC) + wol_cfg |= 1UL << adapter->portnum; + else + wol_cfg &= ~(1UL << adapter->portnum); + + QLCWR32(adapter, QLCNIC_WOL_CONFIG, wol_cfg); + + return 0; } static int qlcnic_set_led(struct net_device *dev,
[...]
quoted hunk ↗ jump to hunk
@@ -1081,50 +1297,6 @@ static int qlcnic_set_led(struct net_device *dev, return err; } -static void -qlcnic_get_wol(struct net_device *dev, struct ethtool_wolinfo *wol) -{ - int err; - struct qlcnic_adapter *adapter = netdev_priv(dev); - u32 wol_cfg; - - wol->supported = 0; - wol->wolopts = 0; - - wol_cfg = QLCRD32(adapter, QLCNIC_WOL_CONFIG_NV, &err); - if (wol_cfg & (1UL << adapter->portnum)) - wol->supported |= WAKE_MAGIC; - - wol_cfg = QLCRD32(adapter, QLCNIC_WOL_CONFIG, &err); - if (wol_cfg & (1UL << adapter->portnum)) - wol->wolopts |= WAKE_MAGIC; -} - -static int -qlcnic_set_wol(struct net_device *dev, struct ethtool_wolinfo *wol) -{ - int err; - struct qlcnic_adapter *adapter = netdev_priv(dev); - u32 wol_cfg; - - if (wol->wolopts & ~WAKE_MAGIC) - return -EOPNOTSUPP; - - wol_cfg = QLCRD32(adapter, QLCNIC_WOL_CONFIG_NV, &err); - if (!(wol_cfg & (1 << adapter->portnum))) - return -EOPNOTSUPP; - - wol_cfg = QLCRD32(adapter, QLCNIC_WOL_CONFIG, &err); - if (wol->wolopts & WAKE_MAGIC) - wol_cfg |= 1UL << adapter->portnum; - else - wol_cfg &= ~(1UL << adapter->portnum); - - QLCWR32(adapter, QLCNIC_WOL_CONFIG, wol_cfg); - - return 0; -}
[...] Again you've moved functions around for no obvious reason. Ben. -- Ben Hutchings, Staff Engineer, Solarflare Not speaking for my employer; that's the marketing department's job. They asked us to note that Solarflare product names are trademarked.