On Thu, May 14, 2026 at 11:58:20AM +0000, Korba, Przemyslaw wrote: > > > > > -----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 😊
Sorry, there was supposed to be some extra text along the lines of seeking clarification. Yes, I think it would be good to add a comment or something to the commit message about this.
