Re: [PATCH v7 net-next 7/7] net: ena: Implement gettimexattrs64 callback for PTP attributes

[netdev-bot+sashiko@kernel.org]

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

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

- [Medium] ena_phc_gettimexattrs64()
  (drivers/net/ethernet/amazon/ena/ena_phc.c:57-60) unconditionally…
- [Low] ena_phc_gettimexattrs64()
  (drivers/net/ethernet/amazon/ena/ena_phc.c:28-62) is a verbatim…

--

net: ena: Implement gettimexattrs64 callback for PTP attributes

Adds ena_phc_gettimexattrs64() to the ENA PHC code and registers it in
ena_ptp_clock_info so the PTP_SYS_OFFSET_EXTENDED_ATTRS ioctl is supported.
The new callback reports the device error_bound value and sets the
corresponding valid bit; other attributes are left unreported.

A couple of questions below.

> diff --git a/drivers/net/ethernet/amazon/ena/ena_phc.c b/drivers/net/ethernet/amazon/ena/ena_phc.c
> index 2bcb5af564e2..725c36fe3f6e 100644
> --- a/drivers/net/ethernet/amazon/ena/ena_phc.c
> +++ b/drivers/net/ethernet/amazon/ena/ena_phc.c
> @@ -25,6 +25,43 @@ static int ena_phc_feature_enable(struct ptp_clock_info *clock_info,
>  	return -EOPNOTSUPP;
>  }
>  
> +static int ena_phc_gettimexattrs64(struct ptp_clock_info *clock_info,
> +				   struct timespec64 *ts,
> +				   struct ptp_system_timestamp *sts,
> +				   struct ptp_clock_attrs *att)
> +{
> +	struct ena_phc_info *phc_info =
> +		container_of(clock_info, struct ena_phc_info, clock_info);
> +	u32 error_bound_nsec;
> +	unsigned long flags;
> +	u64 timestamp_nsec;
> +	int rc;
> +
> +	spin_lock_irqsave(&phc_info->lock, flags);
> +
> +	ptp_read_system_prets(sts);
> +
> +	rc = ena_com_phc_get_timestamp(phc_info->adapter->ena_dev,
> +				       &timestamp_nsec,
> +				       &error_bound_nsec);
> +
> +	ptp_read_system_postts(sts);
> +
> +	spin_unlock_irqrestore(&phc_info->lock, flags);
> +
> +	if (rc)
> +		return rc;
> +
> +	*ts = ns_to_timespec64(timestamp_nsec);
> +

[Severity: Low]
This isn't a bug, but the body above is a verbatim copy of
ena_phc_gettimex64(): same container_of(), same
spin_lock_irqsave(&phc_info->lock, flags) / ptp_read_system_prets() /
ena_com_phc_get_timestamp() / ptp_read_system_postts() /
spin_unlock_irqrestore() sequence, same rc handling and same
*ts = ns_to_timespec64(timestamp_nsec).

Since ena_phc_gettimexattrs64() already tolerates att == NULL (and is in
fact called with sts == NULL and att == NULL from ptp_clock_gettime() in
drivers/ptp/ptp_clock.c), could ena_phc_gettimex64() simply become:

	static int ena_phc_gettimex64(struct ptp_clock_info *clock_info,
				      struct timespec64 *ts,
				      struct ptp_system_timestamp *sts)
	{
		return ena_phc_gettimexattrs64(clock_info, ts, sts, NULL);
	}

Passing a non-NULL error_bound to ena_com_phc_get_timestamp() has no side
effect beyond storing the value, since that function guards the store with
if (error_bound). Keeping two copies of the PHC doorbell/poll sequence means
a later fix can land in only one of them.

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

[Severity: Medium]
Is PTP_ATTRS_VALID_ERROR_BOUND being advertised here without a device
capability gate?

The only capability check in the driver is in ena_com_phc_config(), which
accepts:

	/* Supporting only PHC V0 (readless mode with error bound) */
	if (get_feat_resp.u.phc.version != ENA_ADMIN_PHC_FEATURE_VERSION_0) {

ENA_ADMIN_PHC_FEATURE_VERSION_0 is 0, which is the same numeric value that
pre-series firmware reports for the old name ENA_ADMIN_PHC_TYPE_READLESS.
The preceding commit only renames u8 type to u8 version and re-documents 0
as "readless with error_bound"; it also carves u32 error_bound out of the
previously reserved (must-be-zero) bytes of struct ena_admin_phc_resp.

So can a PHC-capable device whose firmware predates error_bound still pass
that version check, leave the DMA bytes at zero, and have
ena_com_phc_get_timestamp() copy that zero out:

	/* PHC timestamp value is returned to the caller */
	*timestamp = resp->timestamp;
	if (error_bound)
		*error_bound = resp->error_bound;

In that case userspace receives error_bound = 0 ns with the validity bit
asserted, i.e. it is told the timestamp is exact, rather than being told the
attribute is unavailable, which is what the valid bitmask exists to express.

The device-side error signal does not seem to help here either:
ENA_ADMIN_PHC_ERROR_FLAG_ERROR_BOUND (checked via ENA_PHC_ERROR_FLAGS) is a
new flag that error_bound-unaware firmware would never set.

Would it make sense to gate the valid bit on a distinct feature version
value or an explicitly advertised device bit? Alternatively, if every
PHC-capable ENA firmware that reports version 0 does populate error_bound
and no older firmware exists in the field, could the commit message state
that?

> +
> +	return 0;
> +}
> +
>  static int ena_phc_gettimex64(struct ptp_clock_info *clock_info,
>  			      struct timespec64 *ts,
>  			      struct ptp_system_timestamp *sts)

[ ... ]

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