| Message ID | 20190326120307.2953-9-jiri@resnulli.us |
|---|---|
| State | Superseded |
| Delegated to: | David Miller |
| Headers | show |
| Series | net: call for phys_port_name into devlink directly if possible | expand |
On 3/26/2019 5:03 AM, Jiri Pirko wrote: > From: Jiri Pirko <jiri@mellanox.com> > > In order for devlink compat functions to work, implement > ndo_get_devlink_port. Legacy slaves does not have devlink port instances > created for themselves. Not a big fan of de-duplicating the entire set of netdev_ops for legacy vs. non-legacy, can we just check that the devlink instance was r
Wed, Mar 27, 2019 at 08:54:41PM CET, f.fainelli@gmail.com wrote: >On 3/26/2019 5:03 AM, Jiri Pirko wrote: >> From: Jiri Pirko <jiri@mellanox.com> >> >> In order for devlink compat functions to work, implement >> ndo_get_devlink_port. Legacy slaves does not have devlink port instances >> created for themselves. > >Not a big fan of de-duplicating the entire set of netdev_ops for legacy >vs. non-legacy, can we just check that the devlink instance was r In nfp, I make legacy and non-legacy ndos to be shared. However there, they are doing to eventually all use devlink ports. In dsa on the other hand, I don't think that legacy is going to use devlink port, would it? I wan't to eventually have WARN_ON in devlink code in case devlink_port exists and phys_port_name ndo is present at the same time.
On 3/27/19 1:01 PM, Jiri Pirko wrote: > Wed, Mar 27, 2019 at 08:54:41PM CET, f.fainelli@gmail.com wrote: >> On 3/26/2019 5:03 AM, Jiri Pirko wrote: >>> From: Jiri Pirko <jiri@mellanox.com> >>> >>> In order for devlink compat functions to work, implement >>> ndo_get_devlink_port. Legacy slaves does not have devlink port instances >>> created for themselves. >> >> Not a big fan of de-duplicating the entire set of netdev_ops for legacy >> vs. non-legacy, can we just check that the devlink instance was r > > In nfp, I make legacy and non-legacy ndos to be shared. However there, > they are doing to eventually all use devlink ports. In dsa on the other > hand, I don't think that legacy is going to use devlink port, would it? The legacy probing method has not been updated to match what net/dsa/dsa2.c does, but there is no technical reason for not supporting devlink instances over ports registered through the legacy interface. So eventually all of what you did here will be thrown away (not sure by which timeframe, probably after mv88e6060 finally gets converted to the new binding). > I wan't to eventually have WARN_ON in devlink code in case devlink_port > exists and phys_port_name ndo is present at the same time. > No sure, but you could always do something like: if (!dp->devlink_registered) return dsa_slave_get_phys_port_name(); return &dp->devlink_port; is what I had in mind. This just requires adding a boolean to track the registration of devlink ports within net/dsa/dsa2.c which should be fewer lines of code and easier to remove in the future. No strong objections though.
Wed, Mar 27, 2019 at 09:13:00PM CET, f.fainelli@gmail.com wrote: >On 3/27/19 1:01 PM, Jiri Pirko wrote: >> Wed, Mar 27, 2019 at 08:54:41PM CET, f.fainelli@gmail.com wrote: >>> On 3/26/2019 5:03 AM, Jiri Pirko wrote: >>>> From: Jiri Pirko <jiri@mellanox.com> >>>> >>>> In order for devlink compat functions to work, implement >>>> ndo_get_devlink_port. Legacy slaves does not have devlink port instances >>>> created for themselves. >>> >>> Not a big fan of de-duplicating the entire set of netdev_ops for legacy >>> vs. non-legacy, can we just check that the devlink instance was r >> >> In nfp, I make legacy and non-legacy ndos to be shared. However there, >> they are doing to eventually all use devlink ports. In dsa on the other >> hand, I don't think that legacy is going to use devlink port, would it? > >The legacy probing method has not been updated to match what >net/dsa/dsa2.c does, but there is no technical reason for not supporting >devlink instances over ports registered through the legacy interface. So >eventually all of what you did here will be thrown away (not sure by >which timeframe, probably after mv88e6060 finally gets converted to the >new binding). Okay. In that case, when you say legacy will be eventually remove, Will do the same as I did for nfp. Thanks! > >> I wan't to eventually have WARN_ON in devlink code in case devlink_port >> exists and phys_port_name ndo is present at the same time. >> > >No sure, but you could always do something like: > >if (!dp->devlink_registered) > return dsa_slave_get_phys_port_name(); > >return &dp->devlink_port; > >is what I had in mind. This just requires adding a boolean to track the >registration of devlink ports within net/dsa/dsa2.c which should be >fewer lines of code and easier to remove in the future. No strong >objections though. >-- >Florian
diff --git a/net/dsa/dsa_priv.h b/net/dsa/dsa_priv.h index 093b7d145eb1..688c765cd1db 100644 --- a/net/dsa/dsa_priv.h +++ b/net/dsa/dsa_priv.h @@ -176,6 +176,7 @@ void dsa_port_link_unregister_of(struct dsa_port *dp); extern const struct dsa_device_ops notag_netdev_ops; void dsa_slave_mii_bus_init(struct dsa_switch *ds); int dsa_slave_create(struct dsa_port *dp); +int dsa_slave_create_legacy(struct dsa_port *dp); void dsa_slave_destroy(struct net_device *slave_dev); int dsa_slave_suspend(struct net_device *slave_dev); int dsa_slave_resume(struct net_device *slave_dev); diff --git a/net/dsa/legacy.c b/net/dsa/legacy.c index cb42939db776..f9a1f9c4a58c 100644 --- a/net/dsa/legacy.c +++ b/net/dsa/legacy.c @@ -197,7 +197,7 @@ static int dsa_switch_setup_one(struct dsa_switch *ds, if (!dsa_is_user_port(ds, i)) continue; - ret = dsa_slave_create(&ds->ports[i]); + ret = dsa_slave_create_legacy(&ds->ports[i]); if (ret < 0) netdev_err(master, "[%d]: can't create dsa slave device for port %d(%s): %d\n", index, i, cd->port_names[i], ret); diff --git a/net/dsa/slave.c b/net/dsa/slave.c index 093eef6f2599..46c98cb8a2b5 100644 --- a/net/dsa/slave.c +++ b/net/dsa/slave.c @@ -1096,6 +1096,38 @@ int dsa_legacy_fdb_del(struct ndmsg *ndm, struct nlattr *tb[], return dsa_port_fdb_del(dp, addr, vid); } +static struct devlink_port *dsa_slave_get_devlink_port(struct net_device *dev) +{ + struct dsa_port *dp = dsa_slave_to_port(dev); + + return &dp->devlink_port; +} + +static const struct net_device_ops dsa_slave_netdev_ops_legacy = { + .ndo_open = dsa_slave_open, + .ndo_stop = dsa_slave_close, + .ndo_start_xmit = dsa_slave_xmit, + .ndo_change_rx_flags = dsa_slave_change_rx_flags, + .ndo_set_rx_mode = dsa_slave_set_rx_mode, + .ndo_set_mac_address = dsa_slave_set_mac_address, + .ndo_fdb_add = dsa_legacy_fdb_add, + .ndo_fdb_del = dsa_legacy_fdb_del, + .ndo_fdb_dump = dsa_slave_fdb_dump, + .ndo_do_ioctl = dsa_slave_ioctl, + .ndo_get_iflink = dsa_slave_get_iflink, +#ifdef CONFIG_NET_POLL_CONTROLLER + .ndo_netpoll_setup = dsa_slave_netpoll_setup, + .ndo_netpoll_cleanup = dsa_slave_netpoll_cleanup, + .ndo_poll_controller = dsa_slave_poll_controller, +#endif + .ndo_get_phys_port_name = dsa_slave_get_phys_port_name, + .ndo_setup_tc = dsa_slave_setup_tc, + .ndo_get_stats64 = dsa_slave_get_stats64, + .ndo_get_port_parent_id = dsa_slave_get_port_parent_id, + .ndo_vlan_rx_add_vid = dsa_slave_vlan_rx_add_vid, + .ndo_vlan_rx_kill_vid = dsa_slave_vlan_rx_kill_vid, +}; + static const struct net_device_ops dsa_slave_netdev_ops = { .ndo_open = dsa_slave_open, .ndo_stop = dsa_slave_close, @@ -1119,6 +1151,7 @@ static const struct net_device_ops dsa_slave_netdev_ops = { .ndo_get_port_parent_id = dsa_slave_get_port_parent_id, .ndo_vlan_rx_add_vid = dsa_slave_vlan_rx_add_vid, .ndo_vlan_rx_kill_vid = dsa_slave_vlan_rx_kill_vid, + .ndo_get_devlink_port = dsa_slave_get_devlink_port, }; static struct device_type dsa_type = { @@ -1355,7 +1388,8 @@ static void dsa_slave_notify(struct net_device *dev, unsigned long val) call_dsa_notifiers(val, dev, &rinfo.info); } -int dsa_slave_create(struct dsa_port *port) +static int __dsa_slave_create(struct dsa_port *port, + const struct net_device_ops *netdev_ops) { const struct dsa_port *cpu_dp = port->cpu_dp; struct net_device *master = cpu_dp->master; @@ -1380,7 +1414,7 @@ int dsa_slave_create(struct dsa_port *port) slave_dev->ethtool_ops = &dsa_slave_ethtool_ops; eth_hw_addr_inherit(slave_dev, master); slave_dev->priv_flags |= IFF_NO_QUEUE; - slave_dev->netdev_ops = &dsa_slave_netdev_ops; + slave_dev->netdev_ops = netdev_ops; slave_dev->min_mtu = 0; slave_dev->max_mtu = ETH_MAX_MTU; SET_NETDEV_DEVTYPE(slave_dev, &dsa_type); @@ -1434,6 +1468,16 @@ int dsa_slave_create(struct dsa_port *port) return ret; } +int dsa_slave_create(struct dsa_port *port) +{ + return __dsa_slave_create(port, &dsa_slave_netdev_ops); +} + +int dsa_slave_create_legacy(struct dsa_port *port) +{ + return __dsa_slave_create(port, &dsa_slave_netdev_ops_legacy); +} + void dsa_slave_destroy(struct net_device *slave_dev) { struct dsa_port *dp = dsa_slave_to_port(slave_dev);