| Message ID | 20200505132905.10276-4-calvin.johnson@oss.nxp.com |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | Introduce new fwnode based APIs to support phylink and phy layers | expand |
On Tue, May 5, 2020 at 4:29 PM Calvin Johnson <calvin.johnson@oss.nxp.com> wrote: > > Define phylink_fwnode_phy_connect() to connect phy specified by > a fwnode to a phylink instance. ... > + int ret = 0; Redundant assignment. > + if ((IS_ERR(phy_fwnode)) && pl->cfg_link_an_mode == MLO_AN_PHY) No Lisp, please. > + return -ENODEV; ... > + phy_dev = fwnode_phy_find_device(phy_fwnode); > + fwnode_handle_put(phy_fwnode); Hmm... Isn't it racy? I mean if you put fwnode here the phy_dev may already be gone before you call phy_attach_direct, right? > + if (!phy_dev) > + return -ENODEV; > + > + ret = phy_attach_direct(pl->netdev, phy_dev, flags, > + pl->link_interface); > + if (ret) > + return ret; > + > + ret = phylink_bringup_phy(pl, phy_dev, pl->link_config.interface); > + if (ret) > + phy_detach(phy_dev); > + > + return ret;
On Tue, May 05, 2020 at 06:59:03PM +0530, Calvin Johnson wrote: > Define phylink_fwnode_phy_connect() to connect phy specified by > a fwnode to a phylink instance. > > Signed-off-by: Calvin Johnson <calvin.johnson@oss.nxp.com> > --- > > Changes in v3: > remove NULL return check as it is invalid > remove unused phylink_device_phy_connect() > > Changes in v2: > replace of_ and acpi_ code with generic fwnode to get phy-handle. > > drivers/net/phy/phylink.c | 48 +++++++++++++++++++++++++++++++++++++++ > include/linux/phylink.h | 3 +++ > 2 files changed, 51 insertions(+) > > diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c > index 0f23bec431c1..560d1069426c 100644 > --- a/drivers/net/phy/phylink.c > +++ b/drivers/net/phy/phylink.c > @@ -961,6 +961,54 @@ int phylink_connect_phy(struct phylink *pl, struct phy_device *phy) > } > EXPORT_SYMBOL_GPL(phylink_connect_phy); > > +/** > + * phylink_fwnode_phy_connect() - connect the PHY specified in the fwnode. > + * @pl: a pointer to a &struct phylink returned from phylink_create() > + * @fwnode: a pointer to a &struct fwnode_handle. > + * @flags: PHY-specific flags to communicate to the PHY device driver > + * > + * Connect the phy specified @fwnode to the phylink instance specified > + * by @pl. Actions specified in phylink_connect_phy() will be > + * performed. > + * > + * Returns 0 on success or a negative errno. > + */ > +int phylink_fwnode_phy_connect(struct phylink *pl, > + struct fwnode_handle *fwnode, > + u32 flags) > +{ > + struct fwnode_handle *phy_fwnode; > + struct phy_device *phy_dev; > + int ret = 0; > + > + /* Fixed links and 802.3z are handled without needing a PHY */ > + if (pl->cfg_link_an_mode == MLO_AN_FIXED || > + (pl->cfg_link_an_mode == MLO_AN_INBAND && > + phy_interface_mode_is_8023z(pl->link_interface))) > + return 0; > + > + phy_fwnode = fwnode_get_phy_node(fwnode); > + if ((IS_ERR(phy_fwnode)) && pl->cfg_link_an_mode == MLO_AN_PHY) > + return -ENODEV; This doesn't reflect the behaviour of phylink_of_phy_connect() - it is *not* a cleanup of what is there, which is: if (!phy_node) { if (pl->cfg_link_an_mode == MLO_AN_PHY) return -ENODEV; return 0; } which does: - if there is a PHY node, find the PHY and connect it. - if there is no PHY node, then: + if we are expecting a PHY to be present, return an error. + otherwise, it is not a problem, continue. That is very important behaviour - it allows drivers to call phylink_*_phy_connect() without knowing whether there should or should not be a PHY - and keeps that knowledge within phylink. It means network drivers don't have to parse the firmware to find out if there's a fixed link or SFP cage attached, and decide whether to call these functions. > + > + phy_dev = fwnode_phy_find_device(phy_fwnode); > + fwnode_handle_put(phy_fwnode); > + if (!phy_dev) > + return -ENODEV; > + > + ret = phy_attach_direct(pl->netdev, phy_dev, flags, > + pl->link_interface); > + if (ret) > + return ret; > + > + ret = phylink_bringup_phy(pl, phy_dev, pl->link_config.interface); > + if (ret) > + phy_detach(phy_dev); > + > + return ret; > +} > +EXPORT_SYMBOL_GPL(phylink_fwnode_phy_connect); > + I think we need to go further with this, and we need to have phylink_fwnode_phy_connect() functionally identical to phylink_of_phy_connect() for DT-based fwnodes. Doing so will avoid introducing errors such as the one you've added above. The only difference between these two is that DT has a number of legacy properties - these can be omitted if the fwnode is not a DT node. Remember that fwnode is compatible with DT, so fwnode_phy_find_device() can internally decide whether to look for the ACPI property or one of the three DT properties. It also means that phylink_of_phy_connect() can become: int phylink_of_phy_connect(struct phylink *pl, struct device_node *dn, u32 flags) { return phylink_fwnode_phy_connect(pl, of_fwnode_handle(dn), flags); }
diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c index 0f23bec431c1..560d1069426c 100644 --- a/drivers/net/phy/phylink.c +++ b/drivers/net/phy/phylink.c @@ -961,6 +961,54 @@ int phylink_connect_phy(struct phylink *pl, struct phy_device *phy) } EXPORT_SYMBOL_GPL(phylink_connect_phy); +/** + * phylink_fwnode_phy_connect() - connect the PHY specified in the fwnode. + * @pl: a pointer to a &struct phylink returned from phylink_create() + * @fwnode: a pointer to a &struct fwnode_handle. + * @flags: PHY-specific flags to communicate to the PHY device driver + * + * Connect the phy specified @fwnode to the phylink instance specified + * by @pl. Actions specified in phylink_connect_phy() will be + * performed. + * + * Returns 0 on success or a negative errno. + */ +int phylink_fwnode_phy_connect(struct phylink *pl, + struct fwnode_handle *fwnode, + u32 flags) +{ + struct fwnode_handle *phy_fwnode; + struct phy_device *phy_dev; + int ret = 0; + + /* Fixed links and 802.3z are handled without needing a PHY */ + if (pl->cfg_link_an_mode == MLO_AN_FIXED || + (pl->cfg_link_an_mode == MLO_AN_INBAND && + phy_interface_mode_is_8023z(pl->link_interface))) + return 0; + + phy_fwnode = fwnode_get_phy_node(fwnode); + if ((IS_ERR(phy_fwnode)) && pl->cfg_link_an_mode == MLO_AN_PHY) + return -ENODEV; + + phy_dev = fwnode_phy_find_device(phy_fwnode); + fwnode_handle_put(phy_fwnode); + if (!phy_dev) + return -ENODEV; + + ret = phy_attach_direct(pl->netdev, phy_dev, flags, + pl->link_interface); + if (ret) + return ret; + + ret = phylink_bringup_phy(pl, phy_dev, pl->link_config.interface); + if (ret) + phy_detach(phy_dev); + + return ret; +} +EXPORT_SYMBOL_GPL(phylink_fwnode_phy_connect); + /** * phylink_of_phy_connect() - connect the PHY specified in the DT mode. * @pl: a pointer to a &struct phylink returned from phylink_create() diff --git a/include/linux/phylink.h b/include/linux/phylink.h index cc5b452a184e..107fb0afc3bb 100644 --- a/include/linux/phylink.h +++ b/include/linux/phylink.h @@ -367,6 +367,9 @@ void phylink_add_pcs(struct phylink *, const struct phylink_pcs_ops *ops); void phylink_destroy(struct phylink *); int phylink_connect_phy(struct phylink *, struct phy_device *); +int phylink_fwnode_phy_connect(struct phylink *pl, + struct fwnode_handle *fwnode, + u32 flags); int phylink_of_phy_connect(struct phylink *, struct device_node *, u32 flags); void phylink_disconnect_phy(struct phylink *);
Define phylink_fwnode_phy_connect() to connect phy specified by a fwnode to a phylink instance. Signed-off-by: Calvin Johnson <calvin.johnson@oss.nxp.com> --- Changes in v3: remove NULL return check as it is invalid remove unused phylink_device_phy_connect() Changes in v2: replace of_ and acpi_ code with generic fwnode to get phy-handle. drivers/net/phy/phylink.c | 48 +++++++++++++++++++++++++++++++++++++++ include/linux/phylink.h | 3 +++ 2 files changed, 51 insertions(+)