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

Andrzej Hajda
2022-03-14T12:08:59Z

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