| Message ID | 20240702234757.4188344-11-kuba@kernel.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | eth: bnxt: use the new RSS API | expand |
On 7/3/24 01:47, Jakub Kicinski wrote: > Ethtool core stores indirection table with u32 entries, "just to be safe". > Switch the type in the driver, so that it's easier to swap local tables > for the core ones. > > Signed-off-by: Jakub Kicinski <kuba@kernel.org> > --- > drivers/net/ethernet/broadcom/bnxt/bnxt.c | 8 ++++---- > drivers/net/ethernet/broadcom/bnxt/bnxt.h | 4 ++-- > drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c | 6 +++--- > 3 files changed, 9 insertions(+), 9 deletions(-) > > diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c > index 6b8966d3ecb6..4176459921b5 100644 > --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c > +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c > @@ -6225,7 +6225,7 @@ static u16 bnxt_cp_ring_for_tx(struct bnxt *bp, struct bnxt_tx_ring_info *txr) > int bnxt_alloc_rss_indir_tbl(struct bnxt *bp, struct bnxt_rss_ctx *rss_ctx) > { > int entries; > - u16 *tbl; > + u32 *tbl; > > if (bp->flags & BNXT_FLAG_CHIP_P5_PLUS) > entries = BNXT_MAX_RSS_TABLE_ENTRIES_P5; it's a shame that default git context is so small, a few lines below we have tbl alloc: tbl = kmalloc_array(entries, sizeof(*bp->rss_indir_tbl), GFP_KERNEL); > @@ -6248,7 +6248,7 @@ int bnxt_alloc_rss_indir_tbl(struct bnxt *bp, struct bnxt_rss_ctx *rss_ctx) > void bnxt_set_dflt_rss_indir_tbl(struct bnxt *bp, struct bnxt_rss_ctx *rss_ctx) > { > u16 max_rings, max_entries, pad, i; > - u16 *rss_indir_tbl; > + u32 *rss_indir_tbl; > > if (!bp->rx_nr_rings) > return; > @@ -6269,12 +6269,12 @@ void bnxt_set_dflt_rss_indir_tbl(struct bnxt *bp, struct bnxt_rss_ctx *rss_ctx) > > pad = bp->rss_indir_tbl_entries - max_entries; > if (pad) > - memset(&rss_indir_tbl[i], 0, pad * sizeof(u16)); > + memset(&rss_indir_tbl[i], 0, pad * sizeof(u32)); > } without the above allocation line (perhaps you could mention necessary bit of that in commit msg in case of v2) this patch could not be proved correct :) with due diligence done: Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com> OTOH, I assume we need this in-driver table only to keep it over the up→down→up cycle. Could we just keep it as inactive in the core? (And xa_mark() it as inactive to avoid reporting to the user or any other actions that we want to avoid) PS. patch2 is duplicated for me
On Wed, 3 Jul 2024 12:51:58 +0200 Przemek Kitszel wrote: > OTOH, I assume we need this in-driver table only to keep it over the > up→down→up cycle. Could we just keep it as inactive in the core? > (And xa_mark() it as inactive to avoid reporting to the user or any > other actions that we want to avoid) Do you mean keep the table for the default context / context 0 in the core as well?
On 7/3/24 15:49, Jakub Kicinski wrote: > On Wed, 3 Jul 2024 12:51:58 +0200 Przemek Kitszel wrote: >> OTOH, I assume we need this in-driver table only to keep it over the >> up→down→up cycle. Could we just keep it as inactive in the core? >> (And xa_mark() it as inactive to avoid reporting to the user or any >> other actions that we want to avoid) > > Do you mean keep the table for the default context / context 0 > in the core as well? > it was not obvious for me that this is only about the default one, but that would yield more uniformity and cleanups I guess
On Wed, 3 Jul 2024 16:02:38 +0200 Przemek Kitszel wrote: > On 7/3/24 15:49, Jakub Kicinski wrote: > > On Wed, 3 Jul 2024 12:51:58 +0200 Przemek Kitszel wrote: > >> OTOH, I assume we need this in-driver table only to keep it over the > >> up→down→up cycle. Could we just keep it as inactive in the core? > >> (And xa_mark() it as inactive to avoid reporting to the user or any > >> other actions that we want to avoid) > > > > Do you mean keep the table for the default context / context 0 > > in the core as well? > > it was not obvious for me that this is only about the default one, > but that would yield more uniformity and cleanups I guess I'll add a note to the commit message. Initially I was hoping to add context 0, too, but there's a number of gaps. We will hopefully converge to that over time.
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c index 6b8966d3ecb6..4176459921b5 100644 --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c @@ -6225,7 +6225,7 @@ static u16 bnxt_cp_ring_for_tx(struct bnxt *bp, struct bnxt_tx_ring_info *txr) int bnxt_alloc_rss_indir_tbl(struct bnxt *bp, struct bnxt_rss_ctx *rss_ctx) { int entries; - u16 *tbl; + u32 *tbl; if (bp->flags & BNXT_FLAG_CHIP_P5_PLUS) entries = BNXT_MAX_RSS_TABLE_ENTRIES_P5; @@ -6248,7 +6248,7 @@ int bnxt_alloc_rss_indir_tbl(struct bnxt *bp, struct bnxt_rss_ctx *rss_ctx) void bnxt_set_dflt_rss_indir_tbl(struct bnxt *bp, struct bnxt_rss_ctx *rss_ctx) { u16 max_rings, max_entries, pad, i; - u16 *rss_indir_tbl; + u32 *rss_indir_tbl; if (!bp->rx_nr_rings) return; @@ -6269,12 +6269,12 @@ void bnxt_set_dflt_rss_indir_tbl(struct bnxt *bp, struct bnxt_rss_ctx *rss_ctx) pad = bp->rss_indir_tbl_entries - max_entries; if (pad) - memset(&rss_indir_tbl[i], 0, pad * sizeof(u16)); + memset(&rss_indir_tbl[i], 0, pad * sizeof(u32)); } static u16 bnxt_get_max_rss_ring(struct bnxt *bp) { - u16 i, tbl_size, max_ring = 0; + u32 i, tbl_size, max_ring = 0; if (!bp->rss_indir_tbl) return 0; diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.h b/drivers/net/ethernet/broadcom/bnxt/bnxt.h index be40e0513777..1a33824a32a8 100644 --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.h +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.h @@ -1292,7 +1292,7 @@ struct bnxt_vnic_info { struct bnxt_rss_ctx { struct bnxt_vnic_info vnic; - u16 *rss_indir_tbl; + u32 *rss_indir_tbl; u8 index; }; @@ -2331,7 +2331,7 @@ struct bnxt { struct bnxt_vnic_info *vnic_info; u32 num_rss_ctx; int nr_vnics; - u16 *rss_indir_tbl; + u32 *rss_indir_tbl; u16 rss_indir_tbl_entries; u32 rss_hash_cfg; u32 rss_hash_delta; diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c index 74765583405b..e5f687d4a455 100644 --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c @@ -1804,7 +1804,7 @@ static int bnxt_get_rxfh(struct net_device *dev, u32 rss_context = rxfh->rss_context; struct bnxt_rss_ctx *rss_ctx = NULL; struct bnxt *bp = netdev_priv(dev); - u16 *indir_tbl = bp->rss_indir_tbl; + u32 *indir_tbl = bp->rss_indir_tbl; struct bnxt_vnic_info *vnic; u32 i, tbl_size; @@ -1848,7 +1848,7 @@ static void bnxt_modify_rss(struct bnxt *bp, struct bnxt_rss_ctx *rss_ctx, } if (rxfh->indir) { u32 i, pad, tbl_size = bnxt_get_rxfh_indir_size(bp->dev); - u16 *indir_tbl = bp->rss_indir_tbl; + u32 *indir_tbl = bp->rss_indir_tbl; if (rss_ctx) indir_tbl = rss_ctx->rss_indir_tbl; @@ -1856,7 +1856,7 @@ static void bnxt_modify_rss(struct bnxt *bp, struct bnxt_rss_ctx *rss_ctx, indir_tbl[i] = rxfh->indir[i]; pad = bp->rss_indir_tbl_entries - tbl_size; if (pad) - memset(&bp->rss_indir_tbl[i], 0, pad * sizeof(u16)); + memset(&bp->rss_indir_tbl[i], 0, pad * sizeof(u32)); } }
Ethtool core stores indirection table with u32 entries, "just to be safe". Switch the type in the driver, so that it's easier to swap local tables for the core ones. Signed-off-by: Jakub Kicinski <kuba@kernel.org> --- drivers/net/ethernet/broadcom/bnxt/bnxt.c | 8 ++++---- drivers/net/ethernet/broadcom/bnxt/bnxt.h | 4 ++-- drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c | 6 +++--- 3 files changed, 9 insertions(+), 9 deletions(-)