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
