Date:   Thu, 19 Jan 2023 15:03:54 +0000
From:   "Lucero Palau, Alejandro" <alejandro.lucero-palau@....com>
To:     Jiri Pirko <jiri@...nulli.us>,
        "Lucero Palau, Alejandro" <alejandro.lucero-palau@....com>
CC:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "linux-net-drivers (AMD-Xilinx)" <linux-net-drivers@....com>,
        "davem@...emloft.net" <davem@...emloft.net>,
        "kuba@...nel.org" <kuba@...nel.org>,
        "pabeni@...hat.com" <pabeni@...hat.com>,
        "edumazet@...gle.com" <edumazet@...gle.com>,
        "ecree.xilinx@...il.com" <ecree.xilinx@...il.com>
Subject: Re: [PATCH net-next 4/7] sfc: add devlink port support for ef100


On 1/19/23 12:33, Jiri Pirko wrote:
> Thu, Jan 19, 2023 at 12:31:37PM CET, alejandro.lucero-palau@....com wrote:
>> From: Alejandro Lucero <alejandro.lucero-palau@....com>
>>
>> Using the data when enumerating mports, create devlink ports just before
>> netdevs are registered and removing those devlink ports after netdev has
>> been unregister.
>>
>> Signed-off-by: Alejandro Lucero <alejandro.lucero-palau@....com>
>> ---
>> drivers/net/ethernet/sfc/ef100_netdev.c |  14 ++-
>> drivers/net/ethernet/sfc/ef100_rep.c    |  23 +++++
>> drivers/net/ethernet/sfc/ef100_rep.h    |   6 ++
>> drivers/net/ethernet/sfc/efx_devlink.c  | 114 ++++++++++++++++++++++++
>> drivers/net/ethernet/sfc/efx_devlink.h  |   7 ++
>> drivers/net/ethernet/sfc/net_driver.h   |   1 +
>> 6 files changed, 161 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/sfc/ef100_netdev.c b/drivers/net/ethernet/sfc/ef100_netdev.c
>> index ddcc325ed570..4a5d028f757e 100644
>> --- a/drivers/net/ethernet/sfc/ef100_netdev.c
>> +++ b/drivers/net/ethernet/sfc/ef100_netdev.c
>> @@ -24,6 +24,7 @@
>> #include "rx_common.h"
>> #include "ef100_sriov.h"
>> #include "tc_bindings.h"
>> +#include "efx_devlink.h"
>>
>> static void ef100_update_name(struct efx_nic *efx)
>> {
>> @@ -280,6 +281,9 @@ static int ef100_register_netdev(struct efx_nic *efx)
>> 	net_dev->max_mtu = EFX_MAX_MTU;
>> 	net_dev->ethtool_ops = &ef100_ethtool_ops;
>>
>> +	if (!efx->type->is_vf)
>> +		ef100_pf_set_devlink_port(efx);
> Again, why no port for VF while you are at it?


We create the devlink ports with the VF representors.


>
>> +
>> 	rtnl_lock();
>>
>> 	rc = dev_alloc_name(net_dev, net_dev->name);
>> @@ -302,6 +306,7 @@ static int ef100_register_netdev(struct efx_nic *efx)
>>
>> fail_locked:
>> 	rtnl_unlock();
>> +	ef100_pf_unset_devlink_port(efx);
>> 	netif_err(efx, drv, efx->net_dev, "could not register net dev\n");
>> 	return rc;
>> }
>> @@ -312,6 +317,7 @@ static void ef100_unregister_netdev(struct efx_nic *efx)
>> 		efx_fini_mcdi_logging(efx);
>> 		efx->state = STATE_PROBED;
>> 		unregister_netdev(efx->net_dev);
>> +		ef100_pf_unset_devlink_port(efx);
>> 	}
>> }
>>
>> @@ -405,16 +411,16 @@ int ef100_probe_netdev(struct efx_probe_data *probe_data)
>> 	/* Don't fail init if RSS setup doesn't work. */
>> 	efx_mcdi_push_default_indir_table(efx, efx->n_rx_channels);
>>
>> -	rc = ef100_register_netdev(efx);
>> -	if (rc)
>> -		goto fail;
>> -
>> 	if (!efx->type->is_vf) {
>> 		rc = ef100_probe_netdev_pf(efx);
>> 		if (rc)
>> 			goto fail;
>> 	}
>>
>> +	rc = ef100_register_netdev(efx);
>> +	if (rc)
>> +		goto fail;
>> +
>> 	efx->netdev_notifier.notifier_call = ef100_netdev_event;
>> 	rc = register_netdevice_notifier(&efx->netdev_notifier);
>> 	if (rc) {
>> diff --git a/drivers/net/ethernet/sfc/ef100_rep.c b/drivers/net/ethernet/sfc/ef100_rep.c
>> index 9cd1a3ac67e0..ff0c8da61919 100644
>> --- a/drivers/net/ethernet/sfc/ef100_rep.c
>> +++ b/drivers/net/ethernet/sfc/ef100_rep.c
>> @@ -16,6 +16,7 @@
>> #include "mae.h"
>> #include "rx_common.h"
>> #include "tc_bindings.h"
>> +#include "efx_devlink.h"
>>
>> #define EFX_EF100_REP_DRIVER	"efx_ef100_rep"
>>
>> @@ -297,6 +298,7 @@ int efx_ef100_vfrep_create(struct efx_nic *efx, unsigned int i)
>> 			i, rc);
>> 		goto fail1;
>> 	}
>> +	ef100_rep_set_devlink_port(efv);
>> 	rc = register_netdev(efv->net_dev);
>> 	if (rc) {
>> 		pci_err(efx->pci_dev,
>> @@ -304,10 +306,12 @@ int efx_ef100_vfrep_create(struct efx_nic *efx, unsigned int i)
>> 			i, rc);
>> 		goto fail2;
>> 	}
>> +
>> 	pci_dbg(efx->pci_dev, "Representor for VF %d is %s\n", i,
>> 		efv->net_dev->name);
>> 	return 0;
>> fail2:
>> +	ef100_rep_unset_devlink_port(efv);
>> 	efx_ef100_deconfigure_rep(efv);
>> fail1:
>> 	efx_ef100_rep_destroy_netdev(efv);
>> @@ -323,6 +327,7 @@ void efx_ef100_vfrep_destroy(struct efx_nic *efx, struct efx_rep *efv)
>> 		return;
>> 	netif_dbg(efx, drv, rep_dev, "Removing VF representor\n");
>> 	unregister_netdev(rep_dev);
>> +	ef100_rep_unset_devlink_port(efv);
>> 	efx_ef100_deconfigure_rep(efv);
>> 	efx_ef100_rep_destroy_netdev(efv);
>> }
>> @@ -339,6 +344,24 @@ void efx_ef100_fini_vfreps(struct efx_nic *efx)
>> 		efx_ef100_vfrep_destroy(efx, efv);
>> }
>>
>> +bool ef100_mport_is_pcie_vnic(struct mae_mport_desc *mport_desc)
>> +{
>> +	return mport_desc->mport_type == MAE_MPORT_DESC_MPORT_TYPE_VNIC &&
>> +	       mport_desc->vnic_client_type == MAE_MPORT_DESC_VNIC_CLIENT_TYPE_FUNCTION;
>> +}
>> +
>> +bool ef100_mport_on_local_intf(struct efx_nic *efx,
>> +			       struct mae_mport_desc *mport_desc)
>> +{
>> +	bool pcie_func;
>> +	struct ef100_nic_data *nic_data = efx->nic_data;
> Reverse christmas tree ordering please:
> ***************
> *********
> ****
>
Damm it.

I'll fix it.

>
>
>> +
>> +	pcie_func = ef100_mport_is_pcie_vnic(mport_desc);
>> +
>> +	return nic_data->have_local_intf && pcie_func &&
>> +		     mport_desc->interface_idx == nic_data->local_mae_intf;
>> +}
>> +
>> void efx_ef100_init_reps(struct efx_nic *efx)
>> {
>> 	struct ef100_nic_data *nic_data = efx->nic_data;
>> diff --git a/drivers/net/ethernet/sfc/ef100_rep.h b/drivers/net/ethernet/sfc/ef100_rep.h
>> index 328ac0cbb532..9cca41614982 100644
>> --- a/drivers/net/ethernet/sfc/ef100_rep.h
>> +++ b/drivers/net/ethernet/sfc/ef100_rep.h
>> @@ -22,6 +22,8 @@ struct efx_rep_sw_stats {
>> 	atomic64_t rx_dropped, tx_errors;
>> };
>>
>> +struct devlink_port;
>> +
>> /**
>>   * struct efx_rep - Private data for an Efx representor
>>   *
>> @@ -54,6 +56,7 @@ struct efx_rep {
>> 	spinlock_t rx_lock;
>> 	struct napi_struct napi;
>> 	struct efx_rep_sw_stats stats;
>> +	struct devlink_port *dl_port;
>> };
>>
>> int efx_ef100_vfrep_create(struct efx_nic *efx, unsigned int i);
>> @@ -69,4 +72,7 @@ struct efx_rep *efx_ef100_find_rep_by_mport(struct efx_nic *efx, u16 mport);
>> extern const struct net_device_ops efx_ef100_rep_netdev_ops;
>> void efx_ef100_init_reps(struct efx_nic *efx);
>> void efx_ef100_fini_reps(struct efx_nic *efx);
>> +struct mae_mport_desc;
>> +bool ef100_mport_on_local_intf(struct efx_nic *efx,
>> +			       struct mae_mport_desc *mport_desc);
>> #endif /* EF100_REP_H */
>> diff --git a/drivers/net/ethernet/sfc/efx_devlink.c b/drivers/net/ethernet/sfc/efx_devlink.c
>> index c506f8f35d25..bb19d3ad7ffd 100644
>> --- a/drivers/net/ethernet/sfc/efx_devlink.c
>> +++ b/drivers/net/ethernet/sfc/efx_devlink.c
>> @@ -16,6 +16,8 @@
>> #include "mcdi.h"
>> #include "mcdi_functions.h"
>> #include "mcdi_pcol.h"
>> +#include "mae.h"
>> +#include "ef100_rep.h"
>>
>> /* Custom devlink-info version object names for details that do not map to the
>>   * generic standardized names.
>> @@ -381,6 +383,52 @@ struct efx_devlink {
>> 	struct efx_nic *efx;
>> };
>>
>> +static void efx_devlink_del_port(struct devlink_port *dl_port)
>> +{
>> +	if (!dl_port)
>> +		return;
>> +	devlink_port_unregister(dl_port);
>> +	kfree(dl_port);
>> +}
>> +
>> +static int efx_devlink_add_port(struct efx_nic *efx,
>> +				struct mae_mport_desc *mport,
>> +				struct devlink_port *dl_port)
>> +{
>> +	struct devlink_port_attrs attrs = {};
>> +	bool external = false;
>> +	int err;
>> +
>> +	if (!ef100_mport_on_local_intf(efx, mport))
>> +		external = true;
>> +
>> +	switch (mport->mport_type) {
>> +	case MAE_MPORT_DESC_MPORT_TYPE_NET_PORT:
>> +		attrs.flavour = DEVLINK_PORT_FLAVOUR_PHYSICAL;
>> +		attrs.phys.port_number = mport->port_idx;
>> +		devlink_port_attrs_set(dl_port, &attrs);
>> +		break;
>> +	case MAE_MPORT_DESC_MPORT_TYPE_VNIC:
>> +		if (mport->vf_idx != MAE_MPORT_DESC_VF_IDX_NULL) {
> No need for {}'s in this if-else
>

OK


>> +			devlink_port_attrs_pci_vf_set(dl_port, 0, mport->pf_idx,
>> +						      mport->vf_idx,
>> +						      external);
>> +		} else {
>> +			devlink_port_attrs_pci_pf_set(dl_port, 0, mport->pf_idx,
>> +						      external);
>> +		}
>> +		break;
>> +	default:
>> +		/* MAE_MPORT_DESC_MPORT_ALIAS and UNDEFINED */
>> +		return 0;
>> +	}
>> +
>> +	dl_port->index = mport->mport_id;
>> +	err = devlink_port_register(efx->devlink, dl_port, mport->mport_id);
> Just return.
>

OK


>> +
>> +	return err;
>> +}
>> +
>> static int efx_devlink_info_get(struct devlink *devlink,
>> 				struct devlink_info_req *req,
>> 				struct netlink_ext_ack *extack)
>> @@ -396,6 +444,72 @@ static const struct devlink_ops sfc_devlink_ops = {
>> 	.info_get			= efx_devlink_info_get,
>> };
>>
>> +static struct devlink_port *ef100_set_devlink_port(struct efx_nic *efx, u32 idx)
>> +{
>> +	struct mae_mport_desc *mport;
>> +	struct devlink_port *dl_port;
>> +	u32 id;
>> +
>> +	if (efx_mae_lookup_mport(efx, idx, &id)) {
>> +		/* This should not happen. */
>> +		if (idx == MAE_MPORT_DESC_VF_IDX_NULL)
>> +			pci_warn(efx->pci_dev, "No mport ID found for PF.\n");
>> +		else
>> +			pci_warn(efx->pci_dev, "No mport ID found for VF %u.\n",
>> +				 idx);
>> +		return NULL;
>> +	}
>> +
>> +	mport = efx_mae_get_mport(efx, id);
>> +	if (!mport) {
>> +		/* This should not happen. */
>> +		if (idx == MAE_MPORT_DESC_VF_IDX_NULL)
>> +			pci_warn(efx->pci_dev, "No mport found for PF.\n");
>> +		else
>> +			pci_warn(efx->pci_dev, "No mport found for VF %u.\n",
>> +				 idx);
>> +		return NULL;
>> +	}
>> +
>> +	dl_port = kzalloc(sizeof(*dl_port), GFP_KERNEL);
>> +	if (!dl_port)
>> +		return NULL;
>> +
>> +	if (efx_devlink_add_port(efx, mport, dl_port)) {
>> +		if (idx == MAE_MPORT_DESC_VF_IDX_NULL)
>> +			pci_warn(efx->pci_dev,
>> +				 "devlink port creation for PF failed.\n");
>> +		else
>> +			pci_warn(efx->pci_dev,
>> +				 "devlink_port creationg for VF %u failed.\n",
>> +				 idx);
>> +		kfree(dl_port);
>> +		return NULL;
>> +	}
>> +
>> +	return dl_port;
>> +}
>> +
>> +void ef100_rep_set_devlink_port(struct efx_rep *efv)
>> +{
>> +	efv->dl_port = ef100_set_devlink_port(efv->parent, efv->idx);
>> +}
>> +
>> +void ef100_pf_set_devlink_port(struct efx_nic *efx)
>> +{
>> +	efx->dl_port = ef100_set_devlink_port(efx, MAE_MPORT_DESC_VF_IDX_NULL);
>> +}
>> +
>> +void ef100_rep_unset_devlink_port(struct efx_rep *efv)
>> +{
>> +	efx_devlink_del_port(efv->dl_port);
>> +}
>> +
>> +void ef100_pf_unset_devlink_port(struct efx_nic *efx)
>> +{
>> +	efx_devlink_del_port(efx->dl_port);
>> +}
>> +
>> void efx_fini_devlink(struct efx_nic *efx)
>> {
>> 	if (efx->devlink) {
>> diff --git a/drivers/net/ethernet/sfc/efx_devlink.h b/drivers/net/ethernet/sfc/efx_devlink.h
>> index 997f878aea93..a834c393a9ad 100644
>> --- a/drivers/net/ethernet/sfc/efx_devlink.h
>> +++ b/drivers/net/ethernet/sfc/efx_devlink.h
>> @@ -17,4 +17,11 @@
>> int efx_probe_devlink(struct efx_nic *efx);
>> void efx_fini_devlink(struct efx_nic *efx);
>>
>> +struct mae_mport_desc;
>> +struct efx_rep;
>> +
>> +void ef100_pf_set_devlink_port(struct efx_nic *efx);
>> +void ef100_rep_set_devlink_port(struct efx_rep *efv);
>> +void ef100_pf_unset_devlink_port(struct efx_nic *efx);
>> +void ef100_rep_unset_devlink_port(struct efx_rep *efv);
>> #endif	/* _EFX_DEVLINK_H */
>> diff --git a/drivers/net/ethernet/sfc/net_driver.h b/drivers/net/ethernet/sfc/net_driver.h
>> index bc9efbfb3d6b..20f43695d082 100644
>> --- a/drivers/net/ethernet/sfc/net_driver.h
>> +++ b/drivers/net/ethernet/sfc/net_driver.h
>> @@ -1185,6 +1185,7 @@ struct efx_nic {
>> 	struct efx_tc_state *tc;
>>
>> 	struct devlink *devlink;
>> +	struct devlink_port *dl_port;
>> 	unsigned int mem_bar;
>> 	u32 reg_base;
>>
>> -- 
>> 2.17.1
>>

Powered by blists - more mailing lists