> -----Original Message-----
> From: Michal Kubecek <[email protected]>
> Sent: Thursday, June 18, 2020 2:17 PM
> To: [email protected]
> Cc: Kirsher, Jeffrey T <[email protected]>; [email protected];
> Michael, Alice <[email protected]>; [email protected];
> [email protected]; Brady, Alan <[email protected]>; Burra, Phani R
> <[email protected]>; Hay, Joshua A <[email protected]>; Chittim,
> Madhu <[email protected]>; Linga, Pavan Kumar
> <[email protected]>; Skidmore, Donald C
> <[email protected]>; Brandeburg, Jesse
> <[email protected]>; Samudrala, Sridhar
> <[email protected]>
> 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 <[email protected]>
> >
> > Implement ethtool interface for the common module.
> >
> > Signed-off-by: Alice Michael <[email protected]>
> > Signed-off-by: Alan Brady <[email protected]>
> > Signed-off-by: Phani Burra <[email protected]>
> > Signed-off-by: Joshua Hay <[email protected]>
> > Signed-off-by: Madhu Chittim <[email protected]>
> > Signed-off-by: Pavan Kumar Linga <[email protected]>
> > Reviewed-by: Donald Skidmore <[email protected]>
> > Reviewed-by: Jesse Brandeburg <[email protected]>
> > Reviewed-by: Sridhar Samudrala <[email protected]>
> > Signed-off-by: Jeff Kirsher <[email protected]>
> > ---
> [...]
> > +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.
>
You're right it's better to remove these. Will fix.
> > + 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.
>
Also agreed, will fix in next version.
> > + /* 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
>
I'm embarrassed we added a nice comment about stripping this because it doesn't
do anything and then failed to do just that. Agreed the priv flag stuff should
not be here without any private flags existing, will fix. Thanks for the
review.
Alan
