[PATCH net-next] tg3: extract GRXRINGS from .get_rxnfc

[PATCH net-next] tg3: extract GRXRINGS from .get_rxnfc

[Breno Leitao]

Commit 84eaf4359c36 ("net: ethtool: add get_rx_ring_count callback to
optimize RX ring queries") added specific support for GRXRINGS callback,
simplifying .get_rxnfc.

Remove the handling of GRXRINGS in .get_rxnfc() by moving it to the new
.get_rx_ring_count().

Given that tg3_get_rxnfc() only handles ETHTOOL_GRXRINGS, then this
function becomes useless now, and it is removed.

This also fixes the behavior for devices without MSIX support.
Previously, the function would return -EOPNOTSUPP, but now it correctly
returns 1.

The functionality remains the same: return the current queue count
if the device is running, otherwise return the minimum of online
CPUs and TG3_RSS_MAX_NUM_QS.

Signed-off-by: Breno Leitao <leitao@debian.org>
---
PS: This was compiled-tested only and NOT tested on a real hardware.
---
 drivers/net/ethernet/broadcom/tg3.c | 24 ++++++------------------
 1 file changed, 6 insertions(+), 18 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/tg3.c b/drivers/net/ethernet/broadcom/tg3.c
index d78cafdb20949..fa58c3ffceb06 100644
--- a/drivers/net/ethernet/broadcom/tg3.c
+++ b/drivers/net/ethernet/broadcom/tg3.c
@@ -12719,29 +12719,17 @@ static int tg3_get_sset_count(struct net_device *dev, int sset)
 	}
 }
 
-static int tg3_get_rxnfc(struct net_device *dev, struct ethtool_rxnfc *info,
-			 u32 *rules __always_unused)
+static u32 tg3_get_rx_ring_count(struct net_device *dev)
 {
 	struct tg3 *tp = netdev_priv(dev);
 
 	if (!tg3_flag(tp, SUPPORT_MSIX))
-		return -EOPNOTSUPP;
+		return 1;
 
-	switch (info->cmd) {
-	case ETHTOOL_GRXRINGS:
-		if (netif_running(tp->dev))
-			info->data = tp->rxq_cnt;
-		else {
-			info->data = num_online_cpus();
-			if (info->data > TG3_RSS_MAX_NUM_QS)
-				info->data = TG3_RSS_MAX_NUM_QS;
-		}
+	if (netif_running(tp->dev))
+		return tp->rxq_cnt;
 
-		return 0;
-
-	default:
-		return -EOPNOTSUPP;
-	}
+	return min(num_online_cpus(), TG3_RSS_MAX_NUM_QS);
 }
 
 static u32 tg3_get_rxfh_indir_size(struct net_device *dev)
@@ -14268,7 +14256,7 @@ static const struct ethtool_ops tg3_ethtool_ops = {
 	.get_coalesce		= tg3_get_coalesce,
 	.set_coalesce		= tg3_set_coalesce,
 	.get_sset_count		= tg3_get_sset_count,
-	.get_rxnfc		= tg3_get_rxnfc,
+	.get_rx_ring_count	= tg3_get_rx_ring_count,
 	.get_rxfh_indir_size    = tg3_get_rxfh_indir_size,
 	.get_rxfh		= tg3_get_rxfh,
 	.set_rxfh		= tg3_set_rxfh,

---
base-commit: 5d505fdf758ebf197e5e99277e7203b4aabaf367
change-id: 20251105-grxrings_v1-5ed8bc9c1ecd

Best regards,
--  
Breno Leitao <leitao@debian.org>

Re: [PATCH net-next] tg3: extract GRXRINGS from .get_rxnfc

[Michael Chan]

[-- Attachment #1: Type: text/plain, Size: 2745 bytes --]

On Wed, Nov 5, 2025 at 10:02 AM Breno Leitao <leitao@debian.org> wrote:
>
> Commit 84eaf4359c36 ("net: ethtool: add get_rx_ring_count callback to
> optimize RX ring queries") added specific support for GRXRINGS callback,
> simplifying .get_rxnfc.
>
> Remove the handling of GRXRINGS in .get_rxnfc() by moving it to the new
> .get_rx_ring_count().
>
> Given that tg3_get_rxnfc() only handles ETHTOOL_GRXRINGS, then this
> function becomes useless now, and it is removed.
>
> This also fixes the behavior for devices without MSIX support.
> Previously, the function would return -EOPNOTSUPP, but now it correctly
> returns 1.
>
> The functionality remains the same: return the current queue count
> if the device is running, otherwise return the minimum of online
> CPUs and TG3_RSS_MAX_NUM_QS.
>
> Signed-off-by: Breno Leitao <leitao@debian.org>
> ---
> PS: This was compiled-tested only and NOT tested on a real hardware.
> ---
>  drivers/net/ethernet/broadcom/tg3.c | 24 ++++++------------------
>  1 file changed, 6 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/net/ethernet/broadcom/tg3.c b/drivers/net/ethernet/broadcom/tg3.c
> index d78cafdb20949..fa58c3ffceb06 100644
> --- a/drivers/net/ethernet/broadcom/tg3.c
> +++ b/drivers/net/ethernet/broadcom/tg3.c
> @@ -12719,29 +12719,17 @@ static int tg3_get_sset_count(struct net_device *dev, int sset)
>         }
>  }
>
> -static int tg3_get_rxnfc(struct net_device *dev, struct ethtool_rxnfc *info,
> -                        u32 *rules __always_unused)
> +static u32 tg3_get_rx_ring_count(struct net_device *dev)
>  {
>         struct tg3 *tp = netdev_priv(dev);
>
>         if (!tg3_flag(tp, SUPPORT_MSIX))
> -               return -EOPNOTSUPP;
> +               return 1;
>
> -       switch (info->cmd) {
> -       case ETHTOOL_GRXRINGS:
> -               if (netif_running(tp->dev))
> -                       info->data = tp->rxq_cnt;
> -               else {
> -                       info->data = num_online_cpus();
> -                       if (info->data > TG3_RSS_MAX_NUM_QS)
> -                               info->data = TG3_RSS_MAX_NUM_QS;
> -               }
> +       if (netif_running(tp->dev))
> +               return tp->rxq_cnt;
>
> -               return 0;
> -
> -       default:
> -               return -EOPNOTSUPP;
> -       }
> +       return min(num_online_cpus(), TG3_RSS_MAX_NUM_QS);

The existing code to use num_online_cpus() is actually not correct.
This is more correct:

return min(netif_get_num_default_rss_queues(), tp->rxq_max);

I think when netif_get_num_default_rss_queues() was used to replace
num_online_cpus(), tg3_get_rxnfc() was not properly converted.

Thanks.

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 5469 bytes --]

Re: [PATCH net-next] tg3: extract GRXRINGS from .get_rxnfc

[Jakub Kicinski]

On Wed, 5 Nov 2025 11:05:34 -0800 Michael Chan wrote:
> The existing code to use num_online_cpus() is actually not correct.
> This is more correct:
> 
> return min(netif_get_num_default_rss_queues(), tp->rxq_max);
> 
> I think when netif_get_num_default_rss_queues() was used to replace
> num_online_cpus(), tg3_get_rxnfc() was not properly converted.

All true, but perhaps we want to do that change as a follow up?
Someone may show up later insisting that fewer queues cases 
a regression for their workload..

The sensitivity to default queue count was why we didn't change most
of the drivers when netif_get_num_default_rss_queues() got reworked
to a more sane default than 8.

Re: [PATCH net-next] tg3: extract GRXRINGS from .get_rxnfc

[Michael Chan]

[-- Attachment #1: Type: text/plain, Size: 1242 bytes --]

On Wed, Nov 5, 2025 at 5:01 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Wed, 5 Nov 2025 11:05:34 -0800 Michael Chan wrote:
> > The existing code to use num_online_cpus() is actually not correct.
> > This is more correct:
> >
> > return min(netif_get_num_default_rss_queues(), tp->rxq_max);
> >
> > I think when netif_get_num_default_rss_queues() was used to replace
> > num_online_cpus(), tg3_get_rxnfc() was not properly converted.
>
> All true, but perhaps we want to do that change as a follow up?
> Someone may show up later insisting that fewer queues cases
> a regression for their workload..

Sure, we can fix this separately.  This returned value here when
!netif_running() really won't change the ring count when it becomes
netif_running().  tg3_open() will call tg3_enable_msix() which will
set it to netif_get_num_default_rss_queues() regardless of what value
we return here.  IOW, fixing it should not cause any change in the
number of default rings.

Anyway,
Reviewed-by: Michael Chan <michael.chan@broadcom.com>


>
> The sensitivity to default queue count was why we didn't change most
> of the drivers when netif_get_num_default_rss_queues() got reworked
> to a more sane default than 8.

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 5469 bytes --]

Re: [PATCH net-next] tg3: extract GRXRINGS from .get_rxnfc

[Breno Leitao]

On Wed, Nov 05, 2025 at 11:05:34AM -0800, Michael Chan wrote:
> The existing code to use num_online_cpus() is actually not correct.
> This is more correct:
> 
> return min(netif_get_num_default_rss_queues(), tp->rxq_max);
> 
> I think when netif_get_num_default_rss_queues() was used to replace
> num_online_cpus(), tg3_get_rxnfc() was not properly converted.

I can resend the current patch with this additional patch:

Author: Breno Leitao <leitao@debian.org>
Date:   Thu Nov 6 08:05:49 2025 -0800

    tg3: Fix num of RX queues being reported by ethtool
    
    Using num_online_cpus() to report number of queues is actually not
    correct, as reported by Michael[1].
    
    netif_get_num_default_rss_queues() was used to replace num_online_cpus()
    in the past, but tg3 ethtool callbacks didn't get converted. Doing it
    now.
    
    Link: https://lore.kernel.org/all/CACKFLim7ruspmqvjr6bNRq5Z_XXVk3vVaLZOons7kMCzsEG23A@mail.gmail.com/#t [1]
    
    Signed-off-by: Breno Leitao <leitao@debian.org>
    Suggested-by: Michael Chan <michael.chan@broadcom.com>

diff --git a/drivers/net/ethernet/broadcom/tg3.c b/drivers/net/ethernet/broadcom/tg3.c
index fa58c3ffceb06..5fdaee7ef9d7a 100644
--- a/drivers/net/ethernet/broadcom/tg3.c
+++ b/drivers/net/ethernet/broadcom/tg3.c
@@ -12729,7 +12729,7 @@ static u32 tg3_get_rx_ring_count(struct net_device *dev)
 	if (netif_running(tp->dev))
 		return tp->rxq_cnt;
 
-	return min(num_online_cpus(), TG3_RSS_MAX_NUM_QS);
+	return min((u32) netif_get_num_default_rss_queues(), tp->rxq_max);
 }
 
 static u32 tg3_get_rxfh_indir_size(struct net_device *dev)

Re: [PATCH net-next] tg3: extract GRXRINGS from .get_rxnfc

[Michael Chan]

[-- Attachment #1: Type: text/plain, Size: 1231 bytes --]

On Thu, Nov 6, 2025 at 9:06 AM Breno Leitao <leitao@debian.org> wrote:
>     tg3: Fix num of RX queues being reported by ethtool
>
>     Using num_online_cpus() to report number of queues is actually not
>     correct, as reported by Michael[1].
>
>     netif_get_num_default_rss_queues() was used to replace num_online_cpus()
>     in the past, but tg3 ethtool callbacks didn't get converted. Doing it
>     now.
>
>     Link: https://lore.kernel.org/all/CACKFLim7ruspmqvjr6bNRq5Z_XXVk3vVaLZOons7kMCzsEG23A@mail.gmail.com/#t [1]
>
>     Signed-off-by: Breno Leitao <leitao@debian.org>
>     Suggested-by: Michael Chan <michael.chan@broadcom.com>
>
> diff --git a/drivers/net/ethernet/broadcom/tg3.c b/drivers/net/ethernet/broadcom/tg3.c
> index fa58c3ffceb06..5fdaee7ef9d7a 100644
> --- a/drivers/net/ethernet/broadcom/tg3.c
> +++ b/drivers/net/ethernet/broadcom/tg3.c
> @@ -12729,7 +12729,7 @@ static u32 tg3_get_rx_ring_count(struct net_device *dev)
>         if (netif_running(tp->dev))
>                 return tp->rxq_cnt;
>
> -       return min(num_online_cpus(), TG3_RSS_MAX_NUM_QS);
> +       return min((u32) netif_get_num_default_rss_queues(), tp->rxq_max);

Isn't it better to use min_t()?

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 5469 bytes --]

Re: [PATCH net-next] tg3: extract GRXRINGS from .get_rxnfc

[Simon Horman]

On Thu, Nov 06, 2025 at 10:45:21AM -0800, Michael Chan wrote:
> On Thu, Nov 6, 2025 at 9:06 AM Breno Leitao <leitao@debian.org> wrote:
> >     tg3: Fix num of RX queues being reported by ethtool
> >
> >     Using num_online_cpus() to report number of queues is actually not
> >     correct, as reported by Michael[1].
> >
> >     netif_get_num_default_rss_queues() was used to replace num_online_cpus()
> >     in the past, but tg3 ethtool callbacks didn't get converted. Doing it
> >     now.
> >
> >     Link: https://lore.kernel.org/all/CACKFLim7ruspmqvjr6bNRq5Z_XXVk3vVaLZOons7kMCzsEG23A@mail.gmail.com/#t [1]
> >
> >     Signed-off-by: Breno Leitao <leitao@debian.org>
> >     Suggested-by: Michael Chan <michael.chan@broadcom.com>
> >
> > diff --git a/drivers/net/ethernet/broadcom/tg3.c b/drivers/net/ethernet/broadcom/tg3.c
> > index fa58c3ffceb06..5fdaee7ef9d7a 100644
> > --- a/drivers/net/ethernet/broadcom/tg3.c
> > +++ b/drivers/net/ethernet/broadcom/tg3.c
> > @@ -12729,7 +12729,7 @@ static u32 tg3_get_rx_ring_count(struct net_device *dev)
> >         if (netif_running(tp->dev))
> >                 return tp->rxq_cnt;
> >
> > -       return min(num_online_cpus(), TG3_RSS_MAX_NUM_QS);
> > +       return min((u32) netif_get_num_default_rss_queues(), tp->rxq_max);
> 
> Isn't it better to use min_t()?

FWIIW, umin() seems appropriate to me.

Commit 80fcac55385c ("minmax: add umin(a, b) and umax(a, b)")
includes quite a long explanation of why it exists.
And that does seem to match this case.

Re: [PATCH net-next] tg3: extract GRXRINGS from .get_rxnfc

[Breno Leitao]

On Fri, Nov 07, 2025 at 11:04:22AM +0000, Simon Horman wrote:
> On Thu, Nov 06, 2025 at 10:45:21AM -0800, Michael Chan wrote:
> > On Thu, Nov 6, 2025 at 9:06 AM Breno Leitao <leitao@debian.org> wrote:
> > >     tg3: Fix num of RX queues being reported by ethtool
> > >
> > >     Using num_online_cpus() to report number of queues is actually not
> > >     correct, as reported by Michael[1].
> > >
> > >     netif_get_num_default_rss_queues() was used to replace num_online_cpus()
> > >     in the past, but tg3 ethtool callbacks didn't get converted. Doing it
> > >     now.
> > >
> > >     Link: https://lore.kernel.org/all/CACKFLim7ruspmqvjr6bNRq5Z_XXVk3vVaLZOons7kMCzsEG23A@mail.gmail.com/#t [1]
> > >
> > >     Signed-off-by: Breno Leitao <leitao@debian.org>
> > >     Suggested-by: Michael Chan <michael.chan@broadcom.com>
> > >
> > > diff --git a/drivers/net/ethernet/broadcom/tg3.c b/drivers/net/ethernet/broadcom/tg3.c
> > > index fa58c3ffceb06..5fdaee7ef9d7a 100644
> > > --- a/drivers/net/ethernet/broadcom/tg3.c
> > > +++ b/drivers/net/ethernet/broadcom/tg3.c
> > > @@ -12729,7 +12729,7 @@ static u32 tg3_get_rx_ring_count(struct net_device *dev)
> > >         if (netif_running(tp->dev))
> > >                 return tp->rxq_cnt;
> > >
> > > -       return min(num_online_cpus(), TG3_RSS_MAX_NUM_QS);
> > > +       return min((u32) netif_get_num_default_rss_queues(), tp->rxq_max);
> > 
> > Isn't it better to use min_t()?
> 
> FWIIW, umin() seems appropriate to me.
> 
> Commit 80fcac55385c ("minmax: add umin(a, b) and umax(a, b)")
> includes quite a long explanation of why it exists.
> And that does seem to match this case.

I've send the patch using `min_t` in [1] before this reply, and if
I don't hear any concern about replacing min_t by umin(), I will update
that patch with umin().

Link: https://lore.kernel.org/all/20251107-tg3_counts-v1-1-337fe5c8ccb7@debian.org/ [1]

Re: [PATCH net-next] tg3: extract GRXRINGS from .get_rxnfc

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net-next.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Wed, 05 Nov 2025 10:01:12 -0800 you wrote:
> Commit 84eaf4359c36 ("net: ethtool: add get_rx_ring_count callback to
> optimize RX ring queries") added specific support for GRXRINGS callback,
> simplifying .get_rxnfc.
> 
> Remove the handling of GRXRINGS in .get_rxnfc() by moving it to the new
> .get_rx_ring_count().
> 
> [...]

Here is the summary with links:
  - [net-next] tg3: extract GRXRINGS from .get_rxnfc
    https://git.kernel.org/netdev/net-next/c/c04956cccb78

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html