Re: [PATCH] net: fec: don't reset irq coalesce settings to defaults on "ip link up"

[Greg Ungerer <gregungerer@westnet.com.au>]

Hi Rasmus,

On 23 Nov 2022, Rasmus Villemoes <linux@rasmusvillemoes.dk> wrote:
> Currently, when a FEC device is brought up, the irq coalesce settings
> are reset to their default values (1000us, 200 frames). That's
> unexpected, and breaks for example use of an appropriate .link file to
> make systemd-udev apply the desired
> settings (https://www.freedesktop.org/software/systemd/man/systemd.link.html),
> or any other method that would do a one-time setup during early boot.
> 
> Refactor the code so that fec_restart() instead uses
> fec_enet_itr_coal_set(), which simply applies the settings that are
> stored in the private data, and initialize that private data with the
> default values.
> 
> Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>

This breaks The ColdFire parts that use the FEC hardware module at the
very least. It results in an access to a register (FEC_TXIC0) that does
not exist in the ColdFire FEC. Reverting this change fixes it.

So for me this is now broken in 6.1-rc8.

Regards
Greg


> ---
>  drivers/net/ethernet/freescale/fec_main.c | 22 ++++++----------------
>  1 file changed, 6 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
> index f623c12eaf95..2ca2b61b451f 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_init(struct net_device *ndev);
> +static void fec_enet_itr_coal_set(struct net_device *ndev);
>  
>  #define DRIVER_NAME	"fec"
>  
> @@ -1220,8 +1220,7 @@ fec_restart(struct net_device *ndev)
>  		writel(0, fep->hwp + FEC_IMASK);
>  
>  	/* Init the interrupt coalescing */
> -	fec_enet_itr_coal_init(ndev);
> -
> +	fec_enet_itr_coal_set(ndev);
>  }
>  
>  static int fec_enet_ipc_handle_init(struct fec_enet_private *fep)
> @@ -2856,19 +2855,6 @@ static int fec_enet_set_coalesce(struct net_device *ndev,
>  	return 0;
>  }
>  
> -static void fec_enet_itr_coal_init(struct net_device *ndev)
> -{
> -	struct ethtool_coalesce ec;
> -
> -	ec.rx_coalesce_usecs = FEC_ITR_ICTT_DEFAULT;
> -	ec.rx_max_coalesced_frames = FEC_ITR_ICFT_DEFAULT;
> -
> -	ec.tx_coalesce_usecs = FEC_ITR_ICTT_DEFAULT;
> -	ec.tx_max_coalesced_frames = FEC_ITR_ICFT_DEFAULT;
> -
> -	fec_enet_set_coalesce(ndev, &ec, NULL, NULL);
> -}
> -
>  static int fec_enet_get_tunable(struct net_device *netdev,
>  				const struct ethtool_tunable *tuna,
>  				void *data)
> @@ -3623,6 +3609,10 @@ static int fec_enet_init(struct net_device *ndev)
>  	fep->rx_align = 0x3;
>  	fep->tx_align = 0x3;
>  #endif
> +	fep->rx_pkts_itr = FEC_ITR_ICFT_DEFAULT;
> +	fep->tx_pkts_itr = FEC_ITR_ICFT_DEFAULT;
> +	fep->rx_time_itr = FEC_ITR_ICTT_DEFAULT;
> +	fep->tx_time_itr = FEC_ITR_ICTT_DEFAULT;
>  
>  	/* Check mask of the streaming and coherent API */
>  	ret = dma_set_mask_and_coherent(&fep->pdev->dev, DMA_BIT_MASK(32));
> -- 
> 2.37.2