On 11/02/2020 21:06, Andi Shyti wrote:
Hi Tvrtko,
+void intel_gt_sysfs_register(struct intel_gt *gt)
+{
+ struct kobject *parent = kobject_get(>->i915->drm.primary->kdev->kobj);
Does this needs a kobject_put to balance out somewhere?
Yes, I forgot the two kobject_put that are needed.
+ int ret;
+
+ gt->kobj = kobject_create_and_add("gt", parent);
+ if (!gt->kobj) {
+ pr_err("failed to initialize sysfs file\n");
+ return;
+ }
+
+ dev_set_drvdata(kobj_to_dev(gt->kobj), gt);
+
+ ret = sysfs_create_files(gt->kobj, gt_attrs);
+ if (ret)
+ pr_err("failed to create sysfs gt info files\n");
I can't remember which log helper takes in the device and prefixes that
string but I think it could be useful here, since the error is otherwise
eaten.
pr_* is used a lot in gt/. Playing with the dev pointer I
can use "dev_err(dev,...)"
+void intel_gt_sysfs_unregister(struct intel_gt *gt)
+{
+ if (!gt->kobj)
+ return;
+
+ intel_gt_sysfs_pm_remove(gt, gt->kobj);
+ sysfs_remove_files(gt->kobj, gt_attrs);
Why is this asymmetrical to creation?
Because in V1 gt_attrs and whatever was created in sysfs_gt_pm
was in the same group, but it desn't matter.
I mean you call intel_gt_sysfs_pm_init
two times with different roots, so why not intel_gt_sysfs_pm_remove also
twice with different roots?
Because I forgot them in the cleanups :)
Next version incoming soon? :)
+ /*
+ * 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 (strcmp(kobj->name, "gt")) {
+ pr_warn("the interface is obsolete, use gt/\n");
I think the message will need to be a bit more verbose since it is intended
for users. I don't have any suggestions straight away apart from googling to
find similar examples from the past and other subsystems.
Yes, I couldn't come up with a better message in 80 characters,
and I can use dev_warn instead of pr_warn.
Maybe we even need to log this once. Well we may need to log the
offending process name/pid. Not sure if we can manage once on top of
that.. sounds too hard. So maybe just name/pid.
+static ssize_t rc6_enable_show(struct device *dev,
+ struct device_attribute *attr,
+ char *buff)
+{
+ struct intel_gt *gt = intel_gt_sysfs_get_drvdata(dev);
The rest of code is unchanged apart from this same line in all show/store
vfuncs?
yes, more or less.
Cool, just so I know what I don't have to cross-reference too much.
Regards,
Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
