dri-devel.lists.freedesktop.org archive mirror
 help / color / mirror / Atom feed
From: "Iddamsetty, Aravind" <aravind.iddamsetty@intel.com>
To: Tomer Tayar <ttayar@habana.ai>,
	"intel-xe@lists.freedesktop.org" <intel-xe@lists.freedesktop.org>,
	"dri-devel@lists.freedesktop.org"
	<dri-devel@lists.freedesktop.org>
Cc: "alexander.deucher@amd.com" <alexander.deucher@amd.com>,
	Oded Gabbay <ogabbay@kernel.org>
Subject: Re: [Intel-xe] [RFC 1/5] drm/netlink: Add netlink infrastructure
Date: Wed, 21 Jun 2023 12:10:05 +0530	[thread overview]
Message-ID: <d9ce8d28-a116-eef5-aa4a-7dbd09707079@intel.com> (raw)
In-Reply-To: <0de909c4-fa12-378f-62a6-ec0b85417ba3@habana.ai>



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@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
>>>
> 

  reply	other threads:[~2023-06-21  6:40 UTC|newest]

Thread overview: 18+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-05-26 16:20 [RFC 0/5] Proposal to use netlink for RAS and Telemetry across drm subsystem Aravind Iddamsetty
2023-05-26 16:20 ` [RFC 1/5] drm/netlink: Add netlink infrastructure Aravind Iddamsetty
2023-06-04 17:07   ` [Intel-xe] " Tomer Tayar
2023-06-05 17:18     ` Iddamsetty, Aravind
2023-06-06 14:04       ` Tomer Tayar
2023-06-21  6:40         ` Iddamsetty, Aravind [this message]
2023-05-26 16:20 ` [RFC 2/5] drm/xe/RAS: Register a genl netlink family Aravind Iddamsetty
2023-06-04 17:09   ` [Intel-xe] " Tomer Tayar
2023-06-05 17:21     ` Iddamsetty, Aravind
2023-05-26 16:20 ` [RFC 3/5] drm/xe/RAS: Expose the error counters Aravind Iddamsetty
2023-05-26 16:20 ` [RFC 4/5] drm/netlink: define multicast groups Aravind Iddamsetty
2023-05-26 16:20 ` [RFC 5/5] drm/xe/RAS: send multicast event on occurrence of an error Aravind Iddamsetty
2023-06-04 17:07 ` [Intel-xe] [RFC 0/5] Proposal to use netlink for RAS and Telemetry across drm subsystem Tomer Tayar
2023-06-05 17:17   ` Iddamsetty, Aravind
2023-06-05 16:47 ` Alex Deucher
2023-06-06 11:56   ` Iddamsetty, Aravind
2023-06-21 17:24 ` Sebastian Wick
2023-07-17 12:02   ` Oded Gabbay

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=d9ce8d28-a116-eef5-aa4a-7dbd09707079@intel.com \
    --to=aravind.iddamsetty@intel.com \
    --cc=alexander.deucher@amd.com \
    --cc=dri-devel@lists.freedesktop.org \
    --cc=intel-xe@lists.freedesktop.org \
    --cc=ogabbay@kernel.org \
    --cc=ttayar@habana.ai \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).