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

[Iddamsetty, Aravind]

On 06-06-2023 19:34, Tomer Tayar wrote:
> On 05/06/2023 20:18, Iddamsetty, Aravind wrote:
>>
>> On 04-06-2023 22:37, Tomer Tayar wrote:
>>> 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?
>> no they are not, I later came to know that we should not have any of
>> that user can't use so will split the header into 2.
>>> Also, should we add here "#if defined(__cplusplus) extern "C" { ..."?
>> ya will add that
>>>> +
>>>> +#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?
>> so ya these will also likely move into a separate drm header as
>> userspace would define there own policy.
>>>> +
>>>> +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?
>> so each command can have it's own policy defined, i.e what attributes it
>> expects we could define only those, that way we can have a check as
>> well. So, in the present implementation DRM_CMD_QUERY and
>> DRM_CMD_READ_ALL expect only DRM_ATTR_REQUEST and while DRM_CMD_READ_ONE
>> expects only DRM_ATTR_ERROR_ID as part of the incoming message from user.
>>
>> Thanks,
>> Aravind.
> 
> But "struct genl_ops" expects a pointer to "struct nla_policy", and in 
> the definition of "xe_genl_ops", each entry is set with a pointer to 
> these arrays of "struct nla_policy".
> Won't they use the first entry (DRM_ATTR_UNSPEC) of the arrays? 
> Shouldn't we set use there the arrays at indices DRM_ATTR_REQUEST and 
> DRM_ATTR_ERROR_ID?
> If yes, then can't we have a single policy array, each entry defines a 
> policy per attribute, and we will use the suitable policy entry for each 
> command?
Hi Tomer,

apologies for my late reply.

a command can accept more than one attribute. so the genl netlink core
would validate the each attributes passed in the recv message by
checking with the policy array in CMD definition.

Thanks,
Aravind.


> 
> Thanks,
> Tomer
> 
>>> Thanks,
>>> Tomer
>>>
>>>> +
>>>> +#endif
>>>
>