Sat, Mar 02, 2019 at 08:07:24PM CET, jakub.kicin...@netronome.com wrote:
>On Sat, 2 Mar 2019 09:43:47 +0100, Jiri Pirko wrote:
>> Fri, Mar 01, 2019 at 07:04:49PM CET, jakub.kicin...@netronome.com wrote:
>> >Register all representors as devlink ports.
>> >
>> >The port_index is slightly tricky to figure out, we use a bit of
>> >arbitrary math to create unique IDs for PCI ports.
>> >
>> >Signed-off-by: Jakub Kicinski <jakub.kicin...@netronome.com>
>> >---
>> > .../net/ethernet/netronome/nfp/nfp_devlink.c | 40 ++++++++++++++++++-
>> > .../net/ethernet/netronome/nfp/nfp_net_repr.c | 16 +++++++-
>> > 2 files changed, 53 insertions(+), 3 deletions(-)
>> >
>> >diff --git a/drivers/net/ethernet/netronome/nfp/nfp_devlink.c
>> >b/drivers/net/ethernet/netronome/nfp/nfp_devlink.c
>> >index 9af3cb1f2f17..bf7fd9614152 100644
>> >--- a/drivers/net/ethernet/netronome/nfp/nfp_devlink.c
>> >+++ b/drivers/net/ethernet/netronome/nfp/nfp_devlink.c
>> >@@ -350,7 +350,8 @@ const struct devlink_ops nfp_devlink_ops = {
>> > .flash_update = nfp_devlink_flash_update,
>> > };
>> >
>> >-int nfp_devlink_port_init(struct nfp_app *app, struct nfp_port *port)
>> >+static int
>> >+nfp_devlink_port_init_phys(struct devlink *devlink, struct nfp_port *port)
>> > {
>> > struct nfp_eth_table_port eth_port;
>> > int ret;
>> >@@ -368,6 +369,27 @@ int nfp_devlink_port_init(struct nfp_app *app, struct
>> >nfp_port *port)
>> > return 0;
>> > }
>> >
>> >+int nfp_devlink_port_init(struct nfp_app *app, struct nfp_port *port)
>> >+{
>> >+ struct devlink *devlink = priv_to_devlink(app->pf);
>> >+
>> >+ switch (port->type) {
>> >+ case NFP_PORT_PHYS_PORT:
>> >+ return nfp_devlink_port_init_phys(devlink, port);
>> >+ case NFP_PORT_PF_PORT:
>> >+ devlink_port_type_eth_set(&port->dl_port, port->netdev);
>> >+ devlink_port_attrs_pci_pf_set(&port->dl_port, port->pf_id);
>> >+ return 0;
>> >+ case NFP_PORT_VF_PORT:
>> >+ devlink_port_type_eth_set(&port->dl_port, port->netdev);
>> >+ devlink_port_attrs_pci_vf_set(&port->dl_port, port->pf_id,
>> >+ port->vf_id);
>>
>> What is the reason to expose vf/pf id for switch port? Isn't it rather
>> an attribute of a peer?
>
>Naw, its an attribute of the port. I leave the ASIC via PF n or VF m
>of PF n. Whatever is on the other side is isolated from the topology
>of the ASIC.
Ok.
>
>Is the physical port ID an attribute of the other end of the cable?
>
>> >+ return 0;
>> >+ default:
>> >+ return -EINVAL;
>> >+ }
>> >+}
>> >+
>> > void nfp_devlink_port_clean(struct nfp_port *port)
>> > {
>> > }
>> >@@ -376,7 +398,21 @@ int nfp_devlink_port_register(struct nfp_app *app,
>> >struct nfp_port *port)
>> > {
>> > struct devlink *devlink = priv_to_devlink(app->pf);
>> >
>> >- return devlink_port_register(devlink, &port->dl_port, port->eth_id);
>> >+ switch (port->type) {
>> >+ case NFP_PORT_PHYS_PORT:
>> >+ return devlink_port_register(devlink, &port->dl_port,
>> >+ port->eth_id);
>> >+ case NFP_PORT_PF_PORT:
>> >+ return devlink_port_register(devlink, &port->dl_port,
>> >+ (port->pf_id + 1) * 10000 +
>> >+ port->pf_split_id * 1000);
>>
>> Wait. What this 10000/1000 magic about?
>
>port_index has to be unique, I need some unique number here, as I
>stated both in the commit message and the cover letter, this is
>arbitrary.
You can at least use some defines for that.
>
>I can put the datapath port identifier in there but its (a)
>meaningless, (b) a bitfield, so it will look like 8972367083. And it
>may change depending on the FW load, so its not stable either.
>
>> >+ case NFP_PORT_VF_PORT:
>> >+ return devlink_port_register(devlink, &port->dl_port,
>> >+ (port->pf_id + 1) * 10000 +
>> >+ port->vf_id + 1);
>> >+ default:
>> >+ return -EINVAL;
>> >+ }
>> > }
>> >
>> > void nfp_devlink_port_unregister(struct nfp_port *port)
>> >diff --git a/drivers/net/ethernet/netronome/nfp/nfp_net_repr.c
>> >b/drivers/net/ethernet/netronome/nfp/nfp_net_repr.c
>> >index d2c803bb4e56..869d22760a6e 100644
>> >--- a/drivers/net/ethernet/netronome/nfp/nfp_net_repr.c
>> >+++ b/drivers/net/ethernet/netronome/nfp/nfp_net_repr.c
>> >@@ -292,7 +292,9 @@ nfp_repr_transfer_features(struct net_device *netdev,
>> >struct net_device *lower)
>> >
>> > static void nfp_repr_clean(struct nfp_repr *repr)
>> > {
>> >+ nfp_devlink_port_unregister(repr->port);
>> > unregister_netdev(repr->netdev);
>> >+ nfp_devlink_port_clean(repr->port);
>> > nfp_app_repr_clean(repr->app, repr->netdev);
>> > dst_release((struct dst_entry *)repr->dst);
>> > nfp_port_free(repr->port);
>> >@@ -395,12 +397,24 @@ int nfp_repr_init(struct nfp_app *app, struct
>> >net_device *netdev,
>> > if (err)
>> > goto err_clean;
>> >
>> >- err = register_netdev(netdev);
>> >+ err = nfp_devlink_port_init(app, repr->port);
>> > if (err)
>> > goto err_repr_clean;
>> >
>> >+ err = register_netdev(netdev);
>> >+ if (err)
>> >+ goto err_port_clean;
>> >+
>> >+ err = nfp_devlink_port_register(app, repr->port);
>>
>> Don't you want to take my patch ("nfp: register devlink port before
>> netdev") to change order of register_netdev and devlink_port_register,
>> include it to this patchset before this patch and change the order in
>> this patch too? I think it would be clearer to do it from the beginning.
>
>This way both netdev and devlink_port can get registered fully
>initialized. Otherwise we'd get two notifications. Are we trying to
>establish some ordering rules to get around the rtnl locking? :)
The order of devlink_port_register and register_netdev is given by
layering. For example, for port change, the devlink_port is still there
and registered, only the netdev is unregistered and ib_dev registered
instead of vice versa. It has really no relation to rtnl locking.
>
>> >+ if (err)
>> >+ goto err_unreg_netdev;
>> >+
>> > return 0;
>> >
>> >+err_unreg_netdev:
>> >+ unregister_netdev(repr->netdev);
>> >+err_port_clean:
>> >+ nfp_devlink_port_clean(repr->port);
>> > err_repr_clean:
>> > nfp_app_repr_clean(app, netdev);
>> > err_clean:
