Re: [PATCH net-next v5 08/14] bnxt: use snapshot in bnxt_cfg_rx_mode (74e346419df6)

[sdf.kernel@gmail.com]

> This is an AI-generated review of your patch. The human sending this
> email has considered the AI review valid, or at least plausible.
> ---
> bnxt: use snapshot in bnxt_cfg_rx_mode
> 
> With the introduction of ndo_set_rx_mode_async 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.
> 
> Link: https://lore.kernel.org/netdev/CACKFLi=5vj8hPqEUKDd8RTw3au5G+zRgQEqjF+6NZnyoNm90KA@mail.gmail.com/
> 
> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> [ ... ]
> > -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)
> 
> This isn't a bug, but is the mc parameter used in this function? The
> multicast configuration appears to be handled synchronously prior to this call
> by bnxt_mc_list_updated().

True, mc is fully consumed in bnxt_set_rx_mode (bnxt_mc_list_updated), will
drop.

> 
> >  {
> >  	struct net_device *dev = bp->dev;
> >  	struct bnxt_vnic_info *vnic = &bp->vnic_info[BNXT_VNIC_DEFAULT];
> >  	struct netdev_hw_addr *ha;
> >  	int i, off = 0, rc;
> > -	bool uc_update;
> > -
> > -	netif_addr_lock_bh(dev);
> > -	uc_update = bnxt_uc_list_updated(bp, &dev->uc);
> > -	netif_addr_unlock_bh(dev);
> > -
> > -	if (!uc_update)
> > -		goto skip_uc;
> 
> By removing this check, will the driver now unconditionally delete and recreate
> all secondary hardware unicast MAC filters via HWRM commands on any RX mode
> change?
>
> For example, if an unprivileged user joins a multicast group via
> IP_ADD_MEMBERSHIP, does this create a time window where the hardware drops
> valid unicast traffic destined for secondary MACs?
>
> Also, if any of the firmware commands to re-add a filter fail, will the
> function return early and skip programming the RX mask entirely?

I don't think this is problematic, but I will the original uc_update and
bring back skip_uc, that will skip unnecessary uc list transactions.

> 
> [ ... ]
> > @@ -14684,6 +14675,12 @@ static void bnxt_sp_task(struct work_struct *work)
> >  	/* These functions below will clear BNXT_STATE_IN_SP_TASK.  They
> >  	 * must be the last functions to be called before exiting.
> >  	 */
> > +	if (test_and_clear_bit(BNXT_RX_MASK_SP_EVENT, &bp->sp_event)) {
> > +		bnxt_lock_sp(bp);
> 
> Could this introduce a race condition with bnxt_close()?
>
> bnxt_lock_sp() temporarily clears the BNXT_STATE_IN_SP_TASK bit before
> blocking on netdev_lock(bp->dev). If bnxt_close() is called concurrently, it
> holds netdev_lock() and loops waiting for BNXT_STATE_IN_SP_TASK to clear.
>
> When bnxt_lock_sp() clears the bit, bnxt_close() could unblock, shut down the
> device, and free memory including bp->vnic_info via bnxt_free_vnics().
>
> After bnxt_close() completes and releases the lock, bnxt_sp_task() would
> acquire it and unconditionally call bnxt_cfg_rx_mode(), which dereferences the
> now-freed bp->vnic_info.
>
> Would it be safer to check test_bit(BNXT_STATE_OPEN, &bp->state) after
> acquiring the lock, similar to what neighboring handlers do?

Will do, this seems similar to handling in bnxt_reset.