Re: [PATCH v5] drm/xe/gt_throttle: Avoid TOCTOU when monitoring reasons

[Raag Jadav <raag.jadav@intel.com>]

On Wed, Nov 05, 2025 at 02:27:15PM +0100, Raag Jadav wrote:
> On Tue, Nov 04, 2025 at 02:20:51PM -0800, Lucas De Marchi wrote:
> > It's currently not possible to safely monitor if there's throttling
> > happening and what are the reasons. The approach of reading the status
> > and then reading the reasons is not reliable as by the time sysadmin
> > reads the reason, the throttling could not be happening anymore.
> > 
> > Previous tentative to fix that[1] was breaking the ABI and potentially
> > sysadmin's scripts. This takes a different approach of adding and
> > documenting the additional attribute. It's still valuable, though
> > redundant, to provide the simpler 0/1 interface.
> > 
> > In order to avoid userspace knowledge on the bitmask meaning and to be
> > able to maintain the kernel side in sync with possible changes in
> > future, just walk the attribute group and check what are the masks that
> > match the value read.
> > 
> > [1] https://lore.kernel.org/intel-xe/20241025092238.167042-1-raag.jadav@intel.com/
> 
> ...
> 
> > +static const struct attribute_group *get_platform_throttle_group(struct xe_device *xe);
> > +
> > +static ssize_t reasons_show(struct kobject *kobj,
> > +			    struct kobj_attribute *attr, char *buff)
> > +{
> > +	struct xe_gt *gt = throttle_to_gt(kobj);
> > +	struct xe_device *xe = gt_to_xe(gt);
> > +	const struct attribute_group *group;
> > +	struct attribute **pother;
> > +	ssize_t ret = 0;
> > +	u32 reasons;
> > +
> > +	reasons = xe_gt_throttle_get_limit_reasons(gt);
> > +	if (!reasons)
> > +		goto ret_none;
> > +
> > +	group = get_platform_throttle_group(xe);
> > +	for (pother = group->attrs; *pother; pother++) {
> > +		struct kobj_attribute *kattr = container_of(*pother, struct kobj_attribute, attr);
> > +		struct throttle_attribute *other_ta = kobj_attribute_to_throttle(kattr);
> > +
> > +		if (other_ta->mask != U32_MAX && reasons & other_ta->mask)
> > +			ret += sysfs_emit_at(buff, ret, "%s ", (*pother)->name);
> > +	}
> > +
> > +	if (drm_WARN_ONCE(&xe->drm, !ret, "Unknown reason: %#x\n", reasons))
> 
> Nit: I know we're masking it but I'm a bit more used to the full format for
> register values, i.e. 0x%08x

On second thought, I just checked the mask and realized almost half of it
will be redundant 0s so let's keep it as is :)

> Reviewed-by: Raag Jadav <raag.jadav@intel.com>
> 
> > +		goto ret_none;
> > +
> > +	/* Drop extra space from last iteration above */
> > +	ret--;
> > +	ret += sysfs_emit_at(buff, ret, "\n");
> > +
> > +	return ret;
> > +
> > +ret_none:
> > +	return sysfs_emit(buff, "none\n");
> > +}
> > +
> >  #define THROTTLE_ATTR_RO(name, _mask)				\
> >  	struct throttle_attribute attr_##name =	{		\
> >  		.attr = __ATTR(name, 0444, reason_show, NULL),	\
> >  		.mask = _mask,					\
> >  	}
> >  
> > +#define THROTTLE_ATTR_RO_FUNC(name, _mask, _show)		\
> > +	struct throttle_attribute attr_##name =	{		\
> > +		.attr = __ATTR(name, 0444, _show, NULL),	\
> > +		.mask = _mask,					\
> > +	}
> > +
> > +static THROTTLE_ATTR_RO_FUNC(reasons, 0, reasons_show);
> >  static THROTTLE_ATTR_RO(status, U32_MAX);
> >  static THROTTLE_ATTR_RO(reason_pl1, POWER_LIMIT_1_MASK);
> >  static THROTTLE_ATTR_RO(reason_pl2, POWER_LIMIT_2_MASK);
> > @@ -128,6 +180,7 @@ static THROTTLE_ATTR_RO(reason_vr_thermalert, VR_THERMALERT_MASK);
> >  static THROTTLE_ATTR_RO(reason_vr_tdc, VR_TDC_MASK);
> >  
> >  static struct attribute *throttle_attrs[] = {
> > +	&attr_reasons.attr.attr,
> >  	&attr_status.attr.attr,
> >  	&attr_reason_pl1.attr.attr,
> >  	&attr_reason_pl2.attr.attr,
> > @@ -153,6 +206,7 @@ static THROTTLE_ATTR_RO(reason_psys_crit, PSYS_CRIT_MASK);
> >  
> >  static struct attribute *cri_throttle_attrs[] = {
> >  	/* Common */
> > +	&attr_reasons.attr.attr,
> >  	&attr_status.attr.attr,
> >  	&attr_reason_pl1.attr.attr,
> >  	&attr_reason_pl2.attr.attr,
> > 
> > 
> >