[RFC] net: fec: Allow turning off IRQ coalescing

  Setting tx/rx-frames or tx/rx-usecs to zero is currently possible but
has no effect.
Also IRQ coalescing is always enabled on supported hardware.

This is confusing and causes users to believe that they have successfully
disabled IRQ coalescing by setting tx/rx-frames and tx/rx-usecs to zero.

With this change applied it is possible to disable IRQ coalescing by
configuring both tx/rx-frames and tx/rx-usecs to zero.

Setting only one value to zero is still not possible as the hardware
does not support it.
In this case ethtool will face -EINVAL.

Signed-off-by: Richard Weinberger <richard@nod.at>
---
 drivers/net/ethernet/freescale/fec_main.c | 73 ++++++++++++++++-------
 1 file changed, 50 insertions(+), 23 deletions(-)
  

>  /* Set threshold for interrupt coalescing */
> -static void fec_enet_itr_coal_set(struct net_device *ndev)
> +static int fec_enet_itr_coal_set(struct net_device *ndev)
>  {
> +	bool disable_rx_itr = false, disable_tx_itr = false;
>  	struct fec_enet_private *fep = netdev_priv(ndev);
> -	int rx_itr, tx_itr;
> +	struct device *dev = &fep->pdev->dev;
> +	int rx_itr = 0, tx_itr = 0;
>  
> -	/* Must be greater than zero to avoid unpredictable behavior */
> -	if (!fep->rx_time_itr || !fep->rx_pkts_itr ||
> -	    !fep->tx_time_itr || !fep->tx_pkts_itr)
> -		return;
> +	if (!fep->rx_time_itr || !fep->rx_pkts_itr) {
> +		if (fep->rx_time_itr || fep->rx_pkts_itr) {
> +			dev_warn(dev, "Rx coalesced frames and usec have to be "
> +				      "both positive or both zero to disable Rx "
> +				      "coalescence completely\n");
> +			return -EINVAL;
> +		}

Hi Richard

Why do this validation here, and not in fec_enet_set_coalesce() where
there are already checks? fec_enet_set_coalesce() also has extack, so
you can return useful messages to user space, not just the kernel log.

    Andrew
  

> -----Original Message-----
> From: Richard Weinberger <richard@nod.at>
> Sent: 2023年2月19日 5:41
> To: netdev@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org; pabeni@redhat.com; kuba@kernel.org;
> edumazet@google.com; davem@davemloft.net; dl-linux-imx
> <linux-imx@nxp.com>; Clark Wang <xiaoning.wang@nxp.com>; Shenwei
> Wang <shenwei.wang@nxp.com>; Wei Fang <wei.fang@nxp.com>; Richard
> Weinberger <richard@nod.at>
> Subject: [PATCH] [RFC] net: fec: Allow turning off IRQ coalescing
> 
> Setting tx/rx-frames or tx/rx-usecs to zero is currently possible but
> has no effect.
> Also IRQ coalescing is always enabled on supported hardware.
> 
> This is confusing and causes users to believe that they have successfully
> disabled IRQ coalescing by setting tx/rx-frames and tx/rx-usecs to zero.
> 
> With this change applied it is possible to disable IRQ coalescing by
> configuring both tx/rx-frames and tx/rx-usecs to zero.
> 
> Setting only one value to zero is still not possible as the hardware
> does not support it.
> In this case ethtool will face -EINVAL.
> 
> Signed-off-by: Richard Weinberger <richard@nod.at>
> ---
>  drivers/net/ethernet/freescale/fec_main.c | 73 ++++++++++++++++-------
>  1 file changed, 50 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/net/ethernet/freescale/fec_main.c
> b/drivers/net/ethernet/freescale/fec_main.c
> index 2341597408d1..cc3c5e09e02f 100644
> --- a/drivers/net/ethernet/freescale/fec_main.c
> +++ b/drivers/net/ethernet/freescale/fec_main.c
> @@ -74,7 +74,7 @@
>  #include "fec.h"
> 
>  static void set_multicast_list(struct net_device *ndev);
> -static void fec_enet_itr_coal_set(struct net_device *ndev);
> +static int fec_enet_itr_coal_set(struct net_device *ndev);
> 
>  #define DRIVER_NAME	"fec"
> 
> @@ -1217,7 +1217,7 @@ fec_restart(struct net_device *ndev)
> 
>  	/* Init the interrupt coalescing */
>  	if (fep->quirks & FEC_QUIRK_HAS_COALESCE)
> -		fec_enet_itr_coal_set(ndev);
> +		WARN_ON_ONCE(fec_enet_itr_coal_set(ndev));
>  }
> 
>  static int fec_enet_ipc_handle_init(struct fec_enet_private *fep)
> @@ -2867,30 +2867,57 @@ static int fec_enet_us_to_itr_clock(struct
> net_device *ndev, int us)
>  }
> 
>  /* Set threshold for interrupt coalescing */
> -static void fec_enet_itr_coal_set(struct net_device *ndev)
> +static int fec_enet_itr_coal_set(struct net_device *ndev)
>  {
> +	bool disable_rx_itr = false, disable_tx_itr = false;
>  	struct fec_enet_private *fep = netdev_priv(ndev);
disable_rx_itr should be defined below fep to follow the style of the reverse Christmas tree.

> -	int rx_itr, tx_itr;
> +	struct device *dev = &fep->pdev->dev;
> +	int rx_itr = 0, tx_itr = 0;
> 
> -	/* Must be greater than zero to avoid unpredictable behavior */
> -	if (!fep->rx_time_itr || !fep->rx_pkts_itr ||
> -	    !fep->tx_time_itr || !fep->tx_pkts_itr)
> -		return;
> +	if (!fep->rx_time_itr || !fep->rx_pkts_itr) {
> +		if (fep->rx_time_itr || fep->rx_pkts_itr) {

I think the below should be better:
if (!!fep->rx_time_itr == ! fep->rx_pkts_itr)

> +			dev_warn(dev, "Rx coalesced frames and usec have to be "
> +				      "both positive or both zero to disable Rx "
> +				      "coalescence completely\n");
> +			return -EINVAL;
> +		}
> 
> -	/* Select enet system clock as Interrupt Coalescing
> -	 * timer Clock Source
> -	 */
> -	rx_itr = FEC_ITR_CLK_SEL;
> -	tx_itr = FEC_ITR_CLK_SEL;
> +		disable_rx_itr = true;
> +	}
> 
> -	/* set ICFT and ICTT */
> -	rx_itr |= FEC_ITR_ICFT(fep->rx_pkts_itr);
> -	rx_itr |= FEC_ITR_ICTT(fec_enet_us_to_itr_clock(ndev, fep->rx_time_itr));
> -	tx_itr |= FEC_ITR_ICFT(fep->tx_pkts_itr);
> -	tx_itr |= FEC_ITR_ICTT(fec_enet_us_to_itr_clock(ndev, fep->tx_time_itr));
> +	if (!fep->tx_time_itr || !fep->tx_pkts_itr) {
> +		if (fep->tx_time_itr || fep->tx_pkts_itr) {

Same as above.

> +			dev_warn(dev, "Tx coalesced frames and usec have to be "
> +				      "both positive or both zero to disable Tx "
> +				      "coalescence completely\n");
> +			return -EINVAL;
> +		}
> +
> +		disable_tx_itr = true;
> +	}
> +
> +	if (!disable_rx_itr) {
> +		/* Select enet system clock as Interrupt Coalescing
> +		 * timer Clock Source
> +		 */
> +		rx_itr = FEC_ITR_CLK_SEL;
> +
> +		/* set ICFT and ICTT */
> +		rx_itr |= FEC_ITR_ICFT(fep->rx_pkts_itr);
> +		rx_itr |= FEC_ITR_ICTT(fec_enet_us_to_itr_clock(ndev,
> fep->rx_time_itr));
> +
> +		rx_itr |= FEC_ITR_EN;
> +	}
> +
> +	if (!disable_tx_itr) {
> +		tx_itr = FEC_ITR_CLK_SEL;
> +
> +		tx_itr |= FEC_ITR_ICFT(fep->tx_pkts_itr);
> +		tx_itr |= FEC_ITR_ICTT(fec_enet_us_to_itr_clock(ndev,
> fep->tx_time_itr));
> +
> +		tx_itr |= FEC_ITR_EN;
> +	}
> 
> -	rx_itr |= FEC_ITR_EN;
> -	tx_itr |= FEC_ITR_EN;
> 
>  	writel(tx_itr, fep->hwp + FEC_TXIC0);
>  	writel(rx_itr, fep->hwp + FEC_RXIC0);
> @@ -2900,6 +2927,8 @@ static void fec_enet_itr_coal_set(struct net_device
> *ndev)
>  		writel(tx_itr, fep->hwp + FEC_TXIC2);
>  		writel(rx_itr, fep->hwp + FEC_RXIC2);
>  	}
> +
> +	return 0;
>  }
> 
>  static int fec_enet_get_coalesce(struct net_device *ndev,
> @@ -2961,9 +2990,7 @@ static int fec_enet_set_coalesce(struct net_device
> *ndev,
>  	fep->tx_time_itr = ec->tx_coalesce_usecs;
>  	fep->tx_pkts_itr = ec->tx_max_coalesced_frames;
> 
> -	fec_enet_itr_coal_set(ndev);
> -
> -	return 0;
> +	return fec_enet_itr_coal_set(ndev);
>  }
> 
>  static int fec_enet_get_tunable(struct net_device *netdev,
> --
> 2.26.2
  

Andrew,

----- Ursprüngliche Mail -----
>> -	/* Must be greater than zero to avoid unpredictable behavior */
>> -	if (!fep->rx_time_itr || !fep->rx_pkts_itr ||
>> -	    !fep->tx_time_itr || !fep->tx_pkts_itr)
>> -		return;
>> +	if (!fep->rx_time_itr || !fep->rx_pkts_itr) {
>> +		if (fep->rx_time_itr || fep->rx_pkts_itr) {
>> +			dev_warn(dev, "Rx coalesced frames and usec have to be "
>> +				      "both positive or both zero to disable Rx "
>> +				      "coalescence completely\n");
>> +			return -EINVAL;
>> +		}
> 
> Hi Richard
> 
> Why do this validation here, and not in fec_enet_set_coalesce() where
> there are already checks? fec_enet_set_coalesce() also has extack, so
> you can return useful messages to user space, not just the kernel log.

Using extack is a good point, the driver does not use it at all so far.
So I'd do a second patch which cleans this up.

I did the check in fec_enet_itr_coal_set() because the check is used to
set both disable_rx_itr and disable_tx_itr.
Of course I can place the check into fec_enet_set_coalesce() and then
pass disable_rx_itr and disable_tx_itr to fec_enet_itr_coal_set().

Thanks,
//richard
  

Wei Fang,

----- Ursprüngliche Mail -----
> Von: "wei fang" <wei.fang@nxp.com>
>>  /* Set threshold for interrupt coalescing */
>> -static void fec_enet_itr_coal_set(struct net_device *ndev)
>> +static int fec_enet_itr_coal_set(struct net_device *ndev)
>>  {
>> +	bool disable_rx_itr = false, disable_tx_itr = false;
>>  	struct fec_enet_private *fep = netdev_priv(ndev);
> disable_rx_itr should be defined below fep to follow the style of the reverse
> Christmas tree.

Of course, will fix in v2.
 
>> -	int rx_itr, tx_itr;
>> +	struct device *dev = &fep->pdev->dev;
>> +	int rx_itr = 0, tx_itr = 0;
>> 
>> -	/* Must be greater than zero to avoid unpredictable behavior */
>> -	if (!fep->rx_time_itr || !fep->rx_pkts_itr ||
>> -	    !fep->tx_time_itr || !fep->tx_pkts_itr)
>> -		return;
>> +	if (!fep->rx_time_itr || !fep->rx_pkts_itr) {
>> +		if (fep->rx_time_itr || fep->rx_pkts_itr) {
> 
> I think the below should be better:
> if (!!fep->rx_time_itr == ! fep->rx_pkts_itr)

At least it's shorter. :-)
I'm not sure which variant is easier to understand, though.

But in general you are fine with returning -EINVAL in this case?
I'm asking because that a userspace visible change.

Thanks,
//richard
  

> -----Original Message-----
> From: Richard Weinberger <richard@nod.at>
> Sent: 2023年2月21日 4:15
> To: Wei Fang <wei.fang@nxp.com>
> Cc: netdev <netdev@vger.kernel.org>; linux-kernel
> <linux-kernel@vger.kernel.org>; pabeni <pabeni@redhat.com>; kuba
> <kuba@kernel.org>; edumazet <edumazet@google.com>; davem
> <davem@davemloft.net>; dl-linux-imx <linux-imx@nxp.com>; Clark Wang
> <xiaoning.wang@nxp.com>; Shenwei Wang <shenwei.wang@nxp.com>
> Subject: Re: [PATCH] [RFC] net: fec: Allow turning off IRQ coalescing
> 
> Wei Fang,
> 
> ----- Ursprüngliche Mail -----
> > Von: "wei fang" <wei.fang@nxp.com>
> >>  /* Set threshold for interrupt coalescing */ -static void
> >> fec_enet_itr_coal_set(struct net_device *ndev)
> >> +static int fec_enet_itr_coal_set(struct net_device *ndev)
> >>  {
> >> +	bool disable_rx_itr = false, disable_tx_itr = false;
> >>  	struct fec_enet_private *fep = netdev_priv(ndev);
> > disable_rx_itr should be defined below fep to follow the style of the
> > reverse Christmas tree.
> 
> Of course, will fix in v2.
> 
> >> -	int rx_itr, tx_itr;
> >> +	struct device *dev = &fep->pdev->dev;
> >> +	int rx_itr = 0, tx_itr = 0;
> >>
> >> -	/* Must be greater than zero to avoid unpredictable behavior */
> >> -	if (!fep->rx_time_itr || !fep->rx_pkts_itr ||
> >> -	    !fep->tx_time_itr || !fep->tx_pkts_itr)
> >> -		return;
> >> +	if (!fep->rx_time_itr || !fep->rx_pkts_itr) {
> >> +		if (fep->rx_time_itr || fep->rx_pkts_itr) {
> >
> > I think the below should be better:
> > if (!!fep->rx_time_itr == ! fep->rx_pkts_itr)
> 
> At least it's shorter. :-)
> I'm not sure which variant is easier to understand, though.
> 
> But in general you are fine with returning -EINVAL in this case?
> I'm asking because that a userspace visible change.
> 
I think it's fine to return -EINVAL. For hardware, one of the two parameters
is 0 is an invalid value with the interrupt coalescing enabled, and the behavior
of hardware is unpredictable.