From mboxrd@z Thu Jan 1 00:00:00 1970 From: David Ahern Subject: Re: [patch net-next RFC v2 02/11] devlink: Add support for resource abstraction Date: Sun, 19 Nov 2017 08:47:51 -0700 Message-ID: <6f49c38b-8f3d-360e-bd58-d75bfc791e50@cumulusnetworks.com> References: <20171114161852.6633-1-jiri@resnulli.us> <20171114161852.6633-3-jiri@resnulli.us> <71d6e7e9-8bb4-0877-798b-a03afc6b5348@mellanox.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Cc: davem@davemloft.net, mlxsw@mellanox.com, andrew@lunn.ch, vivien.didelot@savoirfairelinux.com, f.fainelli@gmail.com, michael.chan@broadcom.com, ganeshgr@chelsio.com, saeedm@mellanox.com, matanb@mellanox.com, leonro@mellanox.com, idosch@mellanox.com, jakub.kicinski@netronome.com, ast@kernel.org, daniel@iogearbox.net, simon.horman@netronome.com, pieter.jansenvanvuuren@netronome.com, john.hurley@netronome.com, alexander.h.duyck@intel.com, linville@tuxdriver.com, gospo@broadcom.com, steven.lin1@broadcom.com, yuvalm@mellanox.com, ogerlitz@mellanox.com, roopa@cumulusnetworks.com To: Arkadi Sharshevsky , Jiri Pirko , netdev@vger.kernel.org Return-path: Received: from mail-pf0-f195.google.com ([209.85.192.195]:35543 "EHLO mail-pf0-f195.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750815AbdKSPr6 (ORCPT ); Sun, 19 Nov 2017 10:47:58 -0500 Received: by mail-pf0-f195.google.com with SMTP id r88so1487832pfi.2 for ; Sun, 19 Nov 2017 07:47:58 -0800 (PST) In-Reply-To: <71d6e7e9-8bb4-0877-798b-a03afc6b5348@mellanox.com> Content-Language: en-US Sender: netdev-owner@vger.kernel.org List-ID: On 11/19/17 1:17 AM, Arkadi Sharshevsky wrote: > > > On 11/18/2017 08:34 PM, David Ahern wrote: >> On 11/14/17 9:18 AM, Jiri Pirko wrote: >>> diff --git a/include/net/devlink.h b/include/net/devlink.h >>> index 4d2c6fc..960e80a 100644 >>> --- a/include/net/devlink.h >>> +++ b/include/net/devlink.h >> ... >> >>> @@ -469,6 +523,32 @@ devlink_dpipe_match_put(struct sk_buff *skb, >>> return 0; >>> } >>> >>> +static inline int >>> +devlink_resource_register(struct devlink *devlink, >>> + const char *resource_name, >>> + bool top_hierarchy, >>> + bool reload_required, >>> + u64 resource_size, >>> + u64 resource_id, >>> + u64 parent_resource_id, >>> + struct devlink_resource_ops *resource_ops) >>> +{ >>> + return 0; >>> +} >>> + >>> +static inline void >>> +devlink_resources_unregister(struct devlink *devlink, >>> + struct devlink_resource *resource) >>> +{ >>> +} >>> + >>> +static inline int >>> +devlink_resource_size_get(struct devlink *devlink, u64 resource_id, >>> + u64 *p_resource_size) >>> +{ >>> + return -EINVAL; >> >> It's compiled out so -EOPNOTSUPP seems more appropriate. >> > > will fix > >> >> >>> diff --git a/net/core/devlink.c b/net/core/devlink.c >>> index 0114dfc..6ae644f 100644 >>> --- a/net/core/devlink.c >>> +++ b/net/core/devlink.c >>> +static int devlink_nl_cmd_resource_set(struct sk_buff *skb, >>> + struct genl_info *info) >>> +{ >>> + struct devlink *devlink = info->user_ptr[0]; >>> + struct devlink_resource *resource; >>> + u64 resource_id; >>> + u64 size; >>> + int err; >>> + >>> + if (!info->attrs[DEVLINK_ATTR_RESOURCE_ID] || >>> + !info->attrs[DEVLINK_ATTR_RESOURCE_SIZE]) >>> + return -EINVAL; >> >> several of the of the DEVLINK_ATTR_RESOURCE attributes are kernel to >> user only (e.g., DEVLINK_ATTR_RESOURCE_SIZE_NEW and >> DEVLINK_ATTR_RESOURCE_RELOAD_REQUIRED), so if they are given by the user >> that should be an error too right? >> > > Not sure I understood. As you see I only check for the mandatory > attributes, if the user provides not relevant data its ignored. > > We use one single nla_policy for all the commands (devlink_nl_policy) > >> >>> + resource_id = nla_get_u64(info->attrs[DEVLINK_ATTR_RESOURCE_ID]); >> >> I don't see where these attributes are validated for proper size. >> > > right, forgot to update the policy. > >>> + >>> + resource = devlink_resource_find(devlink, NULL, resource_id); >>> + if (!resource) >>> + return -EINVAL; >>> + >>> + if (!resource->resource_ops->size_validate) >>> + return -EINVAL; >> >> genl_info has extack; please add user messages for the above failures. >> > > Isn't EOPNOTSUPP enough ? No, I mean every failure above returns EINVAL. Add an extack message telling the user what is wrong. e.g, resource = devlink_resource_find(devlink, NULL, resource_id); if (!resource) { NL_SET_ERR_MSG(extack, "Invalid resource id"); return -EINVAL; } similarly for the rest.