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@xxxxxxxxx>
> Cc: Daniel Vetter<daniel@xxxxxxxx>
> Cc: Michael J. Ruhl<michael.j.ruhl@xxxxxxxxx>
>
> Signed-off-by: Aravind Iddamsetty<aravind.iddamsetty@xxxxxxxxxxxxxxx>
> ---
> 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
