Re: [PATCH 5/6] net: ethernet: efct: Add ethtool support

[Andrew Lunn <andrew@lunn.ch>]

> +static void efct_ethtool_get_drvinfo(struct net_device *net_dev,
> +				     struct ethtool_drvinfo *info)
> +{
> +	struct efct_device *efct_dev;
> +	struct efct_nic *efct;
> +
> +	efct = efct_netdev_priv(net_dev);
> +	efct_dev = efct_nic_to_device(efct);
> +	strscpy(info->driver, KBUILD_MODNAME, sizeof(info->driver));
> +	if (!in_interrupt()) {
> +		efct_mcdi_print_fwver(efct, info->fw_version, sizeof(info->fw_version));
> +		efct_mcdi_erom_ver(efct, info->erom_version, sizeof(info->erom_version));

What is the code path where ethtool ops are called in interrupt
context?

> +	} else {
> +		strscpy(info->fw_version, "N/A", sizeof(info->fw_version));
> +		strscpy(info->erom_version, "N/A", sizeof(info->erom_version));
> +	}
> +	strscpy(info->bus_info, pci_name(efct_dev->pci_dev), sizeof(info->bus_info));
> +	info->n_priv_flags = 0;

Why set it to zero?

> +	data += EFCT_ETHTOOL_SW_STAT_COUNT;
> +	for (i = 0; i < EFCT_MAX_CORE_TX_QUEUES; i++) {
> +		data[0] = efct->txq[i].tx_packets;
> +		data++;

pretty unusual way to do this, Why not *data++ = efct->txq[i].tx_packets ?

> +static int efct_ethtool_get_coalesce(struct net_device *net_dev,
> +				     struct ethtool_coalesce *coalesce,
> +				     struct kernel_ethtool_coalesce *kernel_coal,
> +				     struct netlink_ext_ack *extack)
> +{
> +	struct efct_nic *efct = efct_netdev_priv(net_dev);
> +	u32 tx_usecs, rx_usecs;
> +
> +	tx_usecs = 0;
> +	rx_usecs = 0;
> +	efct_get_tx_moderation(efct, &tx_usecs);
> +	efct_get_rx_moderation(efct, &rx_usecs);
> +	coalesce->tx_coalesce_usecs = tx_usecs;
> +	coalesce->tx_coalesce_usecs_irq = 0;
> +	coalesce->rx_coalesce_usecs = rx_usecs;
> +	coalesce->rx_coalesce_usecs_irq = 0;
> +	coalesce->use_adaptive_rx_coalesce = efct->irq_rx_adaptive;

As a general rule of thumb, in any linux get callbacks, you only need
to set values if they are not 0. So you can skip the irqs.

> +static int efct_ethtool_set_coalesce(struct net_device *net_dev,
> +				     struct ethtool_coalesce *coalesce,
> +				     struct kernel_ethtool_coalesce *kernel_coal,
> +				     struct netlink_ext_ack *extack)
> +{
> +	struct efct_nic *efct = efct_netdev_priv(net_dev);
> +	struct efct_ev_queue *evq;
> +	u32 tx_usecs, rx_usecs;
> +	u32 timer_max_us;
> +	bool tx = false;
> +	bool rx = false;
> +	int i;
> +
> +	tx_usecs = 0;
> +	rx_usecs = 0;
> +	timer_max_us = efct->timer_max_ns / 1000;
> +	evq = efct->evq;
> +
> +	if (coalesce->rx_coalesce_usecs_irq || coalesce->tx_coalesce_usecs_irq) {
> +		netif_err(efct, drv, efct->net_dev, "Only rx/tx_coalesce_usecs are supported\n");

This is not an error. This is just userspace not knowing what you
hardware can do. netif_dbg(), or nothing at all.

> +		return -EINVAL;

EOPNOTSUP would be more accurate. Your hardware cannot do it, so it is
not supported.

> +static int efct_ethtool_set_ringparam(struct net_device *net_dev,
> +				      struct ethtool_ringparam *ring,
> +				      struct kernel_ethtool_ringparam *kring,
> +				      struct netlink_ext_ack *ext_ack)
> +{
> +	struct efct_nic *efct = efct_netdev_priv(net_dev);
> +	u32 entries_per_buff, min_rx_num_entries;
> +	bool if_up = false;
> +	int rc;
> +
> +	if (ring->tx_pending != efct->txq[0].num_entries) {
> +		netif_err(efct, drv, efct->net_dev,
> +			  "Tx ring size changes not supported\n");

netif_dbg()

> +		return -EOPNOTSUPP;
> +	}
> +
> +	if (ring->rx_pending == efct->rxq[0].num_entries)
> +		/* Nothing to do */
> +		return 0;
> +
> +	min_rx_num_entries = RX_MIN_DRIVER_BUFFS * DIV_ROUND_UP(efct->rxq[0].buffer_size,
> +								efct->rxq[0].pkt_stride);
> +	entries_per_buff = DIV_ROUND_UP(efct->rxq[0].buffer_size, efct->rxq[0].pkt_stride);
> +	if (ring->rx_pending % entries_per_buff || ring->rx_pending < min_rx_num_entries) {
> +		netif_err(efct, drv, efct->net_dev,
> +			  "Unsupported RX ring size. Should be multiple of %u and more than %u",
> +			  entries_per_buff, min_rx_num_entries);

netif_dbg()

> +		return -EINVAL;
> +	}
> +
> +	ASSERT_RTNL();
> +
> +	if (netif_running(net_dev)) {
> +		dev_close(net_dev);
> +		if_up = true;
> +	}
> +
> +	mutex_lock(&efct->state_lock);
> +	rc = efct_realloc_rx_evqs(efct, ring->rx_pending);
> +	mutex_unlock(&efct->state_lock);
> +
> +	if (rc) {
> +		netif_err(efct, drv, efct->net_dev,
> +			  "Failed reallocate rx evqs. Device disabled\n");

This not how it should work, precisely because of this problem of what
to do when there is no memory available. What you should do is first
allocate the memory needed for the new rings, and if that is
successful, free the old rings and swap to the new memory. If you
cannot allocate the new memory, no harm has been done, you still have
the rings, return -ENOMEM and life goes on.

> +int efct_mcdi_phy_set_ksettings(struct efct_nic *efct,
> +				const struct ethtool_link_ksettings *settings,
> +			       unsigned long *advertising)
> +{
> +	const struct ethtool_link_settings *base = &settings->base;
> +	struct efct_mcdi_phy_data *phy_cfg = efct->phy_data;
> +	u32 caps;
> +	int rc;
> +
> +	memcpy(advertising, settings->link_modes.advertising,
> +	       sizeof(__ETHTOOL_DECLARE_LINK_MODE_MASK()));

linkmode_copy()

> +
> +	/* Remove flow control settings that the MAC supports
> +	 * but that the PHY can't advertise.
> +	 */
> +	if (~phy_cfg->supported_cap & (1 << MC_CMD_PHY_CAP_PAUSE_LBN))
> +		__clear_bit(ETHTOOL_LINK_MODE_Pause_BIT, advertising);
> +	if (~phy_cfg->supported_cap & (1 << MC_CMD_PHY_CAP_ASYM_LBN))
> +		__clear_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT, advertising);

linkmode_clear_bit()

> +	.get_coalesce		= efct_ethtool_get_coalesce,
> +	.set_coalesce		= efct_ethtool_set_coalesce,
> +	.get_ringparam      = efct_ethtool_get_ringparam,
> +	.set_ringparam      = efct_ethtool_set_ringparam,

tab vs spaces issues. checkpatch.pl should of told you about that. You
are using checkpatch right?

    Andrew