netdev - Re: [net-next,v2,06/22] net: stmmac: rk: add SoC specific ->init() method

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <aXjq3VisP4XjWYSi@shell.armlinux.org.uk>
Date: Tue, 27 Jan 2026 16:42:05 +0000
From: "Russell King (Oracle)" <linux@...linux.org.uk>
To: Jakub Kicinski <kuba@...nel.org>
Cc: linux-stm32@...md-mailman.stormreply.com, andrew+netdev@...n.ch,
	linux-arm-kernel@...ts.infradead.org, netdev@...r.kernel.org,
	pabeni@...hat.com, linux-rockchip@...ts.infradead.org,
	heiko@...ech.de, alexandre.torgue@...s.st.com, andrew@...n.ch,
	davem@...emloft.net, edumazet@...gle.com
Subject: Re: [net-next,v2,06/22] net: stmmac: rk: add SoC specific ->init()
 method

On Tue, Jan 27, 2026 at 08:18:04AM -0800, Jakub Kicinski wrote:
> On Tue, 27 Jan 2026 11:09:58 +0000 Russell King (Oracle) wrote:
> > > > If ops->init() fails here, does bsp_priv->phy_reset need to be released?
> > > > 
> > > > Earlier in rk_gmac_setup(), at line 1610, the code calls:
> > > > 
> > > >     bsp_priv->phy_reset = of_reset_control_get(plat->phy_node, NULL);
> > > > 
> > > > of_reset_control_get() is not a devm-managed function, so the reset control
> > > > would need to be explicitly released with reset_control_put() on error paths.
> > > > 
> > > > This new error return happens after phy_reset could have been acquired (when
> > > > the device has an integrated PHY), but before the function successfully
> > > > returns. If this path is taken, it appears the reset control would be leaked.
> > > > 
> > > > Currently no SoC sets ops->init so this path cannot trigger, but when a
> > > > future SoC implements this callback, the leak would occur on init failure
> > > > for devices with integrated PHYs.  
> > > 
> > > However, the driver does not release this resource when cleaning up, so
> > > that's already a bug as the driver currently stands. I think this could
> > > be converted to devm_reset_control_get(), which would resolve both
> > > leakages, but not sure.  
> > 
> > Note that fixing this is going to add yet another patch to the series,
> > because this is a pre-existing bug in the driver. It can't be replaced
> > with devm_reset_control_get(), because this driver is getting resources
> > for a foreign device (we don't have the struct device pointer.)
> > 
> > So, it isn't going to be a simple patch to fix this.
> 
> Would it work to make that plus patches 1-4,6 a separate series?

The problem I have with that is the proximity of the merge window, and
the likelyhood of conflicting changes making the _entire_ series very
very very painful, because it changes all the individual SoC support.

If I'm going to have to split it up just for the sake of reducing the
cost of AI review, can I ask for a moritorium on other development
changes to dwmac-rk until this is merged?

As I see it, this is required _because_ of the introduction of AI
review, not because something has actually changed. You have said in
the past to me that the 15 patch limit is only advisory and can be
exceeded where it makes sense to, and for this series, it does make
sense.

Note that fixing _this_ problem has added yet another patch to the
series, as fixing the underlying issue (the lack of release of
bsp_priv->phy_reset on remove) is a separate issue. I've not yet
decided whether that needs to be submitted via the net tree or not,
which will further complicate the submission of this series if it
the fix needs to go through the net tree.


Author: Russell King (Oracle) <rmk+kernel@...linux.org.uk>

    net: stmmac: rk: fix missing reset_control_put()

    rk_gmac_setup() delves into the PHY's DT node to retrieve its reset
    control using of_reset_control_get(). However, it never releases it
    when the driver is removed. Add reset_control_put() to rk_gmac_exit()
    to clean this up.

    Signed-off-by: Russell King (Oracle) <rmk+kernel@...linux.org.uk>

diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c b/drivers/net/ethern
et/stmicro/stmmac/dwmac-rk.c
index 0e66252eb5ae..2237b09b50bb 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
@@ -1779,6 +1779,8 @@ static void rk_gmac_exit(struct device *dev, void *bsp_pri
v_)

        if (priv->plat->phy_node && bsp_priv->integrated_phy)
                clk_put(bsp_priv->clk_phy);
+
+       reset_control_put(bsp_priv->phy_reset);
 }

 static int rk_gmac_probe(struct platform_device *pdev)

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