diff mbox series

[net-next,1/7] bnxt_en: Skip ethtool RSS context configuration in ifdown state

Message ID 20240409215431.41424-2-michael.chan@broadcom.com (mailing list archive)
State Accepted
Commit 17b0dfa1f35bf58c17ae75da4af99e6b2c51ed57
Delegated to: Netdev Maintainers
Headers show
Series bnxt_en: Updates for net-next | expand

Checks

Context Check Description
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Clearly marked for 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: 942 this patch: 942
netdev/build_tools success No tools touched, skip
netdev/cc_maintainers success CCed 4 of 4 maintainers
netdev/build_clang success Errors and warnings before: 953 this patch: 953
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 Fixes tag looks correct
netdev/build_allmodconfig_warn success Errors and warnings before: 954 this patch: 954
netdev/checkpatch warning WARNING: line length of 96 exceeds 80 columns
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

Michael Chan April 9, 2024, 9:54 p.m. UTC 
From: Pavan Chebbi <pavan.chebbi@broadcom.com>

The current implementation requires the ifstate to be up when
configuring the RSS contexts.  It will try to fetch the RX ring
IDs and will crash if it is in ifdown state.  Return error if
!netif_running() to prevent the crash.

An improved implementation is in the works to allow RSS contexts
to be changed while in ifdown state.

Fixes: b3d0083caf9a ("bnxt_en: Support RSS contexts in ethtool .{get|set}_rxfh()")
Reviewed-by: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>
Reviewed-by: Somnath Kotur <somnath.kotur@broadcom.com>
Signed-off-by: Pavan Chebbi <pavan.chebbi@broadcom.com>
Signed-off-by: Michael Chan <michael.chan@broadcom.com>
---
 drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c | 5 +++++
 1 file changed, 5 insertions(+)

Comments

Jacob Keller April 9, 2024, 11:26 p.m. UTC | #1 
On 4/9/2024 2:54 PM, Michael Chan wrote:
> From: Pavan Chebbi <pavan.chebbi@broadcom.com>
> 
> The current implementation requires the ifstate to be up when
> configuring the RSS contexts.  It will try to fetch the RX ring
> IDs and will crash if it is in ifdown state.  Return error if
> !netif_running() to prevent the crash.
> 
> An improved implementation is in the works to allow RSS contexts
> to be changed while in ifdown state.
> 
> Fixes: b3d0083caf9a ("bnxt_en: Support RSS contexts in ethtool .{get|set}_rxfh()")
> Reviewed-by: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>
> Reviewed-by: Somnath Kotur <somnath.kotur@broadcom.com>
> Signed-off-by: Pavan Chebbi <pavan.chebbi@broadcom.com>
> Signed-off-by: Michael Chan <michael.chan@broadcom.com>
> ---

Makes sense, though I think you could send this fix directly to net as
its clearly a bug fix and preventing a crash is good.

Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>

Thanks,
Jake

>  drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
> index 9c49f629d565..68444234b268 100644
> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
> @@ -1876,6 +1876,11 @@ static int bnxt_set_rxfh_context(struct bnxt *bp,
>  		return -EOPNOTSUPP;
>  	}
>  
> +	if (!netif_running(bp->dev)) {
> +		NL_SET_ERR_MSG_MOD(extack, "Unable to set RSS contexts when interface is down");
> +		return -EAGAIN;
> +	}
> +
>  	if (*rss_context != ETH_RXFH_CONTEXT_ALLOC) {
>  		rss_ctx = bnxt_get_rss_ctx_from_index(bp, *rss_context);
>  		if (!rss_ctx) {
Michael Chan April 9, 2024, 11:51 p.m. UTC | #2 
On Tue, Apr 9, 2024 at 4:26 PM Jacob Keller <jacob.e.keller@intel.com> wrote:
>
>
>
> On 4/9/2024 2:54 PM, Michael Chan wrote:
> > From: Pavan Chebbi <pavan.chebbi@broadcom.com>
> >
> > The current implementation requires the ifstate to be up when
> > configuring the RSS contexts.  It will try to fetch the RX ring
> > IDs and will crash if it is in ifdown state.  Return error if
> > !netif_running() to prevent the crash.
> >
> > An improved implementation is in the works to allow RSS contexts
> > to be changed while in ifdown state.
> >
> > Fixes: b3d0083caf9a ("bnxt_en: Support RSS contexts in ethtool .{get|set}_rxfh()")
> > Reviewed-by: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>
> > Reviewed-by: Somnath Kotur <somnath.kotur@broadcom.com>
> > Signed-off-by: Pavan Chebbi <pavan.chebbi@broadcom.com>
> > Signed-off-by: Michael Chan <michael.chan@broadcom.com>
> > ---
>
> Makes sense, though I think you could send this fix directly to net as
> its clearly a bug fix and preventing a crash is good.

This RSS context feature in the driver is new and is only in net-next,
so the patch only applies to net-next.  Thanks.
Jacob Keller April 10, 2024, 7:29 p.m. UTC | #3 
On 4/9/2024 4:51 PM, Michael Chan wrote:
> On Tue, Apr 9, 2024 at 4:26 PM Jacob Keller <jacob.e.keller@intel.com> wrote:
>>
>>
>>
>> On 4/9/2024 2:54 PM, Michael Chan wrote:
>>> From: Pavan Chebbi <pavan.chebbi@broadcom.com>
>>>
>>> The current implementation requires the ifstate to be up when
>>> configuring the RSS contexts.  It will try to fetch the RX ring
>>> IDs and will crash if it is in ifdown state.  Return error if
>>> !netif_running() to prevent the crash.
>>>
>>> An improved implementation is in the works to allow RSS contexts
>>> to be changed while in ifdown state.
>>>
>>> Fixes: b3d0083caf9a ("bnxt_en: Support RSS contexts in ethtool .{get|set}_rxfh()")
>>> Reviewed-by: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>
>>> Reviewed-by: Somnath Kotur <somnath.kotur@broadcom.com>
>>> Signed-off-by: Pavan Chebbi <pavan.chebbi@broadcom.com>
>>> Signed-off-by: Michael Chan <michael.chan@broadcom.com>
>>> ---
>>
>> Makes sense, though I think you could send this fix directly to net as
>> its clearly a bug fix and preventing a crash is good.
> 
> This RSS context feature in the driver is new and is only in net-next,
> so the patch only applies to net-next.  Thanks.

Ah, even better fixing it before it hits a stable.

Thanks,
Jake
diff mbox series

Patch

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
index 9c49f629d565..68444234b268 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
@@ -1876,6 +1876,11 @@  static int bnxt_set_rxfh_context(struct bnxt *bp,
 		return -EOPNOTSUPP;
 	}
 
+	if (!netif_running(bp->dev)) {
+		NL_SET_ERR_MSG_MOD(extack, "Unable to set RSS contexts when interface is down");
+		return -EAGAIN;
+	}
+
 	if (*rss_context != ETH_RXFH_CONTEXT_ALLOC) {
 		rss_ctx = bnxt_get_rss_ctx_from_index(bp, *rss_context);
 		if (!rss_ctx) {