[RFC PATCH net-next 0/3] netlink: formatted extacks

[RFC PATCH net-next 0/3] netlink: formatted extacks

[ecree]

From: Edward Cree <ecree.xilinx@gmail.com>

Currently, netlink extacks can only carry fixed string messages, which
 is limiting when reporting failures in complex systems.  This series
 adds the ability to return printf-formatted messages, and uses it in
 the sfc driver's TC offload code.
Formatted extack messages are limited in length to a fixed buffer size,
 currently 80 characters.
There is no change to the netlink uAPI; only internal kernel changes
 are needed.

Edward Cree (3):
  netlink: add support for formatted extack messages
  sfc: use formatted extacks instead of efx_tc_err()
  sfc: remove 'log-tc-errors' ethtool private flag

 drivers/net/ethernet/sfc/ef100_ethtool.c  |  2 -
 drivers/net/ethernet/sfc/ethtool_common.c | 37 ------------------
 drivers/net/ethernet/sfc/ethtool_common.h |  2 -
 drivers/net/ethernet/sfc/mae.c            |  5 +--
 drivers/net/ethernet/sfc/net_driver.h     |  2 -
 drivers/net/ethernet/sfc/tc.c             | 47 ++++++++++-------------
 drivers/net/ethernet/sfc/tc.h             | 18 ---------
 include/linux/netlink.h                   | 21 +++++++++-
 8 files changed, 42 insertions(+), 92 deletions(-)

[RFC PATCH net-next 1/3] netlink: add support for formatted extack messages

[ecree]

From: Edward Cree <ecree.xilinx@gmail.com>

Include an 80-byte buffer in struct netlink_ext_ack that can be used
 for scnprintf()ed messages.  This does mean that the resulting string
 can't be enumerated, translated etc. in the way NL_SET_ERR_MSG() was
 designed to allow.

Signed-off-by: Edward Cree <ecree.xilinx@gmail.com>
---
 include/linux/netlink.h | 21 +++++++++++++++++++--
 1 file changed, 19 insertions(+), 2 deletions(-)

diff --git a/include/linux/netlink.h b/include/linux/netlink.h
index d51e041d2242..bfab9dbd64fa 100644
--- a/include/linux/netlink.h
+++ b/include/linux/netlink.h
@@ -64,6 +64,7 @@ netlink_kernel_create(struct net *net, int unit, struct netlink_kernel_cfg *cfg)
 
 /* this can be increased when necessary - don't expose to userland */
 #define NETLINK_MAX_COOKIE_LEN	20
+#define NETLINK_MAX_FMTMSG_LEN	80
 
 /**
  * struct netlink_ext_ack - netlink extended ACK report struct
@@ -75,6 +76,8 @@ netlink_kernel_create(struct net *net, int unit, struct netlink_kernel_cfg *cfg)
  * @miss_nest: nest missing an attribute (%NULL if missing top level attr)
  * @cookie: cookie data to return to userspace (for success)
  * @cookie_len: actual cookie data length
+ * @_msg_buf: output buffer for formatted message strings - don't access
+ *	directly, use %NL_SET_ERR_MSG_FMT
  */
 struct netlink_ext_ack {
 	const char *_msg;
@@ -84,13 +87,13 @@ struct netlink_ext_ack {
 	u16 miss_type;
 	u8 cookie[NETLINK_MAX_COOKIE_LEN];
 	u8 cookie_len;
+	char _msg_buf[NETLINK_MAX_FMTMSG_LEN];
 };
 
 /* Always use this macro, this allows later putting the
  * message into a separate section or such for things
  * like translation or listing all possible messages.
- * Currently string formatting is not supported (due
- * to the lack of an output buffer.)
+ * If string formatting is needed use NL_SET_ERR_MSG_FMT.
  */
 #define NL_SET_ERR_MSG(extack, msg) do {		\
 	static const char __msg[] = msg;		\
@@ -102,9 +105,23 @@ struct netlink_ext_ack {
 		__extack->_msg = __msg;			\
 } while (0)
 
+#define NL_SET_ERR_MSG_FMT(extack, fmt, args...) do {		\
+	struct netlink_ext_ack *__extack = (extack);		\
+								\
+	scnprintf(__extack->_msg_buf, NETLINK_MAX_FMTMSG_LEN,	\
+		  (fmt), ##args);				\
+	do_trace_netlink_extack(__extack->_msg_buf);		\
+								\
+	if (__extack)						\
+		__extack->_msg = __extack->_msg_buf;		\
+} while (0)
+
 #define NL_SET_ERR_MSG_MOD(extack, msg)			\
 	NL_SET_ERR_MSG((extack), KBUILD_MODNAME ": " msg)
 
+#define NL_SET_ERR_MSG_FMT_MOD(extack, fmt, args...)	\
+	NL_SET_ERR_MSG_FMT((extack), KBUILD_MODNAME ": " fmt, ##args)
+
 #define NL_SET_BAD_ATTR_POLICY(extack, attr, pol) do {	\
 	if ((extack)) {					\
 		(extack)->bad_attr = (attr);		\

Re: [RFC PATCH net-next 1/3] netlink: add support for formatted extack messages

[Johannes Berg]

> +#define NL_SET_ERR_MSG_FMT(extack, fmt, args...) do {		\
> +	struct netlink_ext_ack *__extack = (extack);		\
> +								\
> +	scnprintf(__extack->_msg_buf, NETLINK_MAX_FMTMSG_LEN,	\
> +		  (fmt), ##args);				\

Maybe that should print some kind of warning if the string was longer
than the buffer? OTOH, I guess the user would notice anyway, and until
you run the code nobody can possibly notice ... too bad then?

Maybe we could at least _statically_ make sure that the *format* string
(fmt) is shorter than say 60 chars or something to give some wiggle room
for the print expansion?

	/* allow 20 chars for format expansion */
	BUILD_BUG_ON(strlen(fmt) > NETLINK_MAX_FMTMSG_LEN - 20);

might even work? Just as a sanity check.

> +	do_trace_netlink_extack(__extack->_msg_buf);		\
> +								\
> +	if (__extack)						\
> +		__extack->_msg = __extack->_msg_buf;		\

That "if (__extack)" check seems a bit strange, you've long crashed with
a NPD if it was really NULL?

johannes

Re: [RFC PATCH net-next 1/3] netlink: add support for formatted extack messages

[Edward Cree]

On 07/10/2022 14:35, Johannes Berg wrote:
> 
>> +#define NL_SET_ERR_MSG_FMT(extack, fmt, args...) do {		\
>> +	struct netlink_ext_ack *__extack = (extack);		\
>> +								\
>> +	scnprintf(__extack->_msg_buf, NETLINK_MAX_FMTMSG_LEN,	\
>> +		  (fmt), ##args);				\
> 
> Maybe that should print some kind of warning if the string was longer
> than the buffer? OTOH, I guess the user would notice anyway, and until
> you run the code nobody can possibly notice ... too bad then?
> 
> Maybe we could at least _statically_ make sure that the *format* string
> (fmt) is shorter than say 60 chars or something to give some wiggle room
> for the print expansion?
> 
> 	/* allow 20 chars for format expansion */
> 	BUILD_BUG_ON(strlen(fmt) > NETLINK_MAX_FMTMSG_LEN - 20);
> 
> might even work? Just as a sanity check.

Hmm, I don't think we want to prohibit the case of (say) a 78-char format
 string with one %d that's always small-valued in practice.
In fact if you have lots of % in the format string the output could be
 significantly *shorter* than fmt.
So while I do like the idea of a sanity check, I don't see how to do it
 without imposing unnecessary limitations.

>> +	do_trace_netlink_extack(__extack->_msg_buf);		\
>> +								\
>> +	if (__extack)						\
>> +		__extack->_msg = __extack->_msg_buf;		\
> 
> That "if (__extack)" check seems a bit strange, you've long crashed with
> a NPD if it was really NULL?

Good point, I blindly copied NL_SET_ERR_MSG without thinking.
The check should enclose the whole body, will fix in v2.

-ed

Re: [RFC PATCH net-next 1/3] netlink: add support for formatted extack messages

[Johannes Berg]

On Fri, 2022-10-07 at 14:46 +0100, Edward Cree wrote:
> On 07/10/2022 14:35, Johannes Berg wrote:
> > 
> > > +#define NL_SET_ERR_MSG_FMT(extack, fmt, args...) do {		\
> > > +	struct netlink_ext_ack *__extack = (extack);		\
> > > +								\
> > > +	scnprintf(__extack->_msg_buf, NETLINK_MAX_FMTMSG_LEN,	\
> > > +		  (fmt), ##args);				\
> > 
> > Maybe that should print some kind of warning if the string was longer
> > than the buffer? OTOH, I guess the user would notice anyway, and until
> > you run the code nobody can possibly notice ... too bad then?
> > 
> > Maybe we could at least _statically_ make sure that the *format* string
> > (fmt) is shorter than say 60 chars or something to give some wiggle room
> > for the print expansion?
> > 
> > 	/* allow 20 chars for format expansion */
> > 	BUILD_BUG_ON(strlen(fmt) > NETLINK_MAX_FMTMSG_LEN - 20);
> > 
> > might even work? Just as a sanity check.
> 
> Hmm, I don't think we want to prohibit the case of (say) a 78-char format
>  string with one %d that's always small-valued in practice.
> In fact if you have lots of % in the format string the output could be
>  significantly *shorter* than fmt.
> So while I do like the idea of a sanity check, I don't see how to do it
>  without imposing unnecessary limitations.
> 

Yeah, I agree. We could runtime warn but that's also pretty useless.

I guess we just have to be careful - but I know from experience that
won't work ;-)

(and some things like %pM or even %p*H can expand a lot anyway)

Unless maybe we printed a warning together with the full string, so the
user could recover it? WARN_ON() isn't useful though, the string should
be enough to understand where it came from.

Anyway just thinking out loud :)

johannes

Re: [RFC PATCH net-next 1/3] netlink: add support for formatted extack messages

[Edward Cree]

On 07/10/2022 14:49, Johannes Berg wrote:
> Unless maybe we printed a warning together with the full string, so the
> user could recover it?
Ooh, I like that.
Would need to net_ratelimit of course, but that should be easy enough.

It's not quite perfect as it'd evaluate the VA_ARGS twice, potentially
 side-effecting, which the original NL_SET_ERR_MSG() took some care to
 avoid, but I think we can live with that.

-ed

Re: [RFC PATCH net-next 1/3] netlink: add support for formatted extack messages

[Jiri Pirko]

Fri, Oct 07, 2022 at 03:49:42PM CEST, johannes@sipsolutions.net wrote:
>On Fri, 2022-10-07 at 14:46 +0100, Edward Cree wrote:
>> On 07/10/2022 14:35, Johannes Berg wrote:
>> > 
>> > > +#define NL_SET_ERR_MSG_FMT(extack, fmt, args...) do {		\
>> > > +	struct netlink_ext_ack *__extack = (extack);		\
>> > > +								\
>> > > +	scnprintf(__extack->_msg_buf, NETLINK_MAX_FMTMSG_LEN,	\
>> > > +		  (fmt), ##args);				\
>> > 
>> > Maybe that should print some kind of warning if the string was longer
>> > than the buffer? OTOH, I guess the user would notice anyway, and until
>> > you run the code nobody can possibly notice ... too bad then?
>> > 
>> > Maybe we could at least _statically_ make sure that the *format* string
>> > (fmt) is shorter than say 60 chars or something to give some wiggle room
>> > for the print expansion?
>> > 
>> > 	/* allow 20 chars for format expansion */
>> > 	BUILD_BUG_ON(strlen(fmt) > NETLINK_MAX_FMTMSG_LEN - 20);
>> > 
>> > might even work? Just as a sanity check.
>> 
>> Hmm, I don't think we want to prohibit the case of (say) a 78-char format
>>  string with one %d that's always small-valued in practice.
>> In fact if you have lots of % in the format string the output could be
>>  significantly *shorter* than fmt.
>> So while I do like the idea of a sanity check, I don't see how to do it
>>  without imposing unnecessary limitations.
>> 
>
>Yeah, I agree. We could runtime warn but that's also pretty useless.

I think that the macro caller need to take the buffer size into account
passing the formatted msg. So if the generated message would not fit
into the buffer, it's a caller bug. WARN_ON() is suitable for such
things, as it most probaly will hit the developer testing newly added
exack message.


>
>I guess we just have to be careful - but I know from experience that
>won't work ;-)
>
>(and some things like %pM or even %p*H can expand a lot anyway)
>
>Unless maybe we printed a warning together with the full string, so the
>user could recover it? WARN_ON() isn't useful though, the string should
>be enough to understand where it came from.
>
>Anyway just thinking out loud :)
>
>johannes

Re: [RFC PATCH net-next 1/3] netlink: add support for formatted extack messages

[Edward Cree]

On 13/10/2022 13:59, Jiri Pirko wrote:
> I think that the macro caller need to take the buffer size into account
> passing the formatted msg. So if the generated message would not fit
> into the buffer, it's a caller bug. WARN_ON() is suitable for such
> things, as it most probaly will hit the developer testing newly added
> exack message.

I disagree, extack is a best-effort diagnostic channel and it's valid
 for a caller to rely on e.g. %pI6c to generate messages that *usually*
 fit but can't be guaranteed to.  And original dev might well not see
 the WARN_ON() because he's using addresses like fc00::123 in his tests
 whereas the end user — who maybe has panic_on_warn enabled — has a
 real-world address that takes 30+ bytes to print.
Then there's things like %d which can vary in length by a factor of 10.
I think the net_warn_ratelimited() with the full message, as I've used
 in v2, is plenty loud enough.

-ed

Re: [RFC PATCH net-next 1/3] netlink: add support for formatted extack messages

[Jakub Kicinski]

On Fri, 7 Oct 2022 14:46:46 +0100 Edward Cree wrote:
> > That "if (__extack)" check seems a bit strange, you've long crashed with
> > a NPD if it was really NULL?  
> 
> Good point, I blindly copied NL_SET_ERR_MSG without thinking.
> The check should enclose the whole body, will fix in v2.

FWIW you can prolly use break; thanks to the do {} while wrapping.
Maybe that's hacky.

Re: [RFC PATCH net-next 1/3] netlink: add support for formatted extack messages

[Jiri Pirko]

Fri, Oct 07, 2022 at 03:25:12PM CEST, ecree@xilinx.com wrote:
>From: Edward Cree <ecree.xilinx@gmail.com>
>
>Include an 80-byte buffer in struct netlink_ext_ack that can be used
> for scnprintf()ed messages.  This does mean that the resulting string
> can't be enumerated, translated etc. in the way NL_SET_ERR_MSG() was
> designed to allow.
>
>Signed-off-by: Edward Cree <ecree.xilinx@gmail.com>
>---
> include/linux/netlink.h | 21 +++++++++++++++++++--
> 1 file changed, 19 insertions(+), 2 deletions(-)
>
>diff --git a/include/linux/netlink.h b/include/linux/netlink.h
>index d51e041d2242..bfab9dbd64fa 100644
>--- a/include/linux/netlink.h
>+++ b/include/linux/netlink.h
>@@ -64,6 +64,7 @@ netlink_kernel_create(struct net *net, int unit, struct netlink_kernel_cfg *cfg)
> 
> /* this can be increased when necessary - don't expose to userland */
> #define NETLINK_MAX_COOKIE_LEN	20
>+#define NETLINK_MAX_FMTMSG_LEN	80
> 
> /**
>  * struct netlink_ext_ack - netlink extended ACK report struct
>@@ -75,6 +76,8 @@ netlink_kernel_create(struct net *net, int unit, struct netlink_kernel_cfg *cfg)
>  * @miss_nest: nest missing an attribute (%NULL if missing top level attr)
>  * @cookie: cookie data to return to userspace (for success)
>  * @cookie_len: actual cookie data length
>+ * @_msg_buf: output buffer for formatted message strings - don't access
>+ *	directly, use %NL_SET_ERR_MSG_FMT
>  */
> struct netlink_ext_ack {
> 	const char *_msg;
>@@ -84,13 +87,13 @@ struct netlink_ext_ack {
> 	u16 miss_type;
> 	u8 cookie[NETLINK_MAX_COOKIE_LEN];
> 	u8 cookie_len;
>+	char _msg_buf[NETLINK_MAX_FMTMSG_LEN];
> };
> 
> /* Always use this macro, this allows later putting the
>  * message into a separate section or such for things
>  * like translation or listing all possible messages.
>- * Currently string formatting is not supported (due
>- * to the lack of an output buffer.)
>+ * If string formatting is needed use NL_SET_ERR_MSG_FMT.
>  */
> #define NL_SET_ERR_MSG(extack, msg) do {		\
> 	static const char __msg[] = msg;		\
>@@ -102,9 +105,23 @@ struct netlink_ext_ack {
> 		__extack->_msg = __msg;			\
> } while (0)
> 
>+#define NL_SET_ERR_MSG_FMT(extack, fmt, args...) do {		\
>+	struct netlink_ext_ack *__extack = (extack);		\
>+								\
>+	scnprintf(__extack->_msg_buf, NETLINK_MAX_FMTMSG_LEN,	\
>+		  (fmt), ##args);				\
>+	do_trace_netlink_extack(__extack->_msg_buf);		\
>+								\
>+	if (__extack)						\
>+		__extack->_msg = __extack->_msg_buf;		\
>+} while (0)
>+
> #define NL_SET_ERR_MSG_MOD(extack, msg)			\
> 	NL_SET_ERR_MSG((extack), KBUILD_MODNAME ": " msg)
> 
>+#define NL_SET_ERR_MSG_FMT_MOD(extack, fmt, args...)	\

I wonder, wouldn't it be better to just have NL_SET_ERR_MSG_MOD which
accepts format string and that's it. I understand there is an extra
overhead for the messages that don't use formatting, but do we care?
This is no fastpath and usually happens only seldom. The API towards
the driver would be more simple.

I like this a lot!


>+	NL_SET_ERR_MSG_FMT((extack), KBUILD_MODNAME ": " fmt, ##args)
>+
> #define NL_SET_BAD_ATTR_POLICY(extack, attr, pol) do {	\
> 	if ((extack)) {					\
> 		(extack)->bad_attr = (attr);		\

Re: [RFC PATCH net-next 1/3] netlink: add support for formatted extack messages

[Edward Cree]

On 13/10/2022 13:55, Jiri Pirko wrote:
> Fri, Oct 07, 2022 at 03:25:12PM CEST, ecree@xilinx.com wrote:
>> #define NL_SET_ERR_MSG_MOD(extack, msg)			\
>> 	NL_SET_ERR_MSG((extack), KBUILD_MODNAME ": " msg)
>>
>> +#define NL_SET_ERR_MSG_FMT_MOD(extack, fmt, args...)	\
> 
> I wonder, wouldn't it be better to just have NL_SET_ERR_MSG_MOD which
> accepts format string and that's it. I understand there is an extra
> overhead for the messages that don't use formatting, but do we care?
> This is no fastpath and usually happens only seldom. The API towards
> the driver would be more simple.

Could do, but this way a fixed string isn't limited to
 NETLINK_MAX_FMTMSG_LEN like it would be if we tried to stuff it
 in _msg_buf.  Unless you're suggesting some kind of macro magic
 that detects whether args is empty and chooses which
 implementation to use, but that seems like excessive hidden
 cleverness — better to have driver authors aware of the
 limitations of each choice.

> I like this a lot!
:)

Re: [RFC PATCH net-next 1/3] netlink: add support for formatted extack messages

[Jakub Kicinski]

On Mon, 17 Oct 2022 13:00:55 +0100 Edward Cree wrote:
> > I wonder, wouldn't it be better to just have NL_SET_ERR_MSG_MOD which
> > accepts format string and that's it. I understand there is an extra
> > overhead for the messages that don't use formatting, but do we care?
> > This is no fastpath and usually happens only seldom. The API towards
> > the driver would be more simple.  
> 
> Could do, but this way a fixed string isn't limited to
>  NETLINK_MAX_FMTMSG_LEN like it would be if we tried to stuff it
>  in _msg_buf.  Unless you're suggesting some kind of macro magic
>  that detects whether args is empty and chooses which
>  implementation to use, but that seems like excessive hidden
>  cleverness — better to have driver authors aware of the
>  limitations of each choice.

No macro magic, if we wanna go the extra mile we'd need to run some
analysis. We can choose the limit based on the longest message today.

If spatch could output matches that'd make the analysis pretty trivial
but IDK if it can :S

[RFC PATCH net-next 2/3] sfc: use formatted extacks instead of efx_tc_err()

[ecree]

From: Edward Cree <ecree.xilinx@gmail.com>

Since we can now get a formatted message back to the user with
 NL_SET_ERR_MSG_FMT_MOD(), there's no need for our special logging.

Signed-off-by: Edward Cree <ecree.xilinx@gmail.com>
---
 drivers/net/ethernet/sfc/mae.c |  5 ++--
 drivers/net/ethernet/sfc/tc.c  | 47 +++++++++++++++-------------------
 drivers/net/ethernet/sfc/tc.h  | 18 -------------
 3 files changed, 23 insertions(+), 47 deletions(-)

diff --git a/drivers/net/ethernet/sfc/mae.c b/drivers/net/ethernet/sfc/mae.c
index 874c765b2465..6f472ea0638a 100644
--- a/drivers/net/ethernet/sfc/mae.c
+++ b/drivers/net/ethernet/sfc/mae.c
@@ -265,9 +265,8 @@ int efx_mae_match_check_caps(struct efx_nic *efx,
 	rc = efx_mae_match_check_cap_typ(supported_fields[MAE_FIELD_INGRESS_PORT],
 					 ingress_port_mask_type);
 	if (rc) {
-		efx_tc_err(efx, "No support for %s mask in field ingress_port\n",
-			   mask_type_name(ingress_port_mask_type));
-		NL_SET_ERR_MSG_MOD(extack, "Unsupported mask type for ingress_port");
+		NL_SET_ERR_MSG_FMT_MOD(extack, "No support for %s mask in field ingress_port",
+				       mask_type_name(ingress_port_mask_type));
 		return rc;
 	}
 	return 0;
diff --git a/drivers/net/ethernet/sfc/tc.c b/drivers/net/ethernet/sfc/tc.c
index 3478860d4023..b21a961eabb1 100644
--- a/drivers/net/ethernet/sfc/tc.c
+++ b/drivers/net/ethernet/sfc/tc.c
@@ -137,17 +137,16 @@ static int efx_tc_flower_parse_match(struct efx_nic *efx,
 		flow_rule_match_control(rule, &fm);
 
 		if (fm.mask->flags) {
-			efx_tc_err(efx, "Unsupported match on control.flags %#x\n",
-				   fm.mask->flags);
-			NL_SET_ERR_MSG_MOD(extack, "Unsupported match on control.flags");
+			NL_SET_ERR_MSG_FMT_MOD(extack, "Unsupported match on control.flags %#x",
+					       fm.mask->flags);
 			return -EOPNOTSUPP;
 		}
 	}
 	if (dissector->used_keys &
 	    ~(BIT(FLOW_DISSECTOR_KEY_CONTROL) |
 	      BIT(FLOW_DISSECTOR_KEY_BASIC))) {
-		efx_tc_err(efx, "Unsupported flower keys %#x\n", dissector->used_keys);
-		NL_SET_ERR_MSG_MOD(extack, "Unsupported flower keys encountered");
+		NL_SET_ERR_MSG_FMT_MOD(extack, "Unsupported flower keys %#x",
+				       dissector->used_keys);
 		return -EOPNOTSUPP;
 	}
 
@@ -156,11 +155,11 @@ static int efx_tc_flower_parse_match(struct efx_nic *efx,
 
 		flow_rule_match_basic(rule, &fm);
 		if (fm.mask->n_proto) {
-			EFX_TC_ERR_MSG(efx, extack, "Unsupported eth_proto match\n");
+			NL_SET_ERR_MSG_MOD(extack, "Unsupported eth_proto match");
 			return -EOPNOTSUPP;
 		}
 		if (fm.mask->ip_proto) {
-			EFX_TC_ERR_MSG(efx, extack, "Unsupported ip_proto match\n");
+			NL_SET_ERR_MSG_MOD(extack, "Unsupported ip_proto match");
 			return -EOPNOTSUPP;
 		}
 	}
@@ -200,13 +199,9 @@ static int efx_tc_flower_replace(struct efx_nic *efx,
 
 	if (efv != from_efv) {
 		/* can't happen */
-		efx_tc_err(efx, "for %s efv is %snull but from_efv is %snull\n",
-			   netdev_name(net_dev), efv ? "non-" : "",
-			   from_efv ? "non-" : "");
-		if (efv)
-			NL_SET_ERR_MSG_MOD(extack, "vfrep filter has PF net_dev (can't happen)");
-		else
-			NL_SET_ERR_MSG_MOD(extack, "PF filter has vfrep net_dev (can't happen)");
+		NL_SET_ERR_MSG_FMT_MOD(extack, "for %s efv is %snull but from_efv is %snull (can't happen)",
+				       netdev_name(net_dev), efv ? "non-" : "",
+				       from_efv ? "non-" : "");
 		return -EINVAL;
 	}
 
@@ -214,7 +209,7 @@ static int efx_tc_flower_replace(struct efx_nic *efx,
 	memset(&match, 0, sizeof(match));
 	rc = efx_tc_flower_external_mport(efx, from_efv);
 	if (rc < 0) {
-		EFX_TC_ERR_MSG(efx, extack, "Failed to identify ingress m-port");
+		NL_SET_ERR_MSG_MOD(extack, "Failed to identify ingress m-port");
 		return rc;
 	}
 	match.value.ingress_port = rc;
@@ -224,7 +219,7 @@ static int efx_tc_flower_replace(struct efx_nic *efx,
 		return rc;
 
 	if (tc->common.chain_index) {
-		EFX_TC_ERR_MSG(efx, extack, "No support for nonzero chain_index");
+		NL_SET_ERR_MSG_MOD(extack, "No support for nonzero chain_index");
 		return -EOPNOTSUPP;
 	}
 	match.mask.recirc_id = 0xff;
@@ -261,7 +256,7 @@ static int efx_tc_flower_replace(struct efx_nic *efx,
 
 		if (!act) {
 			/* more actions after a non-pipe action */
-			EFX_TC_ERR_MSG(efx, extack, "Action follows non-pipe action");
+			NL_SET_ERR_MSG_MOD(extack, "Action follows non-pipe action");
 			rc = -EINVAL;
 			goto release;
 		}
@@ -270,7 +265,7 @@ static int efx_tc_flower_replace(struct efx_nic *efx,
 		case FLOW_ACTION_DROP:
 			rc = efx_mae_alloc_action_set(efx, act);
 			if (rc) {
-				EFX_TC_ERR_MSG(efx, extack, "Failed to write action set to hw (drop)");
+				NL_SET_ERR_MSG_MOD(extack, "Failed to write action set to hw (drop)");
 				goto release;
 			}
 			list_add_tail(&act->list, &rule->acts.list);
@@ -281,20 +276,20 @@ static int efx_tc_flower_replace(struct efx_nic *efx,
 			save = *act;
 			to_efv = efx_tc_flower_lookup_efv(efx, fa->dev);
 			if (IS_ERR(to_efv)) {
-				EFX_TC_ERR_MSG(efx, extack, "Mirred egress device not on switch");
+				NL_SET_ERR_MSG_MOD(extack, "Mirred egress device not on switch");
 				rc = PTR_ERR(to_efv);
 				goto release;
 			}
 			rc = efx_tc_flower_external_mport(efx, to_efv);
 			if (rc < 0) {
-				EFX_TC_ERR_MSG(efx, extack, "Failed to identify egress m-port");
+				NL_SET_ERR_MSG_MOD(extack, "Failed to identify egress m-port");
 				goto release;
 			}
 			act->dest_mport = rc;
 			act->deliver = 1;
 			rc = efx_mae_alloc_action_set(efx, act);
 			if (rc) {
-				EFX_TC_ERR_MSG(efx, extack, "Failed to write action set to hw (mirred)");
+				NL_SET_ERR_MSG_MOD(extack, "Failed to write action set to hw (mirred)");
 				goto release;
 			}
 			list_add_tail(&act->list, &rule->acts.list);
@@ -310,9 +305,9 @@ static int efx_tc_flower_replace(struct efx_nic *efx,
 			*act = save;
 			break;
 		default:
-			efx_tc_err(efx, "Unhandled action %u\n", fa->id);
+			NL_SET_ERR_MSG_FMT_MOD(extack, "Unhandled action %u",
+					       fa->id);
 			rc = -EOPNOTSUPP;
-			NL_SET_ERR_MSG_MOD(extack, "Unsupported action");
 			goto release;
 		}
 	}
@@ -334,7 +329,7 @@ static int efx_tc_flower_replace(struct efx_nic *efx,
 		act->deliver = 1;
 		rc = efx_mae_alloc_action_set(efx, act);
 		if (rc) {
-			EFX_TC_ERR_MSG(efx, extack, "Failed to write action set to hw (deliver)");
+			NL_SET_ERR_MSG_MOD(extack, "Failed to write action set to hw (deliver)");
 			goto release;
 		}
 		list_add_tail(&act->list, &rule->acts.list);
@@ -349,13 +344,13 @@ static int efx_tc_flower_replace(struct efx_nic *efx,
 
 	rc = efx_mae_alloc_action_set_list(efx, &rule->acts);
 	if (rc) {
-		EFX_TC_ERR_MSG(efx, extack, "Failed to write action set list to hw");
+		NL_SET_ERR_MSG_MOD(extack, "Failed to write action set list to hw");
 		goto release;
 	}
 	rc = efx_mae_insert_rule(efx, &rule->match, EFX_TC_PRIO_TC,
 				 rule->acts.fw_id, &rule->fw_id);
 	if (rc) {
-		EFX_TC_ERR_MSG(efx, extack, "Failed to insert rule in hw");
+		NL_SET_ERR_MSG_MOD(extack, "Failed to insert rule in hw");
 		goto release_acts;
 	}
 	return 0;
diff --git a/drivers/net/ethernet/sfc/tc.h b/drivers/net/ethernet/sfc/tc.h
index 196fd74ed973..4373c3243e3c 100644
--- a/drivers/net/ethernet/sfc/tc.h
+++ b/drivers/net/ethernet/sfc/tc.h
@@ -15,24 +15,6 @@
 #include <linux/rhashtable.h>
 #include "net_driver.h"
 
-/* Error reporting: convenience macros.  For indicating why a given filter
- * insertion is not supported; errors in internal operation or in the
- * hardware should be netif_err()s instead.
- */
-/* Used when error message is constant. */
-#define EFX_TC_ERR_MSG(efx, extack, message)	do {			\
-	NL_SET_ERR_MSG_MOD(extack, message);				\
-	if (efx->log_tc_errs)						\
-		netif_info(efx, drv, efx->net_dev, "%s\n", message);	\
-} while (0)
-/* Used when error message is not constant; caller should also supply a
- * constant extack message with NL_SET_ERR_MSG_MOD().
- */
-#define efx_tc_err(efx, fmt, args...)	do {		\
-if (efx->log_tc_errs)					\
-	netif_info(efx, drv, efx->net_dev, fmt, ##args);\
-} while (0)
-
 struct efx_tc_action_set {
 	u16 deliver:1;
 	u32 dest_mport;

[RFC PATCH net-next 3/3] sfc: remove 'log-tc-errors' ethtool private flag

[ecree]

From: Edward Cree <ecree.xilinx@gmail.com>

It no longer does anything now that we're using formatted extacks instead.
So we can remove the driver's whole get/set priv_flags implementation.

Signed-off-by: Edward Cree <ecree.xilinx@gmail.com>
---
 drivers/net/ethernet/sfc/ef100_ethtool.c  |  2 --
 drivers/net/ethernet/sfc/ethtool_common.c | 37 -----------------------
 drivers/net/ethernet/sfc/ethtool_common.h |  2 --
 drivers/net/ethernet/sfc/net_driver.h     |  2 --
 4 files changed, 43 deletions(-)

diff --git a/drivers/net/ethernet/sfc/ef100_ethtool.c b/drivers/net/ethernet/sfc/ef100_ethtool.c
index 135ece2f1375..702abbe59b76 100644
--- a/drivers/net/ethernet/sfc/ef100_ethtool.c
+++ b/drivers/net/ethernet/sfc/ef100_ethtool.c
@@ -43,8 +43,6 @@ const struct ethtool_ops ef100_ethtool_ops = {
 	.get_pauseparam         = efx_ethtool_get_pauseparam,
 	.set_pauseparam         = efx_ethtool_set_pauseparam,
 	.get_sset_count		= efx_ethtool_get_sset_count,
-	.get_priv_flags		= efx_ethtool_get_priv_flags,
-	.set_priv_flags		= efx_ethtool_set_priv_flags,
 	.self_test		= efx_ethtool_self_test,
 	.get_strings		= efx_ethtool_get_strings,
 	.get_link_ksettings	= efx_ethtool_get_link_ksettings,
diff --git a/drivers/net/ethernet/sfc/ethtool_common.c b/drivers/net/ethernet/sfc/ethtool_common.c
index 6649a2327d03..a8cbceeb301b 100644
--- a/drivers/net/ethernet/sfc/ethtool_common.c
+++ b/drivers/net/ethernet/sfc/ethtool_common.c
@@ -101,14 +101,6 @@ static const struct efx_sw_stat_desc efx_sw_stat_desc[] = {
 
 #define EFX_ETHTOOL_SW_STAT_COUNT ARRAY_SIZE(efx_sw_stat_desc)
 
-static const char efx_ethtool_priv_flags_strings[][ETH_GSTRING_LEN] = {
-	"log-tc-errors",
-};
-
-#define EFX_ETHTOOL_PRIV_FLAGS_LOG_TC_ERRS		BIT(0)
-
-#define EFX_ETHTOOL_PRIV_FLAGS_COUNT ARRAY_SIZE(efx_ethtool_priv_flags_strings)
-
 void efx_ethtool_get_drvinfo(struct net_device *net_dev,
 			     struct ethtool_drvinfo *info)
 {
@@ -460,8 +452,6 @@ int efx_ethtool_get_sset_count(struct net_device *net_dev, int string_set)
 		       efx_ptp_describe_stats(efx, NULL);
 	case ETH_SS_TEST:
 		return efx_ethtool_fill_self_tests(efx, NULL, NULL, NULL);
-	case ETH_SS_PRIV_FLAGS:
-		return EFX_ETHTOOL_PRIV_FLAGS_COUNT;
 	default:
 		return -EINVAL;
 	}
@@ -488,39 +478,12 @@ void efx_ethtool_get_strings(struct net_device *net_dev,
 	case ETH_SS_TEST:
 		efx_ethtool_fill_self_tests(efx, NULL, strings, NULL);
 		break;
-	case ETH_SS_PRIV_FLAGS:
-		for (i = 0; i < EFX_ETHTOOL_PRIV_FLAGS_COUNT; i++)
-			strscpy(strings + i * ETH_GSTRING_LEN,
-				efx_ethtool_priv_flags_strings[i],
-				ETH_GSTRING_LEN);
-		break;
 	default:
 		/* No other string sets */
 		break;
 	}
 }
 
-u32 efx_ethtool_get_priv_flags(struct net_device *net_dev)
-{
-	struct efx_nic *efx = efx_netdev_priv(net_dev);
-	u32 ret_flags = 0;
-
-	if (efx->log_tc_errs)
-		ret_flags |= EFX_ETHTOOL_PRIV_FLAGS_LOG_TC_ERRS;
-
-	return ret_flags;
-}
-
-int efx_ethtool_set_priv_flags(struct net_device *net_dev, u32 flags)
-{
-	struct efx_nic *efx = efx_netdev_priv(net_dev);
-
-	efx->log_tc_errs =
-		!!(flags & EFX_ETHTOOL_PRIV_FLAGS_LOG_TC_ERRS);
-
-	return 0;
-}
-
 void efx_ethtool_get_stats(struct net_device *net_dev,
 			   struct ethtool_stats *stats,
 			   u64 *data)
diff --git a/drivers/net/ethernet/sfc/ethtool_common.h b/drivers/net/ethernet/sfc/ethtool_common.h
index 0afc74021a5e..659491932101 100644
--- a/drivers/net/ethernet/sfc/ethtool_common.h
+++ b/drivers/net/ethernet/sfc/ethtool_common.h
@@ -27,8 +27,6 @@ int efx_ethtool_fill_self_tests(struct efx_nic *efx,
 int efx_ethtool_get_sset_count(struct net_device *net_dev, int string_set);
 void efx_ethtool_get_strings(struct net_device *net_dev, u32 string_set,
 			     u8 *strings);
-u32 efx_ethtool_get_priv_flags(struct net_device *net_dev);
-int efx_ethtool_set_priv_flags(struct net_device *net_dev, u32 flags);
 void efx_ethtool_get_stats(struct net_device *net_dev,
 			   struct ethtool_stats *stats __attribute__ ((unused)),
 			   u64 *data);
diff --git a/drivers/net/ethernet/sfc/net_driver.h b/drivers/net/ethernet/sfc/net_driver.h
index 2e9ba0cfe848..7ef823d7a89a 100644
--- a/drivers/net/ethernet/sfc/net_driver.h
+++ b/drivers/net/ethernet/sfc/net_driver.h
@@ -855,7 +855,6 @@ enum efx_xdp_tx_queues_mode {
  * @timer_max_ns: Interrupt timer maximum value, in nanoseconds
  * @irq_rx_adaptive: Adaptive IRQ moderation enabled for RX event queues
  * @irqs_hooked: Channel interrupts are hooked
- * @log_tc_errs: Error logging for TC filter insertion is enabled
  * @irq_rx_mod_step_us: Step size for IRQ moderation for RX event queues
  * @irq_rx_moderation_us: IRQ moderation time for RX event queues
  * @msg_enable: Log message enable flags
@@ -1018,7 +1017,6 @@ struct efx_nic {
 	unsigned int timer_max_ns;
 	bool irq_rx_adaptive;
 	bool irqs_hooked;
-	bool log_tc_errs;
 	unsigned int irq_mod_step_us;
 	unsigned int irq_rx_moderation_us;
 	u32 msg_enable;