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

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

[Rasmus Villemoes]

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>
---
 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

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

[Andrew Lunn]

On Wed, Nov 23, 2022 at 02:38:52PM +0100, Rasmus Villemoes 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>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

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

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (master)
by David S. Miller <davem@davemloft.net>:

On Wed, 23 Nov 2022 14:38:52 +0100 you 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.
> 
> [...]

Here is the summary with links:
  - net: fec: don't reset irq coalesce settings to defaults on "ip link up"
    https://git.kernel.org/netdev/net/c/df727d4547de

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

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

[Greg Ungerer]

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

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

[Rasmus Villemoes]

On 05/12/2022 08.15, Greg Ungerer wrote:
> 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.

Sorry about that.

Since we no longer go through the same path that ethtool would, we need
to add a check of the FEC_QUIRK_HAS_COALESCE bit before calling
fec_enet_itr_coal_set() during initialization. So something like

diff --git a/drivers/net/ethernet/freescale/fec_main.c
b/drivers/net/ethernet/freescale/fec_main.c
index 93a116788ccc..3df1b9be033f 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -1186,7 +1186,8 @@ fec_restart(struct net_device *ndev)
                writel(0, fep->hwp + FEC_IMASK);

        /* Init the interrupt coalescing */
-       fec_enet_itr_coal_set(ndev);
+       if (fep->quirks & FEC_QUIRK_HAS_COALESCE)
+               fec_enet_itr_coal_set(ndev);
 }

Or perhaps it's even better to do the check inside
fec_enet_itr_coal_set() and just return silently?

Either way, I don't know if it's too late to apply this fix, or if
df727d4547 should just be reverted for 6.1 and then redone properly?
Greg, can you check if the above fixes it for you?

Rasmus

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

[Greg Ungerer]

On 5/12/22 18:44, Rasmus Villemoes wrote:
> On 05/12/2022 08.15, Greg Ungerer wrote:
>> 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.
> 
> Sorry about that.
> 
> Since we no longer go through the same path that ethtool would, we need
> to add a check of the FEC_QUIRK_HAS_COALESCE bit before calling
> fec_enet_itr_coal_set() during initialization. So something like
> 
> diff --git a/drivers/net/ethernet/freescale/fec_main.c
> b/drivers/net/ethernet/freescale/fec_main.c
> index 93a116788ccc..3df1b9be033f 100644
> --- a/drivers/net/ethernet/freescale/fec_main.c
> +++ b/drivers/net/ethernet/freescale/fec_main.c
> @@ -1186,7 +1186,8 @@ fec_restart(struct net_device *ndev)
>                  writel(0, fep->hwp + FEC_IMASK);
> 
>          /* Init the interrupt coalescing */
> -       fec_enet_itr_coal_set(ndev);
> +       if (fep->quirks & FEC_QUIRK_HAS_COALESCE)
> +               fec_enet_itr_coal_set(ndev);
>   }

That certainly fixes it.


> Or perhaps it's even better to do the check inside
> fec_enet_itr_coal_set() and just return silently?

That may well be better, yes.


> Either way, I don't know if it's too late to apply this fix, or if
> df727d4547 should just be reverted for 6.1 and then redone properly?
> Greg, can you check if the above fixes it for you?

Yep, that is good. I have no strong opinion on which way to handle
right now, so up to you.

Regards
Greg

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

[Andrew Lunn]

> Either way, I don't know if it's too late to apply this fix, or if
> df727d4547 should just be reverted for 6.1 and then redone properly?

Since the fix is simple, do the fix. Even if it misses 6.1.0 it will
be in 6.1.1.

   Andrew