drivers/net/ethernet/broadcom/tg3.c | 24 ++++++------------------ 1 file changed, 6 insertions(+), 18 deletions(-)
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>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.
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)
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()?
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.
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]
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.
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.
© 2016 - 2025 Red Hat, Inc.