All of lore.kernel.org
 help / color / mirror / Atom feed
From: Alex Elder <elder@linaro.org>
To: Bjorn Andersson <bjorn.andersson@linaro.org>, netdev@vger.kernel.org
Cc: Daniele Palmas <dnlplm@gmail.com>
Subject: Re: [PATCH v2] iplink_rmnet: Allow passing IFLA_RMNET_FLAGS
Date: Mon, 15 Mar 2021 11:16:17 -0500	[thread overview]
Message-ID: <1b6ebc71-5efd-53ea-95b5-85e17d5804d1@linaro.org> (raw)
In-Reply-To: <20210315154629.652824-1-bjorn.andersson@linaro.org>

On 3/15/21 10:46 AM, Bjorn Andersson wrote:
> Parse and pass IFLA_RMNET_FLAGS to the kernel, to allow changing the
> flags from the default of ingress-aggregate only.

To be clear, this default is implemented in the kernel RMNet
driver, not in "iproute2".  And it is ingress deaggregation
(unpacking of aggregated packets from a buffer), not aggregation
(which would be supplying a buffer of aggregated packets to the
hardware).

> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>

I have some suggestions on your help text (and flag names).
The code looks good to me otherwise.  I trust you've
confirmed the RMNet driver uses the flags exactly as
you intend when they're provided this way.

					-Alex
> ---
> 
> Changes since v1:
> - s/ifla_vlan_flags/ifla_rmnet_flags/ in print_opt
> 
>   ip/iplink_rmnet.c | 42 ++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 42 insertions(+)
> 
> diff --git a/ip/iplink_rmnet.c b/ip/iplink_rmnet.c
> index 1d16440c6900..a847c838def2 100644
> --- a/ip/iplink_rmnet.c
> +++ b/ip/iplink_rmnet.c
> @@ -16,6 +16,10 @@ static void print_explain(FILE *f)
>   {
>   	fprintf(f,
>   		"Usage: ... rmnet mux_id MUXID\n"
> +		"                 [ingress-deaggregation]\n"
> +		"                 [ingress-commands]\n"
> +		"                 [ingress-chksumv4]\n"
> +		"                 [egress-chksumv4]\n"

Other help output (in print_explain()) put spaces after
the '[' and before the ']'; so you'd be better to stay
consistent with that.

And I know the name is based on the C symbol, but I think
you should follow the convention that seems to be used for
all others, and use "csum" to mean checksum.

Also it's not clear what the "v4" means.  I'm not sure I
like this suggestion, but...  It comes from QMAP version 4,
as opposed to QMAP version 5, so maybe use "csum-qmap4"
in place of "csumv4?"

Is there any way to disable ingress deaggregation?  Since
it's on by default, you might want to use a "[ on | off ]"
type option for that case (or all of them for that matter).
Otherwise, the deaggregation parameter doesn't really help
anything.

>   		"\n"
>   		"MUXID := 1-254\n"
>   	);
> @@ -29,6 +33,7 @@ static void explain(void)
>   static int rmnet_parse_opt(struct link_util *lu, int argc, char **argv,
>   			   struct nlmsghdr *n)
>   {
> +	struct ifla_rmnet_flags flags = { };
>   	__u16 mux_id;

Do you know why this is __u16?  Is it because it's exposed
to user space?  Not a problem... just curious.

>   	while (argc > 0) {
> @@ -37,6 +42,18 @@ static int rmnet_parse_opt(struct link_util *lu, int argc, char **argv,
>   			if (get_u16(&mux_id, *argv, 0))
>   				invarg("mux_id is invalid", *argv);
>   			addattr16(n, 1024, IFLA_RMNET_MUX_ID, mux_id);
> +		} else if (matches(*argv, "ingress-deaggregation") == 0) {
> +			flags.mask = ~0;
> +			flags.flags |= RMNET_FLAGS_INGRESS_DEAGGREGATION;
> +		} else if (matches(*argv, "ingress-commands") == 0) {
> +			flags.mask = ~0;
> +			flags.flags |= RMNET_FLAGS_INGRESS_MAP_COMMANDS;
> +		} else if (matches(*argv, "ingress-chksumv4") == 0) {
> +			flags.mask = ~0;
> +			flags.flags |= RMNET_FLAGS_INGRESS_MAP_CKSUMV4;
> +		} else if (matches(*argv, "egress-chksumv4") == 0) {
> +			flags.mask = ~0;
> +			flags.flags |= RMNET_FLAGS_EGRESS_MAP_CKSUMV4;
>   		} else if (matches(*argv, "help") == 0) {
>   			explain();
>   			return -1;
> @@ -48,11 +65,28 @@ static int rmnet_parse_opt(struct link_util *lu, int argc, char **argv,
>   		argc--, argv++;
>   	}
>   
> +	if (flags.mask)
> +		addattr_l(n, 1024, IFLA_RMNET_FLAGS, &flags, sizeof(flags));
> +
>   	return 0;
>   }
>   
> +static void rmnet_print_flags(FILE *fp, __u32 flags)
> +{
> +	if (flags & RMNET_FLAGS_INGRESS_DEAGGREGATION)
> +		print_string(PRINT_ANY, NULL, "%s ", "ingress-deaggregation");
> +	if (flags & RMNET_FLAGS_INGRESS_MAP_COMMANDS)
> +		print_string(PRINT_ANY, NULL, "%s ", "ingress-commands");
> +	if (flags & RMNET_FLAGS_INGRESS_MAP_CKSUMV4)
> +		print_string(PRINT_ANY, NULL, "%s ", "ingress-chksumv4");
> +	if (flags & RMNET_FLAGS_EGRESS_MAP_CKSUMV4)
> +		print_string(PRINT_ANY, NULL, "%s ", "egress-cksumv4");
> +}
> +
>   static void rmnet_print_opt(struct link_util *lu, FILE *f, struct rtattr *tb[])
>   {
> +	struct ifla_rmnet_flags *flags;
> +
>   	if (!tb)
>   		return;
>   
> @@ -64,6 +98,14 @@ static void rmnet_print_opt(struct link_util *lu, FILE *f, struct rtattr *tb[])
>   		   "mux_id",
>   		   "mux_id %u ",
>   		   rta_getattr_u16(tb[IFLA_RMNET_MUX_ID]));
> +
> +	if (tb[IFLA_RMNET_FLAGS]) {
> +		if (RTA_PAYLOAD(tb[IFLA_RMNET_FLAGS]) < sizeof(*flags))
> +			return;
> +		flags = RTA_DATA(tb[IFLA_RMNET_FLAGS]);
> +
> +		rmnet_print_flags(f, flags->flags);
> +	}
>   }
>   
>   static void rmnet_print_help(struct link_util *lu, int argc, char **argv,
> 


  reply	other threads:[~2021-03-15 16:17 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-03-15 15:46 [PATCH v2] iplink_rmnet: Allow passing IFLA_RMNET_FLAGS Bjorn Andersson
2021-03-15 16:16 ` Alex Elder [this message]
2021-04-02 17:30   ` Bjorn Andersson
2021-09-24  3:33 Bjorn Andersson
2021-09-27 13:48 ` David Ahern
2021-09-27 22:44   ` Alex Elder
2021-09-28  2:09     ` David Ahern
2021-09-27 22:44 ` Alex Elder

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1b6ebc71-5efd-53ea-95b5-85e17d5804d1@linaro.org \
    --to=elder@linaro.org \
    --cc=bjorn.andersson@linaro.org \
    --cc=dnlplm@gmail.com \
    --cc=netdev@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.