Re: [PATCH 02/17] drm/xe/perf/uapi: Add perf_stream_paranoid sysctl

[Joel Granados <j.granados@samsung.com>]

[-- Attachment #1: Type: text/plain, Size: 3499 bytes --]

On Thu, Dec 07, 2023 at 10:43:14PM -0800, Ashutosh Dixit wrote:
> Normally only superuser/root can access perf counter data. However,
> superuser can set perf_stream_paranoid sysctl to 0 to allow non-privileged
> users to also access perf data. perf_stream_paranoid is introduced at the
> perf layer to allow different perf stream types to share this access
> mechanism.
> 
> Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com>
> ---
>  drivers/gpu/drm/xe/xe_module.c |  5 +++++
>  drivers/gpu/drm/xe/xe_perf.c   | 28 ++++++++++++++++++++++++++++
>  drivers/gpu/drm/xe/xe_perf.h   |  4 ++++
>  3 files changed, 37 insertions(+)
> 
> diff --git a/drivers/gpu/drm/xe/xe_module.c b/drivers/gpu/drm/xe/xe_module.c
> index 51bf69b7ab222..8629330d928b0 100644
> --- a/drivers/gpu/drm/xe/xe_module.c
> +++ b/drivers/gpu/drm/xe/xe_module.c
> @@ -11,6 +11,7 @@
>  #include "xe_drv.h"
>  #include "xe_hw_fence.h"
>  #include "xe_pci.h"
> +#include "xe_perf.h"
>  #include "xe_pmu.h"
>  #include "xe_sched_job.h"
>  
> @@ -71,6 +72,10 @@ static const struct init_funcs init_funcs[] = {
>  		.init = xe_register_pci_driver,
>  		.exit = xe_unregister_pci_driver,
>  	},
> +	{
> +		.init = xe_perf_sysctl_register,
> +		.exit = xe_perf_sysctl_unregister,
> +	},
>  };
>  
>  static int __init xe_init(void)
> diff --git a/drivers/gpu/drm/xe/xe_perf.c b/drivers/gpu/drm/xe/xe_perf.c
> index a130076b59aa2..37538e98dcc04 100644
> --- a/drivers/gpu/drm/xe/xe_perf.c
> +++ b/drivers/gpu/drm/xe/xe_perf.c
> @@ -4,9 +4,13 @@
>   */
>  
>  #include <linux/errno.h>
> +#include <linux/sysctl.h>
>  
>  #include "xe_perf.h"
>  
> +u32 xe_perf_stream_paranoid = true;
> +static struct ctl_table_header *sysctl_header;
> +
>  int xe_perf_ioctl(struct drm_device *dev, void *data, struct drm_file *file)
>  {
>  	struct drm_xe_perf_param *arg = data;
> @@ -19,3 +23,27 @@ int xe_perf_ioctl(struct drm_device *dev, void *data, struct drm_file *file)
>  		return -EINVAL;
>  	}
>  }
> +
> +static struct ctl_table perf_ctl_table[] = {
> +	{
> +	 .procname = "perf_stream_paranoid",
> +	 .data = &xe_perf_stream_paranoid,
> +	 .maxlen = sizeof(xe_perf_stream_paranoid),
> +	 .mode = 0644,
> +	 .proc_handler = proc_dointvec_minmax,
> +	 .extra1 = SYSCTL_ZERO,
> +	 .extra2 = SYSCTL_ONE,
> +	 },
> +	{}
We no longer need the sentinel moving forward. At the moment it will
work with and without a sentinel. But that check will be removed within
in the next two releases. I suggest you just remove the sentinel and end
it like this

...
  .extra2 = SYSCTL_ONE,
  },
...

Furthermore: isn't that space after the tab on the struct supposed to be
a tab?

best

> +};
> +
> +int xe_perf_sysctl_register(void)
> +{
> +	sysctl_header = register_sysctl("dev/xe", perf_ctl_table);
> +	return 0;
> +}
> +
> +void xe_perf_sysctl_unregister(void)
> +{
> +	unregister_sysctl_table(sysctl_header);
> +}
> diff --git a/drivers/gpu/drm/xe/xe_perf.h b/drivers/gpu/drm/xe/xe_perf.h
> index 254cc7cf49fef..1ff0a07ebab30 100644
> --- a/drivers/gpu/drm/xe/xe_perf.h
> +++ b/drivers/gpu/drm/xe/xe_perf.h
> @@ -11,6 +11,10 @@
>  struct drm_device;
>  struct drm_file;
>  
> +extern u32 xe_perf_stream_paranoid;
> +
>  int xe_perf_ioctl(struct drm_device *dev, void *data, struct drm_file *file);
> +int xe_perf_sysctl_register(void);
> +void xe_perf_sysctl_unregister(void);
>  
>  #endif
> -- 
> 2.41.0
> 

-- 

Joel Granados

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]