On 03/23, Michael Chan wrote: > On Thu, Mar 19, 2026 at 6:25 PM Stanislav Fomichev <[email protected]> wrote: > > > > With the introduction of ndo_set_rx_mode_async (as discussed in [0]) > > we can call bnxt_cfg_rx_mode directly. Convert bnxt_cfg_rx_mode to > > use uc/mc snapshots and move its call in bnxt_sp_task to the > > section that resets BNXT_STATE_IN_SP_TASK. Switch to direct call in > > bnxt_set_rx_mode. > > > > 0: > > https://lore.kernel.org/netdev/CACKFLi=5vj8hpqeukdd8rtw3au5g+zrgqeqjf+6nznyonm9...@mail.gmail.com/ > > > > Cc: Michael Chan <[email protected]> > > Cc: Pavan Chebbi <[email protected]> > > Signed-off-by: Stanislav Fomichev <[email protected]> > > --- > > drivers/net/ethernet/broadcom/bnxt/bnxt.c | 26 ++++++++++++++--------- > > 1 file changed, 16 insertions(+), 10 deletions(-) > > > > diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c > > b/drivers/net/ethernet/broadcom/bnxt/bnxt.c > > index 225217b32e4b..12265bd7fda4 100644 > > --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c > > +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c > > @@ -11039,7 +11039,8 @@ static int bnxt_setup_nitroa0_vnic(struct bnxt *bp) > > return rc; > > } > > > > -static int bnxt_cfg_rx_mode(struct bnxt *); > > +static int bnxt_cfg_rx_mode(struct bnxt *, struct netdev_hw_addr_list *, > > + struct netdev_hw_addr_list *); > > static bool bnxt_mc_list_updated(struct bnxt *, u32 *, > > const struct netdev_hw_addr_list *); > > > > @@ -11135,7 +11136,7 @@ static int bnxt_init_chip(struct bnxt *bp, bool > > irq_re_init) > > vnic->rx_mask |= mask; > > } > > > > - rc = bnxt_cfg_rx_mode(bp); > > + rc = bnxt_cfg_rx_mode(bp, &bp->dev->uc, &bp->dev->mc); > > if (rc) > > goto err_out; > > > > @@ -13610,11 +13611,12 @@ static void bnxt_set_rx_mode(struct net_device > > *dev, > > if (mask != vnic->rx_mask || uc_update || mc_update) { > > vnic->rx_mask = mask; > > > > - bnxt_queue_sp_work(bp, BNXT_RX_MASK_SP_EVENT); > > + bnxt_cfg_rx_mode(bp, uc, mc); > > } > > } > > > > -static int bnxt_cfg_rx_mode(struct bnxt *bp) > > +static int bnxt_cfg_rx_mode(struct bnxt *bp, struct netdev_hw_addr_list > > *uc, > > + struct netdev_hw_addr_list *mc) > > { > > struct net_device *dev = bp->dev; > > struct bnxt_vnic_info *vnic = &bp->vnic_info[BNXT_VNIC_DEFAULT]; > > @@ -13623,7 +13625,7 @@ static int bnxt_cfg_rx_mode(struct bnxt *bp) > > bool uc_update; > > > > netif_addr_lock_bh(dev); > > - uc_update = bnxt_uc_list_updated(bp, &dev->uc); > > + uc_update = bnxt_uc_list_updated(bp, uc); > > Will the uc list snapshot change between bnxt_set_rx_mode() and > bnxt_cfg_rx_mode() with the direct call now? In the original deferred > update implementation, the uc list can change and that's why we check > in both functions.
The snapshot is gonna be the same for bnxt_set_rx_mode->bnxt_cfg_rx_mode path. So you're saying that it's ok to remove the one in bnxt_cfg_rx_mode because it's called either from bnxt_set_rx_mode (with a new list) or, explicitly, via the BNXT_RX_MASK_SP_EVENT retry mechanism (where we know that we need to redo the updates anyway)? This makes me wonder whether I need to push the retrying mechanism to the core stack... Right now, if some of the allocations in wq handler fail, we just give up, maybe I should handle it better. And I can plug the signal from the driver (make ndo_set_rx_mode_async return int) in the same retry mechanism.
