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

[Paolo Abeni <pabeni@redhat.com>]

On 4/3/26 9:21 AM, Raju Rangoju wrote:
> +static bool xgbe_kr_training_in_progress(struct xgbe_prv_data *pdata)
> +{
> +	struct xgbe_phy_data *phy_data = pdata->phy_data;
> +	unsigned long kr_start, kr_end;
> +
> +	/* 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 false;
> +
> +	/* If training hasn't completed, ensure it actually started */
> +	kr_start = READ_ONCE(pdata->kr_start_time);
> +	if (!kr_start)
> +		return false;

AFAICS kr_start_time is set to the current jiffies value at
initialization time and 0 is a valid - even if unlikely - jiffies value.
The above test is false-positive prone.

> +
> +	/* Training is complete - no need to wait */
> +	if (READ_ONCE(pdata->an_result) == XGBE_AN_COMPLETE)
> +		return false;
> +
> +	kr_end = kr_start +
> +		msecs_to_jiffies(XGBE_AN_MS_TIMEOUT + XGBE_KRTR_TIME);
> +
> +	/* If we're already past the training window, it's not "in progress" */
> +	if (time_after(jiffies, kr_end))
> +		return false;

Sashiko notes wrap-around (32 bits systems) will lead to wrong return value.

> +
> +	return true;
> +}
> +
> +static void xgbe_wait_for_kr_training_inprogress(struct xgbe_prv_data *pdata)
> +{
> +	unsigned long kr_end;
> +
> +	if (!xgbe_kr_training_in_progress(pdata))
> +		return;
> +
> +	/* Don't block the auto-negotiation state machine work item */
> +	if (current_work() == &pdata->an_work)
> +		return;
> +
> +	kr_end = READ_ONCE(pdata->kr_start_time) +
> +		msecs_to_jiffies(XGBE_AN_MS_TIMEOUT + XGBE_KRTR_TIME);
> +
> +	/* Poll until training completes or the training window expires */
> +	while (time_before(jiffies, kr_end)) {
> +		if (READ_ONCE(pdata->an_result) == XGBE_AN_COMPLETE)
> +			break;
> +
> +		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;
> @@ -2104,6 +2171,13 @@ static void xgbe_phy_perform_ratechange(struct xgbe_prv_data *pdata,
>  	/* Disable PLL re-initialization during FW command processing */
>  	xgbe_phy_pll_ctrl(pdata, false);
>  
> +	/* Serialize firmware mailbox access.
> +	 * Protects entire command sequence including busy check, PLL control,
> +	 * and command execution. Uses explicit lock/unlock for compatibility
> +	 * with goto-based cleanup (per cleanup.h guidelines).
> +	 */
> +	mutex_lock(&pdata->mailbox_lock);

The comment is confusing, due the the `xgbe_phy_pll_ctrl()` invocation
just before acquiring the lock.

> +
>  	/* Log if a previous command did not complete */
>  	if (XP_IOREAD_BITS(pdata, XP_DRIVER_INT_RO, STATUS)) {
>  		netif_dbg(pdata, link, pdata->netdev,
> @@ -2115,7 +2189,7 @@ 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 */
> +	/* Issue the firmware command */
>  	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);
> @@ -2123,11 +2197,13 @@ static void xgbe_phy_perform_ratechange(struct xgbe_prv_data *pdata,
>  	/* Wait for command to complete */
>  	wait = XGBE_RATECHANGE_COUNT;
>  	while (wait--) {
> -		if (!XP_IOREAD_BITS(pdata, XP_DRIVER_INT_RO, STATUS))
> +		if (!XP_IOREAD_BITS(pdata, XP_DRIVER_INT_RO, STATUS)) {
> +			mutex_unlock(&pdata->mailbox_lock);
>  			goto do_rx_adaptation;
> -
> +		}
>  		usleep_range(1000, 2000);
>  	}
> +	mutex_unlock(&pdata->mailbox_lock);
>  
>  	netif_dbg(pdata, link, pdata->netdev,
>  		  "firmware mailbox command did not complete\n");

A `xgbe_phy_rx_reset()` invocation will happen here, outside the lock.
Can that race? will that corrupt the nic status?

/P