Re: [PATCH net 07/13] ice: support SBQ posted writes with non-posted support for CGU

Re: [PATCH net 07/13] ice: support SBQ posted writes with non-posted support for CGU

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
ice: support SBQ posted writes with non-posted support for CGU

This change introduces posted SBQ writes to remove the delay-based
"completion" the driver was relying on, adds a pre-SYNC SBQ flush in
ice_ptp_exec_tmr_cmd() to order posted PHY writes before GLTSYN_CMD_SYNC,
and keeps CGU accesses non-posted. E810 always waits for completion since
its firmware answers every write.

> diff --git a/drivers/net/ethernet/intel/ice/ice_common.c b/drivers/net/ethernet/intel/ice/ice_common.c
> index ef1ce106f81b..53974ebaaffa 100644
> --- a/drivers/net/ethernet/intel/ice/ice_common.c
> +++ b/drivers/net/ethernet/intel/ice/ice_common.c

[ ... ]

> diff --git a/drivers/net/ethernet/intel/ice/ice_controlq.c b/drivers/net/ethernet/intel/ice/ice_controlq.c
> index dcb837cadd18..a6008dc77fa4 100644
> --- a/drivers/net/ethernet/intel/ice/ice_controlq.c
> +++ b/drivers/net/ethernet/intel/ice/ice_controlq.c
> @@ -1086,6 +1086,10 @@ ice_sq_send_cmd(struct ice_hw *hw, struct ice_ctl_q_info *cq,
>  	wr32(hw, cq->sq.tail, cq->sq.next_to_use);
>  	ice_flush(hw);
>  
> +	/* If the message is posted, don't wait for completion. */
> +	if (cd && cd->posted)
> +		goto sq_send_command_error;
> +

[Low]
For the posted success path, could the goto target be renamed (or a
dedicated label added) so the normal success exit no longer jumps to a
label named for the error cleanup path?

The success path here relies on status still being 0 at the point of the
goto. Any future change that sets status earlier in ice_sq_send_cmd() (for
tracing, logging, or a refactor of the send path) would silently turn
successful posted writes into errors.

The posted path also skips the ice_debug() writeback traces, the
cd->wb_desc copy, and the timeout classification block, but neither the
goto site nor the new struct ice_sq_cd posted bit mentions this. Would a
short comment on struct ice_sq_cd's posted field, describing what the
posted path skips, help?

>  	/* Wait for the command to complete. If it finishes within the
>  	 * timeout, copy the descriptor back to temp.
>  	 */

[ ... ]

> diff --git a/drivers/net/ethernet/intel/ice/ice_ptp_hw.c b/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
> index 8e5f97835954..c6049097f49d 100644
> --- a/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
> +++ b/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
> @@ -352,6 +352,20 @@ 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;
> +
> +	/* Flush SBQ to ensure posted writes complete before SYNC command.
> +	 * Skip for E810 - FW always sends completions, so writes are synchronous.
> +	 */
> +	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);
> +	}

[Medium]
When ice_sbq_rw_reg() fails here (for example -EBUSY when
hw->reset_ongoing is set, or -ENOSPC / -EIO / -ETIMEDOUT from
ice_sq_send_cmd()), the code logs a warning and then unconditionally
proceeds to issue wr32(GLTSYN_CMD_SYNC, SYNC_EXEC_CMD).

The commit message states:

    Flush the SBQ by reading address 0 from the PHY 0 before issuing SYNC
    command to ensure that writes to all PHYs were completed

If the flush is what "ensures that writes to all PHYs were completed",
does silently continuing on flush failure reopen the race the patch is
trying to close?

ice_ptp_exec_tmr_cmd() returns void, and none of its callers
(ice_ptp_tmr_cmd, ice_start_phy_timer_e82x / eth56g,
ice_sync_phy_timer_e82x / eth56g,
ice_read_phy_and_phc_time_e82x / eth56g) can react to the failure. Should
this path propagate the error, or at least skip the SYNC issue when the
flush fails?

Also, is a non-rate-limited dev_warn appropriate in this hot PTP path?
Repeated failures (for example during a reset storm) could flood the
kernel log.

[Medium]
Does the phy_0-only flush actually order writes issued to other SBQ
destinations?

ice_write_phy_eth56g() now issues posted writes and picks the
destination via ice_ptp_get_dest_dev_e825():

    static enum ice_sbq_dev_id ice_ptp_get_dest_dev_e825(struct ice_hw *hw,
                                                         u8 port)
    {
        ...
        if ((!ice_is_dual(hw) && tgt_phy == 1) ||
            (ice_is_dual(hw) && tgt_phy != curr_phy))
            return ice_sbq_dev_phy_0_peer;
        else
            return ice_sbq_dev_phy_0;
    }

So on single-complex E825-C with ports >= ports_per_phy, and on
dual-complex E825-C for cross-complex writes, posted writes can go to
ice_sbq_dev_phy_0_peer. The flush here reads from a hardcoded
ice_sbq_dev_phy_0:

    struct ice_sbq_msg_input msg = {
        .dest_dev = ice_sbq_dev_phy_0,
        .opcode = ice_sbq_msg_rd,
    };

If the SBQ does not enforce global commit ordering across distinct
neighbor destinations, does a read completing on phy_0 guarantee that a
previously-issued posted write to phy_0_peer has landed on the destination
device before GLTSYN_CMD_SYNC executes?

The commit message says:

    Flush the SBQ by reading address 0 from the PHY 0 before issuing SYNC
    command to ensure that writes to all PHYs were completed

Is that guarantee documented for cross-destination SBQ traffic, or should
the flush also target ice_sbq_dev_phy_0_peer (and any other destination
that received a posted write) on multi-PHY / dual-complex E825-C?

>  
>  	if (!ice_is_primary(hw))
>  		hw = ice_get_primary_hw(pf);

[ ... ]

Re: [PATCH net 07/13] ice: support SBQ posted writes with non-posted support for CGU

[Przemyslaw Korba]

sure, can do that

> >  	/* Wait for the command to complete. If it finishes within the
> >  	 * timeout, copy the descriptor back to temp.
> >  	 */
> 
> [ ... ]
> 
> > diff --git a/drivers/net/ethernet/intel/ice/ice_ptp_hw.c b/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
> > index 8e5f97835954..c6049097f49d 100644
> > --- a/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
> > +++ b/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
> > @@ -352,6 +352,20 @@ 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;
> > +
> > +	/* Flush SBQ to ensure posted writes complete before SYNC command.
> > +	 * Skip for E810 - FW always sends completions, so writes are synchronous.
> > +	 */
> > +	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);
> > +	}
> 
> [Medium]
> When ice_sbq_rw_reg() fails here (for example -EBUSY when
> hw->reset_ongoing is set, or -ENOSPC / -EIO / -ETIMEDOUT from
> ice_sq_send_cmd()), the code logs a warning and then unconditionally
> proceeds to issue wr32(GLTSYN_CMD_SYNC, SYNC_EXEC_CMD).
> 
> The commit message states:
> 
>     Flush the SBQ by reading address 0 from the PHY 0 before issuing SYNC
>     command to ensure that writes to all PHYs were completed
> 
> If the flush is what "ensures that writes to all PHYs were completed",
> does silently continuing on flush failure reopen the race the patch is
> trying to close?
> 
> ice_ptp_exec_tmr_cmd() returns void, and none of its callers
> (ice_ptp_tmr_cmd, ice_start_phy_timer_e82x / eth56g,
> ice_sync_phy_timer_e82x / eth56g,
> ice_read_phy_and_phc_time_e82x / eth56g) can react to the failure. Should
> this path propagate the error, or at least skip the SYNC issue when the
> flush fails?
> 
> Also, is a non-rate-limited dev_warn appropriate in this hot PTP path?
> Repeated failures (for example during a reset storm) could flood the
> kernel log.
> 

yeah, making sure it returns earlier on failure is a good idea.

> [Medium]
> Does the phy_0-only flush actually order writes issued to other SBQ
> destinations?
> 
> ice_write_phy_eth56g() now issues posted writes and picks the
> destination via ice_ptp_get_dest_dev_e825():
> 
>     static enum ice_sbq_dev_id ice_ptp_get_dest_dev_e825(struct ice_hw *hw,
>                                                          u8 port)
>     {
>         ...
>         if ((!ice_is_dual(hw) && tgt_phy == 1) ||
>             (ice_is_dual(hw) && tgt_phy != curr_phy))
>             return ice_sbq_dev_phy_0_peer;
>         else
>             return ice_sbq_dev_phy_0;
>     }
> 
> So on single-complex E825-C with ports >= ports_per_phy, and on
> dual-complex E825-C for cross-complex writes, posted writes can go to
> ice_sbq_dev_phy_0_peer. The flush here reads from a hardcoded
> ice_sbq_dev_phy_0:
> 
>     struct ice_sbq_msg_input msg = {
>         .dest_dev = ice_sbq_dev_phy_0,
>         .opcode = ice_sbq_msg_rd,
>     };
> 
> If the SBQ does not enforce global commit ordering across distinct
> neighbor destinations, does a read completing on phy_0 guarantee that a
> previously-issued posted write to phy_0_peer has landed on the destination
> device before GLTSYN_CMD_SYNC executes?
> 
> The commit message says:
> 
>     Flush the SBQ by reading address 0 from the PHY 0 before issuing SYNC
>     command to ensure that writes to all PHYs were completed
> 
> Is that guarantee documented for cross-destination SBQ traffic, or should
> the flush also target ice_sbq_dev_phy_0_peer (and any other destination
> that received a posted write) on multi-PHY / dual-complex E825-C?
> 

I tested mainly on E830. If someone is using E-825 device it indeed
might be different. I've heard some information that it
causes failure on some setups, but I asked an waiting on some details
regarding what sort of failure it is and what's the failing device.
Will most likely need that information before sending new version.

> >  
> >  	if (!ice_is_primary(hw))
> >  		hw = ice_get_primary_hw(pf);
> 
> [ ... ]