| 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
| ||
|
Message-ID: <20230818115824.446d1ea7@kernel.org>
Date: Fri, 18 Aug 2023 11:58:24 -0700
From: Jakub Kicinski <kuba@...nel.org>
To: 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>,
pavan.kumar.linga@...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 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?
> + 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.
> + }
> +
> + 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.
> + 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 (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?
> + 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?
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()
> + }
> +}
Powered by blists - more mailing lists