Message-ID: <Y9osFz6wWtxPqp49@nanopsycho>
Date:   Wed, 1 Feb 2023 10:08:39 +0100
From:   Jiri Pirko <jiri@...nulli.us>
To:     "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>,
        "habetsm.xilinx@...il.com" <habetsm.xilinx@...il.com>,
        "ecree.xilinx@...il.com" <ecree.xilinx@...il.com>,
        "linux-doc@...r.kernel.org" <linux-doc@...r.kernel.org>,
        "corbet@....net" <corbet@....net>,
        "jiri@...dia.com" <jiri@...dia.com>
Subject: Re: [PATCH v4 net-next 5/8] sfc: add devlink port support for ef100

Wed, Feb 01, 2023 at 10:03:10AM CET, alejandro.lucero-palau@....com wrote:
>
>
>On 1/31/23 16:12, Jiri Pirko wrote:
>> Tue, Jan 31, 2023 at 03:58:19PM CET, alejandro.lucero-palau@....com <mailto:alejandro.lucero-palau@....com> wrote:
>>> From: Alejandro Lucero <alejandro.lucero-palau@....com <mailto:alejandro.lucero-palau@....com>>
>>>
>>> Using the data when enumerating mports, create devlink ports just before
>>> netdevs are registered and remove those devlink ports after netdev has
>>> been unregistered.
>>>
>>> Signed-off-by: Alejandro Lucero <alejandro.lucero-palau@....com <mailto:alejandro.lucero-palau@....com>>
>>> ---
>>> drivers/net/ethernet/sfc/ef100_netdev.c | 9 +++
>>> drivers/net/ethernet/sfc/ef100_rep.c | 22 ++++++
>>> drivers/net/ethernet/sfc/ef100_rep.h | 7 ++
>>> drivers/net/ethernet/sfc/efx_devlink.c | 97 +++++++++++++++++++++++++
>>> drivers/net/ethernet/sfc/efx_devlink.h | 6 ++
>>> drivers/net/ethernet/sfc/mae.h | 2 +
>>> drivers/net/ethernet/sfc/net_driver.h | 2 +
>>> 7 files changed, 145 insertions(+)
>>>
>>> diff --git a/drivers/net/ethernet/sfc/ef100_netdev.c b/drivers/net/ethernet/sfc/ef100_netdev.c
>>> index b10a226f4a07..36774b55d413 100644
>>> --- a/drivers/net/ethernet/sfc/ef100_netdev.c
>>> +++ b/drivers/net/ethernet/sfc/ef100_netdev.c
>>> @@ -335,7 +335,9 @@ void ef100_remove_netdev(struct efx_probe_data *probe_data)
>>>
>>> /* devlink lock */
>>> efx_fini_devlink_start(efx);
>>> +
>> Does not seem related to the patch.
>>
>>
>>
>>> ef100_unregister_netdev(efx);
>>> + ef100_pf_unset_devlink_port(efx);
>>>
>>> #ifdef CONFIG_SFC_SRIOV
>>> efx_fini_tc(efx);
>>> @@ -423,6 +425,8 @@ int ef100_probe_netdev(struct efx_probe_data *probe_data)
>>> rc = ef100_probe_netdev_pf(efx);
>>> if (rc)
>>> goto fail;
>>> +
>>> + ef100_pf_set_devlink_port(efx);
>>> }
>>>
>>> efx->netdev_notifier.notifier_call = ef100_netdev_event;
>>> @@ -433,7 +437,12 @@ int ef100_probe_netdev(struct efx_probe_data *probe_data)
>>> goto fail;
>>> }
>>>
>>> + /* devlink unlock */
>>> + efx_probe_devlink_done(efx);
>>> + return rc;
>>> fail:
>>> + /* remove devlink port if does exist */
>>> + ef100_pf_unset_devlink_port(efx);
>>> /* devlink unlock */
>>> efx_probe_devlink_done(efx);
>>> return rc;
>>> diff --git a/drivers/net/ethernet/sfc/ef100_rep.c b/drivers/net/ethernet/sfc/ef100_rep.c
>>> index 9cd1a3ac67e0..6b5bc5d6955d 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,
>>> @@ -308,6 +310,7 @@ int efx_ef100_vfrep_create(struct efx_nic *efx, unsigned int 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 +326,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 +343,24 @@ void efx_ef100_fini_vfreps(struct efx_nic *efx)
>>> efx_ef100_vfrep_destroy(efx, efv);
>>> }
>>>
>>> +static 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)
>>> +{
>>> + struct ef100_nic_data *nic_data = efx->nic_data;
>>> + bool pcie_func;
>>> +
>>> + 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..ae6add4b0855 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
>>> *
>>> @@ -39,6 +41,7 @@ struct efx_rep_sw_stats {
>>> * @rx_lock: protects @rx_list
>>> * @napi: NAPI control structure
>>> * @stats: software traffic counters for netdev stats
>>> + * @dl_port: devlink port associated to this netdev representor
>>> */
>>> struct efx_rep {
>>> struct efx_nic *parent;
>>> @@ -54,6 +57,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 +73,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 ff5adfe3905e..b1637eb372ad 100644
>>> --- a/drivers/net/ethernet/sfc/efx_devlink.c
>>> +++ b/drivers/net/ethernet/sfc/efx_devlink.c
>>> @@ -16,11 +16,48 @@
>>> #include "mcdi.h"
>>> #include "mcdi_functions.h"
>>> #include "mcdi_pcol.h"
>>> +#include "mae.h"
>>> +#include "ef100_rep.h"
>>>
>>> struct efx_devlink {
>>> struct efx_nic *efx;
>>> };
>>>
>>> +static void efx_devlink_del_port(struct devlink_port *dl_port)
>>> +{
>>> + if (!dl_port)
>>> + return;
>>> + devl_port_unregister(dl_port);
>>> +}
>>> +
>>> +static int efx_devlink_add_port(struct efx_nic *efx,
>>> + struct mae_mport_desc *mport)
>>> +{
>>> + bool external = false;
>>> +
>>> + if (!ef100_mport_on_local_intf(efx, mport))
>>> + external = true;
>>> +
>>> + switch (mport->mport_type) {
>>> + case MAE_MPORT_DESC_MPORT_TYPE_VNIC:
>>> + if (mport->vf_idx != MAE_MPORT_DESC_VF_IDX_NULL)
>>> + devlink_port_attrs_pci_vf_set(&mport->dl_port, 0, mport->pf_idx,
>>> + mport->vf_idx,
>>> + external);
>>> + else
>>> + devlink_port_attrs_pci_pf_set(&mport->dl_port, 0, mport->pf_idx,
>>> + external);
>>> + break;
>>> + default:
>>> + /* MAE_MPORT_DESC_MPORT_ALIAS and UNDEFINED */
>>> + return 0;
>>> + }
>>> +
>>> + mport->dl_port.index = mport->mport_id;
>>> +
>>> + return devl_port_register(efx->devlink, &mport->dl_port, mport->mport_id);
>>> +}
>>> +
>>> static int efx_devlink_info_nvram_partition(struct efx_nic *efx,
>>> struct devlink_info_req *req,
>>> unsigned int partition_type,
>>> @@ -428,6 +465,66 @@ 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;
>>> + 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. */
>> Should not happen? WARN_ON?
>
>It makes sense.
>
>I will change it.
>
>>
>>> + 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;
>>> + }
>>> +
>>> + if (efx_devlink_add_port(efx, mport)) {
>> Store the return value into variable please.
>>
>
>OK
>
>>> + 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);
>>> + return NULL;
>>> + }
>>> +
>>> + return &mport->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_start(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 8bcd077d8d8d..d453a180c44c 100644
>>> --- a/drivers/net/ethernet/sfc/efx_devlink.h
>>> +++ b/drivers/net/ethernet/sfc/efx_devlink.h
>>> @@ -36,4 +36,10 @@ void efx_probe_devlink_done(struct efx_nic *efx);
>>> void efx_fini_devlink_start(struct efx_nic *efx);
>>> void efx_fini_devlink(struct efx_nic *efx);
>>>
>>> +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/mae.h b/drivers/net/ethernet/sfc/mae.h
>>> index d9adeafc0654..e1b7967132ad 100644
>>> --- a/drivers/net/ethernet/sfc/mae.h
>>> +++ b/drivers/net/ethernet/sfc/mae.h
>>> @@ -13,6 +13,7 @@
>>> #define EF100_MAE_H
>>> /* MCDI interface for the ef100 Match-Action Engine */
>>>
>>> +#include <net/devlink.h>
>>> #include "net_driver.h"
>>> #include "tc.h"
>>> #include "mcdi_pcol.h" /* needed for various MC_CMD_MAE_*_NULL defines */
>>> @@ -44,6 +45,7 @@ struct mae_mport_desc {
>>> };
>>> struct rhash_head linkage;
>>> struct efx_rep *efv;
>>> + struct devlink_port dl_port;
>> This is quite odd to store the devlink port struct in something called
>> "match action engine"... Why don't you put it to struct efx_nic
>> directly? Much more suitable.
>>
>
>Well, it is not called just mae but mae_mport_desc. And mae ports are 
>obviously related to devlink ports.
>
>We need to keep mae ports, and not always a mae port/devlink port will 
>be related to an efx_nic structure.
>
>If I'm not wrong, devlink was created because not everything in today's 
>networking devices can map smoothly to kernel networking structures, not 
>because kernel implementation is lacking such representation but because 
>it does not make sense semantically. Of course, efx_nic is not a generic kernel
>one, but again, we could need devlink ports but not such driver's struct.

Okay, that's fair.


>
>>> };
>>>
>>> int efx_mae_enumerate_mports(struct efx_nic *efx);
>>> diff --git a/drivers/net/ethernet/sfc/net_driver.h b/drivers/net/ethernet/sfc/net_driver.h
>>> index bc9efbfb3d6b..fcd51d3992fa 100644
>>> --- a/drivers/net/ethernet/sfc/net_driver.h
>>> +++ b/drivers/net/ethernet/sfc/net_driver.h
>>> @@ -998,6 +998,7 @@ struct efx_mae;
>>> * @netdev_notifier: Netdevice notifier.
>>> * @tc: state for TC offload (EF100).
>>> * @devlink: reference to devlink structure owned by this device
>>> + * @dl_port: devlink port associated with the PF
>>> * @mem_bar: The BAR that is mapped into membase.
>>> * @reg_base: Offset from the start of the bar to the function control window.
>>> * @monitor_work: Hardware monitor workitem
>>> @@ -1185,6 +1186,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