Date:   Mon, 30 Jan 2023 19:14:41 +0100
From:   Maxime Chevallier <maxime.chevallier@bootlin.com>
To:     Vladimir Oltean <vladimir.oltean@nxp.com>
Cc:     "David S . Miller" <davem@davemloft.net>,
        Jakub Kicinski <kuba@kernel.org>,
        Eric Dumazet <edumazet@google.com>,
        Florian Fainelli <f.fainelli@gmail.com>,
        Andrew Lunn <andrew@lunn.ch>,
        Alexandre Belloni <alexandre.belloni@bootlin.com>,
        Claudiu Manoil <claudiu.manoil@nxp.com>,
        Paolo Abeni <pabeni@redhat.com>,
        Ioana Ciornei <ioana.ciornei@nxp.com>,
        Madalin Bucur <madalin.bucur@nxp.com>,
        Heiner Kallweit <hkallweit1@gmail.com>,
        Russell King <linux@armlinux.org.uk>,
        UNGLinuxDriver@microchip.com, netdev@vger.kernel.org,
        linux-kernel@vger.kernel.org, thomas.petazzoni@bootlin.com
Subject: Re: [PATCH net-next] net: pcs: pcs-lynx: remove
 lynx_get_mdio_device() and refactor cleanup
Message-ID: <20230130191441.7aa46ea9@pc-7.home>
In-Reply-To: <20230128015841.rotwc2arwgn2csef@skbuf>
References: <20230127134031.156143-1-maxime.chevallier@bootlin.com>
        <20230127134351.xlz4wqrubfnvmecd@skbuf>
        <20230127150758.68eb1d29@pc-7.home>
        <20230128015841.rotwc2arwgn2csef@skbuf>
Organization: Bootlin
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Precedence: bulk

Hello Vlad,

On Sat, 28 Jan 2023 03:58:41 +0200
Vladimir Oltean <vladimir.oltean@nxp.com> wrote:

> On Fri, Jan 27, 2023 at 03:07:58PM +0100, Maxime Chevallier wrote:
> > However this current patch still makes sense though right ?  
> 
> I have a pretty hard time saying yes; TL;DR yes it's less code, but
> it's structured that way with a reason.
> 
> I don't think it's lynx_pcs_destroy()'s responsibility to call
> mdio_device_free(), just like it isn't lynx_pcs_create()'s
> responsibility to call mdio_device_create() (or whatever). In fact
> that's the reason why the mdiodev isn't completely absorbed by the
> lynx_pcs - because there isn't a unified way to get a reference to it
> - some platforms have a hardcoded address, others have a phandle in
> the device tree.

I get you and I actually agree, I've been also thinking about that this
weekend and indeed it would create an asymetry that can easily lead to
leaky code.

Let's drop that patch then, thanks a lot for the thourough review and
comments, I appreciate it.

Best regards,

Maxime

> I know this is entirely subjective, but to me, having functions
> organized in pairs which undo precisely what the other has done, and
> not more, really helps with spotting resource leakage issues. I
> realize that it's not the same for everybody. For example, while
> reviewing your patch, I noticed this in the existing code:
> 
> static struct phylink_pcs *memac_pcs_create(struct device_node
> *mac_node, int index)
> {
> 	struct device_node *node;
> 	struct mdio_device *mdiodev = NULL;
> 	struct phylink_pcs *pcs;
> 
> 	node = of_parse_phandle(mac_node, "pcsphy-handle", index);
> 	if (node && of_device_is_available(node))
> 		mdiodev = of_mdio_find_device(node);
> 	of_node_put(node);
> 
> 	if (!mdiodev)
> 		return ERR_PTR(-EPROBE_DEFER);
> 
> 	pcs = lynx_pcs_create(mdiodev); // if this fails, we miss
> calling mdio_device_free() return pcs;
> }
> 
> and it's clear that what is obvious to me was not obvious to the
> author of commit a7c2a32e7f22 ("net: fman: memac: Use lynx pcs
> driver"), since this organization scheme didn't work for him.