Date: Mon, 21 Aug 2023 13:41:15 -0700
From: "Linga, Pavan Kumar" <pavan.kumar.linga@...el.com>
To: Jakub Kicinski <kuba@...nel.org>, Tony Nguyen <anthony.l.nguyen@...el.com>
CC: <davem@...emloft.net>, <pabeni@...hat.com>, <edumazet@...gle.com>,
	<netdev@...r.kernel.org>, Alan Brady <alan.brady@...el.com>,
	<emil.s.tantilov@...el.com>, <jesse.brandeburg@...el.com>,
	<sridhar.samudrala@...el.com>, <shiraz.saleem@...el.com>,
	<sindhu.devale@...el.com>, <willemb@...gle.com>, <decot@...gle.com>,
	<andrew@...n.ch>, <leon@...nel.org>, <mst@...hat.com>,
	<simon.horman@...igine.com>, <shannon.nelson@....com>,
	<stephen@...workplumber.org>, Alice Michael <alice.michael@...el.com>, Joshua
 Hay <joshua.a.hay@...el.com>, Phani Burra <phani.r.burra@...el.com>
Subject: Re: [PATCH net-next v5 14/15] idpf: add ethtool callbacks



On 8/18/2023 11:58 AM, Jakub Kicinski wrote:
> On Tue, 15 Aug 2023 17:43:04 -0700 Tony Nguyen wrote:
>> +static u32 idpf_get_rxfh_indir_size(struct net_device *netdev)
>> +{
>> +	struct idpf_vport *vport = idpf_netdev_to_vport(netdev);
>> +	struct idpf_vport_user_config_data *user_config;
>> +
>> +	if (!vport)
>> +		return -EINVAL;
> 
> defensive programming? how do we have a netdev and no vport?
> 

During a hardware reset, the control plane will reinitialize its vport 
configuration along with the hardware resources which in turn requires 
the driver to reallocate the vports as well. For this reason the vports 
will be freed, but the netdev will be preserved.


>> +	if (!idpf_is_cap_ena_all(vport->adapter, IDPF_RSS_CAPS, IDPF_CAP_RSS)) {
>> +		dev_err(&vport->adapter->pdev->dev, "RSS is not supported on this device\n");
>> +
>> +		return -EOPNOTSUPP;
> 
> Let's drop these prints, EOPNOTSUPP is enough.
> Some random system info gathering daemon will run this get and
> pollute logs with errors for no good reason.
>

Make sense, will remove them.
>> +	}
>> +
>> +	user_config = &vport->adapter->vport_config[vport->idx]->user_config;
>> +
>> +	return user_config->rss_data.rss_lut_size;
>> +}
> 
>> +/**
>> + * idpf_set_channels: set the new channel count
>> + * @netdev: network interface device structure
>> + * @ch: channel information structure
>> + *
>> + * Negotiate a new number of channels with CP. Returns 0 on success, negative
>> + * on failure.
>> + */
>> +static int idpf_set_channels(struct net_device *netdev,
>> +			     struct ethtool_channels *ch)
>> +{
>> +	struct idpf_vport *vport = idpf_netdev_to_vport(netdev);
>> +	struct idpf_vport_config *vport_config;
>> +	u16 combined, num_txq, num_rxq;
>> +	unsigned int num_req_tx_q;
>> +	unsigned int num_req_rx_q;
>> +	struct device *dev;
>> +	int err;
>> +	u16 idx;
>> +
>> +	if (!vport)
>> +		return -EINVAL;
>> +
>> +	idx = vport->idx;
>> +	vport_config = vport->adapter->vport_config[idx];
>> +
>> +	num_txq = vport_config->user_config.num_req_tx_qs;
>> +	num_rxq = vport_config->user_config.num_req_rx_qs;
>> +
>> +	combined = min(num_txq, num_rxq);
>> +
>> +	/* these checks are for cases where user didn't specify a particular
>> +	 * value on cmd line but we get non-zero value anyway via
>> +	 * get_channels(); look at ethtool.c in ethtool repository (the user
>> +	 * space part), particularly, do_schannels() routine
>> +	 */
>> +	if (ch->combined_count == combined)
>> +		ch->combined_count = 0;
>> +	if (ch->combined_count && ch->rx_count == num_rxq - combined)
>> +		ch->rx_count = 0;
>> +	if (ch->combined_count && ch->tx_count == num_txq - combined)
>> +		ch->tx_count = 0;
>> +
>> +	dev = &vport->adapter->pdev->dev;
>> +	if (!(ch->combined_count || (ch->rx_count && ch->tx_count))) {
>> +		dev_err(dev, "Please specify at least 1 Rx and 1 Tx channel\n");
> 
> The error msg doesn't seem to fit the second part of the condition.
> 

The negation part is to the complete check which means it takes 0 
[tx|rx]_count into consideration.

>> +		return -EINVAL;
>> +	}
>> +
>> +	num_req_tx_q = ch->combined_count + ch->tx_count;
>> +	num_req_rx_q = ch->combined_count + ch->rx_count;
>> +
>> +	dev = &vport->adapter->pdev->dev;
>> +	/* It's possible to specify number of queues that exceeds max in a way
>> +	 * that stack won't catch for us, this should catch that.
>> +	 */
> 
> How, tho?
> 

If the user tries to pass the combined along with the txq or rxq values, 
then it is possbile to cross the max supported values. So the following 
checks are needed to protect those cases. Core checks the max values for 
the individual arguments but not the combined + [tx|rx].

>> +	if (num_req_tx_q > vport_config->max_q.max_txq) {
>> +		dev_info(dev, "Maximum TX queues is %d\n",
>> +			 vport_config->max_q.max_txq);
>> +
>> +		return -EINVAL;
>> +	}
>> +	if (num_req_rx_q > vport_config->max_q.max_rxq) {
>> +		dev_info(dev, "Maximum RX queues is %d\n",
>> +			 vport_config->max_q.max_rxq);
>> +
>> +		return -EINVAL;
>> +	}
> 
>> +	if (ring->tx_pending > IDPF_MAX_TXQ_DESC ||
>> +	    ring->tx_pending < IDPF_MIN_TXQ_DESC) {
> 
> Doesn't core check max?
> 

Yes, it does. Thanks for catching that. Will remove the max check here.

>> +		netdev_err(netdev, "Descriptors requested (Tx: %u) out of range [%d-%d] (increment %d)\n",
>> +			   ring->tx_pending,
>> +			   IDPF_MIN_TXQ_DESC, IDPF_MAX_TXQ_DESC,
>> +			   IDPF_REQ_DESC_MULTIPLE);
>> +
>> +		return -EINVAL;
>> +	}
>> +
>> +	if (ring->rx_pending > IDPF_MAX_RXQ_DESC ||
>> +	    ring->rx_pending < IDPF_MIN_RXQ_DESC) {
>> +		netdev_err(netdev, "Descriptors requested (Rx: %u) out of range [%d-%d] (increment %d)\n",
>> +			   ring->rx_pending,
>> +			   IDPF_MIN_RXQ_DESC, IDPF_MAX_RXQ_DESC,
>> +			   IDPF_REQ_RXQ_DESC_MULTIPLE);
>> +
>> +		return -EINVAL;
>> +	}
> 
> 
>> +static const struct idpf_stats idpf_gstrings_port_stats[] = {
>> +	IDPF_PORT_STAT("rx-csum_errors", port_stats.rx_hw_csum_err),
>> +	IDPF_PORT_STAT("rx-hsplit", port_stats.rx_hsplit),
>> +	IDPF_PORT_STAT("rx-hsplit_hbo", port_stats.rx_hsplit_hbo),
>> +	IDPF_PORT_STAT("rx-bad_descs", port_stats.rx_bad_descs),
>> +	IDPF_PORT_STAT("rx-length_errors", port_stats.vport_stats.rx_invalid_frame_length),
>> +	IDPF_PORT_STAT("tx-skb_drops", port_stats.tx_drops),
>> +	IDPF_PORT_STAT("tx-dma_map_errs", port_stats.tx_dma_map_errs),
>> +	IDPF_PORT_STAT("tx-linearized_pkts", port_stats.tx_linearize),
>> +	IDPF_PORT_STAT("tx-busy_events", port_stats.tx_busy),
>> +	IDPF_PORT_STAT("rx_bytes", port_stats.vport_stats.rx_bytes),
>> +	IDPF_PORT_STAT("rx-unicast_pkts", port_stats.vport_stats.rx_unicast),
>> +	IDPF_PORT_STAT("rx-multicast_pkts", port_stats.vport_stats.rx_multicast),
>> +	IDPF_PORT_STAT("rx-broadcast_pkts", port_stats.vport_stats.rx_broadcast),
> 
> how are the basic stats different form the base stats reported via
> if_link?
> 

Found few stats which are common between the if_link and the base stats. 
Will remove the common ones.

> Also what's up with the mix of - and _ in the names?
> 
>> +	IDPF_PORT_STAT("rx-unknown_protocol", port_stats.vport_stats.rx_unknown_protocol),
>> +	IDPF_PORT_STAT("tx_bytes", port_stats.vport_stats.tx_bytes),
>> +	IDPF_PORT_STAT("tx-unicast_pkts", port_stats.vport_stats.tx_unicast),
>> +	IDPF_PORT_STAT("tx-multicast_pkts", port_stats.vport_stats.tx_multicast),
>> +	IDPF_PORT_STAT("tx-broadcast_pkts", port_stats.vport_stats.tx_broadcast),
>> +	IDPF_PORT_STAT("tx_errors", port_stats.vport_stats.tx_errors),
> 
>> +static void idpf_add_stat_strings(u8 **p, const struct idpf_stats *stats,
>> +				  const unsigned int size)
>> +{
>> +	unsigned int i;
>> +
>> +	for (i = 0; i < size; i++) {
>> +		snprintf((char *)*p, ETH_GSTRING_LEN, "%.32s",
>> +			 stats[i].stat_string);
>> +		*p += ETH_GSTRING_LEN;
> 
> ethtool_sprintf()
> 

Will fix.

>> +	}
>> +}

Thanks,
Pavan

Powered by blists - more mailing lists