Re: [PATCH v7 06/23] firmware: arm_scmi: Add basic Telemetry support

[Cristian Marussi <cristian.marussi@arm.com>]

On Mon, Aug 03, 2026 at 11:53:11PM +0100, Fayssal Benmlih wrote:
> Hi Cristian,
> 

Hi,

> I found a few error-path consistency issues inline.
> 
> > 	tde->de.tstamp_support = !!tde->ts_type;
> > 	/* Count timestamped DEs */
> > 	ti->num_des_tstamp += !!tde->de.tstamp_support;
> > 	tde->de.fc_support = IS_FC_SUPPORTED(desc);
> > 	tde->de.name_support = IS_NAME_SUPPORTED(desc);
> > [...]
> > 	if (rx_len < payld_sz)
> > 		return -ENOSPC;
> 
> num_des_tstamp is incremented before all variable parts of the descriptor
> and the optional fast-channel mapping have been validated.
> 
> If a later validation or ioremap operation fails, the descriptor is
> rejected but the timestamp count is not rolled back. GET_ALL state checks
> can then compare against an inflated timestamp count.
> 
> Please update this counter only after the complete descriptor has been
> validated and successfully registered, or undo it on every later failure.

Fixed in V8.

> 
> > err:
> > 	/* DE not enumerated at this point were created in this call */
> > 	if (discovered)
> > 		scmi_telemetry_free_tde_put(ti, tde);
> >
> > 	return ret;
> 
> At this point descriptor parsing may already have changed fields in tde and
> its associated scmi_telemetry_de_info. The object is returned to the free
> list without resetting that partial state.
> 
> Can the descriptor be fully reset before it is made available for reuse,
> or can parsing be done into temporary state that is committed only after
> all validation succeeds?

Fixed in V8 by clearing the tde descriptor on put.

> 
> > 	for (int i = 0; i < ti->info.base.num_groups; i++) {
> > 		struct scmi_telemetry_group *grp = &rinfo->grps[i];
> > [...]
> > 		grp->des = no_free_ptr(des);
> > 		grp->des_str = no_free_ptr(des_str);
> > 		/* Reset group DE counter */
> > 		grp->info->num_des = 0;
> > 	}
> > [...]
> > 	rinfo->num_groups = ti->info.base.num_groups;
> 
> The per-group allocations are transferred out of automatic cleanup during
> the loop, but rinfo->num_groups is only assigned after every group
> succeeds.

Fixed in v8.

> 
> If allocation for a later group fails, resources_free() calls
> scmi_telemetry_groups_free(), which iterates rinfo->num_groups. Since that
> is still zero, allocations already installed in earlier groups are leaked.
> 
> Please increment a cleanup-visible group count as each group is committed,
> or make cleanup iterate base.num_groups and safely free NULL members.
> 
Added also a rollback logic in V8 (which needs a small further fix in V9
apparently listening to Sashiuko complains....)

Thanks,
Cristian