Re: [PATCH v7 net-next 5/7] net: ena: Update PHC admin interface for error bound support

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider:

Critical: 0 · High: 1 · Medium: 1 · Low: 0

- [High] The commit message states it "Introduce[s] a feature version
  mechanism to indicate device supports error_bound", but the mechanism…
- [Medium] Version handshake is implemented as an exact-equality test
  against a field this patch documents as the device's *maximum*…

--

net: ena: Update PHC admin interface for error bound support

This extends the ENA admin definitions with an error_bound field in the PHC
response structure, replaces the PHC type enum with a PHC feature version
enum, and adds an error flag for error_bound retrieval failures.  The
handshake in ena_com_phc_config() is updated to request and check the new
version field.

> diff --git a/drivers/net/ethernet/amazon/ena/ena_admin_defs.h b/drivers/net/ethernet/amazon/ena/ena_admin_defs.h
> index 898ecd96b96a..2d132c4bc590 100644
> --- a/drivers/net/ethernet/amazon/ena/ena_admin_defs.h
> +++ b/drivers/net/ethernet/amazon/ena/ena_admin_defs.h
> @@ -128,12 +128,14 @@ enum ena_admin_get_stats_scope {
>  	ENA_ADMIN_ETH_TRAFFIC                       = 1,
>  };
>  
> -enum ena_admin_phc_type {
> -	ENA_ADMIN_PHC_TYPE_READLESS                 = 0,
> +enum ena_admin_phc_feature_version {
> +	/* Readless with error_bound */
> +	ENA_ADMIN_PHC_FEATURE_VERSION_0             = 0,
>  };

[Severity: High]
The commit message says:

  "Introduce a feature version mechanism to indicate device supports
   error_bound"

Can this mechanism actually indicate that?  ENA_ADMIN_PHC_FEATURE_VERSION_0
is 0, which is the same wire value as the removed ENA_ADMIN_PHC_TYPE_READLESS
(also 0), and ena_com_phc_config() still sends 0 in
feat_common.feature_version and still accepts a response value of 0:

	ret = ena_com_get_feature(ena_dev,
				  &get_feat_resp,
				  ENA_ADMIN_PHC_CONFIG,
				  ENA_ADMIN_PHC_FEATURE_VERSION_0);
	...
	if (get_feat_resp.u.phc.version != ENA_ADMIN_PHC_FEATURE_VERSION_0) {

So the bytes exchanged with the device look identical to the pre-patch
exchange, and firmware predating error_bound reports the same value as
firmware implementing it.  I did not find another discriminator either:
struct ena_admin_acq_common_desc carries no feature_version echo, and
enum ena_admin_aq_caps_id has no PHC/error-bound capability bit.

Does that matter for the new field?  error_bound is carved out of the
previously reserved bytes of the device-written DMA buffer
struct ena_admin_phc_resp:

> @@ -1224,7 +1226,10 @@ struct ena_admin_phc_resp {
>  	/* PHC timestamp (nsec) */
>  	u64 timestamp;
>  
> -	u8 reserved2[12];
> +	u8 reserved2[8];
> +
> +	/* Timestamp error limit (nsec) */
> +	u32 error_bound;
>  
>  	/* Bit field of enum ena_admin_phc_error_flags */
>  	u32 error_flags;

If a device that only implements the old readless layout never writes those
bytes, the driver reads the zero left by the coherent allocation (or stale
data) and ENA_ADMIN_PHC_ERROR_FLAG_ERROR_BOUND is never set.

Later in this series, ena_phc_gettimexattrs64() publishes that value to
userspace as authoritative:

	if (att) {
		att->error_bound = error_bound_nsec;
		att->valid |= PTP_ATTRS_VALID_ERROR_BOUND;
	}

which would report a validated 0 ns error bound on hardware that never
reported one.  Would giving the error_bound-capable format a distinct value
(for example VERSION_1) and accepting version >= the requested one keep old
and new firmware distinguishable?  Once value 0 is shipped as meaning
"readless with error_bound", that distinction cannot be recovered in
released kernels.

> @@ -1035,10 +1037,10 @@ struct ena_admin_queue_ext_feature_desc {
>  };
>  
>  struct ena_admin_feature_phc_desc {
> -	/* PHC type as defined in enum ena_admin_get_phc_type,
> -	 * used only for GET command.
> +	/* PHC version as defined in enum ena_admin_phc_feature_version,
> +	 * used only for GET command as max supported PHC version by the device.
>  	 */
> -	u8 type;
> +	u8 version;
>  
>  	/* Reserved - MBZ */
>  	u8 reserved1[3];

[ ... ]

> diff --git a/drivers/net/ethernet/amazon/ena/ena_com.c b/drivers/net/ethernet/amazon/ena/ena_com.c
> index 297fb36ab8c1..40d1fd64bc34 100644
> --- a/drivers/net/ethernet/amazon/ena/ena_com.c
> +++ b/drivers/net/ethernet/amazon/ena/ena_com.c
> @@ -1694,10 +1694,11 @@ int ena_com_phc_config(struct ena_com_dev *ena_dev)
>  		return ret;
>  	}
>  
> -	/* Supporting only readless PHC retrieval */
> -	if (get_feat_resp.u.phc.type != ENA_ADMIN_PHC_TYPE_READLESS) {
> +	/* Supporting only PHC V0 (readless mode with error bound) */
> +	if (get_feat_resp.u.phc.version != ENA_ADMIN_PHC_FEATURE_VERSION_0) {
>  		netdev_err(ena_dev->net_device,
> -			   "Unsupported PHC type, error: %d\n",
> +			   "Unsupported PHC version (0x%X), error: %d\n",
> +			   get_feat_resp.u.phc.version,
>  			   -EOPNOTSUPP);
>  		return -EOPNOTSUPP;
>  	}

[Severity: Medium]
Should this be an ordering test rather than an equality test?  The new
comment on the same byte in struct ena_admin_feature_phc_desc describes it
as a ceiling:

	/* PHC version as defined in enum ena_admin_phc_feature_version,
	 * used only for GET command as max supported PHC version by the device.
	 */
	u8 version;

With that meaning, firmware that advertises a maximum of 1 while still
serving the V0 layout the driver explicitly asked for through
feat_common.feature_version = ENA_ADMIN_PHC_FEATURE_VERSION_0 fails the
"!=" test, ena_com_phc_config() returns -EOPNOTSUPP, and ena_phc_init()
takes the err_ena_com_phc_config path (ena_com_phc_destroy(),
ena_phc_enable(adapter, false), ena_devlink_disable_phc_param()), leaving
PHC/PTP unavailable on hardware that supports the requested interface.

The two comments in ena_admin_defs.h also seem to disagree about what the
device puts in the response.  ena_admin_get_set_feature_common_desc says:

	/* The driver specifies the max feature version it supports and the
	 * device responds with the currently supported feature version. The
	 * field is zero based
	 */
	u8 feature_version;

that is, a value clamped to what the driver requested, for which equality is
fine, while the new phc comment says the field carries the device maximum,
for which equality is wrong.  Which contract applies here?  If the device
clamps, could the new comment be reworded; if it reports its maximum, should
the check become something like

	if (get_feat_resp.u.phc.version < ENA_ADMIN_PHC_FEATURE_VERSION_0)

or an explicit minimum-required-version comparison?

For reference, the existing precedent in ena_com_get_max_queue_ext() uses
equality (get_resp.u.max_queue_ext.version != ENA_FEATURE_MAX_QUEUE_EXT_VER),
but that field's comment is only "/* version */" and never claims to be a
maximum.

Also, "Unsupported PHC version (0x%X)" prints a u8 with %X; would %u be
clearer here, matching the zero based version numbering?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260909073136.12863-1-akiyano%40amazon.com