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

[Simon Horman <horms@kernel.org>]

On Thu, May 14, 2026 at 11:58:20AM +0000, Korba, Przemyslaw wrote:
> 
> 
> 
> > -----Original Message-----
> > From: Simon Horman <horms@kernel.org>
> > Sent: Tuesday, May 12, 2026 11:28 AM
> > To: Korba, Przemyslaw <przemyslaw.korba@intel.com>
> > Cc: 'Simon Horman' <horms@kernel.org>; intel-wired-lan@lists.osuosl.org; netdev@vger.kernel.org; Nguyen, Anthony L
> > <anthony.l.nguyen@intel.com>; Kitszel, Przemyslaw <przemyslaw.kitszel@intel.com>; Loktionov, Aleksandr <aleksandr.loktionov@intel.com>;
> > Kubalewski, Arkadiusz <arkadiusz.kubalewski@intel.com>
> > Subject: Re: [PATCH iwl-net v2] ice: support SBQ posted writes with non-posted support for CGU
> > 
> > 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?
> 
> 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.