Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.36 as permitted sender) client-ip=23.128.96.36;
Date:   Wed, 29 Nov 2023 09:35:37 +0000
From:   "Russell King (Oracle)" <linux@armlinux.org.uk>
To:     Christian Marangi <ansuelsmth@gmail.com>
Cc:     Andrew Lunn <andrew@lunn.ch>,
        Heiner Kallweit <hkallweit1@gmail.com>,
        "David S. Miller" <davem@davemloft.net>,
        Eric Dumazet <edumazet@google.com>,
        Jakub Kicinski <kuba@kernel.org>,
        Paolo Abeni <pabeni@redhat.com>,
        Andy Gross <agross@kernel.org>,
        Bjorn Andersson <andersson@kernel.org>,
        Konrad Dybcio <konrad.dybcio@linaro.org>,
        linux-kernel@vger.kernel.org, netdev@vger.kernel.org,
        linux-arm-msm@vger.kernel.org
Subject: Re: [net-next PATCH 06/14] net: phy: at803x: move at8031 specific
 data out of generic at803x_priv
Message-ID: <ZWcF6b/Py2gMmwmZ@shell.armlinux.org.uk>
References: <20231129021219.20914-1-ansuelsmth@gmail.com>
 <20231129021219.20914-7-ansuelsmth@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20231129021219.20914-7-ansuelsmth@gmail.com>
Sender: Russell King (Oracle) <linux@armlinux.org.uk>
Precedence: bulk

On Wed, Nov 29, 2023 at 03:12:11AM +0100, Christian Marangi wrote:
> Rework everything related to specific at8031 function to specific
> function and allocate the 2 bool, is_1000basex and is_fiber and the
> regulator structs to a dedicated qca8031_data struct.
> 
> This is needed to keep at803x functions more generic and detach them
> from specific check of at8031/33 PHY.
> 
> Out of all the reworked functions, only config_aneg required some code
> duplication with how the mdix config is handled.
> 
> This also reduces the generic at803x_priv struct by removing variables
> only used by at8031 PHY.

You are changing the order that register writes happen, e.g. for the
set_wol() method. at803x_set_wol() very clearly does stuff like
configuring the ethernet MAC address _before_ enabling WoL, and that
can fail. Your new code enables WoL and then calls at803x_set_wol().
If at803x_set_wol() fails (e.g. because of an invalid MAC address)
you leave WoL enabled. This is a change of behaviour.

I haven't checked anything else, but given the above, I think you
need to think more about how you make this change, and check
whether there are any other similar issues.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!