Re: [PATCH v7 08/23] firmware: arm_scmi: Add Telemetry configuration operations

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

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

Hi,

> I found two configuration issues inline.
> 
> > 	if (!is_group) {
> > 		active_update_interval =
> > 			&ti->info.active_update_interval;
> > 		current_mode = &ti->info.current_mode;
> > 	} else {
> > 		struct scmi_telemetry_res_info *rinfo;
> >
> > 		rinfo = ti->res_get(ti);
> > 		active_update_interval =
> > 			&rinfo->grps[res_id].active_update_interval;
> > 		current_mode = &rinfo->grps[res_id].current_mode;
> > 	}
> > [...]
> > 	if (!ret) {
> > 		ti->info.enabled = tlm_enable;
> > 		*current_mode = next_mode;
> > 		ti->info.notif_enabled =
> > 			*current_mode == SCMI_TLM_NOTIFICATION;
> > 		if (update_interval_ms)
> > 			*active_update_interval =
> > 				le32_to_cpu(interval);
> > 	}
> 
> For a group operation, active_update_interval and current_mode point to the
> group, but enabled and notif_enabled are still written into the global
> ti->info state.
> 
> Disabling one group can therefore make the driver believe the entire
> Telemetry instance is disabled, causing reads for unrelated DEs to return
> no data. A group's collection mode can similarly overwrite the global
> notification state.
> 
> Please select group-versus-instance cached state consistently for every
> field updated here.
> 

Yes indeed...fixed in v8.

> > 			de_offs = le32_to_cpu(resp->shmti_de_offset);
> >
> > 			shmti = &ti->shmti[sid];
> > 			payld = shmti->base;
> > 			/* Check boundary first... */
> > 			if (de_offs + LINE_LENGTH_BYTES(payld) >=
> > 			    shmti->info.len)
> > 				return -EPROTO;
> 
> The addition can wrap, and the payload must end before the TDCF epilogue,
> not merely before the end of the entire SHMTI allocation.
> 
> Please use checked addition and verify the complete line against the
> payload boundary, such as len - TDCF_EPLG_SZ. The same boundary rule
> should be used for an explicitly returned block-timestamp offset.
> 

To be further reworked in v9.

Thanks,
Cristian