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

["Jiawen Wu" <jiawenwu@trustnetic.com>]

On Fri, May 22, 2026 3:35 PM, Simon Horman wrote:
> 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.

Once the firmware interaction returns an error, it should not continue trying.
We believe this will keep printing errors. So just clear it.

> 
> 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?

When will the work be pending? Could it be overthinking?

> 
> 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.
>