Re: [Intel-gfx] [PATCH v2] drm/i915/gt: make a gt sysfs group and move power management files

From: Andi Shyti <andi@etezian.org>
To: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
Cc: Intel GFX <intel-gfx@lists.freedesktop.org>
Subject: Re: [Intel-gfx] [PATCH v2] drm/i915/gt: make a gt sysfs group and move power management files
Date: Tue, 11 Feb 2020 23:06:25 +0200	[thread overview]
Message-ID: <20200211210625.GA6386@jack.zhora.eu> (raw)
In-Reply-To: <2defb42c-9fcc-17f8-0593-8e02f75f9ba4@linux.intel.com>

Hi Tvrtko,

> > +void intel_gt_sysfs_register(struct intel_gt *gt)
> > +{
> > +	struct kobject *parent = kobject_get(&gt->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