| 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, 20 Jun 2013 19:47:10 +0000
From: "Nelson, Shannon" <shannon.nelson@...el.com>
To: Ben Hutchings <bhutchings@...arflare.com>,
"Kirsher, Jeffrey T" <jeffrey.t.kirsher@...el.com>
CC: "davem@...emloft.net" <davem@...emloft.net>,
"Brandeburg, Jesse" <jesse.brandeburg@...el.com>,
"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
"gospo@...hat.com" <gospo@...hat.com>,
"sassmann@...hat.com" <sassmann@...hat.com>,
"Waskiewicz Jr, Peter P" <peter.p.waskiewicz.jr@...el.com>,
"e1000-devel@...ts.sourceforge.net"
<e1000-devel@...ts.sourceforge.net>
Subject: RE: [net-next 3/8] i40e: driver ethtool core
> From: Ben Hutchings [mailto:bhutchings@...arflare.com]
> Sent: Wednesday, June 19, 2013 11:27 AM
>
> On Thu, 2013-06-13 at 20:55 -0700, Jeff Kirsher wrote:
> > From: Jesse Brandeburg <jesse.brandeburg@...el.com>
> >
> > This patch contains the ethtool interface and implementation.
> >
> > The goal in this patch series is minimal functionality while not
> > including much in the way of "set support."
> [...]
> > --- /dev/null
> > +++ b/drivers/net/ethernet/intel/i40e/i40e_ethtool.c
> [...]
> > +static const struct i40e_stats i40e_gstrings_net_stats[] = {
> > + I40E_NETDEV_STAT(rx_packets),
> > + I40E_NETDEV_STAT(tx_packets),
> > + I40E_NETDEV_STAT(rx_bytes),
> > + I40E_NETDEV_STAT(tx_bytes),
> > + I40E_NETDEV_STAT(rx_errors),
> > + I40E_NETDEV_STAT(tx_errors),
> > + I40E_NETDEV_STAT(rx_dropped),
> > + I40E_NETDEV_STAT(tx_dropped),
> > + I40E_NETDEV_STAT(multicast),
> > + I40E_NETDEV_STAT(collisions),
> > + I40E_NETDEV_STAT(rx_length_errors),
> > + I40E_NETDEV_STAT(rx_over_errors),
> > + I40E_NETDEV_STAT(rx_crc_errors),
> > + I40E_NETDEV_STAT(rx_frame_errors),
> > + I40E_NETDEV_STAT(rx_fifo_errors),
> > + I40E_NETDEV_STAT(tx_aborted_errors),
> > + I40E_NETDEV_STAT(tx_carrier_errors),
> > + I40E_NETDEV_STAT(tx_fifo_errors),
> > + I40E_NETDEV_STAT(tx_heartbeat_errors),
> > + I40E_NETDEV_STAT(tx_window_errors),
> > + I40E_NETDEV_STAT(rx_compressed),
> > + I40E_NETDEV_STAT(tx_compressed),
> > +};
>
> So you're doing VJ header compression over Ethernet now? At least drop
> the irrelevant counters.
Yep, needs cleaning.
>
> > +static struct i40e_stats i40e_gstrings_stats[] = {
> > + I40E_PF_STAT("rx_bytes", stats.eth.rx_bytes),
> > + I40E_PF_STAT("tx_bytes", stats.eth.tx_bytes),
> > + I40E_PF_STAT("rx_errors", stats.eth.rx_errors),
> > + I40E_PF_STAT("tx_errors", stats.eth.tx_errors),
>
> Duplicates.
Well, not really are they duplicates - the NETDEV_STAT numbers are specifically for each netdev that is attached to this device. Because the XL710 supports VMDq, SR-IOV, and some other device multiplexing schemes, each netdev's NETDEV_STAT struct is the subset of the traffic going through its portion of the device.
The PF_STAT struct holds the stats for what actually goes through the physical port. This is not necessarily a summation of the various NETDEV_STAT structs, because some connections may be talking to each other and not out the physical port.
When ethtool -S is called on the PF's netdev, we see both the netdev and the ports stats printed. When called on non-PF connection we only see the netdev stats, none of the port stats.
>
> [...]
> > +#define I40E_QUEUE_STATS_LEN \
> > + ((((struct i40e_netdev_priv *)netdev_priv(netdev))->vsi-
> >num_queue_pairs + \
> > + ((struct i40e_netdev_priv *)netdev_priv(netdev))->vsi-
> >num_queue_pairs) * 2)
>
> netdev should be a macro parameter. Actually this could just as well be
> a function.
Sure.
Can you tell we inherited a lot from our previous drivers?
>
> [...]
> > +/**
> > + * i40e_get_settings - Get Link Speed and Duplex settings
> > + * @netdev: network interface device structure
> > + * @ecmd: ethtool command
> > + *
> > + * Reports speed/duplex settings based on media_type
> > + **/
> > +static int i40e_get_settings(struct net_device *netdev,
> > + struct ethtool_cmd *ecmd)
> > +{
> > + struct i40e_netdev_priv *np = netdev_priv(netdev);
> > + struct i40e_pf *pf = np->vsi->back;
> > + struct i40e_hw *hw = &pf->hw;
> > + struct i40e_link_status *hw_link_info = &hw->phy.link_info;
> > + u32 link_speed = hw_link_info->link_speed;
> > + bool link_up = false;
> > +
> > + ecmd->supported = SUPPORTED_10000baseT_Full |
> > + SUPPORTED_1000baseT_Full |
> > + SUPPORTED_Autoneg;
> > +
> > + ecmd->advertising = ADVERTISED_10000baseT_Full |
> > + ADVERTISED_1000baseT_Full;
> > +
> > + if (hw->phy.media_type == I40E_MEDIA_TYPE_BACKPLANE) {
> > + ecmd->supported |= (SUPPORTED_100baseT_Full |
> > + SUPPORTED_Autoneg);
> > + ecmd->advertising |= ADVERTISED_100baseT_Full;
>
> All the link modes are wrong for backplane.
Yep, and as you point out below, there's plenty more to clean up.
>
> > + } else if (hw->phy.media_type == I40E_MEDIA_TYPE_BASET) {
> > + ecmd->supported |= SUPPORTED_TP;
> > + ecmd->advertising |= ADVERTISED_TP;
> > + ecmd->port = PORT_TP;
> > + } else {
> > + ecmd->supported |= SUPPORTED_FIBRE;
> > + ecmd->advertising |= ADVERTISED_FIBRE;
> > + ecmd->port = PORT_FIBRE;
>
> And for fibre, though this is bodged in most drivers. But for backplane
> modes we do have ethtool link mode flags defined so there's no excuse.
>
> > + }
> > +
> > + if (hw->phy.get_link_info == true)
> > + link_up = hw_link_info->link_info & I40E_AQ_LINK_UP;
> > +
> > + ecmd->advertising = ADVERTISED_Autoneg;
>
> So autoneg is always used, even over fibre?
>
> > + if (hw->phy.autoneg_advertised) {
> > + if (hw->phy.autoneg_advertised & I40E_LINK_SPEED_100MB)
> > + ecmd->advertising |= ADVERTISED_100baseT_Full;
> > + if (hw->phy.autoneg_advertised & I40E_LINK_SPEED_1GB)
> > + ecmd->advertising |= ADVERTISED_1000baseT_Full;
> > + if (hw->phy.autoneg_advertised & I40E_LINK_SPEED_10GB)
> > + ecmd->advertising |= ADVERTISED_10000baseT_Full;
> > + } else {
> > + ecmd->advertising |= (ADVERTISED_10000baseT_Full |
> > + ADVERTISED_1000baseT_Full);
> > + }
> > +
> > + ecmd->transceiver = XCVR_EXTERNAL;
>
> Really?
>
> > + if (!in_interrupt()) {
> > + i40e_aq_get_link_info(&pf->hw, true, NULL, NULL);
> > + } else {
> > + /* Reinitialize link variables, a special workaround for
> RHEL5
> > + * bonding that calls this routine from interrupt context
> > + */
> > + link_speed = hw_link_info->link_speed;
> > + link_up = hw_link_info->link_info & I40E_AQ_LINK_UP;
> > + }
>
> I know that bug. In fact, it only got completely fixed in 3.9 (and some
> stable branches). But you can't put this workaround in-tree.
We'll strip this out of the upstream code.
>
> [...]
> > +static void i40e_get_drvinfo(struct net_device *netdev,
> > + struct ethtool_drvinfo *drvinfo)
> > +{
> > + struct i40e_netdev_priv *np = netdev_priv(netdev);
> > + struct i40e_vsi *vsi = np->vsi;
> > + struct i40e_pf *pf = vsi->back;
> > + char firmware_version[32];
> > +
> > + strncpy(drvinfo->driver, i40e_driver_name, sizeof(drvinfo-
> >driver));
> > + strncpy(drvinfo->version, i40e_driver_version_str,
> > + sizeof(drvinfo->version));
>
> Replace strncpy() with strlcpy() throughout this function.
Yep.
>
> > + snprintf(firmware_version, sizeof(firmware_version),
> > + "FW %d.%d API %d.%d NVM %02d.%02d.%02d",
> > + pf->hw.aq.fw_maj_ver, pf->hw.aq.fw_min_ver,
> > + pf->hw.aq.api_maj_ver, pf->hw.aq.api_min_ver,
> > + ((pf->hw.nvm.version >> 12) & 0xf),
> > + ((pf->hw.nvm.version >> 4) & 0xff),
> > + (pf->hw.nvm.version & 0xf));
> > +
> > + strncpy(drvinfo->fw_version, firmware_version,
> > + sizeof(drvinfo->fw_version));
>
> What was the point of the intermediate buffer?
Hmm - I guess we could snprintf() right into the drvinfo->fw_version...
>
> > + strncpy(drvinfo->bus_info, pci_name(pf->pdev),
> > + sizeof(drvinfo->bus_info));
> > + if (vsi == pf->vsi[pf->lan_vsi])
> > + drvinfo->n_stats = I40E_PF_STATS_LEN;
> > + else
> > + drvinfo->n_stats = I40E_VSI_STATS_LEN;
> > + drvinfo->testinfo_len = I40E_TEST_LEN;
> > + drvinfo->regdump_len = i40e_get_regs_len(netdev);
>
> Do not set n_stats or *_len; the ethtool core does that for you.
Okay.
>
> > +}
> [...]
> > +static int i40e_set_phys_id(struct net_device *netdev,
> > + enum ethtool_phys_id_state state)
> > +{
> > + struct i40e_netdev_priv *np = netdev_priv(netdev);
> > + struct i40e_pf *pf = np->vsi->back;
> > + struct i40e_hw *hw = &pf->hw;
> > + int blink_freq = 2;
> > + static u32 led_status;
>
> Ugh, no.
>
> Since ETHTOOL_PHYSID drops the RTNL between calls to set_phys_id, it's
> possible to invoke it on multiple devices at the same time...
Yep, that should become PF netdev specific.
>
> > + switch (state) {
> > + case ETHTOOL_ID_ACTIVE:
> > + led_status = i40e_led_get(hw);
> > + return blink_freq;
> > + case ETHTOOL_ID_ON:
> > + i40e_led_set(hw, 0xF);
> > + break;
> > + case ETHTOOL_ID_OFF:
> > + i40e_led_set(hw, 0x0);
> > + break;
> > + case ETHTOOL_ID_INACTIVE:
> > + i40e_led_set(hw, led_status);
> > + break;
> > + }
> > +
> > + return 0;
> > +}
> [...]
>
> --
> 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.
Again, thanks for your time and comments. We'll be addressing these soon.
sln
Powered by blists - more mailing lists