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 [off-list ref] wrote:

On Fri, Jan 27, 2023 at 03:07:58PM +0100, Maxime Chevallier wrote:

quoted

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.