Re: [PATCH net-next] net: pcs: pcs-lynx: remove lynx_get_mdio_device() and refactor cleanup

[Maxime Chevallier]

Hello Vlad,

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