Re: [RFC v5 2/5] drm/xe/RAS: Register netlink capability

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

On Wed, Jul 30, 2025 at 12:19:53PM +0530, Aravind Iddamsetty wrote:
> Register netlink capability with the DRM and register the driver
> callbacks to DRM RAS netlink commands.
> 
> v2:
> Move the netlink registration parts to DRM susbsytem (Tomer Tayar)
> 
> v3: compile only if CONFIG_NET is enabled
> 
> Reviewed-by: Michael J. Ruhl <michael.j.ruhl@intel.com> #v2
> Signed-off-by: Aravind Iddamsetty <aravind.iddamsetty@linux.intel.com>
> ---
>  drivers/gpu/drm/xe/Makefile          |  2 ++
>  drivers/gpu/drm/xe/xe_device.c       |  6 ++++++
>  drivers/gpu/drm/xe/xe_device_types.h |  1 +
>  drivers/gpu/drm/xe/xe_netlink.c      | 26 ++++++++++++++++++++++++++
>  4 files changed, 35 insertions(+)
>  create mode 100644 drivers/gpu/drm/xe/xe_netlink.c
> 
> diff --git a/drivers/gpu/drm/xe/Makefile b/drivers/gpu/drm/xe/Makefile
> index 80eecd35e807..e960c2dbe658 100644
> --- a/drivers/gpu/drm/xe/Makefile
> +++ b/drivers/gpu/drm/xe/Makefile
> @@ -304,6 +304,8 @@ xe-$(CONFIG_DRM_XE_DISPLAY) += \
>  	i915-display/skl_universal_plane.o \
>  	i915-display/skl_watermark.o
>  
> +xe-$(CONFIG_NET) += xe_netlink.o
> +
>  ifeq ($(CONFIG_ACPI),y)
>  	xe-$(CONFIG_DRM_XE_DISPLAY) += \
>  		i915-display/intel_acpi.o \
> diff --git a/drivers/gpu/drm/xe/xe_device.c b/drivers/gpu/drm/xe/xe_device.c
> index 806dbdf8118c..ca7a17c16aa5 100644
> --- a/drivers/gpu/drm/xe/xe_device.c
> +++ b/drivers/gpu/drm/xe/xe_device.c
> @@ -363,6 +363,8 @@ static const struct file_operations xe_driver_fops = {
>  	.fop_flags = FOP_UNSIGNED_OFFSET,
>  };
>  
> +extern const struct driver_genl_ops xe_genl_ops[];
> +
>  static struct drm_driver driver = {
>  	/* Don't use MTRRs here; the Xserver or userspace app should
>  	 * deal with them for Intel hardware.
> @@ -381,6 +383,10 @@ static struct drm_driver driver = {
>  #ifdef CONFIG_PROC_FS
>  	.show_fdinfo = xe_drm_client_fdinfo,
>  #endif
> +#ifdef CONFIG_NET
> +	.genl_ops = xe_genl_ops,
> +#endif
> +

we should definitely have a drm function to register it instead of hard-coding
it here, regardless if we go with the group split or not.
It is not okay forcing this to every platform, even the ones without any RAS
available for instance.

>  	.ioctls = xe_ioctls,
>  	.num_ioctls = ARRAY_SIZE(xe_ioctls),
>  	.fops = &xe_driver_fops,
> diff --git a/drivers/gpu/drm/xe/xe_device_types.h b/drivers/gpu/drm/xe/xe_device_types.h
> index 3a851c7a55dd..08d3e53e4b37 100644
> --- a/drivers/gpu/drm/xe/xe_device_types.h
> +++ b/drivers/gpu/drm/xe/xe_device_types.h
> @@ -10,6 +10,7 @@
>  
>  #include <drm/drm_device.h>
>  #include <drm/drm_file.h>
> +#include <drm/drm_netlink.h>
>  #include <drm/ttm/ttm_device.h>
>  
>  #include "xe_devcoredump_types.h"
> diff --git a/drivers/gpu/drm/xe/xe_netlink.c b/drivers/gpu/drm/xe/xe_netlink.c
> new file mode 100644
> index 000000000000..9e588fb19631
> --- /dev/null
> +++ b/drivers/gpu/drm/xe/xe_netlink.c
> @@ -0,0 +1,26 @@
> +// SPDX-License-Identifier: MIT
> +/*
> + * Copyright © 2023 Intel Corporation
> + */
> +
> +#include <net/genetlink.h>
> +#include <uapi/drm/drm_netlink.h>
> +
> +#include "xe_device.h"
> +
> +static int xe_genl_list_errors(struct drm_device *drm, struct sk_buff *msg, struct genl_info *info)
> +{
> +	return 0;
> +}
> +
> +static int xe_genl_read_error(struct drm_device *drm, struct sk_buff *msg, struct genl_info *info)
> +{
> +	return 0;
> +}
> +
> +/* driver callbacks to DRM netlink commands*/
> +const struct driver_genl_ops xe_genl_ops[] = {
> +	[DRM_RAS_CMD_QUERY] =		{ .doit = xe_genl_list_errors },
> +	[DRM_RAS_CMD_READ_ONE] =	{ .doit = xe_genl_read_error },
> +	[DRM_RAS_CMD_READ_ALL] =	{ .doit = xe_genl_list_errors, },
> +};

this is another space that is strange. you declare it here and drm
magically uses it. Another reason for more explicity registration.
and with the struct drm_ras where these commands are part of that.
as well as the group name, etc.

> -- 
> 2.25.1
>