| Message ID | 20230131145822.36208-2-alejandro.lucero-palau@amd.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | sfc: devlink support for ef100 | expand |
Tue, Jan 31, 2023 at 03:58:15PM CET, alejandro.lucero-palau@amd.com wrote: >From: Alejandro Lucero <alejandro.lucero-palau@amd.com> > >Basic devlink infrastructure support. > >Signed-off-by: Alejandro Lucero <alejandro.lucero-palau@amd.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. >+ 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. > > 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. >+ >+#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 >
On 1/31/23 16:00, Jiri Pirko wrote: > Tue, Jan 31, 2023 at 03:58:15PM CET, alejandro.lucero-palau@amd.com <mailto:alejandro.lucero-palau@amd.com> wrote: >> From: Alejandro Lucero <alejandro.lucero-palau@amd.com <mailto:alejandro.lucero-palau@amd.com>> >> >> Basic devlink infrastructure support. >> >> Signed-off-by: Alejandro Lucero <alejandro.lucero-palau@amd.com <mailto:alejandro.lucero-palau@amd.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 >>
Wed, Feb 01, 2023 at 09:49:52AM CET, alejandro.lucero-palau@amd.com wrote: > > >On 1/31/23 16:00, Jiri Pirko wrote: >> Tue, Jan 31, 2023 at 03:58:15PM CET, alejandro.lucero-palau@amd.com <mailto:alejandro.lucero-palau@amd.com> wrote: >>> From: Alejandro Lucero <alejandro.lucero-palau@amd.com <mailto:alejandro.lucero-palau@amd.com>> >>> >>> Basic devlink infrastructure support. >>> >>> Signed-off-by: Alejandro Lucero <alejandro.lucero-palau@amd.com <mailto:alejandro.lucero-palau@amd.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); Sounds better. Thanks! > >>> + >>> +#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 >>> > > >
On Wed, 1 Feb 2023 10:07:33 +0100 Jiri Pirko wrote: > >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); > > Sounds better. Thanks! FWIW I'd just take the devl lock in the main driver code. devlink should be viewed as a layer between bus and driver rather than as another subsystem the driver registers with. Otherwise reloads and port creation get awkward. But the above sounds okay, too.
On 2/1/23 19:01, Jakub Kicinski wrote: > On Wed, 1 Feb 2023 10:07:33 +0100 Jiri Pirko wrote: >>> 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); >> Sounds better. Thanks! > FWIW I'd just take the devl lock in the main driver code. > devlink should be viewed as a layer between bus and driver rather > than as another subsystem the driver registers with. Otherwise reloads > and port creation get awkward. > > But the above sounds okay, too. That is what I have tried to do with these extra functions invoked from the main driver code. The problem is one part of the protection needs to be done inside the driver's devlink code (if we want to avoid different calls for allocating then registering or unregistering then releasing), so it looked awkward to me the other protection part being directly in main driver code. So, I think the extra functions are worth for avoiding confusion.
On Wed, Feb 01, 2023 at 11:01:48AM -0800, Jakub Kicinski wrote: > On Wed, 1 Feb 2023 10:07:33 +0100 Jiri Pirko wrote: > > >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); > > > > Sounds better. Thanks! > > FWIW I'd just take the devl lock in the main driver code. > devlink should be viewed as a layer between bus and driver rather > than as another subsystem the driver registers with. Otherwise reloads > and port creation get awkward. I see it a bit differently. For me devlink is another subsystem, it even is an optional subsystem. At the moment we don't support devlink port for VFs. If needed we'll add that at some point, but likely only for newer NICs. Do you think vDPA and RDMA devices will ever register with devlink? At the moment I don't see devlink port ever applying to our older hardware, like our sfn8000 or X2 cards. I do think devlink info and other commands could apply more generally. There definitely is a need to evolve to another layer between bus and devices, and devlink can be used to administer that. But that does not imply the reverse, that all devices register as devlink devices. For security we would want to limit some operations (such as port creation) to specific devlink instance(s). For example, normally we would not want a tennant VM to flash new firmware that applies to the whole NIC. I hope this makes sense. Martin
On Thu, 2 Feb 2023 09:24:56 +0000 Martin Habets wrote: > > FWIW I'd just take the devl lock in the main driver code. > > devlink should be viewed as a layer between bus and driver rather > > than as another subsystem the driver registers with. Otherwise reloads > > and port creation get awkward. > > I see it a bit differently. For me devlink is another subsystem, it even is > an optional subsystem. > At the moment we don't support devlink port for VFs. If needed we'll add that > at some point, but likely only for newer NICs. That's fine. I believe the structure I suggest is the easiest one to get right, but it's not a hard requirement. > Do you think vDPA and RDMA devices will ever register with devlink? Good question, I can't speak for the entire project but personally I have little interest in interfaces to proprietary world, so I hope not. > At the moment I don't see devlink port ever applying to our older hardware, > like our sfn8000 or X2 cards. I do think devlink info and other commands > could apply more generally. > > There definitely is a need to evolve to another layer between bus and > devices, and devlink can be used to administer that. But that does not > imply the reverse, that all devices register as devlink devices. > For security we would want to limit some operations (such as port creation) > to specific devlink instance(s). For example, normally we would not want a > tennant VM to flash new firmware that applies to the whole NIC. > I hope this makes sense.
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)) + 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); - } 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); + +#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;