Re: [net] amd-xgbe: synchronize KR training with device operations

[Paolo Abeni]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.

For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-local.html
---
amd-xgbe: synchronize KR training with device operations

This commit adds synchronization to prevent device operations from
interrupting active KR training by introducing a mailbox mutex and waiting
for KR training completion before proceeding with stop/rate change
operations.

> diff --git a/drivers/net/ethernet/amd/xgbe/xgbe-drv.c b/drivers/net/ethernet/amd/xgbe/xgbe-drv.c
> index 23beea48ae26..3913eb7e1da3 100644
> --- a/drivers/net/ethernet/amd/xgbe/xgbe-drv.c
> +++ b/drivers/net/ethernet/amd/xgbe/xgbe-drv.c

[ ... ]

> diff --git a/drivers/net/ethernet/amd/xgbe/xgbe-phy-v2.c b/drivers/net/ethernet/amd/xgbe/xgbe-phy-v2.c
> index b8cf6ccfe641..142eb952a29c 100644
> --- a/drivers/net/ethernet/amd/xgbe/xgbe-phy-v2.c
> +++ b/drivers/net/ethernet/amd/xgbe/xgbe-phy-v2.c

[ ... ]

> @@ -2095,12 +2095,57 @@ static void xgbe_phy_pll_ctrl(struct xgbe_prv_data *pdata, bool enable)
>  	usleep_range(100, 200);
>  }
>
> +static bool xgbe_phy_port_is_inphi(struct xgbe_prv_data *pdata)
> +{
> +	struct xgbe_phy_data *phy_data = pdata->phy_data;
> +
> +	/* Re-driver models 4223 && 4227 are supported Inphi models */
> +	return phy_data->redrv &&
> +	       (phy_data->redrv_model == XGBE_PHY_REDRV_MODEL_4223 ||
> +		phy_data->redrv_model == XGBE_PHY_REDRV_MODEL_4227);
> +}
> +
> +void xgbe_check_kr_training_in_progress(struct xgbe_prv_data *pdata)
> +{
> +	struct xgbe_phy_data *phy_data = pdata->phy_data;
> +	unsigned long kr_timeout;
> +	int wait;
> +
> +	/* Only wait for KR training in specific conditions:
> +	 *  - Inphi re-driver is present, OR
> +	 *  - Currently in KR mode with autoneg enabled
> +	 */
> +	if (!xgbe_phy_port_is_inphi(pdata) &&
> +	    !(phy_data->cur_mode == XGBE_MODE_KR &&
> +	      pdata->phy.autoneg == AUTONEG_ENABLE))
> +		return;
> +
> +	wait = XGBE_KR_TRAINING_WAIT_ITER;
> +	while (wait--) {
> +		/* Check if we've exceeded the AN timeout window */
> +		kr_timeout = pdata->kr_start_time +
> +			msecs_to_jiffies(XGBE_AN_MS_TIMEOUT +
> +					XGBE_KR_TRAINING_WAIT_MS);
> +		if (time_after(jiffies, kr_timeout))
> +			break;
> +
> +		/* Training is complete - no need to wait */
> +		if (pdata->an_result == XGBE_AN_COMPLETE)
> +			return;
> +
> +		usleep_range(10000, 11000);
> +	}
> +}
> +
>  static void xgbe_phy_perform_ratechange(struct xgbe_prv_data *pdata,
> -					enum xgbe_mb_cmd cmd, enum xgbe_mb_subcmd sub_cmd)
> +					enum xgbe_mb_cmd cmd,
> +					enum xgbe_mb_subcmd sub_cmd)
>  {
>  	unsigned int s0 = 0;
>  	unsigned int wait;
>
> +	xgbe_check_kr_training_in_progress(pdata);
> +
>  	/* Disable PLL re-initialization during FW command processing */
>  	xgbe_phy_pll_ctrl(pdata, false);
>
> @@ -2115,7 +2160,9 @@ static void xgbe_phy_perform_ratechange(struct xgbe_prv_data *pdata,
>  	XP_SET_BITS(s0, XP_DRIVER_SCRATCH_0, COMMAND, cmd);
>  	XP_SET_BITS(s0, XP_DRIVER_SCRATCH_0, SUB_COMMAND, sub_cmd);
>
> -	/* Issue the command */
> +	/* Acquire mailbox lock for firmware command */
> +	guard(mutex)(&pdata->mailbox_lock);
> +

Can this cause a recursive deadlock? xgbe_phy_perform_ratechange() acquires
mailbox_lock, but when called with sub_cmd XGBE_MB_SUBCMD_RX_ADAP, it goes
through do_rx_adaptation which calls xgbe_phy_rx_adaptation().

xgbe_phy_rx_adaptation() has two code paths that both lead back to
xgbe_phy_perform_ratechange():

Path 1:
    xgbe_phy_rx_adaptation()
        -> xgbe_phy_rrc()
            -> xgbe_phy_perform_ratechange(pdata, XGBE_MB_CMD_RRC, ...)
                -> guard(mutex)(&pdata->mailbox_lock)  // deadlock

Path 2:
    xgbe_phy_rx_adaptation()
        -> xgbe_rx_adaptation()
            -> xgbe_set_rx_adap_mode()
                -> xgbe_phy_perform_ratechange(...)
                    -> guard(mutex)(&pdata->mailbox_lock)  // deadlock

Since kernel mutexes are not recursive, attempting to lock an already-held
mutex from the same thread causes a permanent deadlock.

>  	XP_IOWRITE(pdata, XP_DRIVER_SCRATCH_0, s0);
>  	XP_IOWRITE(pdata, XP_DRIVER_SCRATCH_1, 0);
>  	XP_IOWRITE_BITS(pdata, XP_DRIVER_INT_REQ, REQUEST, 1);

[ ... ]