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

[Jakub Kicinski <kuba@kernel.org>]

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?
-- 
pw-bot: cr