Re: [Intel-gfx] [PATCH v5 4/7] drm/i915/gt: create per-tile sysfs interface

[Andrzej Hajda <andrzej.hajda@intel.com>]

On 13.03.2022 20:45, Andi Shyti wrote:
> Hi Andrzej,
>
> I'm sorry, but I'm not fully understanding,
>
>>>>> +struct intel_gt *intel_gt_sysfs_get_drvdata(struct device *dev,
>>>>> +					    const char *name)
>>>>> +{
>>>>> +	struct kobject *kobj = &dev->kobj;
>>>>> +
>>>>> +	/*
>>>>> +	 * We are interested at knowing from where the interface
>>>>> +	 * has been called, whether it's called from gt/ or from
>>>>> +	 * the parent directory.
>>>>> +	 * From the interface position it depends also the value of
>>>>> +	 * the private data.
>>>>> +	 * If the interface is called from gt/ then private data is
>>>>> +	 * of the "struct intel_gt *" type, otherwise it's * a
>>>>> +	 * "struct drm_i915_private *" type.
>>>>> +	 */
>>>>> +	if (!is_object_gt(kobj)) {
>>>>> +		struct drm_i915_private *i915 = kdev_minor_to_i915(dev);
>>>>> +
>>>>> +		pr_devel_ratelimited(DEPRECATED
>>>>> +			"%s (pid %d) is accessing deprecated %s "
>>>>> +			"sysfs control, please use gt/gt<n>/%s instead\n",
>>>>> +			current->comm, task_pid_nr(current), name, name);
>>>>> +		return to_gt(i915);
>>>>> +	}
>>>>> +
>>>>> +	return kobj_to_gt(kobj);
>>>> It took some time for me to understand what is going on here.
>>>> We have dev argument which sometimes can point to "struct device", sometimes
>>>> to "struct kobj_gt", but it's type suggests differently, quite ugly.
>>>> I wonder if wouldn't be better to use __ATTR instead of DEVICE_ATTR* as in
>>>> case of intel_engines_add_sysfs. This way abstractions would look better,
>>>> hopefully.
>>> How would it help?
>>>
>>> The difference is that I'm adding twice different interfaces with
>>> the same name and different location (i.e. different object). The
>>> legacy intrefaces inherit the object from drm and I'm preserving
>>> that reference.
>>>
>>> While the new objects would derive from the previous and they are
>>> pretty much like intel_engines_add_sysfs().
>> I was not clear on the issue. Here in case of 'id' attribute it is defined
>> as device_attribute, but in kobj_type.sysfs_ops you assign formally
>> incompatible &kobj_sysfs_ops.
> 'kobj_sysfs_ops' is of the type 'kobj_type'.

Yes, but for example kobj_sysfs_ops.show points to function 
kobj_attr_show, and kobj_attr_show expects that it's attr argument is 
embedded in kobj_attribute[1], but this is not true in case of 'id' 
attribute - it is embedded in device_attribute.
In short kobj_sysfs_ops should be used only with attrs embeded in 
kobj_attribute, unless I missed sth.

[1]: https://elixir.bootlin.com/linux/latest/source/lib/kobject.c#L836

>
>> kobj_sysfs_ops expects kobj_attribute! Fortunately kobj_attribute is 'binary
>> compatible' with device_attribute and kobj is at beginning of struct device
>> as well, so it does not blow up, but I wouldn't say it is clean solution :)
>> If you look at intel_engines_add_sysfs you can see that all attributes are
>> defined as kobj_attribute.
> That's exactly the approach I use in the next patches for the
> power management files, I use "struct kobj_gt" wrapped around
> "struct kobject". But I'm using that only for the GT files.

But attributes are still defined using DEVICE_ATTR* macros, ie they are 
embedded in device_attribute, so the problem is the same - you are using 
kobj_sysfs_ops with device_attribute.

>
> Are you, btw, suggesting to use this same approache also for the
> legacy files that for now have a pointer to the drm kobject? This
> way I would need to add more information, like the pointer to
> i915 and gt_id. This way I wouldn't need the files above that
> look hacky to you. Is this what you mean?

Positive feedback is more difficult :)
I am little bit lost in possible solutions, after grepping other drivers 
I have not good advice about proper handling of such situation, *beside 
splitting the interface*.
For sure attrs used in device/power must be embedded in 
device_attribute. So if you do not want to split interface, then it 
implies GTs attrs must be also in device_attribute. Then maybe creating 
custom sysfs_ops would help??? I am not sure.

Regards
Andrzej



>
> Andi