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 for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20200618211726.ijsafmx52ha3lamz@lion.mk-sys.cz>
Date:   Thu, 18 Jun 2020 23:17:26 +0200
From:   Michal Kubecek <mkubecek@...e.cz>
To:     netdev@...r.kernel.org
Cc:     Jeff Kirsher <jeffrey.t.kirsher@...el.com>, davem@...emloft.net,
        Alice Michael <alice.michael@...el.com>, nhorman@...hat.com,
        sassmann@...hat.com, Alan Brady <alan.brady@...el.com>,
        Phani Burra <phani.r.burra@...el.com>,
        Joshua Hay <joshua.a.hay@...el.com>,
        Madhu Chittim <madhu.chittim@...el.com>,
        Pavan Kumar Linga <pavan.kumar.linga@...el.com>,
        Donald Skidmore <donald.c.skidmore@...el.com>,
        Jesse Brandeburg <jesse.brandeburg@...el.com>,
        Sridhar Samudrala <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.

> +	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.

> +	/* 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

> +struct iecm_priv_flags {
> +	char flag_string[ETH_GSTRING_LEN];
> +	bool read_only;
> +	u32 flag;
> +};
> +
> +#define IECM_PRIV_FLAG(_name, _flag, _read_only) { \
> +	.read_only = _read_only, \
> +	.flag_string = _name, \
> +	.flag = _flag, \
> +}
> +
> +static const struct iecm_priv_flags iecm_gstrings_priv_flags[] = {
> +	IECM_PRIV_FLAG("", 0, 0),
> +};
> +
> +#define IECM_PRIV_FLAGS_STR_LEN ARRAY_SIZE(iecm_gstrings_priv_flags)
[...]
> +static void iecm_get_priv_flag_strings(struct net_device *netdev, u8 *data)
> +{
> +	unsigned int i;
> +
> +	for (i = 0; i < IECM_PRIV_FLAGS_STR_LEN; i++) {
> +		snprintf((char *)data, ETH_GSTRING_LEN, "%s",
> +			 iecm_gstrings_priv_flags[i].flag_string);
> +		data += ETH_GSTRING_LEN;
> +	}
> +}