On 11/10/23 17:54, Tomer Tayar wrote:
> 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
Thanks for catching it will add.
>
>> + * @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?
the present series is adding this feature for primary device only and has
no knowledge how it will be used for accel device, so when accel device
start using this infra should make that particular change or do you think
it should be added as part of this series only?
>
>> + 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"
will clean it.
>
>> +{
>> + 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?
well I wanted to pass on the details the netlink subsystem sends and leave it to the driver
if it wants to use it anyway.
>
>> + 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?
right missed that.
Thanks,
Aravind.
>
> 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
>
