Messages in this thread | | | Date | Mon, 30 Jan 2023 19:14:41 +0100 | From | Maxime Chevallier <> | 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@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.
| |