diff mbox series

[iwl-next] ice: alter feature support check for SRIOV and LAG

Message ID 20231207182158.2199799-1-david.m.ertman@intel.com (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series [iwl-next] ice: alter feature support check for SRIOV and LAG | expand

Checks

Context Check Description
netdev/series_format warning Single patches do not need cover letters; Target tree name not specified in the subject
netdev/tree_selection success Guessed tree name to be net-next
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 1115 this patch: 1115
netdev/cc_maintainers warning 4 maintainers not CCed: anthony.l.nguyen@intel.com kuba@kernel.org edumazet@google.com pabeni@redhat.com
netdev/build_clang success Errors and warnings before: 1142 this patch: 1142
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 1142 this patch: 1142
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 8 lines checked
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Ertman, David M Dec. 7, 2023, 6:21 p.m. UTC 
Previously, the ice driver had support for using a hanldler for bonding
netdev events to ensure that conflicting features were not allowed to be
activated at the same time.  While this was still in place, additional
support was added to specifically support SRIOV and LAG together.  These
both utilized the netdev event handler, but the SRIOV and LAG feature was
behind a capabilities feature check to make sure the current NVM has
support.

The exclusion part of the event handler should be removed since there are
users who have custom made solutions that depend on the non-exclusion of
features.

Wrap the creation/registration and cleanup of the event handler and
associated structs in the probe flow with a feature check so that the
only systems that support the full implementation of LAG features will
initialize support.  This will leave other systems unhindered with
functionality as it existed before any LAG code was added.

Reviewed-by: Jesse Brandeburg <jesse.brandeburg@intel.com>
Signed-off-by: Dave Ertman <david.m.ertman@intel.com>
---
 drivers/net/ethernet/intel/ice/ice_lag.c | 2 ++
 1 file changed, 2 insertions(+)

Comments

Tony Nguyen Dec. 8, 2023, 9:18 p.m. UTC | #1 
On 12/7/2023 10:21 AM, Dave Ertman wrote:
> Previously, the ice driver had support for using a hanldler for bonding
> netdev events to ensure that conflicting features were not allowed to be
> activated at the same time.  While this was still in place, additional
> support was added to specifically support SRIOV and LAG together.  These
> both utilized the netdev event handler, but the SRIOV and LAG feature was
> behind a capabilities feature check to make sure the current NVM has
> support.
> 
> The exclusion part of the event handler should be removed since there are
> users who have custom made solutions that depend on the non-exclusion of
> features.
> 
> Wrap the creation/registration and cleanup of the event handler and
> associated structs in the probe flow with a feature check so that the
> only systems that support the full implementation of LAG features will
> initialize support.  This will leave other systems unhindered with
> functionality as it existed before any LAG code was added.

This sounds like a bug fix? Should it be for iwl-net?

> Reviewed-by: Jesse Brandeburg <jesse.brandeburg@intel.com>
> Signed-off-by: Dave Ertman <david.m.ertman@intel.com>
> ---
>   drivers/net/ethernet/intel/ice/ice_lag.c | 2 ++
>   1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/net/ethernet/intel/ice/ice_lag.c b/drivers/net/ethernet/intel/ice/ice_lag.c
> index 280994ee5933..b47cd43ae871 100644
> --- a/drivers/net/ethernet/intel/ice/ice_lag.c
> +++ b/drivers/net/ethernet/intel/ice/ice_lag.c
> @@ -1981,6 +1981,8 @@ int ice_init_lag(struct ice_pf *pf)
>   	int n, err;
>   
>   	ice_lag_init_feature_support_flag(pf);
> +	if (!ice_is_feature_supported(pf, ICE_F_SRIOV_LAG))
> +		return 0;
>   
>   	pf->lag = kzalloc(sizeof(*lag), GFP_KERNEL);
>   	if (!pf->lag)
Ertman, David M Dec. 8, 2023, 10:24 p.m. UTC | #2 
> -----Original Message-----
> From: Nguyen, Anthony L <anthony.l.nguyen@intel.com>
> Sent: Friday, December 8, 2023 1:18 PM
> To: Ertman, David M <david.m.ertman@intel.com>; intel-wired-
> lan@lists.osuosl.org
> Cc: netdev@vger.kernel.org; Brandeburg, Jesse
> <jesse.brandeburg@intel.com>
> Subject: Re: [PATCH iwl-next] ice: alter feature support check for SRIOV and
> LAG
> 
> 
> 
> On 12/7/2023 10:21 AM, Dave Ertman wrote:
> > Previously, the ice driver had support for using a hanldler for bonding
> > netdev events to ensure that conflicting features were not allowed to be
> > activated at the same time.  While this was still in place, additional
> > support was added to specifically support SRIOV and LAG together.  These
> > both utilized the netdev event handler, but the SRIOV and LAG feature was
> > behind a capabilities feature check to make sure the current NVM has
> > support.
> >
> > The exclusion part of the event handler should be removed since there are
> > users who have custom made solutions that depend on the non-exclusion
> of
> > features.
> >
> > Wrap the creation/registration and cleanup of the event handler and
> > associated structs in the probe flow with a feature check so that the
> > only systems that support the full implementation of LAG features will
> > initialize support.  This will leave other systems unhindered with
> > functionality as it existed before any LAG code was added.
> 
> This sounds like a bug fix? Should it be for iwl-net?
>

To my knowledge, this issue has not been reported by any users and was found
through code inspection.  Would you still recommend iwl-net?

DaveE
 
> > Reviewed-by: Jesse Brandeburg <jesse.brandeburg@intel.com>
> > Signed-off-by: Dave Ertman <david.m.ertman@intel.com>
> > ---
> >   drivers/net/ethernet/intel/ice/ice_lag.c | 2 ++
> >   1 file changed, 2 insertions(+)
> >
> > diff --git a/drivers/net/ethernet/intel/ice/ice_lag.c
> b/drivers/net/ethernet/intel/ice/ice_lag.c
> > index 280994ee5933..b47cd43ae871 100644
> > --- a/drivers/net/ethernet/intel/ice/ice_lag.c
> > +++ b/drivers/net/ethernet/intel/ice/ice_lag.c
> > @@ -1981,6 +1981,8 @@ int ice_init_lag(struct ice_pf *pf)
> >   	int n, err;
> >
> >   	ice_lag_init_feature_support_flag(pf);
> > +	if (!ice_is_feature_supported(pf, ICE_F_SRIOV_LAG))
> > +		return 0;
> >
> >   	pf->lag = kzalloc(sizeof(*lag), GFP_KERNEL);
> >   	if (!pf->lag)
Jay Vosburgh Dec. 8, 2023, 11:30 p.m. UTC | #3 
Ertman, David M <david.m.ertman@intel.com> wrote:

>> -----Original Message-----
>> From: Nguyen, Anthony L <anthony.l.nguyen@intel.com>
>> Sent: Friday, December 8, 2023 1:18 PM
>> To: Ertman, David M <david.m.ertman@intel.com>; intel-wired-
>> lan@lists.osuosl.org
>> Cc: netdev@vger.kernel.org; Brandeburg, Jesse
>> <jesse.brandeburg@intel.com>
>> Subject: Re: [PATCH iwl-next] ice: alter feature support check for SRIOV and
>> LAG
>> 
>> 
>> 
>> On 12/7/2023 10:21 AM, Dave Ertman wrote:
>> > Previously, the ice driver had support for using a hanldler for bonding
>> > netdev events to ensure that conflicting features were not allowed to be
>> > activated at the same time.  While this was still in place, additional
>> > support was added to specifically support SRIOV and LAG together.  These
>> > both utilized the netdev event handler, but the SRIOV and LAG feature was
>> > behind a capabilities feature check to make sure the current NVM has
>> > support.
>> >
>> > The exclusion part of the event handler should be removed since there are
>> > users who have custom made solutions that depend on the non-exclusion
>> of
>> > features.
>> >
>> > Wrap the creation/registration and cleanup of the event handler and
>> > associated structs in the probe flow with a feature check so that the
>> > only systems that support the full implementation of LAG features will
>> > initialize support.  This will leave other systems unhindered with
>> > functionality as it existed before any LAG code was added.
>> 
>> This sounds like a bug fix? Should it be for iwl-net?
>>
>
>To my knowledge, this issue has not been reported by any users and was found
>through code inspection.  Would you still recommend iwl-net?

	We have a customer experiencing intermittent issues with
transmit timeouts that go away if we disable the LAG integration as
suggested at [0] (or don't use bonding).  This is on the Ubuntu 5.15
based distro kernel, not upstream, but it does not manifest with the OOT
driver, and seems somehow related to the LAG offloading functionality.

	There was also a post to the list describing similar effects
last month [1], that one seems to be on an Ubuntu 6.2 distro kernel.

	Could these issues be plausibly related to the change in this
patch?

	-J
	
[0]
https://bugs.launchpad.net/ubuntu/+source/linux/+bug/2036239/comments/40
[1]
https://lists.osuosl.org/pipermail/intel-wired-lan/Week-of-Mon-20231120/038096.html



>DaveE
> 
>> > Reviewed-by: Jesse Brandeburg <jesse.brandeburg@intel.com>
>> > Signed-off-by: Dave Ertman <david.m.ertman@intel.com>
>> > ---
>> >   drivers/net/ethernet/intel/ice/ice_lag.c | 2 ++
>> >   1 file changed, 2 insertions(+)
>> >
>> > diff --git a/drivers/net/ethernet/intel/ice/ice_lag.c
>> b/drivers/net/ethernet/intel/ice/ice_lag.c
>> > index 280994ee5933..b47cd43ae871 100644
>> > --- a/drivers/net/ethernet/intel/ice/ice_lag.c
>> > +++ b/drivers/net/ethernet/intel/ice/ice_lag.c
>> > @@ -1981,6 +1981,8 @@ int ice_init_lag(struct ice_pf *pf)
>> >   	int n, err;
>> >
>> >   	ice_lag_init_feature_support_flag(pf);
>> > +	if (!ice_is_feature_supported(pf, ICE_F_SRIOV_LAG))
>> > +		return 0;
>> >
>> >   	pf->lag = kzalloc(sizeof(*lag), GFP_KERNEL);
>> >   	if (!pf->lag)

---
	-Jay Vosburgh, jay.vosburgh@canonical.com
Ertman, David M Dec. 8, 2023, 11:46 p.m. UTC | #4 
> -----Original Message-----
> From: Jay Vosburgh <jay.vosburgh@canonical.com>
> Sent: Friday, December 8, 2023 3:31 PM
> To: Ertman, David M <david.m.ertman@intel.com>
> Cc: Nguyen, Anthony L <anthony.l.nguyen@intel.com>; intel-wired-
> lan@lists.osuosl.org; netdev@vger.kernel.org; Brandeburg, Jesse
> <jesse.brandeburg@intel.com>; Robert Malz <robert.malz@canonical.com>;
> Heitor Alves de Siqueira <heitor.de.siqueira@canonical.com>
> Subject: Re: [PATCH iwl-next] ice: alter feature support check for SRIOV and
> LAG
> 
> Ertman, David M <david.m.ertman@intel.com> wrote:
> 
> >> -----Original Message-----
> >> From: Nguyen, Anthony L <anthony.l.nguyen@intel.com>
> >> Sent: Friday, December 8, 2023 1:18 PM
> >> To: Ertman, David M <david.m.ertman@intel.com>; intel-wired-
> >> lan@lists.osuosl.org
> >> Cc: netdev@vger.kernel.org; Brandeburg, Jesse
> >> <jesse.brandeburg@intel.com>
> >> Subject: Re: [PATCH iwl-next] ice: alter feature support check for SRIOV
> and
> >> LAG
> >>
> >>
> >>
> >> On 12/7/2023 10:21 AM, Dave Ertman wrote:
> >> > Previously, the ice driver had support for using a hanldler for bonding
> >> > netdev events to ensure that conflicting features were not allowed to
> be
> >> > activated at the same time.  While this was still in place, additional
> >> > support was added to specifically support SRIOV and LAG together.
> These
> >> > both utilized the netdev event handler, but the SRIOV and LAG feature
> was
> >> > behind a capabilities feature check to make sure the current NVM has
> >> > support.
> >> >
> >> > The exclusion part of the event handler should be removed since there
> are
> >> > users who have custom made solutions that depend on the non-
> exclusion
> >> of
> >> > features.
> >> >
> >> > Wrap the creation/registration and cleanup of the event handler and
> >> > associated structs in the probe flow with a feature check so that the
> >> > only systems that support the full implementation of LAG features will
> >> > initialize support.  This will leave other systems unhindered with
> >> > functionality as it existed before any LAG code was added.
> >>
> >> This sounds like a bug fix? Should it be for iwl-net?
> >>
> >
> >To my knowledge, this issue has not been reported by any users and was
> found
> >through code inspection.  Would you still recommend iwl-net?
> 
> 	We have a customer experiencing intermittent issues with
> transmit timeouts that go away if we disable the LAG integration as
> suggested at [0] (or don't use bonding).  This is on the Ubuntu 5.15
> based distro kernel, not upstream, but it does not manifest with the OOT
> driver, and seems somehow related to the LAG offloading functionality.
> 
> 	There was also a post to the list describing similar effects
> last month [1], that one seems to be on an Ubuntu 6.2 distro kernel.
> 
> 	Could these issues be plausibly related to the change in this
> patch?
> 
> 	-J
>

From your description, it is plausibly related to this patch.  Looks like we should also
send this to iwl-net.

Tony, do you need me to do anything to facilitate this?

DaveE
 
> [0]
> https://bugs.launchpad.net/ubuntu/+source/linux/+bug/2036239/comment
> s/40
> [1]
> https://lists.osuosl.org/pipermail/intel-wired-lan/Week-of-Mon-
> 20231120/038096.html
> 
> 
> 
> >DaveE
> >
> >> > Reviewed-by: Jesse Brandeburg <jesse.brandeburg@intel.com>
> >> > Signed-off-by: Dave Ertman <david.m.ertman@intel.com>
> >> > ---
> >> >   drivers/net/ethernet/intel/ice/ice_lag.c | 2 ++
> >> >   1 file changed, 2 insertions(+)
> >> >
> >> > diff --git a/drivers/net/ethernet/intel/ice/ice_lag.c
> >> b/drivers/net/ethernet/intel/ice/ice_lag.c
> >> > index 280994ee5933..b47cd43ae871 100644
> >> > --- a/drivers/net/ethernet/intel/ice/ice_lag.c
> >> > +++ b/drivers/net/ethernet/intel/ice/ice_lag.c
> >> > @@ -1981,6 +1981,8 @@ int ice_init_lag(struct ice_pf *pf)
> >> >   	int n, err;
> >> >
> >> >   	ice_lag_init_feature_support_flag(pf);
> >> > +	if (!ice_is_feature_supported(pf, ICE_F_SRIOV_LAG))
> >> > +		return 0;
> >> >
> >> >   	pf->lag = kzalloc(sizeof(*lag), GFP_KERNEL);
> >> >   	if (!pf->lag)
> 
> ---
> 	-Jay Vosburgh, jay.vosburgh@canonical.com
Tony Nguyen Dec. 11, 2023, 9:12 p.m. UTC | #5 
On 12/8/2023 3:46 PM, Ertman, David M wrote:
> 
>  From your description, it is plausibly related to this patch.  Looks like we should also
> send this to iwl-net.
> 
> Tony, do you need me to do anything to facilitate this?

This applies ok to iwl-net so we're ok in that regards. I do need an 
accompanying Fixes for net though; I believe it's bb52f42acef6 ("ice: 
Add driver support for firmware changes for LAG")?

Thanks,
Tony
Ertman, David M Dec. 11, 2023, 9:15 p.m. UTC | #6 
> -----Original Message-----
> From: Nguyen, Anthony L <anthony.l.nguyen@intel.com>
> Sent: Monday, December 11, 2023 1:12 PM
> To: Ertman, David M <david.m.ertman@intel.com>; Jay Vosburgh
> <jay.vosburgh@canonical.com>
> Cc: intel-wired-lan@lists.osuosl.org; netdev@vger.kernel.org; Brandeburg,
> Jesse <jesse.brandeburg@intel.com>; Robert Malz
> <robert.malz@canonical.com>; Heitor Alves de Siqueira
> <heitor.de.siqueira@canonical.com>
> Subject: Re: [PATCH iwl-next] ice: alter feature support check for SRIOV and
> LAG
> 
> On 12/8/2023 3:46 PM, Ertman, David M wrote:
> >
> >  From your description, it is plausibly related to this patch.  Looks like we
> should also
> > send this to iwl-net.
> >
> > Tony, do you need me to do anything to facilitate this?
> 
> This applies ok to iwl-net so we're ok in that regards. I do need an
> accompanying Fixes for net though; I believe it's bb52f42acef6 ("ice:
> Add driver support for firmware changes for LAG")?
> 
> Thanks,
> Tony

Sending out v2 - thanks for the help Tony!

DaveE
diff mbox series

Patch

diff --git a/drivers/net/ethernet/intel/ice/ice_lag.c b/drivers/net/ethernet/intel/ice/ice_lag.c
index 280994ee5933..b47cd43ae871 100644
--- a/drivers/net/ethernet/intel/ice/ice_lag.c
+++ b/drivers/net/ethernet/intel/ice/ice_lag.c
@@ -1981,6 +1981,8 @@  int ice_init_lag(struct ice_pf *pf)
 	int n, err;
 
 	ice_lag_init_feature_support_flag(pf);
+	if (!ice_is_feature_supported(pf, ICE_F_SRIOV_LAG))
+		return 0;
 
 	pf->lag = kzalloc(sizeof(*lag), GFP_KERNEL);
 	if (!pf->lag)