Re: [PATCH iwl-net v5] igc: Expose tx-usecs coalesce setting to user

[Simon Horman <horms@kernel.org>]

On Sun, Sep 10, 2023 at 02:41:50PM +0000, Zulkifli, Muhammad Husaini wrote:
> Dear Simon,
> 
> Thanks for reviewing. Replied inline
> 
> > -----Original Message-----
> > From: Simon Horman <horms@kernel.org>
> > Sent: Sunday, 10 September, 2023 10:24 PM
> > To: Zulkifli, Muhammad Husaini <muhammad.husaini.zulkifli@intel.com>
> > Cc: intel-wired-lan@osuosl.org; Neftin, Sasha <sasha.neftin@intel.com>;
> > bcreeley@amd.com; davem@davemloft.net; kuba@kernel.org;
> > pabeni@redhat.com; edumazet@google.com; netdev@vger.kernel.org;
> > naamax.meir@linux.intel.com; Nguyen, Anthony L
> > <anthony.l.nguyen@intel.com>; husainizulkifli@gmail.com
> > Subject: Re: [PATCH iwl-net v5] igc: Expose tx-usecs coalesce setting to user
> > 
> > On Fri, Sep 08, 2023 at 04:17:34PM +0800, Muhammad Husaini Zulkifli wrote:
> > > When users attempt to obtain the coalesce setting using the ethtool
> > > command, current code always returns 0 for tx-usecs.
> > > This is because I225/6 always uses a queue pair setting, hence
> > > tx_coalesce_usecs does not return a value during the
> > > igc_ethtool_get_coalesce() callback process. The pair queue condition
> > > checking in igc_ethtool_get_coalesce() is removed by this patch so
> > > that the user gets information of the value of tx-usecs.
> > >
> > > Even if i225/6 is using queue pair setting, there is no harm in
> > > notifying the user of the tx-usecs. The implementation of the current
> > > code may have previously been a copy of the legacy code i210.
> > > Since I225 has the queue pair setting enabled, tx-usecs will always
> > > adhere to the user-set rx-usecs value. An error message will appear
> > > when the user attempts to set the tx-usecs value for the input
> > > parameters because, by default, they should only set the rx-usecs value.
> > 
> > Hi Muhammad,
> > 
> > Most likely I'm misunderstanding things. And even if that is not the case
> > perhaps this is as good as it gets. But my reading is that an error will not be
> > raised if a user provides an input value for tx-usecs that matches the current
> > value of tx-usecs, even if a different value is provided for rx-usecs (which will
> > also be applied to tx-usecs).
> 
> Yes you are right. This is what I mentioned in previous version discussion.
> https://lore.kernel.org/netdev/20230905101504.4a9da6b8@kernel.org/
> But at least IMHO, better than current or my previous design submission.
> 
> Previously, I had considered changing the ".supported_coalesce_params"
> during ethtool set ops to only set ETHTOOL_COALESCE_RX_USECS with a new
> define of ETHTOOL_QUEUE_PAIR_COALESCE_USECS. But, if we change the
> queue/cpu during runtime setting, I believe this
> ".supported_coalesce_params" need to change as well...

Thanks Muhammad, and sorry for missing that thread.

With that discussion in mind, I think that what this patch does is as good
as it gets with the current uAPI, and changes to the uAPI is a follow-up
topic.

So, FWIIW, I am happy with this patch as it is.

Reviewed-by: Simon Horman <horms@kernel.org>