> -----Original Message-----
> From: Simon Horman <[email protected]>
> Sent: Tuesday, May 12, 2026 11:28 AM
> To: Korba, Przemyslaw <[email protected]>
> Cc: 'Simon Horman' <[email protected]>; [email protected];
> [email protected]; Nguyen, Anthony L
> <[email protected]>; Kitszel, Przemyslaw
> <[email protected]>; Loktionov, Aleksandr
> <[email protected]>;
> Kubalewski, Arkadiusz <[email protected]>
> Subject: Re: [PATCH iwl-net v2] ice: support SBQ posted writes with
> non-posted support for CGU
>
> 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?
Hi, thanks for review!
With E810 only opcode 0x01 for writing is supported.
On E810, the FW always sends completion responses for opcode 0x01, so the
driver waits for each write to complete.
On newer E8XX devices, opcode 0x01 is truly posted (no completion response),
which is why the SBQ flush is needed before SYNC_EXEC_CMD.
Since E810 writes are synchronous (driver waits for completion), flushing SBQ
is unnecessary - all writes are already complete when the function returns.
I can add comments explaining that, or make it a bit clearer in the code if
you'd like 😊
