Date:   Wed, 1 Feb 2023 08:49:52 +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>,
        "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 1/8] sfc: add devlink support for ef100



On 1/31/23 16:00, Jiri Pirko wrote:
> Tue, Jan 31, 2023 at 03:58:15PM CET, alejandro.lucero-palau@....com <mailto:alejandro.lucero-palau@....com> wrote:
>> From: Alejandro Lucero <alejandro.lucero-palau@....com <mailto:alejandro.lucero-palau@....com>>
>>
>> Basic devlink infrastructure support.
>>
>> Signed-off-by: Alejandro Lucero <alejandro.lucero-palau@....com <mailto:alejandro.lucero-palau@....com>>
>> ---
>> drivers/net/ethernet/sfc/Kconfig | 1 +
>> drivers/net/ethernet/sfc/Makefile | 3 +-
>> drivers/net/ethernet/sfc/ef100_netdev.c | 12 +++++
>> drivers/net/ethernet/sfc/ef100_nic.c | 3 +-
>> drivers/net/ethernet/sfc/efx_devlink.c | 71 +++++++++++++++++++++++++
>> drivers/net/ethernet/sfc/efx_devlink.h | 22 ++++++++
>> drivers/net/ethernet/sfc/net_driver.h | 2 +
>> 7 files changed, 111 insertions(+), 3 deletions(-)
>> create mode 100644 drivers/net/ethernet/sfc/efx_devlink.c
>> create mode 100644 drivers/net/ethernet/sfc/efx_devlink.h
>>
>> diff --git a/drivers/net/ethernet/sfc/Kconfig b/drivers/net/ethernet/sfc/Kconfig
>> index 0950e6b0508f..4af36ba8906b 100644
>> --- a/drivers/net/ethernet/sfc/Kconfig
>> +++ b/drivers/net/ethernet/sfc/Kconfig
>> @@ -22,6 +22,7 @@ config SFC
>> depends on PTP_1588_CLOCK_OPTIONAL
>> select MDIO
>> select CRC32
>> + select NET_DEVLINK
>> help
>> This driver supports 10/40-gigabit Ethernet cards based on
>> the Solarflare SFC9100-family controllers.
>> diff --git a/drivers/net/ethernet/sfc/Makefile b/drivers/net/ethernet/sfc/Makefile
>> index 712a48d00069..55b9c73cd8ef 100644
>> --- a/drivers/net/ethernet/sfc/Makefile
>> +++ b/drivers/net/ethernet/sfc/Makefile
>> @@ -6,7 +6,8 @@ sfc-y += efx.o efx_common.o efx_channels.o nic.o \
>> mcdi.o mcdi_port.o mcdi_port_common.o \
>> mcdi_functions.o mcdi_filters.o mcdi_mon.o \
>> ef100.o ef100_nic.o ef100_netdev.o \
>> - ef100_ethtool.o ef100_rx.o ef100_tx.o
>> + ef100_ethtool.o ef100_rx.o ef100_tx.o \
>> + efx_devlink.o
>> sfc-$(CONFIG_SFC_MTD) += mtd.o
>> sfc-$(CONFIG_SFC_SRIOV) += sriov.o ef10_sriov.o ef100_sriov.o ef100_rep.o \
>> mae.o tc.o tc_bindings.o tc_counters.o
>> diff --git a/drivers/net/ethernet/sfc/ef100_netdev.c b/drivers/net/ethernet/sfc/ef100_netdev.c
>> index ddcc325ed570..b10a226f4a07 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)
>> {
>> @@ -332,6 +333,8 @@ void ef100_remove_netdev(struct efx_probe_data *probe_data)
>> efx_ef100_pci_sriov_disable(efx, true);
>> #endif
>>
>> + /* devlink lock */
>> + efx_fini_devlink_start(efx);
>> ef100_unregister_netdev(efx);
>>
>> #ifdef CONFIG_SFC_SRIOV
>> @@ -345,6 +348,9 @@ void ef100_remove_netdev(struct efx_probe_data *probe_data)
>> kfree(efx->phy_data);
>> efx->phy_data = NULL;
>>
>> + /* devlink unlock */
>> + efx_fini_devlink(efx);
>> +
>> free_netdev(efx->net_dev);
>> efx->net_dev = NULL;
>> efx->state = STATE_PROBED;
>> @@ -405,6 +411,10 @@ 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);
>>
>> + /* devlink creation, registration and lock */
>> + if (efx_probe_devlink(efx))
> Use variable to store the return value and check that in if.
>

I'll do.

>> + pci_info(efx->pci_dev, "devlink registration failed");
>> +
>> rc = ef100_register_netdev(efx);
>> if (rc)
>> goto fail;
>> @@ -424,5 +434,7 @@ int ef100_probe_netdev(struct efx_probe_data *probe_data)
>> }
>>
>> fail:
>> + /* devlink unlock */
>> + efx_probe_devlink_done(efx);
>> return rc;
>> }
>> diff --git a/drivers/net/ethernet/sfc/ef100_nic.c b/drivers/net/ethernet/sfc/ef100_nic.c
>> index ad686c671ab8..e4aacb4ec666 100644
>> --- a/drivers/net/ethernet/sfc/ef100_nic.c
>> +++ b/drivers/net/ethernet/sfc/ef100_nic.c
>> @@ -1120,11 +1120,10 @@ int ef100_probe_netdev_pf(struct efx_nic *efx)
>> return rc;
>>
>> rc = efx_ef100_get_base_mport(efx);
>> - if (rc) {
>> + if (rc)
>> netif_warn(efx, probe, net_dev,
>> "Failed to probe base mport rc %d; representors will not function\n",
>> rc);
>> - }
> I don't see how this hunk is related to this patch. Please remove.
>
>

Running checkpatch on this specific patch triggered a warning about the 
netif_warn requiring brackets.

It is true it is not related to the patch itself, so I'll remove it.

>> rc = efx_init_tc(efx);
>> if (rc) {
>> diff --git a/drivers/net/ethernet/sfc/efx_devlink.c b/drivers/net/ethernet/sfc/efx_devlink.c
>> new file mode 100644
>> index 000000000000..fab06aaa4b8a
>> --- /dev/null
>> +++ b/drivers/net/ethernet/sfc/efx_devlink.c
>> @@ -0,0 +1,71 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/****************************************************************************
>> + * Driver for AMD network controllers and boards
>> + * Copyright (C) 2023, Advanced Micro Devices, Inc.
>> + *
>> + * This program is free software; you can redistribute it and/or modify it
>> + * under the terms of the GNU General Public License version 2 as published
>> + * by the Free Software Foundation, incorporated herein by reference.
>> + */
>> +
>> +#include <linux/rtc.h>
>> +#include "net_driver.h"
>> +#include "ef100_nic.h"
>> +#include "efx_devlink.h"
>> +#include "nic.h"
>> +#include "mcdi.h"
>> +#include "mcdi_functions.h"
>> +#include "mcdi_pcol.h"
>> +
>> +struct efx_devlink {
>> + struct efx_nic *efx;
>> +};
>> +
>> +static const struct devlink_ops sfc_devlink_ops = {
>> +};
>> +
>> +void efx_fini_devlink_start(struct efx_nic *efx)
>> +{
>> + if (efx->devlink)
>> + devl_lock(efx->devlink);
>> +}
>> +
>> +void efx_fini_devlink(struct efx_nic *efx)
>> +{
>> + if (efx->devlink) {
>> + devl_unregister(efx->devlink);
>> + devl_unlock(efx->devlink);
>> + devlink_free(efx->devlink);
>> + efx->devlink = NULL;
>> + }
>> +}
>> +
>> +int efx_probe_devlink(struct efx_nic *efx)
>> +{
>> + struct efx_devlink *devlink_private;
>> +
>> + if (efx->type->is_vf)
>> + return 0;
>> +
>> + efx->devlink = devlink_alloc(&sfc_devlink_ops,
>> + sizeof(struct efx_devlink),
>> + &efx->pci_dev->dev);
>> + if (!efx->devlink)
>> + return -ENOMEM;
>> +
>> + devl_lock(efx->devlink);
>> + devlink_private = devlink_priv(efx->devlink);
>> + devlink_private->efx = efx;
>> +
>> + devl_register(efx->devlink);
>> +
>> + return 0;
>> +}
>> +
>> +void efx_probe_devlink_done(struct efx_nic *efx)
>> +{
>> + if (!efx->devlink)
>> + return;
>> +
>> + devl_unlock(efx->devlink);
>> +}
>> diff --git a/drivers/net/ethernet/sfc/efx_devlink.h b/drivers/net/ethernet/sfc/efx_devlink.h
>> new file mode 100644
>> index 000000000000..55d0d8aeca1e
>> --- /dev/null
>> +++ b/drivers/net/ethernet/sfc/efx_devlink.h
>> @@ -0,0 +1,22 @@
>> +/* SPDX-License-Identifier: GPL-2.0-only */
>> +/****************************************************************************
>> + * Driver for AMD network controllers and boards
>> + * Copyright (C) 2023, Advanced Micro Devices, Inc.
>> + *
>> + * This program is free software; you can redistribute it and/or modify it
>> + * under the terms of the GNU General Public License version 2 as published
>> + * by the Free Software Foundation, incorporated herein by reference.
>> + */
>> +
>> +#ifndef _EFX_DEVLINK_H
>> +#define _EFX_DEVLINK_H
>> +
>> +#include "net_driver.h"
>> +#include <net/devlink.h>
>> +
>> +int efx_probe_devlink(struct efx_nic *efx);
>> +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);
> Odd naming... Just saying.
>

This is due to the recommended/required devlink lock/unlock during 
driver initialization/removal.

I think it is better to keep the lock/unlock inside the specific driver 
devlink code, and the functions naming reflects a time window when 
devlink related/dependent processing is being done.

I'm not against changing this, maybe adding the lock/unlock suffix would 
be preferable?:

int efx_probe_devlink_and_lock(struct efx_nic *efx);
void efx_probe_devlink_unlock(struct efx_nic *efx);
void efx_fini_devlink_lock(struct efx_nic *efx);
void efx_fini_devlink_and_unlock(struct efx_nic *efx);

>> +
>> +#endif /* _EFX_DEVLINK_H */
>> diff --git a/drivers/net/ethernet/sfc/net_driver.h b/drivers/net/ethernet/sfc/net_driver.h
>> index 3b49e216768b..d036641dc043 100644
>> --- a/drivers/net/ethernet/sfc/net_driver.h
>> +++ b/drivers/net/ethernet/sfc/net_driver.h
>> @@ -994,6 +994,7 @@ enum efx_xdp_tx_queues_mode {
>> * xdp_rxq_info structures?
>> * @netdev_notifier: Netdevice notifier.
>> * @tc: state for TC offload (EF100).
>> + * @devlink: reference to devlink structure owned by this device
>> * @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
>> @@ -1179,6 +1180,7 @@ struct efx_nic {
>> struct notifier_block netdev_notifier;
>> struct efx_tc_state *tc;
>>
>> + struct devlink *devlink;
>> unsigned int mem_bar;
>> u32 reg_base;
>>
>> -- 
>> 2.17.1
>>

Powered by blists - more mailing lists