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 :)
> > + /*
> > + * 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.
> > +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.
Andi
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
