> -----Original Message-----
> From: Jakub Kicinski <k...@kernel.org>
> Sent: Tuesday, July 21, 2020 11:28 AM
> To: Nguyen, Anthony L <anthony.l.ngu...@intel.com>
> Cc: da...@davemloft.net; Michael, Alice <alice.mich...@intel.com>;
> netdev@vger.kernel.org; nhor...@redhat.com; sassm...@redhat.com;
> Kirsher, Jeffrey T <jeffrey.t.kirs...@intel.com>; Brady, Alan
> <alan.br...@intel.com>; Burra, Phani R <phani.r.bu...@intel.com>; Hay,
> Joshua A <joshua.a....@intel.com>; Chittim, Madhu
> <madhu.chit...@intel.com>; Linga, Pavan Kumar
> <pavan.kumar.li...@intel.com>; Skidmore, Donald C
> <donald.c.skidm...@intel.com>; Brandeburg, Jesse
> <jesse.brandeb...@intel.com>; Samudrala, Sridhar
> <sridhar.samudr...@intel.com>
> Subject: Re: [net-next v4 13/15] iecm: Add ethtool
>
> On Mon, 20 Jul 2020 17:38:08 -0700 Tony Nguyen wrote:
> > +/**
> > + * iecm_set_rxfh - set the Rx flow hash indirection table
> > + * @netdev: network interface device structure
> > + * @indir: indirection table
> > + * @key: hash key
> > + * @hfunc: hash function to use
> > + *
> > + * Returns -EINVAL if the table specifies an invalid queue id,
> > +otherwise
> > + * returns 0 after programming the table.
> > + */
> > +static int iecm_set_rxfh(struct net_device *netdev, const u32 *indir,
> > + const u8 *key, const u8 hfunc)
> > +{
> > + struct iecm_vport *vport = iecm_netdev_to_vport(netdev);
> > + struct iecm_adapter *adapter;
> > + u16 *qid_list;
> > + u16 lut;
> > +
> > + adapter = vport->adapter;
> > +
> > + if (!iecm_is_cap_ena(adapter, VIRTCHNL_CAP_RSS)) {
> > + dev_info(&adapter->pdev->dev, "RSS is not supported on this
> device\n");
> > + return 0;
> > + }
> > + if (adapter->state != __IECM_UP)
> > + return 0;
> > +
> > + if (hfunc != ETH_RSS_HASH_NO_CHANGE && hfunc !=
> ETH_RSS_HASH_TOP)
> > + return -EOPNOTSUPP;
> > +
> > + if (key)
> > + memcpy(adapter->rss_data.rss_key, key,
> > + adapter->rss_data.rss_key_size);
> > +
> > + qid_list = kcalloc(vport->num_rxq, sizeof(u16), GFP_KERNEL);
> > + if (!qid_list)
> > + return -ENOMEM;
> > +
> > + iecm_get_rx_qid_list(vport, qid_list);
> > +
> > + if (indir) {
> > + for (lut = 0; lut < adapter->rss_data.rss_lut_size; lut++) {
> > + int index = indir[lut];
> > +
> > + if (index >= vport->num_rxq) {
> > + kfree(qid_list);
> > + return -EINVAL;
> > + }
>
> Core checks this already.
Will fix.
>
> > + adapter->rss_data.rss_lut[lut] = qid_list[index];
> > + }
> > + } else {
> > + iecm_fill_dflt_rss_lut(vport, qid_list);
>
> indir == NULL means no change, not reset.
>
Yes this is wrong, will fix.
> > + }
> > +
> > + kfree(qid_list);
> > +
> > + return iecm_config_rss(vport);
> > +}
> > +
> > +/**
> > + * iecm_get_channels: get the number of channels supported by the
> > +device
> > + * @netdev: network interface device structure
> > + * @ch: channel information structure
> > + *
> > + * Report maximum of TX and RX. Report one extra channel to match our
> > +mailbox
> > + * Queue.
> > + */
> > +static void iecm_get_channels(struct net_device *netdev,
> > + struct ethtool_channels *ch) {
> > + struct iecm_vport *vport = iecm_netdev_to_vport(netdev);
> > +
> > + /* Report maximum channels */
> > + ch->max_combined = IECM_MAX_Q;
> > +
> > + ch->max_other = IECM_MAX_NONQ;
> > + ch->other_count = IECM_MAX_NONQ;
> > +
> > + ch->combined_count = max(vport->num_txq, vport->num_rxq);
>
> If you allow different counts of rxq and txq - the calculation is
>
> combined = min(rxq, txq)
> rx = rxq - combined
> tx = txq - combined
>
> not very intuitive, but that's my interpretation of the API.
>
> Can rxq != txq?
>
Ultimately yes. We're still missing some of the finer details required to
manage buffers in this new split queue model so we're not quite ready to enable
that in set_channels, but yes I we should certainly fix the reporting in
get_channels to be accurate. Will fix.
> > +}
>
> > +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);
>
> I think we agreed to remove this, what happened?
>
My sincere apologies, not intentional. This will be gone in the next version.
-Alan
