| Message ID | 20230609211626.621968-3-david.m.ertman@intel.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Implement support for SRIOV + LAG | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Guessing tree name failed - patch did not apply |
On Fri, Jun 09, 2023 at 02:16:18PM -0700, Dave Ertman wrote: ... Hi Dave, some minor feedback from my side. > @@ -5576,10 +5579,18 @@ static int __init ice_module_init(void) > return -ENOMEM; > } > > + ice_lag_wq = alloc_ordered_workqueue("ice_lag_wq", 0); > + if (!ice_lag_wq) { > + pr_err("Failed to create LAG workqueue\n"); Is the allocation failure already logged by core code? If so, perhaps this is unnecessary? > + destroy_workqueue(ice_wq); > + return -ENOMEM; > + } > + > status = pci_register_driver(&ice_driver); > if (status) { > pr_err("failed to register PCI driver, err %d\n", status); > destroy_workqueue(ice_wq); > + destroy_workqueue(ice_lag_wq); > } > > return status; As there are now a few things (more than zero) to unwind I think it would be best to use the Kernel's idiomatic goto-based approach. (Completely untested!) ice_lag_wq = alloc_ordered_workqueue("ice_lag_wq", 0); if (!ice_lag_wq) { pr_err("Failed to create LAG workqueue\n"); status = -ENOMEM; goto err_destroy_ice_wq; } status = pci_register_driver(&ice_driver); if (status) { pr_err("failed to register PCI driver, err %d\n", status); goto err_destroy_lag_wq; } return status; err_destroy_lag_wq: destroy_workqueue(ice_lag_wq); err_destroy_ice_wq: destroy_workqueue(ice_wq); return status > @@ -5596,6 +5607,7 @@ static void __exit ice_module_exit(void) > { > pci_unregister_driver(&ice_driver); > destroy_workqueue(ice_wq); > + destroy_workqueue(ice_lag_wq); > pr_info("module unloaded\n"); > } > module_exit(ice_module_exit); ...
> -----Original Message----- > From: Simon Horman <simon.horman@corigine.com> > Sent: Wednesday, June 14, 2023 4:18 AM > To: Ertman, David M <david.m.ertman@intel.com> > Cc: intel-wired-lan@lists.osuosl.org; daniel.machon@microchip.com; > netdev@vger.kernel.org > Subject: Re: [PATCH iwl-next v4 02/10] ice: Add driver support for firmware > changes for LAG > > On Fri, Jun 09, 2023 at 02:16:18PM -0700, Dave Ertman wrote: > > ... > > Hi Dave, > > some minor feedback from my side. > > > @@ -5576,10 +5579,18 @@ static int __init ice_module_init(void) > > return -ENOMEM; > > } > > > > + ice_lag_wq = alloc_ordered_workqueue("ice_lag_wq", 0); > > + if (!ice_lag_wq) { > > + pr_err("Failed to create LAG workqueue\n"); > > Is the allocation failure already logged by core code? > If so, perhaps this is unnecessary? I do not see any messaging from the core, so I should probably leave this here unless you can point out something I missed
On Wed, Jun 14, 2023 at 04:56:53PM +0000, Ertman, David M wrote: > > -----Original Message----- > > From: Simon Horman <simon.horman@corigine.com> > > Sent: Wednesday, June 14, 2023 4:18 AM > > To: Ertman, David M <david.m.ertman@intel.com> > > Cc: intel-wired-lan@lists.osuosl.org; daniel.machon@microchip.com; > > netdev@vger.kernel.org > > Subject: Re: [PATCH iwl-next v4 02/10] ice: Add driver support for firmware > > changes for LAG > > > > On Fri, Jun 09, 2023 at 02:16:18PM -0700, Dave Ertman wrote: > > > > ... > > > > Hi Dave, > > > > some minor feedback from my side. > > > > > @@ -5576,10 +5579,18 @@ static int __init ice_module_init(void) > > > return -ENOMEM; > > > } > > > > > > + ice_lag_wq = alloc_ordered_workqueue("ice_lag_wq", 0); > > > + if (!ice_lag_wq) { > > > + pr_err("Failed to create LAG workqueue\n"); > > > > Is the allocation failure already logged by core code? > > If so, perhaps this is unnecessary? > > I do not see any messaging from the core, so I should probably leave this here > unless you can point out something I missed
On 6/9/2023 2:16 PM, Dave Ertman wrote: > Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding. > > > Add the defines, fields, and detection code for FW support of LAG for > SRIOV. Also exposes some previously static functions to allow access > in the lag code. > > Clean up code that is unused or not needed for LAG support. Also add > an ordered workqueue for processing LAG events. > > Reviewed-by: Daniel Machon <daniel.machon@microchip.com> > Signed-off-by: Dave Ertman <david.m.ertman@intel.com> > --- > drivers/net/ethernet/intel/ice/ice.h | 5 ++ > .../net/ethernet/intel/ice/ice_adminq_cmd.h | 3 ++ > drivers/net/ethernet/intel/ice/ice_common.c | 8 +++ > drivers/net/ethernet/intel/ice/ice_lag.c | 53 ++++++++++--------- > drivers/net/ethernet/intel/ice/ice_lib.c | 2 +- > drivers/net/ethernet/intel/ice/ice_lib.h | 1 + > drivers/net/ethernet/intel/ice/ice_main.c | 12 +++++ > drivers/net/ethernet/intel/ice/ice_type.h | 2 + > 8 files changed, 59 insertions(+), 27 deletions(-) > > diff --git a/drivers/net/ethernet/intel/ice/ice.h b/drivers/net/ethernet/intel/ice/ice.h > index 9109006336f0..5ac0ad12f9f1 100644 > --- a/drivers/net/ethernet/intel/ice/ice.h > +++ b/drivers/net/ethernet/intel/ice/ice.h > @@ -200,6 +200,8 @@ enum ice_feature { > ICE_F_PTP_EXTTS, > ICE_F_SMA_CTRL, > ICE_F_GNSS, > + ICE_F_ROCE_LAG, > + ICE_F_SRIOV_LAG, > ICE_F_MAX > }; > > @@ -569,6 +571,7 @@ struct ice_pf { > struct mutex sw_mutex; /* lock for protecting VSI alloc flow */ > struct mutex tc_mutex; /* lock to protect TC changes */ > struct mutex adev_mutex; /* lock to protect aux device access */ > + struct mutex lag_mutex; /* protect ice_lag struct in PF */ > u32 msg_enable; > struct ice_ptp ptp; > struct gnss_serial *gnss_serial; > @@ -639,6 +642,8 @@ struct ice_pf { > struct ice_agg_node vf_agg_node[ICE_MAX_VF_AGG_NODES]; > }; > > +extern struct workqueue_struct *ice_lag_wq; > + > struct ice_netdev_priv { > struct ice_vsi *vsi; > struct ice_repr *repr; > diff --git a/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h b/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h > index 6ea0d4c017f0..6b5ba9a02bad 100644 > --- a/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h > +++ b/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h > @@ -120,6 +120,9 @@ struct ice_aqc_list_caps_elem { > #define ICE_AQC_CAPS_PCIE_RESET_AVOIDANCE 0x0076 > #define ICE_AQC_CAPS_POST_UPDATE_RESET_RESTRICT 0x0077 > #define ICE_AQC_CAPS_NVM_MGMT 0x0080 > +#define ICE_AQC_CAPS_FW_LAG_SUPPORT 0x0092 > +#define ICE_AQC_BIT_ROCEV2_LAG 0x01 > +#define ICE_AQC_BIT_SRIOV_LAG 0x02 > > u8 major_ver; > u8 minor_ver; > diff --git a/drivers/net/ethernet/intel/ice/ice_common.c b/drivers/net/ethernet/intel/ice/ice_common.c > index 09e2e38d538e..6ddf607d2edf 100644 > --- a/drivers/net/ethernet/intel/ice/ice_common.c > +++ b/drivers/net/ethernet/intel/ice/ice_common.c > @@ -2241,6 +2241,14 @@ ice_parse_common_caps(struct ice_hw *hw, struct ice_hw_common_caps *caps, > "%s: reset_restrict_support = %d\n", prefix, > caps->reset_restrict_support); > break; > + case ICE_AQC_CAPS_FW_LAG_SUPPORT: > + caps->roce_lag = !!(number & ICE_AQC_BIT_ROCEV2_LAG); > + ice_debug(hw, ICE_DBG_INIT, "%s: roce_lag = %u\n", > + prefix, caps->roce_lag); > + caps->sriov_lag = !!(number & ICE_AQC_BIT_SRIOV_LAG); > + ice_debug(hw, ICE_DBG_INIT, "%s: sriov_lag = %u\n", > + prefix, caps->sriov_lag); > + break; > default: > /* Not one of the recognized common capabilities */ > found = false; > diff --git a/drivers/net/ethernet/intel/ice/ice_lag.c b/drivers/net/ethernet/intel/ice/ice_lag.c > index 5a7753bda324..73bfc5cd8b37 100644 > --- a/drivers/net/ethernet/intel/ice/ice_lag.c > +++ b/drivers/net/ethernet/intel/ice/ice_lag.c > @@ -4,8 +4,12 @@ > /* Link Aggregation code */ > > #include "ice.h" > +#include "ice_lib.h" > #include "ice_lag.h" > > +#define ICE_LAG_RES_SHARED BIT(14) > +#define ICE_LAG_RES_VALID BIT(15) > + > /** > * ice_lag_set_primary - set PF LAG state as Primary > * @lag: LAG info struct > @@ -225,6 +229,26 @@ static void ice_lag_unregister(struct ice_lag *lag, struct net_device *netdev) > lag->role = ICE_LAG_NONE; > } > > +/** > + * ice_lag_check_nvm_support - Check for NVM support for LAG > + * @pf: PF struct > + */ > +static void ice_lag_check_nvm_support(struct ice_pf *pf) Nit, but this name is a bit misleading to me. I would expect it to be called something like "ice_lag_init_feature_support_flag()" or something similar that better describes what the function is doing. > +{ > + struct ice_hw_dev_caps *caps; > + > + caps = &pf->hw.dev_caps; Nit, but since you are already creating a local variable you could go one level further to the common_cap, so it could be: common_cap->roce_lag and common_cap->sriov_lag > + if (caps->common_cap.roce_lag) > + ice_set_feature_support(pf, ICE_F_ROCE_LAG); > + else > + ice_clear_feature_support(pf, ICE_F_ROCE_LAG); > + > + if (caps->common_cap.sriov_lag) > + ice_set_feature_support(pf, ICE_F_SRIOV_LAG); > + else > + ice_clear_feature_support(pf, ICE_F_SRIOV_LAG); > +} > + [...]
> -----Original Message----- > From: Brett Creeley <bcreeley@amd.com> > Sent: Wednesday, June 14, 2023 2:24 PM > To: Ertman, David M <david.m.ertman@intel.com>; intel-wired- > lan@lists.osuosl.org > Cc: daniel.machon@microchip.com; simon.horman@corigine.com; > netdev@vger.kernel.org > Subject: Re: [PATCH iwl-next v4 02/10] ice: Add driver support for firmware > changes for LAG > > On 6/9/2023 2:16 PM, Dave Ertman wrote: > > Caution: This message originated from an External Source. Use proper > caution when opening attachments, clicking links, or responding. > > > > > > Add the defines, fields, and detection code for FW support of LAG for > > SRIOV. Also exposes some previously static functions to allow access > > in the lag code. > > > > Clean up code that is unused or not needed for LAG support. Also add > > an ordered workqueue for processing LAG events. > > > > Reviewed-by: Daniel Machon <daniel.machon@microchip.com> > > Signed-off-by: Dave Ertman <david.m.ertman@intel.com> ... > > +/** > > + * ice_lag_check_nvm_support - Check for NVM support for LAG > > + * @pf: PF struct > > + */ > > +static void ice_lag_check_nvm_support(struct ice_pf *pf) > > Nit, but this name is a bit misleading to me. I would expect it to be > called something like "ice_lag_init_feature_support_flag()" or something > similar that better describes what the function is doing. Name changed > > > +{ > > + struct ice_hw_dev_caps *caps; > > + > > + caps = &pf->hw.dev_caps; > > Nit, but since you are already creating a local variable you could go > one level further to the common_cap, so it could be: > > common_cap->roce_lag and common_cap->sriov_lag Variable changed to go down one mor level. Changes to come out in v5 DaveE > > > + if (caps->common_cap.roce_lag) > > + ice_set_feature_support(pf, ICE_F_ROCE_LAG); > > + else > > + ice_clear_feature_support(pf, ICE_F_ROCE_LAG); > > + > > + if (caps->common_cap.sriov_lag) > > + ice_set_feature_support(pf, ICE_F_SRIOV_LAG); > > + else > > + ice_clear_feature_support(pf, ICE_F_SRIOV_LAG); > > +} > > + > > [...] >
diff --git a/drivers/net/ethernet/intel/ice/ice.h b/drivers/net/ethernet/intel/ice/ice.h index 9109006336f0..5ac0ad12f9f1 100644 --- a/drivers/net/ethernet/intel/ice/ice.h +++ b/drivers/net/ethernet/intel/ice/ice.h @@ -200,6 +200,8 @@ enum ice_feature { ICE_F_PTP_EXTTS, ICE_F_SMA_CTRL, ICE_F_GNSS, + ICE_F_ROCE_LAG, + ICE_F_SRIOV_LAG, ICE_F_MAX }; @@ -569,6 +571,7 @@ struct ice_pf { struct mutex sw_mutex; /* lock for protecting VSI alloc flow */ struct mutex tc_mutex; /* lock to protect TC changes */ struct mutex adev_mutex; /* lock to protect aux device access */ + struct mutex lag_mutex; /* protect ice_lag struct in PF */ u32 msg_enable; struct ice_ptp ptp; struct gnss_serial *gnss_serial; @@ -639,6 +642,8 @@ struct ice_pf { struct ice_agg_node vf_agg_node[ICE_MAX_VF_AGG_NODES]; }; +extern struct workqueue_struct *ice_lag_wq; + struct ice_netdev_priv { struct ice_vsi *vsi; struct ice_repr *repr; diff --git a/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h b/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h index 6ea0d4c017f0..6b5ba9a02bad 100644 --- a/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h +++ b/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h @@ -120,6 +120,9 @@ struct ice_aqc_list_caps_elem { #define ICE_AQC_CAPS_PCIE_RESET_AVOIDANCE 0x0076 #define ICE_AQC_CAPS_POST_UPDATE_RESET_RESTRICT 0x0077 #define ICE_AQC_CAPS_NVM_MGMT 0x0080 +#define ICE_AQC_CAPS_FW_LAG_SUPPORT 0x0092 +#define ICE_AQC_BIT_ROCEV2_LAG 0x01 +#define ICE_AQC_BIT_SRIOV_LAG 0x02 u8 major_ver; u8 minor_ver; diff --git a/drivers/net/ethernet/intel/ice/ice_common.c b/drivers/net/ethernet/intel/ice/ice_common.c index 09e2e38d538e..6ddf607d2edf 100644 --- a/drivers/net/ethernet/intel/ice/ice_common.c +++ b/drivers/net/ethernet/intel/ice/ice_common.c @@ -2241,6 +2241,14 @@ ice_parse_common_caps(struct ice_hw *hw, struct ice_hw_common_caps *caps, "%s: reset_restrict_support = %d\n", prefix, caps->reset_restrict_support); break; + case ICE_AQC_CAPS_FW_LAG_SUPPORT: + caps->roce_lag = !!(number & ICE_AQC_BIT_ROCEV2_LAG); + ice_debug(hw, ICE_DBG_INIT, "%s: roce_lag = %u\n", + prefix, caps->roce_lag); + caps->sriov_lag = !!(number & ICE_AQC_BIT_SRIOV_LAG); + ice_debug(hw, ICE_DBG_INIT, "%s: sriov_lag = %u\n", + prefix, caps->sriov_lag); + break; default: /* Not one of the recognized common capabilities */ found = false; diff --git a/drivers/net/ethernet/intel/ice/ice_lag.c b/drivers/net/ethernet/intel/ice/ice_lag.c index 5a7753bda324..73bfc5cd8b37 100644 --- a/drivers/net/ethernet/intel/ice/ice_lag.c +++ b/drivers/net/ethernet/intel/ice/ice_lag.c @@ -4,8 +4,12 @@ /* Link Aggregation code */ #include "ice.h" +#include "ice_lib.h" #include "ice_lag.h" +#define ICE_LAG_RES_SHARED BIT(14) +#define ICE_LAG_RES_VALID BIT(15) + /** * ice_lag_set_primary - set PF LAG state as Primary * @lag: LAG info struct @@ -225,6 +229,26 @@ static void ice_lag_unregister(struct ice_lag *lag, struct net_device *netdev) lag->role = ICE_LAG_NONE; } +/** + * ice_lag_check_nvm_support - Check for NVM support for LAG + * @pf: PF struct + */ +static void ice_lag_check_nvm_support(struct ice_pf *pf) +{ + struct ice_hw_dev_caps *caps; + + caps = &pf->hw.dev_caps; + if (caps->common_cap.roce_lag) + ice_set_feature_support(pf, ICE_F_ROCE_LAG); + else + ice_clear_feature_support(pf, ICE_F_ROCE_LAG); + + if (caps->common_cap.sriov_lag) + ice_set_feature_support(pf, ICE_F_SRIOV_LAG); + else + ice_clear_feature_support(pf, ICE_F_SRIOV_LAG); +} + /** * ice_lag_changeupper_event - handle LAG changeupper event * @lag: LAG info struct @@ -264,26 +288,6 @@ static void ice_lag_changeupper_event(struct ice_lag *lag, void *ptr) ice_display_lag_info(lag); } -/** - * ice_lag_changelower_event - handle LAG changelower event - * @lag: LAG info struct - * @ptr: opaque data pointer - * - * ptr to be cast to netdev_notifier_changelowerstate_info - */ -static void ice_lag_changelower_event(struct ice_lag *lag, void *ptr) -{ - struct net_device *netdev = netdev_notifier_info_to_dev(ptr); - - if (netdev != lag->netdev) - return; - - netdev_dbg(netdev, "bonding info\n"); - - if (!netif_is_lag_port(netdev)) - netdev_dbg(netdev, "CHANGELOWER rcvd, but netdev not in LAG. Bail\n"); -} - /** * ice_lag_event_handler - handle LAG events from netdev * @notif_blk: notifier block registered by this netdev @@ -310,9 +314,6 @@ ice_lag_event_handler(struct notifier_block *notif_blk, unsigned long event, case NETDEV_CHANGEUPPER: ice_lag_changeupper_event(lag, ptr); break; - case NETDEV_CHANGELOWERSTATE: - ice_lag_changelower_event(lag, ptr); - break; case NETDEV_BONDING_INFO: ice_lag_info_event(lag, ptr); break; @@ -379,6 +380,8 @@ int ice_init_lag(struct ice_pf *pf) struct ice_vsi *vsi; int err; + ice_lag_check_nvm_support(pf); + pf->lag = kzalloc(sizeof(*lag), GFP_KERNEL); if (!pf->lag) return -ENOMEM; @@ -435,9 +438,7 @@ void ice_deinit_lag(struct ice_pf *pf) if (lag->pf) ice_unregister_lag_handler(lag); - dev_put(lag->upper_netdev); - - dev_put(lag->peer_netdev); + flush_workqueue(ice_lag_wq); kfree(lag); diff --git a/drivers/net/ethernet/intel/ice/ice_lib.c b/drivers/net/ethernet/intel/ice/ice_lib.c index 984b381386ff..cdb352e0fa07 100644 --- a/drivers/net/ethernet/intel/ice/ice_lib.c +++ b/drivers/net/ethernet/intel/ice/ice_lib.c @@ -3997,7 +3997,7 @@ bool ice_is_feature_supported(struct ice_pf *pf, enum ice_feature f) * @pf: pointer to the struct ice_pf instance * @f: feature enum to set */ -static void ice_set_feature_support(struct ice_pf *pf, enum ice_feature f) +void ice_set_feature_support(struct ice_pf *pf, enum ice_feature f) { if (f < 0 || f >= ICE_F_MAX) return; diff --git a/drivers/net/ethernet/intel/ice/ice_lib.h b/drivers/net/ethernet/intel/ice/ice_lib.h index 1628385a9672..dd53fe968ad8 100644 --- a/drivers/net/ethernet/intel/ice/ice_lib.h +++ b/drivers/net/ethernet/intel/ice/ice_lib.h @@ -163,6 +163,7 @@ int ice_vsi_del_vlan_zero(struct ice_vsi *vsi); bool ice_vsi_has_non_zero_vlans(struct ice_vsi *vsi); u16 ice_vsi_num_non_zero_vlans(struct ice_vsi *vsi); bool ice_is_feature_supported(struct ice_pf *pf, enum ice_feature f); +void ice_set_feature_support(struct ice_pf *pf, enum ice_feature f); void ice_clear_feature_support(struct ice_pf *pf, enum ice_feature f); void ice_init_feature_support(struct ice_pf *pf); bool ice_vsi_is_rx_queue_active(struct ice_vsi *vsi); diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c index f358018337af..e01834d0417e 100644 --- a/drivers/net/ethernet/intel/ice/ice_main.c +++ b/drivers/net/ethernet/intel/ice/ice_main.c @@ -64,6 +64,7 @@ struct device *ice_hw_to_dev(struct ice_hw *hw) } static struct workqueue_struct *ice_wq; +struct workqueue_struct *ice_lag_wq; static const struct net_device_ops ice_netdev_safe_mode_ops; static const struct net_device_ops ice_netdev_ops; @@ -3795,6 +3796,7 @@ u16 ice_get_avail_rxq_count(struct ice_pf *pf) static void ice_deinit_pf(struct ice_pf *pf) { ice_service_task_stop(pf); + mutex_destroy(&pf->lag_mutex); mutex_destroy(&pf->adev_mutex); mutex_destroy(&pf->sw_mutex); mutex_destroy(&pf->tc_mutex); @@ -3875,6 +3877,7 @@ static int ice_init_pf(struct ice_pf *pf) mutex_init(&pf->sw_mutex); mutex_init(&pf->tc_mutex); mutex_init(&pf->adev_mutex); + mutex_init(&pf->lag_mutex); INIT_HLIST_HEAD(&pf->aq_wait_list); spin_lock_init(&pf->aq_wait_lock); @@ -5576,10 +5579,18 @@ static int __init ice_module_init(void) return -ENOMEM; } + ice_lag_wq = alloc_ordered_workqueue("ice_lag_wq", 0); + if (!ice_lag_wq) { + pr_err("Failed to create LAG workqueue\n"); + destroy_workqueue(ice_wq); + return -ENOMEM; + } + status = pci_register_driver(&ice_driver); if (status) { pr_err("failed to register PCI driver, err %d\n", status); destroy_workqueue(ice_wq); + destroy_workqueue(ice_lag_wq); } return status; @@ -5596,6 +5607,7 @@ static void __exit ice_module_exit(void) { pci_unregister_driver(&ice_driver); destroy_workqueue(ice_wq); + destroy_workqueue(ice_lag_wq); pr_info("module unloaded\n"); } module_exit(ice_module_exit); diff --git a/drivers/net/ethernet/intel/ice/ice_type.h b/drivers/net/ethernet/intel/ice/ice_type.h index a073616671ef..5e353b0cbe6f 100644 --- a/drivers/net/ethernet/intel/ice/ice_type.h +++ b/drivers/net/ethernet/intel/ice/ice_type.h @@ -277,6 +277,8 @@ struct ice_hw_common_caps { u8 dcb; u8 ieee_1588; u8 rdma; + u8 roce_lag; + u8 sriov_lag; bool nvm_update_pending_nvm; bool nvm_update_pending_orom;