netdev - Re: [PATCH net-next 1/6] devlink: extend devlink

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <fa6e7d19-e18a-4146-983b-63642c2bf8c0@intel.com>
Date: Wed, 24 Apr 2024 11:20:49 +0200
From: Przemek Kitszel <przemyslaw.kitszel@...el.com>
To: Alexander Lobakin <aleksander.lobakin@...el.com>, Jiri Pirko
	<jiri@...dia.com>, Mateusz Polchlopek <mateusz.polchlopek@...el.com>
CC: Tony Nguyen <anthony.l.nguyen@...el.com>, <davem@...emloft.net>,
	<kuba@...nel.org>, <pabeni@...hat.com>, <edumazet@...gle.com>,
	<netdev@...r.kernel.org>
Subject: Re: [PATCH net-next 1/6] devlink: extend devlink_param *set pointer

On 4/24/24 11:05, Alexander Lobakin wrote:
> From: Tony Nguyen <anthony.l.nguyen@...el.com>
> Date: Mon, 22 Apr 2024 13:39:06 -0700
> 
>> From: Mateusz Polchlopek <mateusz.polchlopek@...el.com>
>>
>> Extend devlink_param *set function pointer to take extack as a param.
>> Sometimes it is needed to pass information to the end user from set
>> function. It is more proper to use for that netlink instead of passing
>> message to dmesg.
>>
>> Reviewed-by: Jiri Pirko <jiri@...dia.com>
>> Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@...el.com>
>> Signed-off-by: Mateusz Polchlopek <mateusz.polchlopek@...el.com>
>> Signed-off-by: Tony Nguyen <anthony.l.nguyen@...el.com>
> 
> [...]
> 
>> diff --git a/include/net/devlink.h b/include/net/devlink.h
>> index d31769a116ce..35eb0f884386 100644
>> --- a/include/net/devlink.h
>> +++ b/include/net/devlink.h
>> @@ -483,7 +483,8 @@ struct devlink_param {
>>   	int (*get)(struct devlink *devlink, u32 id,
>>   		   struct devlink_param_gset_ctx *ctx);
>>   	int (*set)(struct devlink *devlink, u32 id,
>> -		   struct devlink_param_gset_ctx *ctx);
>> +		   struct devlink_param_gset_ctx *ctx,
>> +		   struct netlink_ext_ack *extack);
> 
> Sorry for the late comment. Can't we embed extack to
> devlink_param_gset_ctx instead? It would take much less lines.

But then we will want to remove the extack param from .validate() too:

> 
>>   	int (*validate)(struct devlink *devlink, u32 id,
>>   			union devlink_param_value val,
>>   			struct netlink_ext_ack *extack);

right there.
This would amount to roughly the same scope for changes, but would spare
us yet another round when someone would like to extend .get(), so I like
this idea.

>> diff --git a/include/net/dsa.h b/include/net/dsa.h
>> index 7edfd8de8882..a6ef7e4c503f 100644
>> --- a/include/net/dsa.h
>> +++ b/include/net/dsa.h
>> @@ -1258,7 +1258,8 @@ struct dsa_switch_ops {
>>   int dsa_devlink_param_get(struct devlink *dl, u32 id,
>>   			  struct devlink_param_gset_ctx *ctx);
>>   int dsa_devlink_param_set(struct devlink *dl, u32 id,
>> -			  struct devlink_param_gset_ctx *ctx);
>> +			  struct devlink_param_gset_ctx *ctx,
>> +			  struct netlink_ext_ack *extack);
>>   int dsa_devlink_params_register(struct dsa_switch *ds,
>>   				const struct devlink_param *params,
>>   				size_t params_count);
>> diff --git a/net/devlink/param.c b/net/devlink/param.c
>> index 22bc3b500518..dcf0d1ccebba 100644
>> --- a/net/devlink/param.c
>> +++ b/net/devlink/param.c
>> @@ -158,11 +158,12 @@ static int devlink_param_get(struct devlink *devlink,
>>   
>>   static int devlink_param_set(struct devlink *devlink,
>>   			     const struct devlink_param *param,
>> -			     struct devlink_param_gset_ctx *ctx)
>> +			     struct devlink_param_gset_ctx *ctx,
>> +			     struct netlink_ext_ack *extack)
>>   {
>>   	if (!param->set)
>>   		return -EOPNOTSUPP;
>> -	return param->set(devlink, param->id, ctx);
>> +	return param->set(devlink, param->id, ctx, extack);
>>   }
>>   
>>   static int
>> @@ -571,7 +572,7 @@ static int __devlink_nl_cmd_param_set_doit(struct devlink *devlink,
>>   			return -EOPNOTSUPP;
>>   		ctx.val = value;
>>   		ctx.cmode = cmode;
>> -		err = devlink_param_set(devlink, param, &ctx);
>> +		err = devlink_param_set(devlink, param, &ctx, info->extack);
>>   		if (err)
>>   			return err;
>>   	}
>> diff --git a/net/dsa/devlink.c b/net/dsa/devlink.c
>> index 431bf52290a1..0aac887d0098 100644
>> --- a/net/dsa/devlink.c
>> +++ b/net/dsa/devlink.c
>> @@ -194,7 +194,8 @@ int dsa_devlink_param_get(struct devlink *dl, u32 id,
>>   EXPORT_SYMBOL_GPL(dsa_devlink_param_get);
>>   
>>   int dsa_devlink_param_set(struct devlink *dl, u32 id,
>> -			  struct devlink_param_gset_ctx *ctx)
>> +			  struct devlink_param_gset_ctx *ctx,
>> +			  struct netlink_ext_ack *extack)
>>   {
>>   	struct dsa_switch *ds = dsa_devlink_to_ds(dl);
> 
> Thanks,
> Olek