[Intel-xe] [RFC v4 1/5] drm/netlink: Add netlink infrastructure

Tomer Tayar ttayar at habana.ai
Fri Nov 10 12:24:58 UTC 2023


On 20/10/2023 18:58, Aravind Iddamsetty wrote:
> Define the netlink registration interface and commands, attributes that
> can be commonly used across by drm drivers. This patch intends to use
> the generic netlink family to expose various stats of device. At present
> it defines some commands that shall be used to expose RAS error counters.
>
> v2:
> define common interfaces to genl netlink subsystem that all drm drivers
> can leverage.(Tomer Tayar)
>
> v3: drop DRIVER_NETLINK flag and use the driver_genl_ops structure to
> register to netlink subsystem (Daniel Vetter)
>
> v4:(Michael J. Ruhl)
> 1. rename drm_genl_send to drm_genl_reply
> 2. catch error from xa_store and handle appropriately
>
> Cc: Tomer Tayar<ttayar at habana.ai>
> Cc: Daniel Vetter<daniel at ffwll.ch>
> Cc: Michael J. Ruhl<michael.j.ruhl at intel.com>
>
> Signed-off-by: Aravind Iddamsetty<aravind.iddamsetty at linux.intel.com>
> ---
>   drivers/gpu/drm/Makefile       |   1 +
>   drivers/gpu/drm/drm_drv.c      |   7 ++
>   drivers/gpu/drm/drm_netlink.c  | 188 +++++++++++++++++++++++++++++++++
>   include/drm/drm_device.h       |   8 ++
>   include/drm/drm_drv.h          |   7 ++
>   include/drm/drm_netlink.h      |  30 ++++++
>   include/uapi/drm/drm_netlink.h |  83 +++++++++++++++
>   7 files changed, 324 insertions(+)
>   create mode 100644 drivers/gpu/drm/drm_netlink.c
>   create mode 100644 include/drm/drm_netlink.h
>   create mode 100644 include/uapi/drm/drm_netlink.h
>
> diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
> index ee64c51274ad..60864369adaa 100644
> --- a/drivers/gpu/drm/Makefile
> +++ b/drivers/gpu/drm/Makefile
> @@ -35,6 +35,7 @@ drm-y := \
>   	drm_mode_object.o \
>   	drm_modes.o \
>   	drm_modeset_lock.o \
> +	drm_netlink.o \
>   	drm_plane.o \
>   	drm_prime.o \
>   	drm_print.o \
> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> index 535f16e7882e..31f55c1c7524 100644
> --- a/drivers/gpu/drm/drm_drv.c
> +++ b/drivers/gpu/drm/drm_drv.c
> @@ -937,6 +937,12 @@ int drm_dev_register(struct drm_device *dev, unsigned long flags)
>   	if (ret)
>   		goto err_minors;
>   
> +	if (driver->genl_ops) {
> +		ret = drm_genl_register(dev);
> +		if (ret)
> +			goto err_minors;
> +	}
> +
>   	ret = create_compat_control_link(dev);
>   	if (ret)
>   		goto err_minors;
> @@ -1074,6 +1080,7 @@ static void drm_core_exit(void)
>   {
>   	drm_privacy_screen_lookup_exit();
>   	accel_core_exit();
> +	drm_genl_exit();
>   	unregister_chrdev(DRM_MAJOR, "drm");
>   	debugfs_remove(drm_debugfs_root);
>   	drm_sysfs_destroy();
> diff --git a/drivers/gpu/drm/drm_netlink.c b/drivers/gpu/drm/drm_netlink.c
> new file mode 100644
> index 000000000000..8add249c1da3
> --- /dev/null
> +++ b/drivers/gpu/drm/drm_netlink.c
> @@ -0,0 +1,188 @@
> +// SPDX-License-Identifier: MIT
> +/*
> + * Copyright © 2023 Intel Corporation
> + */
> +
> +#include <drm/drm_device.h>
> +#include <drm/drm_drv.h>
> +#include <drm/drm_file.h>
> +#include <drm/drm_managed.h>
> +#include <drm/drm_netlink.h>
> +#include <drm/drm_print.h>
> +
> +DEFINE_XARRAY(drm_dev_xarray);
> +
> +/**
> + * drm_genl_reply - response to a request
> + * @msg: socket buffer
> + * @info: receiver information
> + * @usrhdr: pointer to user specific header in the message buffer
> + *
> + * RETURNS:
> + * 0 on success and negative error code on failure
> + */
> +int drm_genl_reply(struct sk_buff *msg, struct genl_info *info, void *usrhdr)
> +{
> +	int ret;
> +
> +	genlmsg_end(msg, usrhdr);
> +
> +	ret = genlmsg_reply(msg, info);
> +	if (ret)
> +		nlmsg_free(msg);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL(drm_genl_reply);
> +
> +/**
> + * drm_genl_alloc_msg - allocate genl message buffer
> + * @dev: drm_device for which the message is being allocated
> + * @info: receiver information

a description for msg_size is missing

> + * @usrhdr: pointer to user specific header in the message buffer
> + *
> + * RETURNS:
> + * pointer to new allocated buffer on success, NULL on failure
> + */
> +struct sk_buff *
> +drm_genl_alloc_msg(struct drm_device *dev,
> +		   struct genl_info *info,
> +		   size_t msg_size, void **usrhdr)
> +{
> +	struct sk_buff *new_msg;
> +
> +	new_msg = genlmsg_new(msg_size, GFP_KERNEL);
> +	if (!new_msg)
> +		return new_msg;
> +
> +	*usrhdr = genlmsg_put_reply(new_msg, info, &dev->drm_genl_family, 0, info->genlhdr->cmd);
> +	if (!*usrhdr) {
> +		nlmsg_free(new_msg);
> +		new_msg = NULL;
> +	}
> +
> +	return new_msg;
> +}
> +EXPORT_SYMBOL(drm_genl_alloc_msg);
> +
> +static struct drm_device *genl_to_dev(struct genl_info *info)
> +{
> +	return xa_load(&drm_dev_xarray, info->nlhdr->nlmsg_type);
> +}
> +
> +static int drm_genl_list_errors(struct sk_buff *msg, struct genl_info *info)
> +{
> +	struct drm_device *dev = genl_to_dev(info);
> +
> +	if (GENL_REQ_ATTR_CHECK(info, DRM_RAS_ATTR_REQUEST))
> +		return -EINVAL;
> +
> +	if (WARN_ON(!dev->driver->genl_ops[info->genlhdr->cmd].doit))
> +		return -EOPNOTSUPP;
> +
> +	return dev->driver->genl_ops[info->genlhdr->cmd].doit(dev, msg, info);
> +}
> +
> +static int drm_genl_read_error(struct sk_buff *msg, struct genl_info *info)
> +{
> +	struct drm_device *dev = genl_to_dev(info);
> +
> +	if (GENL_REQ_ATTR_CHECK(info, DRM_RAS_ATTR_ERROR_ID))
> +		return -EINVAL;
> +
> +	if (WARN_ON(!dev->driver->genl_ops[info->genlhdr->cmd].doit))
> +		return -EOPNOTSUPP;
> +
> +	return dev->driver->genl_ops[info->genlhdr->cmd].doit(dev, msg, info);
> +}
> +
> +/* attribute policies */
> +static const struct nla_policy drm_attr_policy_query[DRM_ATTR_MAX + 1] = {
> +	[DRM_RAS_ATTR_REQUEST] = { .type = NLA_U8 },
> +};
> +
> +static const struct nla_policy drm_attr_policy_read_one[DRM_ATTR_MAX + 1] = {
> +	[DRM_RAS_ATTR_ERROR_ID] = { .type = NLA_U64 },
> +};
> +
> +/* drm genl operations definition */
> +const struct genl_ops drm_genl_ops[] = {
> +	{
> +		.cmd = DRM_RAS_CMD_QUERY,
> +		.doit = drm_genl_list_errors,
> +		.policy = drm_attr_policy_query,
> +	},
> +	{
> +		.cmd = DRM_RAS_CMD_READ_ONE,
> +		.doit = drm_genl_read_error,
> +		.policy = drm_attr_policy_read_one,
> +	},
> +	{
> +		.cmd = DRM_RAS_CMD_READ_ALL,
> +		.doit = drm_genl_list_errors,
> +		.policy = drm_attr_policy_query,
> +	},
> +};
> +
> +static void drm_genl_family_init(struct drm_device *dev)
> +{
> +	/* Use drm primary node name eg: card0 to name the genl family */
> +	snprintf(dev->drm_genl_family.name, sizeof(dev->drm_genl_family.name), "%s", dev->primary->kdev->kobj.name);

dev_name() can be used.
Also, what about accel? Maybe check dev->primary and use primary/accel 
accordingly?

> +	dev->drm_genl_family.version = DRM_GENL_VERSION;
> +	dev->drm_genl_family.parallel_ops = true;
> +	dev->drm_genl_family.ops = drm_genl_ops;
> +	dev->drm_genl_family.n_ops = ARRAY_SIZE(drm_genl_ops);
> +	dev->drm_genl_family.maxattr = DRM_ATTR_MAX;
> +	dev->drm_genl_family.module = dev->dev->driver->owner;
> +}
> +
> +static void drm_genl_deregister(struct drm_device *dev,  void *arg)

Redundant space before "void *arg"

> +{
> +	drm_dbg_driver(dev, "unregistering genl family %s\n", dev->drm_genl_family.name);
> +
> +	xa_erase(&drm_dev_xarray, dev->drm_genl_family.id);
> +
> +	genl_unregister_family(&dev->drm_genl_family);
> +}
> +
> +/**
> + * drm_genl_register - Register genl family
> + * @dev: drm_device for which genl family needs to be registered
> + *
> + * RETURNS:
> + * 0 on success and negative error code on failure
> + */
> +int drm_genl_register(struct drm_device *dev)
> +{
> +	int ret;
> +
> +	drm_genl_family_init(dev);
> +
> +	ret = genl_register_family(&dev->drm_genl_family);
> +	if (ret < 0) {
> +		drm_warn(dev, "genl family registration failed\n");
> +		return ret;
> +	}
> +
> +	drm_dbg_driver(dev, "genl family id %d and name %s\n", dev->drm_genl_family.id, dev->drm_genl_family.name);
> +
> +	ret = xa_err(xa_store(&drm_dev_xarray, dev->drm_genl_family.id, dev, GFP_KERNEL));
> +	if (ret)
> +		goto genl_unregister;
> +
> +	ret = drmm_add_action_or_reset(dev, drm_genl_deregister, NULL);
> +
> +	return ret;
> +
> +genl_unregister:
> +	genl_unregister_family(&dev->drm_genl_family);
> +	return ret;
> +}
> +
> +/**
> + * drm_genl_exit: destroy drm_dev_xarray
> + */
> +void drm_genl_exit(void)
> +{
> +	xa_destroy(&drm_dev_xarray);
> +}
> diff --git a/include/drm/drm_device.h b/include/drm/drm_device.h
> index c490977ee250..d3ae91b7714d 100644
> --- a/include/drm/drm_device.h
> +++ b/include/drm/drm_device.h
> @@ -8,6 +8,7 @@
>   
>   #include <drm/drm_legacy.h>
>   #include <drm/drm_mode_config.h>
> +#include <drm/drm_netlink.h>
>   
>   struct drm_driver;
>   struct drm_minor;
> @@ -318,6 +319,13 @@ struct drm_device {
>   	 */
>   	struct dentry *debugfs_root;
>   
> +	/**
> +	 * @drm_genl_family:
> +	 *
> +	 * Generic netlink family registration structure.
> +	 */
> +	struct genl_family drm_genl_family;
> +
>   	/* Everything below here is for legacy driver, never use! */
>   	/* private: */
>   #if IS_ENABLED(CONFIG_DRM_LEGACY)
> diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
> index e2640dc64e08..ebdb7850d235 100644
> --- a/include/drm/drm_drv.h
> +++ b/include/drm/drm_drv.h
> @@ -434,6 +434,13 @@ struct drm_driver {
>   	 */
>   	const struct file_operations *fops;
>   
> +	/**
> +	 * @genl_ops:
> +	 *
> +	 * Drivers private callback to genl commands
> +	 */
> +	const struct driver_genl_ops *genl_ops;
> +
>   #ifdef CONFIG_DRM_LEGACY
>   	/* Everything below here is for legacy driver, never use! */
>   	/* private: */
> diff --git a/include/drm/drm_netlink.h b/include/drm/drm_netlink.h
> new file mode 100644
> index 000000000000..54527dae7847
> --- /dev/null
> +++ b/include/drm/drm_netlink.h
> @@ -0,0 +1,30 @@
> +/* SPDX-License-Identifier: MIT */
> +/*
> + * Copyright © 2023 Intel Corporation
> + */
> +
> +#ifndef __DRM_NETLINK_H__
> +#define __DRM_NETLINK_H__
> +
> +#include <linux/netdevice.h>
> +#include <net/genetlink.h>
> +#include <net/sock.h>
> +#include <uapi/drm/drm_netlink.h>
> +
> +struct drm_device;
> +
> +struct driver_genl_ops {
> +	int		       (*doit)(struct drm_device *dev,
> +				       struct sk_buff *skb,

The skb parameter is currently not used (both xe_genl_list_errors() and 
xe_genl_read_error() allocate a new skb).
Did you add because it might be needed for future ops?

> +				       struct genl_info *info);
> +};
> +
> +int drm_genl_register(struct drm_device *dev);
> +void drm_genl_exit(void);
> +int drm_genl_reply(struct sk_buff *msg, struct genl_info *info, void *usrhdr);
> +struct sk_buff *
> +drm_genl_alloc_msg(struct drm_device *dev,
> +		   struct genl_info *info,
> +		   size_t msg_size, void **usrhdr);
> +#endif
> +
> diff --git a/include/uapi/drm/drm_netlink.h b/include/uapi/drm/drm_netlink.h
> new file mode 100644
> index 000000000000..aab42147a20e
> --- /dev/null
> +++ b/include/uapi/drm/drm_netlink.h
> @@ -0,0 +1,83 @@
> +/* SPDX-License-Identifier: MIT */
> +/*
> + * Copyright 2023 Intel Corporation
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice (including the next
> + * paragraph) shall be included in all copies or substantial portions of the
> + * Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> + * VA LINUX SYSTEMS AND/OR ITS SUPPLIERS BE LIABLE FOR ANY CLAIM, DAMAGES OR
> + * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
> + * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
> + * OTHER DEALINGS IN THE SOFTWARE.
> + */
> +
> +#ifndef _DRM_NETLINK_H_
> +#define _DRM_NETLINK_H_
> +
> +#define DRM_GENL_VERSION 1
> +
> +#if defined(__cplusplus)
> +extern "C" {
> +#endif
> +
> +/**
> + * enum drm_genl_error_cmds - Supported error commands
> + *
> + */
> +enum drm_genl_error_cmds {
> +	DRM_CMD_UNSPEC,
> +	/** @DRM_RAS_CMD_QUERY: Command to list all errors names with config-id */
> +	DRM_RAS_CMD_QUERY,
> +	/** @DRM_RAS_CMD_READ_ONE: Command to get a counter for a specific error */
> +	DRM_RAS_CMD_READ_ONE,
> +	/** @DRM_RAS_CMD_READ_ALL: Command to get counters of all errors */
> +	DRM_RAS_CMD_READ_ALL,
> +
> +	__DRM_CMD_MAX,
> +	DRM_CMD_MAX = __DRM_CMD_MAX - 1,
> +};
> +
> +/**
> + * enum drm_error_attr - Attributes to use with drm_genl_error_cmds
> + *
> + */
> +enum drm_error_attr {
> +	DRM_ATTR_UNSPEC,
> +	DRM_ATTR_PAD = DRM_ATTR_UNSPEC,
> +	/**
> +	 * @DRM_RAS_ATTR_REQUEST: Should be used with DRM_RAS_CMD_QUERY,
> +	 * DRM_RAS_CMD_READ_ALL
> +	 */
> +	DRM_RAS_ATTR_REQUEST, /* NLA_U8 */
> +	/**
> +	 * @DRM_RAS_ATTR_QUERY_REPLY: First Nested attributed sent as a
> +	 * response to DRM_RAS_CMD_QUERY, DRM_RAS_CMD_READ_ALL commands.
> +	 */
> +	DRM_RAS_ATTR_QUERY_REPLY, /*NLA_NESTED*/

Maybe a space before and after NLA_NESTED?

Thanks,
Tomer

> +	/** @DRM_RAS_ATTR_ERROR_NAME: Used to pass error name */
> +	DRM_RAS_ATTR_ERROR_NAME, /* NLA_NUL_STRING */
> +	/** @DRM_RAS_ATTR_ERROR_ID: Used to pass error id */
> +	DRM_RAS_ATTR_ERROR_ID, /* NLA_U64 */
> +	/** @DRM_RAS_ATTR_ERROR_VALUE: Used to pass error value */
> +	DRM_RAS_ATTR_ERROR_VALUE, /* NLA_U64 */
> +
> +	__DRM_ATTR_MAX,
> +	DRM_ATTR_MAX = __DRM_ATTR_MAX - 1,
> +};
> +
> +#if defined(__cplusplus)
> +}
> +#endif
> +
> +#endif




More information about the Intel-xe mailing list