Re: [net-next PATCH v5 15/15] net: dpaa2-mac: Add ACPI support for DPAA2 MAC driver
On Mon, Feb 15, 2021 at 05:15:36PM +0200, Andy Shevchenko wrote:
> On Mon, Feb 15, 2021 at 5:13 PM Andy Shevchenko
> wrote:
> >
> > On Mon, Feb 15, 2021 at 2:33 PM Calvin Johnson
> > wrote:
> > > On Mon, Feb 08, 2021 at 04:28:31PM +, 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
Re: [net-next PATCH v5 15/15] net: dpaa2-mac: Add ACPI support for DPAA2 MAC driver
On Mon, Feb 15, 2021 at 5:13 PM Andy Shevchenko
wrote:
>
> On Mon, Feb 15, 2021 at 2:33 PM Calvin Johnson
> wrote:
> > On Mon, Feb 08, 2021 at 04:28:31PM +, 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.
--
With Best Regards,
Andy Shevchenko
Re: [net-next PATCH v5 15/15] net: dpaa2-mac: Add ACPI support for DPAA2 MAC driver
On Mon, Feb 15, 2021 at 2:33 PM Calvin Johnson
wrote:
> On Mon, Feb 08, 2021 at 04:28:31PM +, 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) {
--
With Best Regards,
Andy Shevchenko
Re: [net-next PATCH v5 15/15] net: dpaa2-mac: Add ACPI support for DPAA2 MAC driver
On Mon, Feb 08, 2021 at 04:28:31PM +, 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
>
> 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
Re: [net-next PATCH v5 15/15] net: dpaa2-mac: Add ACPI support for DPAA2 MAC driver
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
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.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
Re: [net-next PATCH v5 15/15] net: dpaa2-mac: Add ACPI support for DPAA2 MAC driver
On Mon, Feb 8, 2021 at 5:15 PM 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.
...
> + 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
[net-next PATCH v5 15/15] net: dpaa2-mac: Add ACPI support for DPAA2 MAC driver
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
---
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(-)
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
+#include
+
#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_ma
