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

[Tomer Tayar]

On 26/05/2023 19:20, Aravind Iddamsetty wrote:
> Define the netlink commands and attributes that can be commonly used
> across by drm drivers.
>
> Signed-off-by: Aravind Iddamsetty <aravind.iddamsetty at intel.com>
> ---
>   include/uapi/drm/drm_netlink.h | 68 ++++++++++++++++++++++++++++++++++
>   1 file changed, 68 insertions(+)
>   create mode 100644 include/uapi/drm/drm_netlink.h
>
> diff --git a/include/uapi/drm/drm_netlink.h b/include/uapi/drm/drm_netlink.h
> new file mode 100644
> index 000000000000..28e7a334d0c7
> --- /dev/null
> +++ b/include/uapi/drm/drm_netlink.h
> @@ -0,0 +1,68 @@
> +/*
> + * 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_
> +
> +#include <linux/netdevice.h>
> +#include <net/genetlink.h>
> +#include <net/sock.h>

This is a uapi header.
Are all header files here available for user?
Also, should we add here "#if defined(__cplusplus) extern "C" { ..."?

> +
> +#define DRM_GENL_VERSION 1
> +
> +enum error_cmds {
> +	DRM_CMD_UNSPEC,
> +	/* command to list all errors names with config-id */
> +	DRM_CMD_QUERY,
> +	/* command to get a counter for a specific error */
> +	DRM_CMD_READ_ONE,
> +	/* command to get counters of all errors */
> +	DRM_CMD_READ_ALL,
> +
> +	__DRM_CMD_MAX,
> +	DRM_CMD_MAX = __DRM_CMD_MAX - 1,
> +};
> +
> +enum error_attr {
> +	DRM_ATTR_UNSPEC,
> +	DRM_ATTR_PAD = DRM_ATTR_UNSPEC,
> +	DRM_ATTR_REQUEST, /* NLA_U8 */
> +	DRM_ATTR_QUERY_REPLY, /*NLA_NESTED*/

Missing spaces in /*NLA_NESTED*/

> +	DRM_ATTR_ERROR_NAME, /* NLA_NUL_STRING */
> +	DRM_ATTR_ERROR_ID, /* NLA_U64 */
> +	DRM_ATTR_ERROR_VALUE, /* NLA_U64 */
> +
> +	__DRM_ATTR_MAX,
> +	DRM_ATTR_MAX = __DRM_ATTR_MAX - 1,
> +};
> +
> +/* attribute policies */
> +static const struct nla_policy drm_attr_policy_query[DRM_ATTR_MAX + 1] = {
> +	[DRM_ATTR_REQUEST] = { .type = NLA_U8 },
> +};

Should these policies structures be in uapi?

> +
> +static const struct nla_policy drm_attr_policy_read_one[DRM_ATTR_MAX + 1] = {
> +	[DRM_ATTR_ERROR_ID] = { .type = NLA_U64 },
> +};

I might miss something here, but why it is not a single policy structure 
with entries for DRM_ATTR_REQUEST and DRM_ATTR_ERROR_ID?

Thanks,
Tomer

> +
> +#endif