Re: [Intel-wired-lan] [PATCH iwl-net v2] ice: support SBQ posted writes with non-posted support for CGU

[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
---
ice: support SBQ posted writes with non-posted support for CGU

This commit introduces non-posted SBQ writes while allowing posted writes
to skip completion waits. It also adds an SBQ flush by reading address 0
from PHY 0 before issuing a SYNC command to ensure writes are completed.

> diff --git a/drivers/net/ethernet/intel/ice/ice_ptp_hw.c b/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
> index 24fb7a3e14d63..ec4c85ad12e4d 100644
> --- a/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
> +++ b/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
> @@ -352,6 +352,17 @@ void ice_ptp_src_cmd(struct ice_hw *hw, enum ice_ptp_tmr_cmd cmd)
>  static void ice_ptp_exec_tmr_cmd(struct ice_hw *hw)
>  {
>  	struct ice_pf *pf = container_of(hw, struct ice_pf, hw);
> +	struct ice_sbq_msg_input msg = {
> +		.dest_dev = ice_sbq_dev_phy_0,
> +		.opcode = ice_sbq_msg_rd,
> +	};
> +	int err;
> +
> +	if (hw->mac_type != ICE_MAC_E810) {
> +		err = ice_sbq_rw_reg(hw, &msg, LIBIE_AQ_FLAG_RD);
> +		if (err)
> +			dev_warn(ice_hw_to_dev(hw), "Failed to flush SBQ: %d\n", err);
> +	}

[ ... ]

> @@ -4469,15 +4482,15 @@ static int ice_read_phy_reg_e810(struct ice_hw *hw, u32 addr, u32 *val)
>   */
>  static int ice_write_phy_reg_e810(struct ice_hw *hw, u32 addr, u32 val)
>  {
> -	struct ice_sbq_msg_input msg = {0};
> +	struct ice_sbq_msg_input msg = {
> +		.dest_dev = ice_sbq_dev_phy_0,
> +		.opcode = ice_sbq_msg_wr_p,
> +		.msg_addr_low = lower_16_bits(addr),
> +		.msg_addr_high = upper_16_bits(addr),
> +		.data = val
> +	};

Since ice_write_phy_reg_e810() is updated to use the posted write opcode
(ice_sbq_msg_wr_p), it will skip the completion wait and return immediately.

If the SBQ flush in ice_ptp_exec_tmr_cmd() explicitly skips E810 devices
via the check for hw->mac_type != ICE_MAC_E810, does this leave E810 PHY
writes entirely asynchronous and unguarded?

Could this lead to a race condition where the writes have not actually
completed before the SYNC_EXEC_CMD is executed?