netdev - RE: [net-next 13/15] iecm: Add ethtool

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
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <MW3PR11MB4522EDBA35498230F418F96F8F980@MW3PR11MB4522.namprd11.prod.outlook.com>
Date:   Fri, 19 Jun 2020 00:36:21 +0000
From:   "Brady, Alan" <alan.brady@...el.com>
To:     Michal Kubecek <mkubecek@...e.cz>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>
CC:     "Kirsher, Jeffrey T" <jeffrey.t.kirsher@...el.com>,
        "davem@...emloft.net" <davem@...emloft.net>,
        "Michael, Alice" <alice.michael@...el.com>,
        "nhorman@...hat.com" <nhorman@...hat.com>,
        "sassmann@...hat.com" <sassmann@...hat.com>,
        "Burra, Phani R" <phani.r.burra@...el.com>,
        "Hay, Joshua A" <joshua.a.hay@...el.com>,
        "Chittim, Madhu" <madhu.chittim@...el.com>,
        "Linga, Pavan Kumar" <pavan.kumar.linga@...el.com>,
        "Skidmore, Donald C" <donald.c.skidmore@...el.com>,
        "Brandeburg, Jesse" <jesse.brandeburg@...el.com>,
        "Samudrala, Sridhar" <sridhar.samudrala@...el.com>
Subject: RE: [net-next 13/15] iecm: Add ethtool

> -----Original Message-----
> From: Michal Kubecek <mkubecek@...e.cz>
> Sent: Thursday, June 18, 2020 2:17 PM
> To: netdev@...r.kernel.org
> Cc: Kirsher, Jeffrey T <jeffrey.t.kirsher@...el.com>; davem@...emloft.net;
> Michael, Alice <alice.michael@...el.com>; nhorman@...hat.com;
> sassmann@...hat.com; Brady, Alan <alan.brady@...el.com>; Burra, Phani R
> <phani.r.burra@...el.com>; Hay, Joshua A <joshua.a.hay@...el.com>; Chittim,
> Madhu <madhu.chittim@...el.com>; Linga, Pavan Kumar
> <pavan.kumar.linga@...el.com>; Skidmore, Donald C
> <donald.c.skidmore@...el.com>; Brandeburg, Jesse
> <jesse.brandeburg@...el.com>; Samudrala, Sridhar
> <sridhar.samudrala@...el.com>
> Subject: Re: [net-next 13/15] iecm: Add ethtool
> 
> On Wed, Jun 17, 2020 at 10:13:42PM -0700, Jeff Kirsher wrote:
> > From: Alice Michael <alice.michael@...el.com>
> >
> > Implement ethtool interface for the common module.
> >
> > Signed-off-by: Alice Michael <alice.michael@...el.com>
> > Signed-off-by: Alan Brady <alan.brady@...el.com>
> > Signed-off-by: Phani Burra <phani.r.burra@...el.com>
> > Signed-off-by: Joshua Hay <joshua.a.hay@...el.com>
> > Signed-off-by: Madhu Chittim <madhu.chittim@...el.com>
> > Signed-off-by: Pavan Kumar Linga <pavan.kumar.linga@...el.com>
> > Reviewed-by: Donald Skidmore <donald.c.skidmore@...el.com>
> > Reviewed-by: Jesse Brandeburg <jesse.brandeburg@...el.com>
> > Reviewed-by: Sridhar Samudrala <sridhar.samudrala@...el.com>
> > Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@...el.com>
> > ---
> [...]
> > +static int iecm_set_channels(struct net_device *netdev,
> > +			     struct ethtool_channels *ch)
> > +{
> > +	struct iecm_vport *vport = iecm_netdev_to_vport(netdev);
> > +	int num_req_q = ch->combined_count;
> > +
> > +	if (num_req_q == max(vport->num_txq, vport->num_rxq))
> > +		return 0;
> > +
> > +	/* All of these should have already been checked by ethtool before this
> > +	 * even gets to us, but just to be sure.
> > +	 */
> > +	if (num_req_q <= 0 || num_req_q > IECM_MAX_Q)
> > +		return -EINVAL;
> > +
> > +	if (ch->rx_count || ch->tx_count || ch->other_count !=
> IECM_MAX_NONQ)
> > +		return -EINVAL;
> 
> I don't see much sense in duplicating the checks. Having the checks in common
> code allows us to simplify driver callbacks.
> 

You're right it's better to remove these.  Will fix.

> > +	vport->adapter->config_data.num_req_qs = num_req_q;
> > +
> > +	return iecm_initiate_soft_reset(vport, __IECM_SR_Q_CHANGE); }
> [...]
> > +static int iecm_set_ringparam(struct net_device *netdev,
> > +			      struct ethtool_ringparam *ring) {
> > +	struct iecm_vport *vport = iecm_netdev_to_vport(netdev);
> > +	u32 new_rx_count, new_tx_count;
> > +
> > +	if (ring->rx_mini_pending || ring->rx_jumbo_pending)
> > +		return -EINVAL;
> > +
> > +	new_tx_count = clamp_t(u32, ring->tx_pending,
> > +			       IECM_MIN_TXQ_DESC,
> > +			       IECM_MAX_TXQ_DESC);
> > +	new_tx_count = ALIGN(new_tx_count, IECM_REQ_DESC_MULTIPLE);
> > +
> > +	new_rx_count = clamp_t(u32, ring->rx_pending,
> > +			       IECM_MIN_RXQ_DESC,
> > +			       IECM_MAX_RXQ_DESC);
> > +	new_rx_count = ALIGN(new_rx_count, IECM_REQ_DESC_MULTIPLE);
> 
> Same here. This is actually a bit misleading as it seems that count exceeding
> maximum would be silently clamped to it but such value would be rejected by
> common code.
> 

Also agreed, will fix in next version.

> > +	/* if nothing to do return success */
> > +	if (new_tx_count == vport->txq_desc_count &&
> > +	    new_rx_count == vport->rxq_desc_count)
> > +		return 0;
> > +
> > +	vport->adapter->config_data.num_req_txq_desc = new_tx_count;
> > +	vport->adapter->config_data.num_req_rxq_desc = new_rx_count;
> > +
> > +	return iecm_initiate_soft_reset(vport, __IECM_SR_Q_DESC_CHANGE); }
> [...]
> > +/* For now we have one and only one private flag and it is only
> > +defined
> > + * when we have support for the SKIP_CPU_SYNC DMA attribute.  Instead
> > + * of leaving all this code sitting around empty we will strip it
> > +unless
> > + * our one private flag is actually available.
> > + */
> 
> The code below will always return 1 for ETH_SS_PRIV_FLAGS in
> iecm_get_sset_count() and an array of one string in iecm_get_strings().
> This would e.g. result in "ethtool -i" saying "supports-priv-flags: yes"
> but then "ethtool --show-priv-flags" failing with -EOPNOTSUPP. IMHO you
> should not return bogus string set if private flags are not implemented.
> 
> Michal
> 

I'm embarrassed we added a nice comment about stripping this because it doesn't do anything and then failed to do just that.  Agreed the priv flag stuff should not be here without any private flags existing, will fix.  Thanks for the review.

Alan