On Thu, 25 Jun 2020 19:07:35 -0700 Jeff Kirsher wrote:
> From: Alice Michael <alice.mich...@intel.com>
>
> Implement ethtool interface for the common module.
>
> Signed-off-by: Alice Michael <alice.mich...@intel.com>
> Signed-off-by: Alan Brady <alan.br...@intel.com>
> Signed-off-by: Phani Burra <phani.r.bu...@intel.com>
> Signed-off-by: Joshua Hay <joshua.a....@intel.com>
> Signed-off-by: Madhu Chittim <madhu.chit...@intel.com>
> Signed-off-by: Pavan Kumar Linga <pavan.kumar.li...@intel.com>
> Reviewed-by: Donald Skidmore <donald.c.skidm...@intel.com>
> Reviewed-by: Jesse Brandeburg <jesse.brandeb...@intel.com>
> Reviewed-by: Sridhar Samudrala <sridhar.samudr...@intel.com>
> Signed-off-by: Jeff Kirsher <jeffrey.t.kirs...@intel.com>
> +/* Helper macro to define an iecm_stat structure with proper size and type.
> + * Use this when defining constant statistics arrays. Note that @_type
> expects
> + * only a type name and is used multiple times.
> + */
> +#define IECM_STAT(_type, _name, _stat) { \
> + .stat_string = _name, \
> + .sizeof_stat = sizeof_field(_type, _stat), \
> + .stat_offset = offsetof(_type, _stat) \
> +}
> +
> +/* Helper macro for defining some statistics related to queues */
> +#define IECM_QUEUE_STAT(_name, _stat) \
> + IECM_STAT(struct iecm_queue, _name, _stat)
> +
> +/* Stats associated with a Tx queue */
> +static const struct iecm_stats iecm_gstrings_tx_queue_stats[] = {
> + IECM_QUEUE_STAT("%s-%u.packets", q_stats.tx.packets),
> + IECM_QUEUE_STAT("%s-%u.bytes", q_stats.tx.bytes),
> +};
> +
> +/* Stats associated with an Rx queue */
> +static const struct iecm_stats iecm_gstrings_rx_queue_stats[] = {
> + IECM_QUEUE_STAT("%s-%u.packets", q_stats.rx.packets),
> + IECM_QUEUE_STAT("%s-%u.bytes", q_stats.rx.bytes),
> + IECM_QUEUE_STAT("%s-%u.generic_csum", q_stats.rx.generic_csum),
> + IECM_QUEUE_STAT("%s-%u.basic_csum", q_stats.rx.basic_csum),
What's basic and generic? perhaps given them the Linux names?
> + IECM_QUEUE_STAT("%s-%u.csum_err", q_stats.rx.csum_err),
> + IECM_QUEUE_STAT("%s-%u.hsplit_buf_overflow", q_stats.rx.hsplit_hbo),
> +};
> +/**
> + * __iecm_set_q_coalesce - set ITR values for specific queue
> + * @ec: ethtool structure from user to update ITR settings
> + * @q: queue for which ITR values has to be set
> + *
> + * Returns 0 on success, negative otherwise.
> + */
> +static int
> +__iecm_set_q_coalesce(struct ethtool_coalesce *ec, struct iecm_queue *q)
> +{
> + const char *q_type_str = (q->q_type == VIRTCHNL_QUEUE_TYPE_RX)
> + ? "Rx" : "Tx";
> + u32 use_adaptive_coalesce, coalesce_usecs;
> + struct iecm_vport *vport;
> + u16 itr_setting;
> +
> + itr_setting = IECM_ITR_SETTING(q->itr.target_itr);
> + vport = q->vport;
> + if (q->q_type == VIRTCHNL_QUEUE_TYPE_RX) {
> + use_adaptive_coalesce = ec->use_adaptive_rx_coalesce;
> + coalesce_usecs = ec->rx_coalesce_usecs;
> + } else {
> + use_adaptive_coalesce = ec->use_adaptive_tx_coalesce;
> + coalesce_usecs = ec->tx_coalesce_usecs;
> + }
> +
> + if (itr_setting != coalesce_usecs && use_adaptive_coalesce) {
> + netdev_info(vport->netdev, "%s ITR cannot be changed if
> adaptive-%s is enabled\n",
> + q_type_str, q_type_str);
> + return -EINVAL;
> + }
> +
> + if (coalesce_usecs > IECM_ITR_MAX) {
> + netdev_info(vport->netdev,
> + "Invalid value, %d-usecs range is 0-%d\n",
> + coalesce_usecs, IECM_ITR_MAX);
> + return -EINVAL;
> + }
> +
> + /* hardware only supports an ITR granularity of 2us */
> + if (coalesce_usecs % 2 != 0) {
> + netdev_info(vport->netdev,
> + "Invalid value, %d-usecs must be even\n",
> + coalesce_usecs);
> + return -EINVAL;
> + }
Most drivers just round things up or down, but up to you.
> + q->itr.target_itr = coalesce_usecs;
> + if (use_adaptive_coalesce)
> + q->itr.target_itr |= IECM_ITR_DYNAMIC;
> + /* Update of static/dynamic ITR will be taken care when interrupt is
> + * fired
> + */
> + return 0;
> +}
> +
> +/**
> + * iecm_set_q_coalesce - set ITR values for specific queue
> + * @vport: vport associated to the queue that need updating
> + * @ec: coalesce settings to program the device with
> + * @q_num: update ITR/INTRL (coalesce) settings for this queue number/index
> + * @is_rxq: is queue type Rx
> + *
> + * Return 0 on success, and negative on failure
> + */
> +static int
> +iecm_set_q_coalesce(struct iecm_vport *vport, struct ethtool_coalesce *ec,
> + int q_num, bool is_rxq)
> +{
> + if (is_rxq) {
> + struct iecm_queue *rxq = iecm_find_rxq(vport, q_num);
> +
> + if (rxq && __iecm_set_q_coalesce(ec, rxq))
> + return -EINVAL;
> + } else {
> + struct iecm_queue *txq = iecm_find_txq(vport, q_num);
> +
> + if (txq && __iecm_set_q_coalesce(ec, txq))
> + return -EINVAL;
> + }
What's the point? Callers always call this function with tx, then rx.
Just set both.
> + return 0;
> +}
> +
> +/**
> + * iecm_set_coalesce - set ITR values as requested by user
> + * @netdev: pointer to the netdev associated with this query
> + * @ec: coalesce settings to program the device with
> + *
> + * Return 0 on success, and negative on failure
> + */
> +static int
> +iecm_set_coalesce(struct net_device *netdev, struct ethtool_coalesce *ec)
> +{
> + struct iecm_vport *vport = iecm_netdev_to_vport(netdev);
> + int i, err = 0;
> +
> + if (vport->adapter->state != __IECM_UP)
> + return 0;
> +
> + for (i = 0; i < vport->num_txq; i++) {
> + err = iecm_set_q_coalesce(vport, ec, i, false);
> + if (err)
> + goto set_coalesce_err;
> + }
> +
> + for (i = 0; i < vport->num_rxq; i++) {
> + err = iecm_set_q_coalesce(vport, ec, i, true);
> + if (err)
> + goto set_coalesce_err;
> + }
> +set_coalesce_err:
label is unnecessary, just return
> + return err;
> +}
> +
> +/**
> + * iecm_set_per_q_coalesce - set ITR values as requested by user
> + * @netdev: pointer to the netdev associated with this query
> + * @q_num: queue for which the ITR values has to be set
> + * @ec: coalesce settings to program the device with
> + *
> + * Return 0 on success, and negative on failure
> + */
> +static int
> +iecm_set_per_q_coalesce(struct net_device *netdev, u32 q_num,
> + struct ethtool_coalesce *ec)
> +{
> + struct iecm_vport *vport = iecm_netdev_to_vport(netdev);
> + int err;
> +
> + if (vport->adapter->state != __IECM_UP)
> + return 0;
> +
> + err = iecm_set_q_coalesce(vport, ec, q_num, false);
> + if (!err)
> + err = iecm_set_q_coalesce(vport, ec, q_num, true);
> +
> + return err;
> +}
> +/**
> + * iecm_get_link_ksettings - Get Link Speed and Duplex settings
> + * @netdev: network interface device structure
> + * @cmd: ethtool command
> + *
> + * Reports speed/duplex settings.
> + **/
> +static int iecm_get_link_ksettings(struct net_device *netdev,
> + struct ethtool_link_ksettings *cmd)
> +{
> + struct iecm_netdev_priv *np = netdev_priv(netdev);
> + struct iecm_adapter *adapter = np->vport->adapter;
> +
> + ethtool_link_ksettings_zero_link_mode(cmd, supported);
> + cmd->base.autoneg = AUTONEG_DISABLE;
> + cmd->base.port = PORT_NONE;
> + /* Set speed and duplex */
> + switch (adapter->link_speed) {
> + case VIRTCHNL_LINK_SPEED_40GB:
> + cmd->base.speed = SPEED_40000;
> + break;
> + case VIRTCHNL_LINK_SPEED_25GB:
> +#ifdef SPEED_25000
> + cmd->base.speed = SPEED_25000;
> +#else
> + netdev_info(netdev,
> + "Speed is 25G, display not supported by this
> version of ethtool.\n");
> +#endif
Maybe drop the Intel review tags from this.
Clearly nobody looked at this patch :/
> + break;
> + case VIRTCHNL_LINK_SPEED_20GB:
> + cmd->base.speed = SPEED_20000;
> + break;
> + case VIRTCHNL_LINK_SPEED_10GB:
> + cmd->base.speed = SPEED_10000;
> + break;
> + case VIRTCHNL_LINK_SPEED_1GB:
> + cmd->base.speed = SPEED_1000;
> + break;
> + case VIRTCHNL_LINK_SPEED_100MB:
> + cmd->base.speed = SPEED_100;
> + break;
> + default:
> + break;
> + }
> + cmd->base.duplex = DUPLEX_FULL;
> +
> + return 0;
> +}
> +
> +/**
> + * iecm_get_drvinfo - Get driver info
> + * @netdev: network interface device structure
> + * @drvinfo: ethtool driver info structure
> + *
> + * Returns information about the driver and device for display to the user.
> + */
> +static void iecm_get_drvinfo(struct net_device *netdev,
> + struct ethtool_drvinfo *drvinfo)
> +{
> + struct iecm_adapter *adapter = iecm_netdev_to_adapter(netdev);
> +
> + strlcpy(drvinfo->driver, iecm_drv_name, 32);
> + strlcpy(drvinfo->fw_version, "N/A", 4);
Then don't report it.
The other two pieces of information are filled in by the core if the
callback is not set, so you can skip implementing this altogether.
> + strlcpy(drvinfo->bus_info, pci_name(adapter->pdev), 32);
> +}
