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

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

On Mon, Nov 03, 2025 at 09:20:40AM -0600, Lucas De Marchi wrote:
> On Sat, Nov 01, 2025 at 07:38:00AM +0100, Raag Jadav wrote:
> > On Fri, Oct 31, 2025 at 09:47:25AM -0700, 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);
> > > +	group = get_platform_throttle_group(xe);
> > > +
> > > +	for (pother = group->attrs; *pother; pother++) {
> > 
> > Shouldn't this entire loop be inside if (reasons), or did I miss something?
> 
> it could be an optimization, but should work regardless.

Yep, upto you.

> > > +		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)
> > 
> > With above in place this will be much simpler.
> 
> why would it be simpler? we would still skip the status attribute
> (conviniently defined with mask == U32_MAX the status attribute) and
> still do a & with mask

Hm, I had a misconception that we've indexed the attributes as per their
mask value. With that perhaps for_each_set_bit() would've been a great
alternative.

> > > +			ret += sysfs_emit_at(buff, ret, "%s ", (*pother)->name);
> > > +	}
> > > +
> > > +	if (ret)
> > > +		/* Drop extra space from last iteration above */
> > > +		ret--;
> > > +	else
> > > +		ret += sysfs_emit_at(buff, ret, "none");
> > 
> > I find the lack of '+' a bit more readable as it guarantees that nothing
> > else is inherited here, or perhaps it's my needless paranoia at work :(
> > Upto you.
> 
> Keeping the pattern is good IMO. If we'd change then it would be for a
> sysfs_emit().
> 
> 	if (!ret)
> 		return sysfs_emit(buff, "none\n");
> 
> 	ret--;
> 	ret += sysfs_emit_at(buff, ret, "\n");
> 
> 	return ret;

Either way works for me :)

Reviewed-by: Raag Jadav <raag.jadav@intel.com>