Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <72eb4e63-10a8-702b-1113-7588fcade9fc@rasmusvillemoes.dk>
Date:   Mon, 5 Dec 2022 09:44:29 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.4.2
Subject: Re: [PATCH] net: fec: don't reset irq coalesce settings to defaults
 on "ip link up"
Content-Language: en-US, da
To:     Greg Ungerer <gregungerer@westnet.com.au>
Cc:     Joakim Zhang <qiangqing.zhang@nxp.com>,
        "David S. Miller" <davem@davemloft.net>,
        Eric Dumazet <edumazet@google.com>,
        Jakub Kicinski <kuba@kernel.org>,
        Paolo Abeni <pabeni@redhat.com>, netdev@vger.kernel.org,
        linux-kernel@vger.kernel.org
References: <c69c1ff1-4da9-89f8-df2e-824cb7183fe9@westnet.com.au>
From:   Rasmus Villemoes <linux@rasmusvillemoes.dk>
In-Reply-To: <c69c1ff1-4da9-89f8-df2e-824cb7183fe9@westnet.com.au>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

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