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

From: Simon Horman <[email-masked]>
Date: Mon, 11 Sep 2023 07:55:20 +0200

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 <[email-masked]>
> > Sent: Sunday, 10 September, 2023 10:24 PM
> > To: Zulkifli, Muhammad Husaini <[email-masked]>
> > Cc: [email-masked]; Neftin, Sasha <[email-masked]>;
> > [email-masked]; [email-masked]; [email-masked];
> > [email-masked]; [email-masked]; [email-masked];
> > [email-masked]; Nguyen, Anthony L
> > <[email-masked]>; [email-masked]
> > 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/[email-masked]/
> 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 <[email-masked]>