linux-kernel - Re: [PATCH net-next] net: pcs: pcs-lynx: remove lynx_get_mdio

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] [day] [month] [year] [list]

Message-ID: <20230130191441.7aa46ea9@pc-7.home>
Date:   Mon, 30 Jan 2023 19:14:41 +0100
From:   Maxime Chevallier <maxime.chevallier@...tlin.com>
To:     Vladimir Oltean <vladimir.oltean@....com>
Cc:     "David S . Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Eric Dumazet <edumazet@...gle.com>,
        Florian Fainelli <f.fainelli@...il.com>,
        Andrew Lunn <andrew@...n.ch>,
        Alexandre Belloni <alexandre.belloni@...tlin.com>,
        Claudiu Manoil <claudiu.manoil@....com>,
        Paolo Abeni <pabeni@...hat.com>,
        Ioana Ciornei <ioana.ciornei@....com>,
        Madalin Bucur <madalin.bucur@....com>,
        Heiner Kallweit <hkallweit1@...il.com>,
        Russell King <linux@...linux.org.uk>,
        UNGLinuxDriver@...rochip.com, netdev@...r.kernel.org,
        linux-kernel@...r.kernel.org, thomas.petazzoni@...tlin.com
Subject: Re: [PATCH net-next] net: pcs: pcs-lynx: remove
 lynx_get_mdio_device() and refactor cleanup

Hello Vlad,

On Sat, 28 Jan 2023 03:58:41 +0200
Vladimir Oltean <vladimir.oltean@....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.