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: <20230128015841.rotwc2arwgn2csef@skbuf>
Date:   Sat, 28 Jan 2023 03:58:41 +0200
From:   Vladimir Oltean <vladimir.oltean@....com>
To:     Maxime Chevallier <maxime.chevallier@...tlin.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

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 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.

Powered by blists - more mailing lists