Re: [PATCH net-next v2 3/7] nfp: register devlink ports of all reprs

Jiri Pirko
2019-03-02T08:53:44Z

Fri, Mar 01, 2019 at 07:04:49PM CET, jakub.kicinski@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.kicinski@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?

>+ 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?

>+ 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.

>+ 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:
>--
>2.19.2
>