On Sat, 18 Jul 2020 20:20:10 +0300 Claudiu Manoil wrote:
> On 17.07.2020 22:32, Jakub Kicinski wrote:
> > On Fri, 17 Jul 2020 18:37:03 +0300 Claudiu Manoil wrote:
> >> + if (ic->rx_max_coalesced_frames != ENETC_RXIC_PKTTHR)
> >> + netif_warn(priv, hw, ndev, "rx-frames fixed to %d\n",
> >> + ENETC_RXIC_PKTTHR);
> >> +
> >> + if (ic->tx_max_coalesced_frames != ENETC_TXIC_PKTTHR)
> >> + netif_warn(priv, hw, ndev, "tx-frames fixed to %d\n",
> >> + ENETC_TXIC_PKTTHR);
> >
> > On second thought - why not return an error here? Since only one value
> > is supported seems like the right way to communicate to the users that
> > they can't change this.
>
> Do you mean to return -EOPNOTSUPP without any error message instead?
Yes.
> If so, I think it's less punishing not to return an error code and
> invalidate the rest of the ethtool -C parameters that might have been
> correct if the user forgets that rx/tx-frames cannot be changed.
IMHO if configuring manually - user can correct the params on the CLI.
If there's an orchestration system trying to configure those - it's
better to return an error and alert the administrator than confuse
the orchestration by allowing a write which is then not reflected
on read.
> There's also this flag:
> .supported_coalesce_params = .. |
> ETHTOOL_COALESCE_MAX_FRAMES |
> ..,
> needed for printing the preconfigured values for the rx/tx packet
> thresholds, and this flag basically says that the 'rx/tx-frames'
> parameters are supported (just that they cannot be changed... :) ).
Right, unfortunately core can't do the checking here, but I think
the driver should.
> But I don't have a strong bias for this, if you prefer the return
> -EOPNOTSUPP option I'll make this change, just let me know if I got
> it right.
>
> >> + if (netif_running(ndev) && changed) {
> >> + /* reconfigure the operation mode of h/w interrupts,
> >> + * traffic needs to be paused in the process
> >> + */
> >> + enetc_stop(ndev);
> >> + enetc_start(ndev);
> >
> > Is start going to print an error when it fails? Kinda scary if this
> > could turn into a silent failure.
>
> enetc_start() returns void, just like enetc_stop(). If you look it up
> it only sets some run-time configurable registers and calls some basic
> run-time commands that should no fail like napi_enable(), enable_irq(),
> phy_start(), all void returning functions. This function doesn't
> allocate resources or anything of that sort, and should be kept that
> way. And indeed, it should not fail. But regarding error codes there's
> nothing I can do for this function, as nothing inside it generates any
> error code.
Got it!
