Re: [PATCH net-next v1 3/3] net: txgbe: rework service event handling

[Simon Horman <horms@kernel.org>]

From: 'Simon Horman' <horms@kernel.org>

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://sashiko.dev
---
net: txgbe: rework service event handling

This commit converts the link event subtasks to use test_and_clear_bit()
and changes how module detection retries are handled. It also explicitly
cancels the service task during device teardown.

> diff --git a/drivers/net/ethernet/wangxun/txgbe/txgbe_main.c b/drivers/net/ethernet/wangxun/txgbe/txgbe_main.c
> index 6755464b46371..ce82e13aa8ae0 100644
> --- a/drivers/net/ethernet/wangxun/txgbe/txgbe_main.c
> +++ b/drivers/net/ethernet/wangxun/txgbe/txgbe_main.c
> @@ -93,31 +93,23 @@ static void txgbe_module_detection_subtask(struct wx *wx)
>  {
>  	int err;
>  
> -	if (!test_bit(WX_FLAG_NEED_MODULE_RESET, wx->flags))
> +	if (!test_and_clear_bit(WX_FLAG_NEED_MODULE_RESET, wx->flags))
>  		return;
>  
>  	/* wait for SFF module ready */
>  	msleep(200);
>  
>  	err = txgbe_identify_module(wx);
> -	if (err)
> -		return;
> -
> -	clear_bit(WX_FLAG_NEED_MODULE_RESET, wx->flags);
> +	if (err == -ENODEV)
> +		set_bit(WX_FLAG_NEED_MODULE_RESET, wx->flags);
>  }

Does this change prevent the driver from retrying transient firmware errors?

Since txgbe_identify_module() communicates with firmware via
txgbe_identify_module_hostif(), it can fail with transient errors like
-EIO or -ETIMEDOUT.

Before this patch, the WX_FLAG_NEED_MODULE_RESET flag was only cleared upon
successful completion, ensuring the periodic timer would retry on any error.

Because the flag is now cleared immediately and only re-armed on -ENODEV,
could a single transient timeout cause the driver to permanently abandon
module setup?

>  static void txgbe_link_config_subtask(struct wx *wx)
>  {
> -	int err;
> -
> -	if (!test_bit(WX_FLAG_NEED_LINK_CONFIG, wx->flags))
> +	if (!test_and_clear_bit(WX_FLAG_NEED_LINK_CONFIG, wx->flags))
>  		return;
>  
> -	err = txgbe_set_phy_link(wx);
> -	if (err)
> -		return;
> -
> -	clear_bit(WX_FLAG_NEED_LINK_CONFIG, wx->flags);
> +	txgbe_set_phy_link(wx);
>  }

Similarly, does this silently drop link configuration on failure?

Without a way to re-arm WX_FLAG_NEED_LINK_CONFIG, any transient failure
in txgbe_set_phy_link() will result in the driver no longer retrying link
configuration.

[ ... ]

> @@ -233,6 +225,7 @@ static void txgbe_disable_device(struct wx *wx)
>  	wx_napi_disable_all(wx);
>  
>  	timer_delete_sync(&wx->service_timer);
> +	cancel_work_sync(&wx->service_task);
>  
>  	if (wx->bus.func < 2)
>  		wr32m(wx, TXGBE_MIS_PRB_CTL, TXGBE_MIS_PRB_CTL_LAN_UP(wx->bus.func), 0);

Will canceling the service_task here leave the WX_STATE_SERVICE_SCHED bit
permanently set if the work is pending?

The wx_service_event_schedule() function sets WX_STATE_SERVICE_SCHED before
queuing the task, and this bit is normally cleared exclusively at the end
of the work function.

If cancel_work_sync() dequeues the work without running it, the bit is never
cleared. When the device is later brought back up, attempts to schedule the
service task will fail, which seems like it would permanently break module
detection, link configuration, and stats collection until the driver is
reloaded.