| Message ID | 20231220042632.26825-8-luizluca@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: dsa: realtek: variants to drivers, interfaces to a common module | expand |
On Wed, Dec 20, 2023 at 01:24:30AM -0300, Luiz Angelo Daros de Luca wrote: > This reverts commit fe7324b932222574a0721b80e72c6c5fe57960d1. > > The use of user_mii_bus is inappropriate when the hardware is described > with a device-tree [1]. > > Since all drivers currently implementing ds_switch_ops.phy_{read,write} > were not updated to utilize the MDIO information from OF with the > generic "dsa user mii", they might not be affected by this change. /might/ not? I think this paragraph could be more precisely worded. > > [1] https://lkml.kernel.org/netdev/20231213120656.x46fyad6ls7sqyzv@skbuf/T/#u > > Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.com> > --- > net/dsa/dsa.c | 7 +------ > 1 file changed, 1 insertion(+), 6 deletions(-) > > diff --git a/net/dsa/dsa.c b/net/dsa/dsa.c > index ac7be864e80d..cea364c81b70 100644 > --- a/net/dsa/dsa.c > +++ b/net/dsa/dsa.c > @@ -15,7 +15,6 @@ > #include <linux/slab.h> > #include <linux/rtnetlink.h> > #include <linux/of.h> > -#include <linux/of_mdio.h> > #include <linux/of_net.h> > #include <net/dsa_stubs.h> > #include <net/sch_generic.h> > @@ -626,7 +625,6 @@ static void dsa_switch_teardown_tag_protocol(struct dsa_switch *ds) > > static int dsa_switch_setup(struct dsa_switch *ds) > { > - struct device_node *dn; > int err; > > if (ds->setup) > @@ -666,10 +664,7 @@ static int dsa_switch_setup(struct dsa_switch *ds) > > dsa_user_mii_bus_init(ds); > > - dn = of_get_child_by_name(ds->dev->of_node, "mdio"); > - > - err = of_mdiobus_register(ds->user_mii_bus, dn); > - of_node_put(dn); > + err = mdiobus_register(ds->user_mii_bus, dn); > if (err < 0) > goto free_user_mii_bus; > } > -- > 2.43.0 >
Hi Luiz, kernel test robot noticed the following build errors: [auto build test ERROR on net-next/main] url: https://github.com/intel-lab-lkp/linux/commits/Luiz-Angelo-Daros-de-Luca/net-dsa-realtek-drop-cleanup-from-realtek_ops/20231220-122907 base: net-next/main patch link: https://lore.kernel.org/r/20231220042632.26825-8-luizluca%40gmail.com patch subject: [PATCH net-next v2 7/7] Revert "net: dsa: OF-ware slave_mii_bus" config: arm64-defconfig (https://download.01.org/0day-ci/archive/20231221/202312210853.t6DhuvdS-lkp@intel.com/config) compiler: aarch64-linux-gcc (GCC) 13.2.0 reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231221/202312210853.t6DhuvdS-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202312210853.t6DhuvdS-lkp@intel.com/ All errors (new ones prefixed by >>): net/dsa/dsa.c: In function 'dsa_switch_setup': >> net/dsa/dsa.c:667:60: error: macro "mdiobus_register" passed 2 arguments, but takes just 1 667 | err = mdiobus_register(ds->user_mii_bus, dn); | ^ In file included from include/linux/of_net.h:9, from net/dsa/dsa.c:18: include/linux/phy.h:456: note: macro "mdiobus_register" defined here 456 | #define mdiobus_register(bus) __mdiobus_register(bus, THIS_MODULE) | >> net/dsa/dsa.c:667:23: error: 'mdiobus_register' undeclared (first use in this function); did you mean 'mdiobus_unregister'? 667 | err = mdiobus_register(ds->user_mii_bus, dn); | ^~~~~~~~~~~~~~~~ | mdiobus_unregister net/dsa/dsa.c:667:23: note: each undeclared identifier is reported only once for each function it appears in vim +/mdiobus_register +667 net/dsa/dsa.c 625 626 static int dsa_switch_setup(struct dsa_switch *ds) 627 { 628 int err; 629 630 if (ds->setup) 631 return 0; 632 633 /* Initialize ds->phys_mii_mask before registering the user MDIO bus 634 * driver and before ops->setup() has run, since the switch drivers and 635 * the user MDIO bus driver rely on these values for probing PHY 636 * devices or not 637 */ 638 ds->phys_mii_mask |= dsa_user_ports(ds); 639 640 err = dsa_switch_devlink_alloc(ds); 641 if (err) 642 return err; 643 644 err = dsa_switch_register_notifier(ds); 645 if (err) 646 goto devlink_free; 647 648 ds->configure_vlan_while_not_filtering = true; 649 650 err = ds->ops->setup(ds); 651 if (err < 0) 652 goto unregister_notifier; 653 654 err = dsa_switch_setup_tag_protocol(ds); 655 if (err) 656 goto teardown; 657 658 if (!ds->user_mii_bus && ds->ops->phy_read) { 659 ds->user_mii_bus = mdiobus_alloc(); 660 if (!ds->user_mii_bus) { 661 err = -ENOMEM; 662 goto teardown; 663 } 664 665 dsa_user_mii_bus_init(ds); 666 > 667 err = mdiobus_register(ds->user_mii_bus, dn); 668 if (err < 0) 669 goto free_user_mii_bus; 670 } 671 672 dsa_switch_devlink_register(ds); 673 674 ds->setup = true; 675 return 0; 676 677 free_user_mii_bus: 678 if (ds->user_mii_bus && ds->ops->phy_read) 679 mdiobus_free(ds->user_mii_bus); 680 teardown: 681 if (ds->ops->teardown) 682 ds->ops->teardown(ds); 683 unregister_notifier: 684 dsa_switch_unregister_notifier(ds); 685 devlink_free: 686 dsa_switch_devlink_free(ds); 687 return err; 688 } 689
> > This reverts commit fe7324b932222574a0721b80e72c6c5fe57960d1. > > > > The use of user_mii_bus is inappropriate when the hardware is described > > with a device-tree [1]. > > > > Since all drivers currently implementing ds_switch_ops.phy_{read,write} > > were not updated to utilize the MDIO information from OF with the > > generic "dsa user mii", they might not be affected by this change. > > /might/ not? I think this paragraph could be more precisely worded. Yes, it should be "should". That's what I get from working instead of sleeping. I checked all changes that have happened since that commit touching drivers with the ds_switch_ops.phy_{read,write}. How about: The generic "dsa user mii" driver will only be utilized by drivers implementing ds_switch_ops.phy_{read,write} and leaving ds.user_mii_bus unallocated. For these drivers, no commits have been made since the one being reverted that altered the usage of ds.user_mii_bus. Consequently, these drivers should not be affected by this change. > > [1] https://lkml.kernel.org/netdev/20231213120656.x46fyad6ls7sqyzv@skbuf/T/#u > > > > Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.com> > > --- > > net/dsa/dsa.c | 7 +------ > > 1 file changed, 1 insertion(+), 6 deletions(-) > > > > diff --git a/net/dsa/dsa.c b/net/dsa/dsa.c > > index ac7be864e80d..cea364c81b70 100644 > > --- a/net/dsa/dsa.c > > +++ b/net/dsa/dsa.c > > @@ -15,7 +15,6 @@ > > #include <linux/slab.h> > > #include <linux/rtnetlink.h> > > #include <linux/of.h> > > -#include <linux/of_mdio.h> > > #include <linux/of_net.h> > > #include <net/dsa_stubs.h> > > #include <net/sch_generic.h> > > @@ -626,7 +625,6 @@ static void dsa_switch_teardown_tag_protocol(struct dsa_switch *ds) > > > > static int dsa_switch_setup(struct dsa_switch *ds) > > { > > - struct device_node *dn; > > int err; > > > > if (ds->setup) > > @@ -666,10 +664,7 @@ static int dsa_switch_setup(struct dsa_switch *ds) > > > > dsa_user_mii_bus_init(ds); > > > > - dn = of_get_child_by_name(ds->dev->of_node, "mdio"); > > - > > - err = of_mdiobus_register(ds->user_mii_bus, dn); > > - of_node_put(dn); > > + err = mdiobus_register(ds->user_mii_bus, dn); This patch also has an obvious compile error that I didn't catch during my builds. I might have built just a subtree and during each step in a rebase, missing the last one. Sorry. It should be err = mdiobus_register(ds->user_mii_bus); > > if (err < 0) > > goto free_user_mii_bus; > > } > > -- > > 2.43.0 > >
diff --git a/net/dsa/dsa.c b/net/dsa/dsa.c index ac7be864e80d..cea364c81b70 100644 --- a/net/dsa/dsa.c +++ b/net/dsa/dsa.c @@ -15,7 +15,6 @@ #include <linux/slab.h> #include <linux/rtnetlink.h> #include <linux/of.h> -#include <linux/of_mdio.h> #include <linux/of_net.h> #include <net/dsa_stubs.h> #include <net/sch_generic.h> @@ -626,7 +625,6 @@ static void dsa_switch_teardown_tag_protocol(struct dsa_switch *ds) static int dsa_switch_setup(struct dsa_switch *ds) { - struct device_node *dn; int err; if (ds->setup) @@ -666,10 +664,7 @@ static int dsa_switch_setup(struct dsa_switch *ds) dsa_user_mii_bus_init(ds); - dn = of_get_child_by_name(ds->dev->of_node, "mdio"); - - err = of_mdiobus_register(ds->user_mii_bus, dn); - of_node_put(dn); + err = mdiobus_register(ds->user_mii_bus, dn); if (err < 0) goto free_user_mii_bus; }
This reverts commit fe7324b932222574a0721b80e72c6c5fe57960d1. The use of user_mii_bus is inappropriate when the hardware is described with a device-tree [1]. Since all drivers currently implementing ds_switch_ops.phy_{read,write} were not updated to utilize the MDIO information from OF with the generic "dsa user mii", they might not be affected by this change. [1] https://lkml.kernel.org/netdev/20231213120656.x46fyad6ls7sqyzv@skbuf/T/#u Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.com> --- net/dsa/dsa.c | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-)