On 21/10/23 02:06, Ruhl, Michael J wrote:
>> -----Original Message-----
>> From: Aravind Iddamsetty <aravind.iddamsetty@xxxxxxxxxxxxxxx>
>> Sent: Friday, October 20, 2023 11:59 AM
>> To: intel-xe@xxxxxxxxxxxxxxxxxxxxx; dri-devel@xxxxxxxxxxxxxxxxxxxxx;
>> alexander.deucher@xxxxxxx; airlied@xxxxxxxxx; daniel@xxxxxxxx;
>> joonas.lahtinen@xxxxxxxxxxxxxxx; ogabbay@xxxxxxxxxx; Tayar, Tomer (Habana)
>> <ttayar@xxxxxxxxx>; Hawking.Zhang@xxxxxxx;
>> Harish.Kasiviswanathan@xxxxxxx; Felix.Kuehling@xxxxxxx;
>> Luben.Tuikov@xxxxxxx; Ruhl, Michael J <michael.j.ruhl@xxxxxxxxx>
>> Subject: [RFC v4 1/5] drm/netlink: Add netlink infrastructure
>>
>> 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
> Hi Aravind,
>
> This looks reasonable to me.
>
> Reviewed-by: Michael J. Ruhl <michael.j.ruhl@xxxxxxxxx>
Hi Mike,
Thanks a lot for your reviews and r-b.
Regards,
Aravind.
>
> M
>
>> 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
>> + * @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->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)
>> +{
>> + 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,
>> + 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*/
>> + /** @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
>> --
>> 2.25.1
