RE: [PATCH net-next] net: libwx: rename wx_pf_flags to wx_flags

On Sat, Aug 15, 2026 4:01 AM, Jakub Kicinski wrote:
> This is an AI-generated review of your patch. The human sending this
> email says "Sashiko points out a pre-existing issue. Let's fix it
> first since the rename will conflict with the fix?"
> ---
> net: libwx: rename wx_pf_flags to wx_flags
> 
> This renames enum wx_pf_flags to enum wx_flags and its terminator
> WX_PF_FLAGS_NBITS to WX_FLAGS_NBITS, since the flags are used by both the
> PF and VF drivers.  The three users of the old terminator name in wx_hw.c,
> wx_ptp.c and wx_type.h are updated accordingly.
> 
> > diff --git a/drivers/net/ethernet/wangxun/libwx/wx_ptp.c b/drivers/net/ethernet/wangxun/libwx/wx_ptp.c
> > index 3eea647c4742c..e35e854652ece 100644
> > --- a/drivers/net/ethernet/wangxun/libwx/wx_ptp.c
> > +++ b/drivers/net/ethernet/wangxun/libwx/wx_ptp.c
> > @@ -558,7 +558,7 @@ static int wx_ptp_set_timestamp_mode(struct wx *wx,
> >  {
> >  	u32 tsync_tx_ctl = WX_TSC_1588_CTL_ENABLED;
> >  	u32 tsync_rx_ctl = WX_PSR_1588_CTL_ENABLED;
> > -	DECLARE_BITMAP(flags, WX_PF_FLAGS_NBITS);
> > +	DECLARE_BITMAP(flags, WX_FLAGS_NBITS);
> >  	u32 tsync_rx_mtrl = PTP_EV_PORT << 16;
> >  	bool is_l2 = false;
> >  	u32 regval;
> 
> [Severity: Medium]
> This isn't a bug introduced by this patch, but while this local bitmap is
> being touched by the rename: can the copy-in/copy-out of wx->flags in
> wx_ptp_set_timestamp_mode() drop bits that another CPU sets atomically?
> 
> The function snapshots the whole bitmap, changes only the two
> WX_FLAG_RX_HWTSTAMP_* bits in the local copy, programs several registers,
> and then writes the entire bitmap back:
> 
> drivers/net/ethernet/wangxun/libwx/wx_ptp.c:wx_ptp_set_timestamp_mode() {
> 	DECLARE_BITMAP(flags, WX_FLAGS_NBITS);
> 	...
> 	memcpy(flags, wx->flags, sizeof(wx->flags));
> 	...
> 	set_bit(WX_FLAG_RX_HWTSTAMP_ENABLED, flags);
> 	set_bit(WX_FLAG_RX_HWTSTAMP_IN_REGISTER, flags);
> 	...
> 	WX_WRITE_FLUSH(wx);
> 
> 	/* configure adapter flags only when HW is actually configured */
> 	memcpy(wx->flags, flags, sizeof(wx->flags));
> }
> 
> All WX_FLAG_* bits fit in one unsigned long, and every other user in the
> driver updates individual bits with atomic bitops.  Some of those writers do
> not take rtnl, so they are not serialized against the ndo_hwtstamp_set path:
> 
> drivers/net/ethernet/wangxun/txgbe/txgbe_aml.c:txgbe_gpio_irq_handler_aml()
> 	set_bit(WX_FLAG_NEED_MODULE_RESET, wx->flags);
> 
> drivers/net/ethernet/wangxun/libwx/wx_err.c:wx_tx_timeout_reset()
> 	set_bit(WX_FLAG_NEED_DO_RESET, wx->flags);
> 
> drivers/net/ethernet/wangxun/libwx/wx_err.c:wx_tx_timeout_recovery()
> 	set_bit(WX_FLAG_NEED_PCIE_RECOVERY, wx->flags);
> 
> So a request set between the two memcpy calls would be overwritten by the
> write-back, and the service task would then find nothing to do:
> 
> drivers/net/ethernet/wangxun/txgbe/txgbe_main.c:txgbe_service_event_sfp() {
> 	if (!test_and_clear_bit(WX_FLAG_NEED_MODULE_RESET, wx->flags))
> 		return;
> 	...
> }
> 
> The mirror case is a bit another CPU has just cleared being restored by the
> write-back, for example the clear in wx_err.c:
> 
> 	if (!test_and_clear_bit(WX_FLAG_NEED_DO_RESET, wx->flags))
> 
> which would then look like a fresh reset request.
> 
> Would applying the two bit changes directly to wx->flags with set_bit() and
> clear_bit() after the registers are programmed, instead of copying the whole
> bitmap in and out, work here?

Thanks, I'll send a separate patch to fix it first.
Local boolean variables and assign_bit() will be used here.