| Message ID | E1q56y1-00Bsum-Hx@rmk-PC.armlinux.org.uk (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | complete Lynx mdio device handling | expand |
| Context | Check | Description |
|---|---|---|
| netdev/series_format | success | Posting correctly formatted |
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/fixes_present | success | Fixes tag not required for -next series |
| netdev/header_inline | success | No static functions without inline keyword in header files |
| netdev/build_32bit | success | Errors and warnings before: 8 this patch: 8 |
| netdev/cc_maintainers | success | CCed 9 of 9 maintainers |
| netdev/build_clang | success | Errors and warnings before: 8 this patch: 8 |
| netdev/verify_signedoff | success | Signed-off-by tag matches author and committer |
| netdev/deprecated_api | success | None detected |
| netdev/check_selftest | success | No net selftest shell script |
| netdev/verify_fixes | success | No Fixes tag |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 8 this patch: 8 |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 42 lines checked |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/source_inline | success | Was 0 now: 0 |
On 6/2/23 11:45, Russell King (Oracle) wrote: > Add a helper to create a lynx PCS from a fwnode handle. > > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> > --- > drivers/net/pcs/pcs-lynx.c | 29 +++++++++++++++++++++++++++++ > include/linux/pcs-lynx.h | 1 + > 2 files changed, 30 insertions(+) > > diff --git a/drivers/net/pcs/pcs-lynx.c b/drivers/net/pcs/pcs-lynx.c > index a90f74172f49..b0907c67d469 100644 > --- a/drivers/net/pcs/pcs-lynx.c > +++ b/drivers/net/pcs/pcs-lynx.c > @@ -353,6 +353,35 @@ struct phylink_pcs *lynx_pcs_create_mdiodev(struct mii_bus *bus, int addr) > } > EXPORT_SYMBOL(lynx_pcs_create_mdiodev); > > +struct phylink_pcs *lynx_pcs_create_fwnode(struct fwnode_handle *node) > +{ > + struct mdio_device *mdio; > + struct phylink_pcs *pcs; I think you should put the available check here as well. > + mdio = fwnode_mdio_find_device(node); > + if (!mdio) > + return ERR_PTR(-EPROBE_DEFER); > + > + pcs = lynx_pcs_create(mdio); > + > + /* Convert failure to create the PCS to an error pointer, so this > + * function has a consistent return value strategy. > + */ > + if (!pcs) > + pcs = ERR_PTR(-ENOMEM); > + > + /* lynx_create() has taken a refcount on the mdiodev if it was > + * successful. If lynx_create() fails, this will free the mdio > + * device here. In any case, we don't need to hold our reference > + * anymore, and putting it here will allow mdio_device_put() in > + * lynx_destroy() to automatically free the mdio device. > + */ > + mdio_device_put(mdio); > + > + return pcs; > +} > +EXPORT_SYMBOL_GPL(lynx_pcs_create_fwnode); > + > void lynx_pcs_destroy(struct phylink_pcs *pcs) > { > struct lynx_pcs *lynx = phylink_pcs_to_lynx(pcs); > diff --git a/include/linux/pcs-lynx.h b/include/linux/pcs-lynx.h > index 25f68a096bfe..123e813df771 100644 > --- a/include/linux/pcs-lynx.h > +++ b/include/linux/pcs-lynx.h > @@ -11,6 +11,7 @@ > > struct phylink_pcs *lynx_pcs_create(struct mdio_device *mdio); > struct phylink_pcs *lynx_pcs_create_mdiodev(struct mii_bus *bus, int addr); > +struct phylink_pcs *lynx_pcs_create_fwnode(struct fwnode_handle *node); > > void lynx_pcs_destroy(struct phylink_pcs *pcs); > Anyway, the rest of this series looks good to me. --Sean
On Fri, Jun 02, 2023 at 11:51:23AM -0400, Sean Anderson wrote: > On 6/2/23 11:45, Russell King (Oracle) wrote: > > Add a helper to create a lynx PCS from a fwnode handle. > > > > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> > > --- > > drivers/net/pcs/pcs-lynx.c | 29 +++++++++++++++++++++++++++++ > > include/linux/pcs-lynx.h | 1 + > > 2 files changed, 30 insertions(+) > > > > diff --git a/drivers/net/pcs/pcs-lynx.c b/drivers/net/pcs/pcs-lynx.c > > index a90f74172f49..b0907c67d469 100644 > > --- a/drivers/net/pcs/pcs-lynx.c > > +++ b/drivers/net/pcs/pcs-lynx.c > > @@ -353,6 +353,35 @@ struct phylink_pcs *lynx_pcs_create_mdiodev(struct mii_bus *bus, int addr) > > } > > EXPORT_SYMBOL(lynx_pcs_create_mdiodev); > > > > +struct phylink_pcs *lynx_pcs_create_fwnode(struct fwnode_handle *node) > > +{ > > + struct mdio_device *mdio; > > + struct phylink_pcs *pcs; > > I think you should put the available check here as well. Sorry, I totally missed your comment. Yes, that would also fix the refcount leak in memac_pcs_create(). I thought about that, but I decided against it because in dpaa2: if (!fwnode_device_is_available(node)) { netdev_err(mac->net_dev, "pcs-handle node not available\n"); fwnode_handle_put(node); return -ENODEV; } would become: if (IS_ERR(pcs)) { netdev_err(mac->net_dev, "lynx_pcs_create_fwnode() failed: %pe\n", pcs); If the device is not available, the error message changes from pcs-handle node not available to lynx_pcs_create_fwnode() failed: ENODEV which doesn't really say what the problem was. Is this something that the DPAA2 maintainers care about?
diff --git a/drivers/net/pcs/pcs-lynx.c b/drivers/net/pcs/pcs-lynx.c index a90f74172f49..b0907c67d469 100644 --- a/drivers/net/pcs/pcs-lynx.c +++ b/drivers/net/pcs/pcs-lynx.c @@ -353,6 +353,35 @@ struct phylink_pcs *lynx_pcs_create_mdiodev(struct mii_bus *bus, int addr) } EXPORT_SYMBOL(lynx_pcs_create_mdiodev); +struct phylink_pcs *lynx_pcs_create_fwnode(struct fwnode_handle *node) +{ + struct mdio_device *mdio; + struct phylink_pcs *pcs; + + mdio = fwnode_mdio_find_device(node); + if (!mdio) + return ERR_PTR(-EPROBE_DEFER); + + pcs = lynx_pcs_create(mdio); + + /* Convert failure to create the PCS to an error pointer, so this + * function has a consistent return value strategy. + */ + if (!pcs) + pcs = ERR_PTR(-ENOMEM); + + /* lynx_create() has taken a refcount on the mdiodev if it was + * successful. If lynx_create() fails, this will free the mdio + * device here. In any case, we don't need to hold our reference + * anymore, and putting it here will allow mdio_device_put() in + * lynx_destroy() to automatically free the mdio device. + */ + mdio_device_put(mdio); + + return pcs; +} +EXPORT_SYMBOL_GPL(lynx_pcs_create_fwnode); + void lynx_pcs_destroy(struct phylink_pcs *pcs) { struct lynx_pcs *lynx = phylink_pcs_to_lynx(pcs); diff --git a/include/linux/pcs-lynx.h b/include/linux/pcs-lynx.h index 25f68a096bfe..123e813df771 100644 --- a/include/linux/pcs-lynx.h +++ b/include/linux/pcs-lynx.h @@ -11,6 +11,7 @@ struct phylink_pcs *lynx_pcs_create(struct mdio_device *mdio); struct phylink_pcs *lynx_pcs_create_mdiodev(struct mii_bus *bus, int addr); +struct phylink_pcs *lynx_pcs_create_fwnode(struct fwnode_handle *node); void lynx_pcs_destroy(struct phylink_pcs *pcs);
Add a helper to create a lynx PCS from a fwnode handle. Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> --- drivers/net/pcs/pcs-lynx.c | 29 +++++++++++++++++++++++++++++ include/linux/pcs-lynx.h | 1 + 2 files changed, 30 insertions(+)