From: 'Simon Horman' <[email protected]>
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?
