linux-kernel - Re: [PATCH v2 1/1] drm/panfrost: Replace fdinfo's profiling debugfs knob with sysfs

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20240304180350.74e7e385@collabora.com>
Date: Mon, 4 Mar 2024 18:03:50 +0100
From: Boris Brezillon <boris.brezillon@...labora.com>
To: Steven Price <steven.price@....com>
Cc: Adrián Larumbe <adrian.larumbe@...labora.com>, Rob
 Herring <robh@...nel.org>, David Airlie <airlied@...il.com>, Daniel Vetter
 <daniel@...ll.ch>, Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
 Maxime Ripard <mripard@...nel.org>, Thomas Zimmermann
 <tzimmermann@...e.de>, Jonathan Corbet <corbet@....net>,
 linux-kernel@...r.kernel.org, dri-devel@...ts.freedesktop.org,
 linux-doc@...r.kernel.org
Subject: Re: [PATCH v2 1/1] drm/panfrost: Replace fdinfo's profiling debugfs
 knob with sysfs

On Mon, 4 Mar 2024 16:04:34 +0000
Steven Price <steven.price@....com> wrote:

> On 02/03/2024 15:48, Adrián Larumbe wrote:
> > Debugfs isn't always available in production builds that try to squeeze
> > every single byte out of the kernel image, but we still need a way to
> > toggle the timestamp and cycle counter registers so that jobs can be
> > profiled for fdinfo's drm engine and cycle calculations.
> > 
> > Drop the debugfs knob and replace it with a sysfs file that accomplishes
> > the same functionality, and document its ABI in a separate file.
> > 
> > Signed-off-by: Adrián Larumbe <adrian.larumbe@...labora.com>  
> 
> I'm happy with this.
> 
> Reviewed-by: Steven Price <steven.price@....com>
> 
> Boris: are you happy with the sysfs ABI, or would you like to
> investigate further the implications of leaving the counters enabled all
> the time during execution before committing to the sysfs ABI?

No, that's fine, but I have a few comments on the implementation.

> > +static ssize_t
> > +profiling_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf)
> > +{
> > +	bool *profile_mode =
> > +		&container_of(kobj, struct panfrost_device,
> > +			      profiling.base)->profiling.profile_mode;
> > +
> > +	return sysfs_emit(buf, "%d\n", *profile_mode);
> > +}
> > +
> > +static ssize_t
> > +profiling_store(struct kobject *kobj, struct kobj_attribute *attr,
> > +	       const char *buf, size_t count)
> > +{
> > +	bool *profile_mode =
> > +		&container_of(kobj, struct panfrost_device,
> > +			      profiling.base)->profiling.profile_mode;
> > +	int err, value;
> > +
> > +	err = kstrtoint(buf, 0, &value);

I'd suggest using kstrtobool() since you make the result a boolean
anyway.

> > +	if (err)
> > +		return err;
> > +
> > +	*profile_mode = !!value;
> > +
> > +	return count;
> > +}
> > +
> > +static const struct kobj_attribute profiling_status =
> > +__ATTR(status, 0644, profiling_show, profiling_store);
> > +
> > +static const struct kobj_type kobj_profile_type = {
> > +	.sysfs_ops = &kobj_sysfs_ops,
> > +};

DEVICE_ATTR(profiling, 0644, profiling_show, profiling_store);

?

> > +
> > +int panfrost_sysfs_init(struct panfrost_device *pfdev)
> > +{
> > +	struct device *kdev = pfdev->ddev->primary->kdev;
> > +	int err;
> > +
> > +	kobject_init(&pfdev->profiling.base, &kobj_profile_type);
> > +
> > +	err = kobject_add(&pfdev->profiling.base, &kdev->kobj, "%s", "profiling");

Can we make it a device attribute instead of adding an extra kboj?

> > +	if (err)
> > +		return err;
> > +
> > +	err = sysfs_create_file(&pfdev->profiling.base, &profiling_status.attr);
> > +	if (err)
> > +		kobject_del(&pfdev->profiling.base);
> > +
> > +	return err;
> > +}