From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 5674D3A6EFB for ; Tue, 7 Apr 2026 09:58:19 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=170.10.129.124 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1775555901; cv=none; b=D6G+Us4O6ZM3NIl1dBCXvE6OetKbC/K2Gg/NkjcMOWxIp4cGY4LS6BPimuv6vzqlkMkukZsDfM+JdkKC75UT70yYJYPhfw14GM804tv1H3CprCc5hwbAYEHXF+aGWWLlxkctMI/NynAE39iihFXze1f6QEtKPkFob2U+z7E4bRA= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1775555901; c=relaxed/simple; bh=wEAMNlkQCnRjouKyWtNgJ7sl+TnyCZUrhRB11EHBhnE=; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From: In-Reply-To:Content-Type; b=eVXj97mNUg8gn+fczgvK/ETtDFcyPwJB00BtX6VYbIo7uCyTasGBhDFSjaVdB9B2djWF9wdxx4hkmRs+tIfnDNANlVSImWaXlF/EVWz1hL//G95SqVSi+MXkl6kCW4xGdH54PiS75IwA9X9wc8E4Q0K7TLUiGhkJvUtC5uTxzAI= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=quarantine dis=none) header.from=redhat.com; spf=pass smtp.mailfrom=redhat.com; dkim=pass (1024-bit key) header.d=redhat.com header.i=@redhat.com header.b=jHDa1eC/; dkim=pass (2048-bit key) header.d=redhat.com header.i=@redhat.com header.b=O9+bc5Vk; arc=none smtp.client-ip=170.10.129.124 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=quarantine dis=none) header.from=redhat.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=redhat.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (1024-bit key) header.d=redhat.com header.i=@redhat.com header.b="jHDa1eC/"; dkim=pass (2048-bit key) header.d=redhat.com header.i=@redhat.com header.b="O9+bc5Vk" DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1775555898; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=uLREW3RimYXp6pMWCsCdtY17DI2aIBPhZJcOYp3npp0=; b=jHDa1eC/J3ZsZohkr6pmuDBMoYzbhe/8W4SHWUQ9kA154uiVOy2+EUAU98xLgBJo84kxml xxb+j1DBq1ZOZotFgHXsIlTTDfsetCRyyuGYqglcikpPL60NwwqWk83KH1yWPSfRsb15R3 pSpIRMZv3L29fOeEp+4fXFYd1vP3hVs= Received: from mail-wr1-f69.google.com (mail-wr1-f69.google.com [209.85.221.69]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-371-3NFMO95rPGKie7ZGfjr3-A-1; Tue, 07 Apr 2026 05:58:15 -0400 X-MC-Unique: 3NFMO95rPGKie7ZGfjr3-A-1 X-Mimecast-MFC-AGG-ID: 3NFMO95rPGKie7ZGfjr3-A_1775555894 Received: by mail-wr1-f69.google.com with SMTP id ffacd0b85a97d-43cf5b4dac8so4939174f8f.0 for ; Tue, 07 Apr 2026 02:58:14 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=google; t=1775555894; x=1776160694; darn=vger.kernel.org; h=content-transfer-encoding:in-reply-to:from:content-language :references:cc:to:subject:user-agent:mime-version:date:message-id :from:to:cc:subject:date:message-id:reply-to; bh=uLREW3RimYXp6pMWCsCdtY17DI2aIBPhZJcOYp3npp0=; b=O9+bc5VkAeIjaquLbEG7G3BsVTWLtCoXOv7g3Icp4rmDUfoR/0Jke6bmpHGAgUnoSz 44g2XlTdi6MstQXTdi96wAMgYmrpfs7wPUINwvJpXE2M3QFP7CqaUVw8KYbKHVOKfXc0 S/b6k+w9Ujfo4mkuG3YcP9IOqrvXS3fFoEXW1dkaN82ToJNdjavzsZtkRogC39PCWdUu LqQOUTNTTACWBHVILxquQLcxwS61ATUqRkdQkYeJiQYpqSKoPIIhIWgyzm2SySTG4+Cs eQqQHR++blmrXeP9vGPC6/tMSxK+rQxPwyB1F3br3O3MQvl2O0O/jGo0bHp2po8Q5Svz fC2g== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20251104; t=1775555894; x=1776160694; h=content-transfer-encoding:in-reply-to:from:content-language :references:cc:to:subject:user-agent:mime-version:date:message-id :x-gm-gg:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=uLREW3RimYXp6pMWCsCdtY17DI2aIBPhZJcOYp3npp0=; b=pLnf7oqiPIuRK4CYuCVG+JhPv8BdOHOx/NLC/aplTdBLNjNUwO5ZRxZ0WUN0rJLFS1 cuWtz5lAWBDXuEg3eKqMD+MkVXJ3LMi57BrUgYPnytTIOlFOfyfjWNTstNTBQR/8wsBl npwlOVXzPJuMbNAjgCWf9/cXzjkvkI+sZfOidznAxSGSc89NUbQO341cmeRkp7Tdu1Ki NntvXtDdqdpdV9/QSxSt5rM6m0ioe9SvNf7nXbACN1ta9YBRwXj6vhVFhtMcM3IJnEQ6 RPyISRf12LIsgwmCqIWVgXApX3q4ApTdEqxWmx0XPx3TRdrcPUeQqQOmpt6HQl81y0rw Bi5Q== X-Forwarded-Encrypted: i=1; AJvYcCVSxDe3i0c1v3M3DEy2UkyY9auXibb/zPYytNoA6XBuJ2zmhwGkIUeubKqae3aa4TgqBWJrq+f248ATDIU=@vger.kernel.org X-Gm-Message-State: AOJu0YxBgMLrHiZ6vBLaWJIvxEbXDK2duLGWBwuuNWbys/YTFAdmqa5L NtjpvSWSdYJ9zUVvg+Qga6ZI2UomjM7zv8XpxuFVg8jLXZQvqKsQ21aguiI+Z+Nyw+EkRl59ZEj YqxCH+E98IUxyXonnFP0brNLFxqrSYyVq7hFE649Q6YKLZH3I6edOEJktcWdl+3XMOw== X-Gm-Gg: AeBDietW2cBMIe9MMbqmb/+ldBHNiaFvcM/nfnnOMxei2TIovJZk0h6TFFRQBu+Xyuv zArwNHvlVJRTqsZi7dRugOxKbQLI/04rjoqp0+tQmTJAUf51vbDJVyFEj7qy19Kyo1lVPqCNYVf Y3GemM1jqU+6r8shdYaE36q6ScTSWfuuvI2bIjmDdxF0NMsLROy9kqEAzviOaO9ZQlAEd1Y1fI5 6WngtVyNkGmyjDX6q44CpvnV9j+WVLkOxUwjDfJSTYW8+hWU8hZsOuNP3cdA+9S+44x+Qr4tkjt r/M7QfuT6Po4INVeA247Nln/qqpoqJJWpe4F5FI9xutgJJSKsa+uDMXcsTn6OEHTdZw418yIalD rvSVZLkRjW6W3qzQXNjzrIBeiBCQnZxUbcrgbcom9gyBb0Rt8tnz6hbzCfw== X-Received: by 2002:a05:6000:230c:b0:43d:2581:3053 with SMTP id ffacd0b85a97d-43d292ff9c6mr22013874f8f.45.1775555893741; Tue, 07 Apr 2026 02:58:13 -0700 (PDT) X-Received: by 2002:a05:6000:230c:b0:43d:2581:3053 with SMTP id ffacd0b85a97d-43d292ff9c6mr22013794f8f.45.1775555893076; Tue, 07 Apr 2026 02:58:13 -0700 (PDT) Received: from [192.168.88.32] ([212.105.153.231]) by smtp.gmail.com with ESMTPSA id ffacd0b85a97d-43d1e4f843dsm46955754f8f.37.2026.04.07.02.58.11 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Tue, 07 Apr 2026 02:58:12 -0700 (PDT) Message-ID: Date: Tue, 7 Apr 2026 11:58:09 +0200 Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v10 net-next 3/6] devlink: Implement devlink param multi attribute nested data values To: Ratheesh Kannoth , netdev@vger.kernel.org, linux-kernel@vger.kernel.org, linux-rdma@vger.kernel.org Cc: sgoutham@marvell.com, andrew+netdev@lunn.ch, davem@davemloft.net, edumazet@google.com, kuba@kernel.org, donald.hunter@gmail.com, horms@kernel.org, jiri@resnulli.us, chuck.lever@oracle.com, matttbe@kernel.org, cjubran@nvidia.com, saeedm@nvidia.com, leon@kernel.org, tariqt@nvidia.com, mbloch@nvidia.com, dtatulea@nvidia.com References: <20260403025533.6250-1-rkannoth@marvell.com> <20260403025533.6250-4-rkannoth@marvell.com> Content-Language: en-US From: Paolo Abeni In-Reply-To: <20260403025533.6250-4-rkannoth@marvell.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit On 4/3/26 4:55 AM, Ratheesh Kannoth wrote: > From: Saeed Mahameed > > Devlink param value attribute is not defined since devlink is handling > the value validating and parsing internally, this allows us to implement > multi attribute values without breaking any policies. > > Devlink param multi-attribute values are considered to be dynamically > sized arrays of u64 values, by introducing a new devlink param type > DEVLINK_PARAM_TYPE_U64_ARRAY, driver and user space can set a variable > count of u32 values into the DEVLINK_ATTR_PARAM_VALUE_DATA attribute. > > Implement get/set parsing and add to the internal value structure passed > to drivers. > > This is useful for devices that need to configure a list of values for > a specific configuration. > > example: > $ devlink dev param show pci/... name multi-value-param > name multi-value-param type driver-specific > values: > cmode permanent value: 0,1,2,3,4,5,6,7 > > $ devlink dev param set pci/... name multi-value-param \ > value 4,5,6,7,0,1,2,3 cmode permanent > > Signed-off-by: Saeed Mahameed > Signed-off-by: Ratheesh Kannoth > --- > Documentation/netlink/specs/devlink.yaml | 4 ++ > include/net/devlink.h | 8 +++ > include/uapi/linux/devlink.h | 1 + > net/devlink/netlink_gen.c | 2 + > net/devlink/param.c | 91 +++++++++++++++++++----- > 5 files changed, 89 insertions(+), 17 deletions(-) > > diff --git a/Documentation/netlink/specs/devlink.yaml b/Documentation/netlink/specs/devlink.yaml > index b495d56b9137..b619de4fe08a 100644 > --- a/Documentation/netlink/specs/devlink.yaml > +++ b/Documentation/netlink/specs/devlink.yaml > @@ -226,6 +226,10 @@ definitions: > value: 10 > - > name: binary > + - > + name: u64-array > + value: 129 > + > - > name: rate-tc-index-max > type: const > diff --git a/include/net/devlink.h b/include/net/devlink.h > index 3038af6ec017..3a355fea8189 100644 > --- a/include/net/devlink.h > +++ b/include/net/devlink.h > @@ -432,6 +432,13 @@ enum devlink_param_type { > DEVLINK_PARAM_TYPE_U64 = DEVLINK_VAR_ATTR_TYPE_U64, > DEVLINK_PARAM_TYPE_STRING = DEVLINK_VAR_ATTR_TYPE_STRING, > DEVLINK_PARAM_TYPE_BOOL = DEVLINK_VAR_ATTR_TYPE_FLAG, > + DEVLINK_PARAM_TYPE_U64_ARRAY = DEVLINK_VAR_ATTR_TYPE_U64_ARRAY, > +}; > + > +#define __DEVLINK_PARAM_MAX_ARRAY_SIZE 32 > +struct devlink_param_u64_array { > + u64 size; > + u64 val[__DEVLINK_PARAM_MAX_ARRAY_SIZE]; > }; > > union devlink_param_value { > @@ -441,6 +448,7 @@ union devlink_param_value { > u64 vu64; > char vstr[__DEVLINK_PARAM_MAX_STRING_VALUE]; > bool vbool; > + struct devlink_param_u64_array u64arr; Sashiko as a couple of relevant remarks here, specifically: --- Does this increase the size of union devlink_param_value from 32 bytes to over 264 bytes? Looking at existing functions like devlink_nl_param_value_fill_one() and devlink_nl_param_value_put(), they take multiple copies of this union by value. Passing two of these unions by value consumes over 528 bytes of stack space, and combined in a call chain this pushes nearly 800 bytes of arguments onto the stack. Could this create a risk of hitting CONFIG_FRAME_WARN limits deep in driver notification contexts? Should the signatures of the internal functions and exported APIs be updated to pass the unions by pointer instead? --- > }; > > struct devlink_param_gset_ctx { > diff --git a/include/uapi/linux/devlink.h b/include/uapi/linux/devlink.h > index 7de2d8cc862f..5332223dd6d0 100644 > --- a/include/uapi/linux/devlink.h > +++ b/include/uapi/linux/devlink.h > @@ -406,6 +406,7 @@ enum devlink_var_attr_type { > DEVLINK_VAR_ATTR_TYPE_BINARY, > __DEVLINK_VAR_ATTR_TYPE_CUSTOM_BASE = 0x80, > /* Any possible custom types, unrelated to NLA_* values go below */ > + DEVLINK_VAR_ATTR_TYPE_U64_ARRAY, > }; > > enum devlink_attr { > diff --git a/net/devlink/netlink_gen.c b/net/devlink/netlink_gen.c > index eb35e80e01d1..7aaf462f27ee 100644 > --- a/net/devlink/netlink_gen.c > +++ b/net/devlink/netlink_gen.c > @@ -37,6 +37,8 @@ devlink_attr_param_type_validate(const struct nlattr *attr, > case DEVLINK_VAR_ATTR_TYPE_NUL_STRING: > fallthrough; > case DEVLINK_VAR_ATTR_TYPE_BINARY: > + fallthrough; > + case DEVLINK_VAR_ATTR_TYPE_U64_ARRAY: > return 0; > } > NL_SET_ERR_MSG_ATTR(extack, attr, "invalid enum value"); > diff --git a/net/devlink/param.c b/net/devlink/param.c > index cf95268da5b0..2ec85dffd8ac 100644 > --- a/net/devlink/param.c > +++ b/net/devlink/param.c > @@ -252,6 +252,14 @@ devlink_nl_param_value_put(struct sk_buff *msg, enum devlink_param_type type, > return -EMSGSIZE; > } > break; > + case DEVLINK_PARAM_TYPE_U64_ARRAY: > + if (val.u64arr.size > __DEVLINK_PARAM_MAX_ARRAY_SIZE) > + return -EMSGSIZE; > + > + for (int i = 0; i < val.u64arr.size; i++) > + if (nla_put_uint(msg, nla_type, val.u64arr.val[i])) > + return -EMSGSIZE; > + break; > } > return 0; > } > @@ -304,56 +312,78 @@ static int devlink_nl_param_fill(struct sk_buff *msg, struct devlink *devlink, > u32 portid, u32 seq, int flags, > struct netlink_ext_ack *extack) > { > - union devlink_param_value default_value[DEVLINK_PARAM_CMODE_MAX + 1]; > - union devlink_param_value param_value[DEVLINK_PARAM_CMODE_MAX + 1]; > bool default_value_set[DEVLINK_PARAM_CMODE_MAX + 1] = {}; > bool param_value_set[DEVLINK_PARAM_CMODE_MAX + 1] = {}; > const struct devlink_param *param = param_item->param; > - struct devlink_param_gset_ctx ctx; > + union devlink_param_value *default_value; > + union devlink_param_value *param_value; > + struct devlink_param_gset_ctx *ctx; > struct nlattr *param_values_list; > struct nlattr *param_attr; > void *hdr; > int err; > int i; > > + default_value = kcalloc(DEVLINK_PARAM_CMODE_MAX + 1, > + sizeof(*default_value), GFP_KERNEL); > + if (!default_value) > + return -ENOMEM; > + > + param_value = kcalloc(DEVLINK_PARAM_CMODE_MAX + 1, > + sizeof(*param_value), GFP_KERNEL); > + if (!param_value) { > + kfree(default_value); > + return -ENOMEM; > + } > + > + ctx = kmalloc_obj(*ctx); > + if (!ctx) { > + kfree(param_value); > + kfree(default_value); > + return -ENOMEM; > + } > + > /* Get value from driver part to driverinit configuration mode */ > for (i = 0; i <= DEVLINK_PARAM_CMODE_MAX; i++) { > if (!devlink_param_cmode_is_supported(param, i)) > continue; > if (i == DEVLINK_PARAM_CMODE_DRIVERINIT) { > - if (param_item->driverinit_value_new_valid) > + if (param_item->driverinit_value_new_valid) { > param_value[i] = param_item->driverinit_value_new; > - else if (param_item->driverinit_value_valid) > + } else if (param_item->driverinit_value_valid) { > param_value[i] = param_item->driverinit_value; > - else > - return -EOPNOTSUPP; > + } else { > + err = -EOPNOTSUPP; > + goto get_put_fail; > + } > > if (param_item->driverinit_value_valid) { > default_value[i] = param_item->driverinit_default; > default_value_set[i] = true; > } > } else { > - ctx.cmode = i; > - err = devlink_param_get(devlink, param, &ctx, extack); > + ctx->cmode = i; > + err = devlink_param_get(devlink, param, ctx, extack); > if (err) > - return err; > - param_value[i] = ctx.val; > + goto get_put_fail; > + param_value[i] = ctx->val; > > - err = devlink_param_get_default(devlink, param, &ctx, > + err = devlink_param_get_default(devlink, param, ctx, > extack); > if (!err) { > - default_value[i] = ctx.val; > + default_value[i] = ctx->val; > default_value_set[i] = true; > } else if (err != -EOPNOTSUPP) { > - return err; > + goto get_put_fail; > } > } > param_value_set[i] = true; > } > > + err = -EMSGSIZE; > hdr = genlmsg_put(msg, portid, seq, &devlink_nl_family, flags, cmd); > if (!hdr) > - return -EMSGSIZE; > + goto get_put_fail; > > if (devlink_nl_put_handle(msg, devlink)) > goto genlmsg_cancel; > @@ -393,6 +423,9 @@ static int devlink_nl_param_fill(struct sk_buff *msg, struct devlink *devlink, > nla_nest_end(msg, param_values_list); > nla_nest_end(msg, param_attr); > genlmsg_end(msg, hdr); > + kfree(default_value); > + kfree(param_value); > + kfree(ctx); > return 0; > > values_list_nest_cancel: > @@ -401,7 +434,11 @@ static int devlink_nl_param_fill(struct sk_buff *msg, struct devlink *devlink, > nla_nest_cancel(msg, param_attr); > genlmsg_cancel: > genlmsg_cancel(msg, hdr); > - return -EMSGSIZE; > +get_put_fail: > + kfree(default_value); > + kfree(param_value); > + kfree(ctx); > + return err; > } > > static void devlink_param_notify(struct devlink *devlink, > @@ -507,7 +544,7 @@ devlink_param_value_get_from_info(const struct devlink_param *param, > union devlink_param_value *value) > { > struct nlattr *param_data; > - int len; > + int len, cnt, rem; > > param_data = info->attrs[DEVLINK_ATTR_PARAM_VALUE_DATA]; > > @@ -547,6 +584,26 @@ devlink_param_value_get_from_info(const struct devlink_param *param, > return -EINVAL; > value->vbool = nla_get_flag(param_data); > break; > + > + case DEVLINK_PARAM_TYPE_U64_ARRAY: > + cnt = 0; > + nla_for_each_attr_type(param_data, > + DEVLINK_ATTR_PARAM_VALUE_DATA, > + genlmsg_data(info->genlhdr), > + genlmsg_len(info->genlhdr), rem) { > + if (cnt >= __DEVLINK_PARAM_MAX_ARRAY_SIZE) > + return -EMSGSIZE; > + > + if ((nla_len(param_data) != sizeof(u64)) && > + (nla_len(param_data) != sizeof(u32))) > + return -EINVAL; > + > + value->u64arr.val[cnt] = (u64)nla_get_uint(param_data); > + cnt++; > + } > + > + value->u64arr.size = cnt; > + break; Sashiko says: --- Does this make it impossible to set an empty array to clear a multi-value parameter? If userspace provides 0 elements, param_data will be NULL. Earlier in devlink_param_value_get_from_info(), there is a check: param_data = info->attrs[DEVLINK_ATTR_PARAM_VALUE_DATA]; if (param->type != DEVLINK_PARAM_TYPE_BOOL && !param_data) return -EINVAL; If the parameter is a U64_ARRAY and no data is provided, this check will immediately return -EINVAL. The kernel can successfully emit an empty array on a GET request if the size is 0. Should the SET path similarly support receiving 0 elements to allow userspace to clear a multi-value parameter? --- There are several others NIC-specific remarks, which IMHO are mostly pre-existing issues, but please have a look: https://sashiko.dev/#/patchset/20260403025533.6250-1-rkannoth%40marvell.com /P