> -----Original Message-----
> From: Simon Horman <[email protected]>
> Sent: Friday, May 15, 2026 6:22 PM
> To: Korba, Przemyslaw <[email protected]>
> Cc: [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
>
> 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.
Sure, can do
