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@xxxxxxxxx>
>>> ---
>>> 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?
Thanks,
Tomer
>> Thanks,
>> Tomer
>>
>>> +
>>> +#endif
>>
