Received-SPF: pass (google.com: domain of linux-kernel+bounces-137746-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Message-ID: <bf86b3f9-6f99-45e7-8246-839747205622@intel.com>
Date: Tue, 9 Apr 2024 17:11:09 -0700
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH net-next] cxgb4: flower: use NL_SET_ERR_MSG_MOD for
 validation errors
To: =?UTF-8?Q?Asbj=C3=B8rn_Sloth_T=C3=B8nnesen?= <ast@fiberby.net>, "Raju
 Rangoju" <rajur@chelsio.com>, "David S. Miller" <davem@davemloft.net>, "Eric
 Dumazet" <edumazet@google.com>, Jakub Kicinski <kuba@kernel.org>, Paolo Abeni
	<pabeni@redhat.com>
CC: <netdev@vger.kernel.org>, <linux-kernel@vger.kernel.org>
References: <20240408165506.94483-1-ast@fiberby.net>
Content-Language: en-US
From: Jacob Keller <jacob.e.keller@intel.com>
In-Reply-To: <20240408165506.94483-1-ast@fiberby.net>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 8bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?ZGNwQjJpRkhoLzVRMisvMEJpZ2dwbmg5ZHpRT1VOcHlwSnlCU0E4Z0Y0dUxM?=
 =?utf-8?B?WDNRRjIwRlp6S0FYNWlJUFplZ0g0ZTkyd1MxWnJPT3B3U3BlREpIemZGS3lv?=
 =?utf-8?B?dHc0RUVkUHZ3Wmx5M0xrQWxpOVZhcWtDTHMxNEt4T2NGUEs5S0RWcHJRSW50?=
 =?utf-8?B?QWw2MTVpZ2UzN1RTeGxBWGxOUk5NODVuTEU1aEE5R1h2UWVkeCt2ckJUdUZX?=
 =?utf-8?B?UXNkcFZHWkNxS3JjUEF1VDV6Zkx5ajMzOFNCdUR6WXlPMUxNbHBHUWVyQitl?=
 =?utf-8?B?NnlNM01XRzF3ZFpoSkpJYlN4bmZZRVliTVR2MU51bHlDY2lIZFVEZk1RNFdw?=
 =?utf-8?B?WWhDUk5ZbjQ2dE9uU1ZFdnRvaUFLS2xFZ0d1SndlZndwN2h1dkJVUFpYYW5M?=
 =?utf-8?B?WSt6c2hEeVFRc2hiU0lvdDBIdXNrZGFscDdEUWpzTXFnUGROWjhYemF1Rmsx?=
 =?utf-8?B?QTJ6V29Nbi9rNlRIODgxNnNpd0lDVjM4YzJQRzM2SjlGU0Z2Zk5LaHY4YkNv?=
 =?utf-8?B?YzlVd1M5b0tjb2JvdkpHY0VmODVXYzZRZnFrazFqdzVCYnlDaTNOU2FBandI?=
 =?utf-8?B?QzhJQ1VlZHY1WURubjRMbmVieGxnZEtBUnZpNUtKUEo0T1F0UG5vOUR1b2N3?=
 =?utf-8?B?ZjBUSXhZTDh0Um5FeUt4R3JPYkFTa3FFMTFsUEdIQzBvWDNudGJ0WjN1QllW?=
 =?utf-8?B?ZExYR2dhTTJSMTVVVnowNE5tWkYvenhCRnpRSFJsZUlpOFRJZlRpRnhFODBP?=
 =?utf-8?B?ZkhOL0V2eXJoVHd3WGJ2bUFYUzc4MlpkRnNQNXBIK2hOUFJmOGExQzBoWlFE?=
 =?utf-8?B?dzhyUmgrRVkzaXNjb1BOWkpkWEEvcFA0bXVHbFhlN2xqZTB5MlhEMm1rRXJQ?=
 =?utf-8?B?V1pGVDRTSU5RUXloSkNGMDRSZTZUUFQ3ajBuc2lFaDEvdVBsc3QwSnhDS1hW?=
 =?utf-8?B?eXoyV2M4MnNNOUIzNCtVM0llVzJmL0JpWUhrVnZma1VIVDBHVGlhQzdQNVdX?=
 =?utf-8?B?RVAzWk14Wm4weHpKQS9FVWw1Z1pnc05Lclk1TS83bTJKazFvVkZLV1ozRndv?=
 =?utf-8?B?S25UOENlZ3lmclBVQmd0OCswMWpJWHVhVm03OVl0QzZGYUo3b2JobUVCUTR1?=
 =?utf-8?B?eTFhOW5LQzdHR2x4a0QxTEZCN05WcjN5TkNMeWFJRGh3QVp2K3lkRnk4eTZB?=
 =?utf-8?B?UitWN2UwTlNIOE90eEhucjRuVUZIR0RBQTFOZ3JlZjJVUHVqd2tadjRESWxU?=
 =?utf-8?B?czhjOG5UV1g3WXRZZjdjK2EwcDVRdUU5dFYrZUNCcEUzNnVWV1QzbkJneXc4?=
 =?utf-8?B?TFdQa3dhdUU3bUMyNDIxV05GQjNyRFFHL3Z1a29jZmNiaDRqYjZEWmdkcTNV?=
 =?utf-8?B?eUxvSENOaElpMGFVclJlREJ6eVlPTXJabnB4TjdOWE5nSVVtbThRdFBxN3RB?=
 =?utf-8?B?SEdlS1lYRG9vYVRSckVjZVBoeFh4cm5Hc2V0Sk9tZ2p4aU16ME9aeXdUQ3RX?=
 =?utf-8?B?Nm5weGFsaFdUd1FxOW4rK3p0enZlVXhtaTVtMVJHWUROTHRMRzdTaTRMMTR1?=
 =?utf-8?B?b0dIcXh5OGlSOWFHOTlLT3ZNZmo1K1dJRmpHRDNYNzRkdlJZUnRRWVh5WVZH?=
 =?utf-8?B?MGtoR0VSNWx2Q3kvT1Y5SHRuYzVheTQzeHl2M2x4WG9wL2pTNkxncXBzWkU0?=
 =?utf-8?B?OGkyV1JDaWloMSsyYUQxYkJZUDJxN3VxSXphTGJoT0NwbU5QZ2JxY1BFZkxO?=
 =?utf-8?B?VDdGTEg3Y3BHYWFyL0dBR0lJYStndE9rUUQ3VG1xcjVSTWxHVHphdll5L0lt?=
 =?utf-8?B?Y3RMckdpV0tBWmV6dE5VbGhmVUo1a3licDVlSlVRSHk2ekt6ZU9TODVMVWhR?=
 =?utf-8?B?ZFR4MktCcTRRa3FzRUd1WVZHUDdoTWQwRktVK0ZpTGtoeW5nbWpVc0syU2Jz?=
 =?utf-8?B?bHVma0ZTT2kzem1lcWtiK0hGL0kzNVRnNHFUQ2NyLzRnczNOaDRPdWxuVkFQ?=
 =?utf-8?B?NmFndFppOENXek5SVllXQStwQjVvaVNta1prWkRkY2t4S1RhK3lYc2RncWt4?=
 =?utf-8?B?d0l1TzNSR09Dc3Q3MHBwbEsvdnc0WEY5eUxhNFVlTWlZNHNwbTByZXFmVWVu?=
 =?utf-8?B?RjJPQW1RRWZWTW1zdmZNajBRZjM3SmlTaDc1bkJwR016ZEZ6cEVkUE9MQmtW?=
 =?utf-8?B?M0E9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 020dc6ba-80cd-4230-e231-08dc58f2ba66
X-MS-Exchange-CrossTenant-AuthSource: CO1PR11MB5089.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 10 Apr 2024 00:11:11.3033
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: eyJ5zrVEaq9pDo16PDfryIykdNH/YBSiAXuEqWjkLEh2g9N+s4pD0btYXMnyAoDHafyruTX9hNw1go8Xu2Gn8tny5cdi4ATjRSuQ9eCNCZU=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: IA0PR11MB7353
X-OriginatorOrg: intel.com


On 4/8/2024 9:55 AM, Asbjørn Sloth Tønnesen wrote:
> Replace netdev_{warn,err} with NL_SET_ERR_MSG_{FMT_,}MOD
> to better inform the user about the problem.
> 
> Only compile-tested, no access to HW.
> 
> Signed-off-by: Asbjørn Sloth Tønnesen <ast@fiberby.net>
> ---

Good improvement. Its a straightforward replacement.

Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>

>  .../ethernet/chelsio/cxgb4/cxgb4_tc_flower.c  | 64 +++++++++----------
>  1 file changed, 30 insertions(+), 34 deletions(-)
> 
> diff --git a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_tc_flower.c b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_tc_flower.c
> index 72ac4a34424b9..3a6987cafe590 100644
> --- a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_tc_flower.c
> +++ b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_tc_flower.c
> @@ -305,7 +305,7 @@ static void cxgb4_process_flow_match(struct net_device *dev,
>  	fs->mask.iport = ~0;
>  }
>  
> -static int cxgb4_validate_flow_match(struct net_device *dev,
> +static int cxgb4_validate_flow_match(struct netlink_ext_ack *extack,
>  				     struct flow_rule *rule)
>  {
>  	struct flow_dissector *dissector = rule->match.dissector;
> @@ -321,8 +321,9 @@ static int cxgb4_validate_flow_match(struct net_device *dev,
>  	      BIT_ULL(FLOW_DISSECTOR_KEY_ENC_KEYID) |
>  	      BIT_ULL(FLOW_DISSECTOR_KEY_VLAN) |
>  	      BIT_ULL(FLOW_DISSECTOR_KEY_IP))) {
> -		netdev_warn(dev, "Unsupported key used: 0x%llx\n",
> -			    dissector->used_keys);
> +		NL_SET_ERR_MSG_FMT_MOD(extack,
> +				       "Unsupported key used: 0x%llx",
> +				       dissector->used_keys);
>  		return -EOPNOTSUPP;
>  	}
>  
> @@ -339,13 +340,15 @@ static int cxgb4_validate_flow_match(struct net_device *dev,
>  		struct flow_match_ip match;
>  
>  		if (eth_ip_type != ETH_P_IP && eth_ip_type != ETH_P_IPV6) {
> -			netdev_err(dev, "IP Key supported only with IPv4/v6");
> +			NL_SET_ERR_MSG_MOD(extack,
> +					   "IP Key supported only with IPv4/v6");
>  			return -EINVAL;
>  		}
>  
>  		flow_rule_match_ip(rule, &match);
>  		if (match.mask->ttl) {
> -			netdev_warn(dev, "ttl match unsupported for offload");
> +			NL_SET_ERR_MSG_MOD(extack,
> +					   "ttl match unsupported for offload");
>  			return -EOPNOTSUPP;
>  		}
>  	}
> @@ -576,7 +579,7 @@ static bool valid_l4_mask(u32 mask)
>  	return hi && lo ? false : true;
>  }
>  
> -static bool valid_pedit_action(struct net_device *dev,
> +static bool valid_pedit_action(struct netlink_ext_ack *extack,
>  			       const struct flow_action_entry *act,
>  			       u8 *natmode_flags)
>  {
> @@ -595,8 +598,7 @@ static bool valid_pedit_action(struct net_device *dev,
>  		case PEDIT_ETH_SMAC_47_16:
>  			break;
>  		default:
> -			netdev_err(dev, "%s: Unsupported pedit field\n",
> -				   __func__);
> +			NL_SET_ERR_MSG_MOD(extack, "Unsupported pedit field");
>  			return false;
>  		}
>  		break;
> @@ -609,8 +611,7 @@ static bool valid_pedit_action(struct net_device *dev,
>  			*natmode_flags |= CXGB4_ACTION_NATMODE_DIP;
>  			break;
>  		default:
> -			netdev_err(dev, "%s: Unsupported pedit field\n",
> -				   __func__);
> +			NL_SET_ERR_MSG_MOD(extack, "Unsupported pedit field");
>  			return false;
>  		}
>  		break;
> @@ -629,8 +630,7 @@ static bool valid_pedit_action(struct net_device *dev,
>  			*natmode_flags |= CXGB4_ACTION_NATMODE_DIP;
>  			break;
>  		default:
> -			netdev_err(dev, "%s: Unsupported pedit field\n",
> -				   __func__);
> +			NL_SET_ERR_MSG_MOD(extack, "Unsupported pedit field");
>  			return false;
>  		}
>  		break;
> @@ -638,8 +638,8 @@ static bool valid_pedit_action(struct net_device *dev,
>  		switch (offset) {
>  		case PEDIT_TCP_SPORT_DPORT:
>  			if (!valid_l4_mask(~mask)) {
> -				netdev_err(dev, "%s: Unsupported mask for TCP L4 ports\n",
> -					   __func__);
> +				NL_SET_ERR_MSG_MOD(extack,
> +						   "Unsupported mask for TCP L4 ports");
>  				return false;
>  			}
>  			if (~mask & PEDIT_TCP_UDP_SPORT_MASK)
> @@ -648,8 +648,7 @@ static bool valid_pedit_action(struct net_device *dev,
>  				*natmode_flags |= CXGB4_ACTION_NATMODE_DPORT;
>  			break;
>  		default:
> -			netdev_err(dev, "%s: Unsupported pedit field\n",
> -				   __func__);
> +			NL_SET_ERR_MSG_MOD(extack, "Unsupported pedit field");
>  			return false;
>  		}
>  		break;
> @@ -657,8 +656,8 @@ static bool valid_pedit_action(struct net_device *dev,
>  		switch (offset) {
>  		case PEDIT_UDP_SPORT_DPORT:
>  			if (!valid_l4_mask(~mask)) {
> -				netdev_err(dev, "%s: Unsupported mask for UDP L4 ports\n",
> -					   __func__);
> +				NL_SET_ERR_MSG_MOD(extack,
> +						   "Unsupported mask for UDP L4 ports");
>  				return false;
>  			}
>  			if (~mask & PEDIT_TCP_UDP_SPORT_MASK)
> @@ -667,13 +666,12 @@ static bool valid_pedit_action(struct net_device *dev,
>  				*natmode_flags |= CXGB4_ACTION_NATMODE_DPORT;
>  			break;
>  		default:
> -			netdev_err(dev, "%s: Unsupported pedit field\n",
> -				   __func__);
> +			NL_SET_ERR_MSG_MOD(extack, "Unsupported pedit field");
>  			return false;
>  		}
>  		break;
>  	default:
> -		netdev_err(dev, "%s: Unsupported pedit type\n", __func__);
> +		NL_SET_ERR_MSG_MOD(extack, "Unsupported pedit type");
>  		return false;
>  	}
>  	return true;
> @@ -727,8 +725,7 @@ int cxgb4_validate_flow_actions(struct net_device *dev,
>  			 * the provided output port is not valid
>  			 */
>  			if (!found) {
> -				netdev_err(dev, "%s: Out port invalid\n",
> -					   __func__);
> +				NL_SET_ERR_MSG_MOD(extack, "Out port invalid");
>  				return -EINVAL;
>  			}
>  			act_redir = true;
> @@ -745,21 +742,21 @@ int cxgb4_validate_flow_actions(struct net_device *dev,
>  			case FLOW_ACTION_VLAN_PUSH:
>  			case FLOW_ACTION_VLAN_MANGLE:
>  				if (proto != ETH_P_8021Q) {
> -					netdev_err(dev, "%s: Unsupported vlan proto\n",
> -						   __func__);
> +					NL_SET_ERR_MSG_MOD(extack,
> +							   "Unsupported vlan proto");
>  					return -EOPNOTSUPP;
>  				}
>  				break;
>  			default:
> -				netdev_err(dev, "%s: Unsupported vlan action\n",
> -					   __func__);
> +				NL_SET_ERR_MSG_MOD(extack,
> +						   "Unsupported vlan action");
>  				return -EOPNOTSUPP;
>  			}
>  			act_vlan = true;
>  			}
>  			break;
>  		case FLOW_ACTION_MANGLE: {
> -			bool pedit_valid = valid_pedit_action(dev, act,
> +			bool pedit_valid = valid_pedit_action(extack, act,
>  							      &natmode_flags);
>  
>  			if (!pedit_valid)
> @@ -771,14 +768,14 @@ int cxgb4_validate_flow_actions(struct net_device *dev,
>  			/* Do nothing. cxgb4_set_filter will validate */
>  			break;
>  		default:
> -			netdev_err(dev, "%s: Unsupported action\n", __func__);
> +			NL_SET_ERR_MSG_MOD(extack, "Unsupported action");
>  			return -EOPNOTSUPP;
>  		}
>  	}
>  
>  	if ((act_pedit || act_vlan) && !act_redir) {
> -		netdev_err(dev, "%s: pedit/vlan rewrite invalid without egress redirect\n",
> -			   __func__);
> +		NL_SET_ERR_MSG_MOD(extack,
> +				   "pedit/vlan rewrite invalid without egress redirect");
>  		return -EINVAL;
>  	}
>  
> @@ -864,7 +861,7 @@ int cxgb4_flow_rule_replace(struct net_device *dev, struct flow_rule *rule,
>  	if (cxgb4_validate_flow_actions(dev, &rule->action, extack, 0))
>  		return -EOPNOTSUPP;
>  
> -	if (cxgb4_validate_flow_match(dev, rule))
> +	if (cxgb4_validate_flow_match(extack, rule))
>  		return -EOPNOTSUPP;
>  
>  	cxgb4_process_flow_match(dev, rule, fs);
> @@ -901,8 +898,7 @@ int cxgb4_flow_rule_replace(struct net_device *dev, struct flow_rule *rule,
>  	init_completion(&ctx.completion);
>  	ret = __cxgb4_set_filter(dev, fidx, fs, &ctx);
>  	if (ret) {
> -		netdev_err(dev, "%s: filter creation err %d\n",
> -			   __func__, ret);
> +		NL_SET_ERR_MSG_FMT_MOD(extack, "filter creation err %d", ret);
>  		return ret;
>  	}
>