diff mbox series

[ethtool] ethtool: fec: Change the prompt string to adapt to current situations

Message ID 20220602053626.62512-1-simon.horman@corigine.com (mailing list archive)
State Superseded
Delegated to: Michal Kubecek
Headers show
Series [ethtool] ethtool: fec: Change the prompt string to adapt to current situations | expand

Checks

Context Check Description
netdev/tree_selection success Not a local patch

Commit Message

Simon Horman June 2, 2022, 5:36 a.m. UTC 
From: Yu Xiao <yu.xiao@corigine.com>

Majority upstream drivers uses `Configured FEC encodings` to report
supported modes. At which point it is better to change the text in
ethtool user space that changes the meaning of the field, which is
better to suit for the current situations.

So changing `Configured FEC encodings` to `Supported/Configured FEC
encodings` to adapt to both implementations.

Signed-off-by: Yu Xiao <yu.xiao@corigine.com>
Signed-off-by: Simon Horman <simon.horman@corigine.com>
---

This patch resulted from a discussion on netdev regarding
updating the behaviour of the NFP driver. It was concluded in
that thread that it would be better to update the ethtool documentation
to reflect current implementations of the feature.

Ref: [PATCH net] nfp: correct the output of `ethtool --show-fec <intf>`
     https://lore.kernel.org/netdev/20220530084842.21258-1-simon.horman@corigine.com/
---
 ethtool.c     | 2 +-
 netlink/fec.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

Comments

Michal Kubecek June 2, 2022, 6:55 p.m. UTC | #1 
On Thu, Jun 02, 2022 at 07:36:26AM +0200, Simon Horman wrote:
> From: Yu Xiao <yu.xiao@corigine.com>
> 
> Majority upstream drivers uses `Configured FEC encodings` to report
> supported modes. At which point it is better to change the text in
> ethtool user space that changes the meaning of the field, which is
> better to suit for the current situations.
> 
> So changing `Configured FEC encodings` to `Supported/Configured FEC
> encodings` to adapt to both implementations.
> 
> Signed-off-by: Yu Xiao <yu.xiao@corigine.com>
> Signed-off-by: Simon Horman <simon.horman@corigine.com>
> ---
> 
> This patch resulted from a discussion on netdev regarding
> updating the behaviour of the NFP driver. It was concluded in
> that thread that it would be better to update the ethtool documentation
> to reflect current implementations of the feature.
> 
> Ref: [PATCH net] nfp: correct the output of `ethtool --show-fec <intf>`
>      https://lore.kernel.org/netdev/20220530084842.21258-1-simon.horman@corigine.com/
> ---
>  ethtool.c     | 2 +-
>  netlink/fec.c | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/ethtool.c b/ethtool.c
> index 277253090245..8654f70de03b 100644
> --- a/ethtool.c
> +++ b/ethtool.c
> @@ -5567,7 +5567,7 @@ static int do_gfec(struct cmd_context *ctx)
>  	}
>  
>  	fprintf(stdout, "FEC parameters for %s:\n", ctx->devname);
> -	fprintf(stdout, "Configured FEC encodings:");
> +	fprintf(stdout, "Supported/Configured FEC encodings:");

I'm OK with this part, even if I would prefer if we could unify what the
drivers present.

>  	dump_fec(feccmd.fec);
>  	fprintf(stdout, "\n");
>  
> diff --git a/netlink/fec.c b/netlink/fec.c
> index f2659199c157..1762ae349ca6 100644
> --- a/netlink/fec.c
> +++ b/netlink/fec.c
> @@ -153,7 +153,7 @@ int fec_reply_cb(const struct nlmsghdr *nlhdr, void *data)
>  	print_string(PRINT_ANY, "ifname", "FEC parameters for %s:\n",
>  		     nlctx->devname);
>  
> -	open_json_array("config", "Configured FEC encodings:");
> +	open_json_array("support/config", "Supported/Configured FEC encodings:");

AFAICS this would result in backword incompatible change of the JSON
output structure which is something we should avoid if we want people to
use JSON for scripting rather than the human readable text output.

Michal

>  	fa = tb[ETHTOOL_A_FEC_AUTO] && mnl_attr_get_u8(tb[ETHTOOL_A_FEC_AUTO]);
>  	if (fa)
>  		print_string(PRINT_ANY, NULL, " %s", "Auto");
> -- 
> 2.30.2
>
Macris Xiao June 6, 2022, 3:13 a.m. UTC | #2 
> -----Original Message-----
> From: Michal Kubecek <mkubecek@suse.cz>
> Sent: 2022年6月3日 2:55
> To: Simon Horman <simon.horman@corigine.com>
> Cc: netdev@vger.kernel.org; oss-drivers <oss-drivers@corigine.com>; Macris
> Xiao <yu.xiao@corigine.com>
> Subject: Re: [PATCH ethtool] ethtool: fec: Change the prompt string to adapt
> to current situations
> 
> On Thu, Jun 02, 2022 at 07:36:26AM +0200, Simon Horman wrote:
> > From: Yu Xiao <yu.xiao@corigine.com>
> >
> > Majority upstream drivers uses `Configured FEC encodings` to report
> > supported modes. At which point it is better to change the text in
> > ethtool user space that changes the meaning of the field, which is
> > better to suit for the current situations.
> >
> > So changing `Configured FEC encodings` to `Supported/Configured FEC
> > encodings` to adapt to both implementations.
> >
> > Signed-off-by: Yu Xiao <yu.xiao@corigine.com>
> > Signed-off-by: Simon Horman <simon.horman@corigine.com>
> > ---
> >
> > This patch resulted from a discussion on netdev regarding updating the
> > behaviour of the NFP driver. It was concluded in that thread that it
> > would be better to update the ethtool documentation to reflect current
> > implementations of the feature.
> >
> > Ref: [PATCH net] nfp: correct the output of `ethtool --show-fec <intf>`
> >
> > https://lore.kernel.org/netdev/20220530084842.21258-1-
> simon.horman@cor
> > igine.com/
> > ---
> >  ethtool.c     | 2 +-
> >  netlink/fec.c | 2 +-
> >  2 files changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/ethtool.c b/ethtool.c
> > index 277253090245..8654f70de03b 100644
> > --- a/ethtool.c
> > +++ b/ethtool.c
> > @@ -5567,7 +5567,7 @@ static int do_gfec(struct cmd_context *ctx)
> >  	}
> >
> >  	fprintf(stdout, "FEC parameters for %s:\n", ctx->devname);
> > -	fprintf(stdout, "Configured FEC encodings:");
> > +	fprintf(stdout, "Supported/Configured FEC encodings:");
> 
> I'm OK with this part, even if I would prefer if we could unify what the drivers
> present.
> 
> >  	dump_fec(feccmd.fec);
> >  	fprintf(stdout, "\n");
> >
> > diff --git a/netlink/fec.c b/netlink/fec.c index
> > f2659199c157..1762ae349ca6 100644
> > --- a/netlink/fec.c
> > +++ b/netlink/fec.c
> > @@ -153,7 +153,7 @@ int fec_reply_cb(const struct nlmsghdr *nlhdr, void
> *data)
> >  	print_string(PRINT_ANY, "ifname", "FEC parameters for %s:\n",
> >  		     nlctx->devname);
> >
> > -	open_json_array("config", "Configured FEC encodings:");
> > +	open_json_array("support/config", "Supported/Configured FEC
> > +encodings:");
> 
> AFAICS this would result in backword incompatible change of the JSON
> output structure which is something we should avoid if we want people to
> use JSON for scripting rather than the human readable text output.
> 
> Michal
> 

Thank you for advice. So whether to use only the first part, and is this part not
needed?

Macris Xiao

> >  	fa = tb[ETHTOOL_A_FEC_AUTO] &&
> mnl_attr_get_u8(tb[ETHTOOL_A_FEC_AUTO]);
> >  	if (fa)
> >  		print_string(PRINT_ANY, NULL, " %s", "Auto");
> > --
> > 2.30.2
> >
diff mbox series

Patch

diff --git a/ethtool.c b/ethtool.c
index 277253090245..8654f70de03b 100644
--- a/ethtool.c
+++ b/ethtool.c
@@ -5567,7 +5567,7 @@  static int do_gfec(struct cmd_context *ctx)
 	}
 
 	fprintf(stdout, "FEC parameters for %s:\n", ctx->devname);
-	fprintf(stdout, "Configured FEC encodings:");
+	fprintf(stdout, "Supported/Configured FEC encodings:");
 	dump_fec(feccmd.fec);
 	fprintf(stdout, "\n");
 
diff --git a/netlink/fec.c b/netlink/fec.c
index f2659199c157..1762ae349ca6 100644
--- a/netlink/fec.c
+++ b/netlink/fec.c
@@ -153,7 +153,7 @@  int fec_reply_cb(const struct nlmsghdr *nlhdr, void *data)
 	print_string(PRINT_ANY, "ifname", "FEC parameters for %s:\n",
 		     nlctx->devname);
 
-	open_json_array("config", "Configured FEC encodings:");
+	open_json_array("support/config", "Supported/Configured FEC encodings:");
 	fa = tb[ETHTOOL_A_FEC_AUTO] && mnl_attr_get_u8(tb[ETHTOOL_A_FEC_AUTO]);
 	if (fa)
 		print_string(PRINT_ANY, NULL, " %s", "Auto");