| Message ID | 20210208151244.16338-16-calvin.johnson@oss.nxp.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | ACPI support for dpaa2 driver | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/subject_prefix | success | Link |
| netdev/cc_maintainers | success | CCed 6 of 6 maintainers |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | warning | WARNING: line length of 82 exceeds 80 columns WARNING: line length of 85 exceeds 80 columns |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/header_inline | success | Link |
| netdev/stable | success | Stable not CCed |
On Mon, Feb 8, 2021 at 5:15 PM Calvin Johnson <calvin.johnson@oss.nxp.com> wrote: > > Modify dpaa2_mac_connect() to support ACPI along with DT. > Modify dpaa2_mac_get_node() to get the dpmac fwnode from either > DT or ACPI. > > Replace of_get_phy_mode with fwnode_get_phy_mode to get > phy-mode for a dpmac_node. > > Use helper function phylink_fwnode_phy_connect() to find phy_dev and > connect to mac->phylink. ... > + if (is_of_node(dev->parent->fwnode)) { > + dpmacs = of_find_node_by_name(NULL, "dpmacs"); > + if (!dpmacs) > + return NULL; > + parent = of_fwnode_handle(dpmacs); > + } else if (is_acpi_node(dev->parent->fwnode)) { > + parent = dev->parent->fwnode; dev_fwnode(dev->parent) ? > + } ... > + if (err) { > continue; > + } else if (id == dpmac_id) { Useless 'else' > + if (is_of_node(dev->parent->fwnode)) dev_fwnode() ? > + of_node_put(dpmacs); > + return child; > + } ... > + if (is_of_node(dev->parent->fwnode)) Ditto ? > + of_node_put(dpmacs); -- With Best Regards, Andy Shevchenko
On Mon, Feb 08, 2021 at 08:42:44PM +0530, Calvin Johnson wrote: > Modify dpaa2_mac_connect() to support ACPI along with DT. > Modify dpaa2_mac_get_node() to get the dpmac fwnode from either > DT or ACPI. > > Replace of_get_phy_mode with fwnode_get_phy_mode to get > phy-mode for a dpmac_node. > > Use helper function phylink_fwnode_phy_connect() to find phy_dev and > connect to mac->phylink. > > Signed-off-by: Calvin Johnson <calvin.johnson@oss.nxp.com> I don't think this does the full job. > static int dpaa2_pcs_create(struct dpaa2_mac *mac, > - struct device_node *dpmac_node, int id) > + struct fwnode_handle *dpmac_node, > + int id) > { > struct mdio_device *mdiodev; > - struct device_node *node; > + struct fwnode_handle *node; > > - node = of_parse_phandle(dpmac_node, "pcs-handle", 0); > - if (!node) { > + node = fwnode_find_reference(dpmac_node, "pcs-handle", 0); > + if (IS_ERR(node)) { > /* do not error out on old DTS files */ > netdev_warn(mac->net_dev, "pcs-handle node not found\n"); > return 0; > } > > - if (!of_device_is_available(node)) { > + if (!of_device_is_available(to_of_node(node))) { If "node" is an ACPI node, then to_of_node() returns NULL, and of_device_is_available(NULL) is false. So, if we're using ACPI and we enter this path, we will always hit the error below: > netdev_err(mac->net_dev, "pcs-handle node not available\n"); > - of_node_put(node); > + of_node_put(to_of_node(node)); > return -ENODEV; > } > @@ -306,7 +321,7 @@ int dpaa2_mac_connect(struct dpaa2_mac *mac) > * error out if the interface mode requests them and there is no PHY > * to act upon them > */ > - if (of_phy_is_fixed_link(dpmac_node) && > + if (of_phy_is_fixed_link(to_of_node(dpmac_node)) && If "dpmac_node" is an ACPI node, to_of_node() will return NULL, and of_phy_is_fixed_link() will oops.
On Mon, Feb 08, 2021 at 04:28:31PM +0000, Russell King - ARM Linux admin wrote: > On Mon, Feb 08, 2021 at 08:42:44PM +0530, Calvin Johnson wrote: > > Modify dpaa2_mac_connect() to support ACPI along with DT. > > Modify dpaa2_mac_get_node() to get the dpmac fwnode from either > > DT or ACPI. > > > > Replace of_get_phy_mode with fwnode_get_phy_mode to get > > phy-mode for a dpmac_node. > > > > Use helper function phylink_fwnode_phy_connect() to find phy_dev and > > connect to mac->phylink. > > > > Signed-off-by: Calvin Johnson <calvin.johnson@oss.nxp.com> > > I don't think this does the full job. > > > static int dpaa2_pcs_create(struct dpaa2_mac *mac, > > - struct device_node *dpmac_node, int id) > > + struct fwnode_handle *dpmac_node, > > + int id) > > { > > struct mdio_device *mdiodev; > > - struct device_node *node; > > + struct fwnode_handle *node; > > > > - node = of_parse_phandle(dpmac_node, "pcs-handle", 0); > > - if (!node) { > > + node = fwnode_find_reference(dpmac_node, "pcs-handle", 0); > > + if (IS_ERR(node)) { > > /* do not error out on old DTS files */ > > netdev_warn(mac->net_dev, "pcs-handle node not found\n"); > > return 0; > > } > > > > - if (!of_device_is_available(node)) { > > + if (!of_device_is_available(to_of_node(node))) { > > If "node" is an ACPI node, then to_of_node() returns NULL, and > of_device_is_available(NULL) is false. So, if we're using ACPI > and we enter this path, we will always hit the error below: > > > netdev_err(mac->net_dev, "pcs-handle node not available\n"); > > - of_node_put(node); > > + of_node_put(to_of_node(node)); > > return -ENODEV; > > } > > > @@ -306,7 +321,7 @@ int dpaa2_mac_connect(struct dpaa2_mac *mac) > > * error out if the interface mode requests them and there is no PHY > > * to act upon them > > */ > > - if (of_phy_is_fixed_link(dpmac_node) && > > + if (of_phy_is_fixed_link(to_of_node(dpmac_node)) && > > If "dpmac_node" is an ACPI node, to_of_node() will return NULL, and > of_phy_is_fixed_link() will oops. I think of_phy_is_fixed_link() needs to be fixed. I'll add below fix. --- a/drivers/net/mdio/of_mdio.c +++ b/drivers/net/mdio/of_mdio.c @@ -439,6 +439,9 @@ bool of_phy_is_fixed_link(struct device_node *np) int len, err; const char *managed; + if (!np) + return false; + /* New binding */ dn = of_get_child_by_name(np, "fixed-link"); if (dn) { Regards Calvin
On Mon, Feb 15, 2021 at 2:33 PM Calvin Johnson <calvin.johnson@oss.nxp.com> wrote: > On Mon, Feb 08, 2021 at 04:28:31PM +0000, Russell King - ARM Linux admin wrote: ... > I think of_phy_is_fixed_link() needs to be fixed. I'll add below fix. > > --- a/drivers/net/mdio/of_mdio.c > +++ b/drivers/net/mdio/of_mdio.c > @@ -439,6 +439,9 @@ bool of_phy_is_fixed_link(struct device_node *np) > int len, err; > const char *managed; > > + if (!np) > + return false; AFAICS this doesn't add anything: all of the of_* APIs should handle OF nodes being NULL below. > /* New binding */ > dn = of_get_child_by_name(np, "fixed-link"); > if (dn) {
On Mon, Feb 15, 2021 at 5:13 PM Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > On Mon, Feb 15, 2021 at 2:33 PM Calvin Johnson > <calvin.johnson@oss.nxp.com> wrote: > > On Mon, Feb 08, 2021 at 04:28:31PM +0000, Russell King - ARM Linux admin wrote: > > ... > > > I think of_phy_is_fixed_link() needs to be fixed. I'll add below fix. > > > > --- a/drivers/net/mdio/of_mdio.c > > +++ b/drivers/net/mdio/of_mdio.c > > @@ -439,6 +439,9 @@ bool of_phy_is_fixed_link(struct device_node *np) > > int len, err; > > const char *managed; > > > > + if (!np) > > + return false; > > AFAICS this doesn't add anything: all of the of_* APIs should handle > OF nodes being NULL below. > > > /* New binding */ > > dn = of_get_child_by_name(np, "fixed-link"); > > if (dn) { Yes, of_get_next_child() and of_get_property() are NULL aware. So, the check is redundant.
On Mon, Feb 15, 2021 at 05:15:36PM +0200, Andy Shevchenko wrote: > On Mon, Feb 15, 2021 at 5:13 PM Andy Shevchenko > <andy.shevchenko@gmail.com> wrote: > > > > On Mon, Feb 15, 2021 at 2:33 PM Calvin Johnson > > <calvin.johnson@oss.nxp.com> wrote: > > > On Mon, Feb 08, 2021 at 04:28:31PM +0000, Russell King - ARM Linux admin wrote: > > > > ... > > > > > I think of_phy_is_fixed_link() needs to be fixed. I'll add below fix. > > > > > > --- a/drivers/net/mdio/of_mdio.c > > > +++ b/drivers/net/mdio/of_mdio.c > > > @@ -439,6 +439,9 @@ bool of_phy_is_fixed_link(struct device_node *np) > > > int len, err; > > > const char *managed; > > > > > > + if (!np) > > > + return false; > > > > AFAICS this doesn't add anything: all of the of_* APIs should handle > > OF nodes being NULL below. > > > > > /* New binding */ > > > dn = of_get_child_by_name(np, "fixed-link"); > > > if (dn) { > > Yes, of_get_next_child() and of_get_property() are NULL aware. > > So, the check is redundant. Yes, all the function calls in of_phy_is_fixed_link() handles NULL properly. I don't see any way this function can oops. Regards Calvin
diff --git a/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c b/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c index ccaf7e35abeb..87bb49722611 100644 --- a/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c +++ b/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c @@ -1,6 +1,9 @@ // SPDX-License-Identifier: (GPL-2.0+ OR BSD-3-Clause) /* Copyright 2019 NXP */ +#include <linux/acpi.h> +#include <linux/property.h> + #include "dpaa2-eth.h" #include "dpaa2-mac.h" @@ -34,39 +37,51 @@ static int phy_mode(enum dpmac_eth_if eth_if, phy_interface_t *if_mode) return 0; } -/* Caller must call of_node_put on the returned value */ -static struct device_node *dpaa2_mac_get_node(u16 dpmac_id) +static struct fwnode_handle *dpaa2_mac_get_node(struct device *dev, + u16 dpmac_id) { - struct device_node *dpmacs, *dpmac = NULL; - u32 id; + struct fwnode_handle *parent, *child = NULL; + struct device_node *dpmacs = NULL; int err; + u32 id; - dpmacs = of_find_node_by_name(NULL, "dpmacs"); - if (!dpmacs) - return NULL; + if (is_of_node(dev->parent->fwnode)) { + dpmacs = of_find_node_by_name(NULL, "dpmacs"); + if (!dpmacs) + return NULL; + parent = of_fwnode_handle(dpmacs); + } else if (is_acpi_node(dev->parent->fwnode)) { + parent = dev->parent->fwnode; + } - while ((dpmac = of_get_next_child(dpmacs, dpmac)) != NULL) { - err = of_property_read_u32(dpmac, "reg", &id); - if (err) + fwnode_for_each_child_node(parent, child) { + err = -EINVAL; + if (is_acpi_device_node(child)) + err = acpi_get_local_address(ACPI_HANDLE_FWNODE(child), &id); + else if (is_of_node(child)) + err = of_property_read_u32(to_of_node(child), "reg", &id); + if (err) { continue; - if (id == dpmac_id) - break; + } else if (id == dpmac_id) { + if (is_of_node(dev->parent->fwnode)) + of_node_put(dpmacs); + return child; + } } - - of_node_put(dpmacs); - - return dpmac; + if (is_of_node(dev->parent->fwnode)) + of_node_put(dpmacs); + return NULL; } -static int dpaa2_mac_get_if_mode(struct device_node *node, +static int dpaa2_mac_get_if_mode(struct fwnode_handle *dpmac_node, struct dpmac_attr attr) { phy_interface_t if_mode; int err; - err = of_get_phy_mode(node, &if_mode); - if (!err) - return if_mode; + err = fwnode_get_phy_mode(dpmac_node); + if (err > 0) + return err; err = phy_mode(attr.eth_if, &if_mode); if (!err) @@ -235,26 +250,27 @@ static const struct phylink_mac_ops dpaa2_mac_phylink_ops = { }; static int dpaa2_pcs_create(struct dpaa2_mac *mac, - struct device_node *dpmac_node, int id) + struct fwnode_handle *dpmac_node, + int id) { struct mdio_device *mdiodev; - struct device_node *node; + struct fwnode_handle *node; - node = of_parse_phandle(dpmac_node, "pcs-handle", 0); - if (!node) { + node = fwnode_find_reference(dpmac_node, "pcs-handle", 0); + if (IS_ERR(node)) { /* do not error out on old DTS files */ netdev_warn(mac->net_dev, "pcs-handle node not found\n"); return 0; } - if (!of_device_is_available(node)) { + if (!of_device_is_available(to_of_node(node))) { netdev_err(mac->net_dev, "pcs-handle node not available\n"); - of_node_put(node); + of_node_put(to_of_node(node)); return -ENODEV; } - mdiodev = of_mdio_find_device(node); - of_node_put(node); + mdiodev = fwnode_mdio_find_device(node); + fwnode_handle_put(node); if (!mdiodev) return -EPROBE_DEFER; @@ -283,13 +299,12 @@ static void dpaa2_pcs_destroy(struct dpaa2_mac *mac) int dpaa2_mac_connect(struct dpaa2_mac *mac) { struct net_device *net_dev = mac->net_dev; - struct device_node *dpmac_node; + struct fwnode_handle *dpmac_node = NULL; struct phylink *phylink; int err; mac->if_link_type = mac->attr.link_type; - - dpmac_node = dpaa2_mac_get_node(mac->attr.id); + dpmac_node = dpaa2_mac_get_node(&mac->mc_dev->dev, mac->attr.id); if (!dpmac_node) { netdev_err(net_dev, "No dpmac@%d node found.\n", mac->attr.id); return -ENODEV; @@ -306,7 +321,7 @@ int dpaa2_mac_connect(struct dpaa2_mac *mac) * error out if the interface mode requests them and there is no PHY * to act upon them */ - if (of_phy_is_fixed_link(dpmac_node) && + if (of_phy_is_fixed_link(to_of_node(dpmac_node)) && (mac->if_mode == PHY_INTERFACE_MODE_RGMII_ID || mac->if_mode == PHY_INTERFACE_MODE_RGMII_RXID || mac->if_mode == PHY_INTERFACE_MODE_RGMII_TXID)) { @@ -327,7 +342,7 @@ int dpaa2_mac_connect(struct dpaa2_mac *mac) mac->phylink_config.type = PHYLINK_NETDEV; phylink = phylink_create(&mac->phylink_config, - of_fwnode_handle(dpmac_node), mac->if_mode, + dpmac_node, mac->if_mode, &dpaa2_mac_phylink_ops); if (IS_ERR(phylink)) { err = PTR_ERR(phylink); @@ -338,13 +353,14 @@ int dpaa2_mac_connect(struct dpaa2_mac *mac) if (mac->pcs) phylink_set_pcs(mac->phylink, &mac->pcs->pcs); - err = phylink_of_phy_connect(mac->phylink, dpmac_node, 0); + err = phylink_fwnode_phy_connect(mac->phylink, dpmac_node, 0); if (err) { - netdev_err(net_dev, "phylink_of_phy_connect() = %d\n", err); + netdev_err(net_dev, "phylink_fwnode_phy_connect() = %d\n", err); goto err_phylink_destroy; } - of_node_put(dpmac_node); + if (is_of_node(dpmac_node)) + fwnode_handle_put(dpmac_node); return 0; @@ -353,7 +369,8 @@ int dpaa2_mac_connect(struct dpaa2_mac *mac) err_pcs_destroy: dpaa2_pcs_destroy(mac); err_put_node: - of_node_put(dpmac_node); + if (is_of_node(dpmac_node)) + fwnode_handle_put(dpmac_node); return err; }
Modify dpaa2_mac_connect() to support ACPI along with DT. Modify dpaa2_mac_get_node() to get the dpmac fwnode from either DT or ACPI. Replace of_get_phy_mode with fwnode_get_phy_mode to get phy-mode for a dpmac_node. Use helper function phylink_fwnode_phy_connect() to find phy_dev and connect to mac->phylink. Signed-off-by: Calvin Johnson <calvin.johnson@oss.nxp.com> --- Changes in v5: - replace fwnode_get_id() with OF and ACPI function calls Changes in v4: None Changes in v3: None Changes in v2: - Refactor OF functions to use fwnode functions .../net/ethernet/freescale/dpaa2/dpaa2-mac.c | 91 +++++++++++-------- 1 file changed, 54 insertions(+), 37 deletions(-)