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

[Aravind Iddamsetty <aravind.iddamsetty@linux.intel.com>]

On 16-08-2025 03:22, Rodrigo Vivi wrote:
> 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.
ok.
>>  	.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.

agree, this shall be part of explicit registration.

Thanks,
Aravind.
>
>> -- 
>> 2.25.1
>>