[PATCH] net: qualcomm: rmnet: Allow partial updates of IFLA_FLAGS

[PATCH] net: qualcomm: rmnet: Allow partial updates of IFLA_FLAGS

[Bjorn Andersson]

The idiomatic way to handle the changelink flags/mask pair seems to be
allow partial updates of the driver's link flags. In contrast the rmnet
driver masks the incoming flags and then use that as the new flags.

Change the rmnet driver to follow the common scheme, before the
introduction of IFLA_RMNET_FLAGS handling in iproute2 et al.

Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
---
 drivers/net/ethernet/qualcomm/rmnet/rmnet_config.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/qualcomm/rmnet/rmnet_config.c b/drivers/net/ethernet/qualcomm/rmnet/rmnet_config.c
index 8d51b0cb545c..2c8db2fcc53d 100644
--- a/drivers/net/ethernet/qualcomm/rmnet/rmnet_config.c
+++ b/drivers/net/ethernet/qualcomm/rmnet/rmnet_config.c
@@ -336,7 +336,8 @@ static int rmnet_changelink(struct net_device *dev, struct nlattr *tb[],
 
 		old_data_format = port->data_format;
 		flags = nla_data(data[IFLA_RMNET_FLAGS]);
-		port->data_format = flags->flags & flags->mask;
+		port->data_format &= ~flags->mask;
+		port->data_format |= flags->flags & flags->mask;
 
 		if (rmnet_vnd_update_dev_mtu(port, real_dev)) {
 			port->data_format = old_data_format;
-- 
2.31.0

Re: [PATCH] net: qualcomm: rmnet: Allow partial updates of IFLA_FLAGS

[Alex Elder]

On 4/22/21 1:20 PM, Bjorn Andersson wrote:
> The idiomatic way to handle the changelink flags/mask pair seems to be
> allow partial updates of the driver's link flags. In contrast the rmnet
> driver masks the incoming flags and then use that as the new flags.
> 
> Change the rmnet driver to follow the common scheme, before the
> introduction of IFLA_RMNET_FLAGS handling in iproute2 et al.

I like this a lot.  It should have been implemented this way
to begin with; there's not much point to have the mask if
it's only applied to the passed-in value.

KS, are you aware of *any* existing user space code that
would not work correctly if this were accepted?

I.e., the way it was (is), the value passed in *assigns*
the data format flags.  But with Bjorn's changes, the
data format flags would be *updated* (i.e., any bits not
set in the mask field would remain with their previous
value).

Reviewed-by: Alex Elder <elder@linaro.org>

> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> ---
>   drivers/net/ethernet/qualcomm/rmnet/rmnet_config.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/qualcomm/rmnet/rmnet_config.c b/drivers/net/ethernet/qualcomm/rmnet/rmnet_config.c
> index 8d51b0cb545c..2c8db2fcc53d 100644
> --- a/drivers/net/ethernet/qualcomm/rmnet/rmnet_config.c
> +++ b/drivers/net/ethernet/qualcomm/rmnet/rmnet_config.c
> @@ -336,7 +336,8 @@ static int rmnet_changelink(struct net_device *dev, struct nlattr *tb[],
>   
>   		old_data_format = port->data_format;
>   		flags = nla_data(data[IFLA_RMNET_FLAGS]);
> -		port->data_format = flags->flags & flags->mask;
> +		port->data_format &= ~flags->mask;
> +		port->data_format |= flags->flags & flags->mask;
>   
>   		if (rmnet_vnd_update_dev_mtu(port, real_dev)) {
>   			port->data_format = old_data_format;
> 

Re: [PATCH] net: qualcomm: rmnet: Allow partial updates of IFLA_FLAGS

[subashab]

On 2021-04-22 12:29, Alex Elder wrote:
> On 4/22/21 1:20 PM, Bjorn Andersson wrote:
>> The idiomatic way to handle the changelink flags/mask pair seems to be
>> allow partial updates of the driver's link flags. In contrast the 
>> rmnet
>> driver masks the incoming flags and then use that as the new flags.
>> 
>> Change the rmnet driver to follow the common scheme, before the
>> introduction of IFLA_RMNET_FLAGS handling in iproute2 et al.
> 
> I like this a lot.  It should have been implemented this way
> to begin with; there's not much point to have the mask if
> it's only applied to the passed-in value.
> 
> KS, are you aware of *any* existing user space code that
> would not work correctly if this were accepted?
> 
> I.e., the way it was (is), the value passed in *assigns*
> the data format flags.  But with Bjorn's changes, the
> data format flags would be *updated* (i.e., any bits not
> set in the mask field would remain with their previous
> value).
> 
> Reviewed-by: Alex Elder <elder@linaro.org>

What rmnet functionality which was broken without this change.
That doesnt seem to be listed in this patch commit text.

If this is an enhancement, then patch needs to be targeted to net-next
instead of net

Re: [PATCH] net: qualcomm: rmnet: Allow partial updates of IFLA_FLAGS

[Alex Elder]

On 4/22/21 6:28 PM, subashab@codeaurora.org wrote:
> On 2021-04-22 12:29, Alex Elder wrote:
>> On 4/22/21 1:20 PM, Bjorn Andersson wrote:
>>> The idiomatic way to handle the changelink flags/mask pair seems to be
>>> allow partial updates of the driver's link flags. In contrast the rmnet
>>> driver masks the incoming flags and then use that as the new flags.
>>>
>>> Change the rmnet driver to follow the common scheme, before the
>>> introduction of IFLA_RMNET_FLAGS handling in iproute2 et al.
>>
>> I like this a lot.  It should have been implemented this way
>> to begin with; there's not much point to have the mask if
>> it's only applied to the passed-in value.
>>
>> KS, are you aware of *any* existing user space code that
>> would not work correctly if this were accepted?
>>
>> I.e., the way it was (is), the value passed in *assigns*
>> the data format flags.  But with Bjorn's changes, the
>> data format flags would be *updated* (i.e., any bits not
>> set in the mask field would remain with their previous
>> value).
>>
>> Reviewed-by: Alex Elder <elder@linaro.org>
> 
> What rmnet functionality which was broken without this change.
> That doesnt seem to be listed in this patch commit text.

The broken functionality is that RMNet is not using the
value/flag pair in the proper way.

Currently, the RMNet driver assigns the flags value,
and (strangly) applies the mask to that value.

The intent of the value/flag pair interface is to allow
a value to be provided, with a mask of bits that indicate
which bits in the value should be *updated* in the target
field stored in the kernel.

That way, one can *assign* a value (by providing a value
with flag value 0xffffffff), but one can also update one
or any number of bits, preserving existing values.

It means, for example, that a request can preserve
existing settings, while *adding* a receive checksum
offload.

> If this is an enhancement, then patch needs to be targeted to net-next
> instead of net

Bjorn targeted neither net nor net-next.  He just posted
the patch.  I think it could be either.

					-Alex

Re: [PATCH] net: qualcomm: rmnet: Allow partial updates of IFLA_FLAGS

[Bjorn Andersson]

On Thu 22 Apr 18:28 CDT 2021, subashab@codeaurora.org wrote:

> On 2021-04-22 12:29, Alex Elder wrote:
> > On 4/22/21 1:20 PM, Bjorn Andersson wrote:
> > > The idiomatic way to handle the changelink flags/mask pair seems to be
> > > allow partial updates of the driver's link flags. In contrast the
> > > rmnet
> > > driver masks the incoming flags and then use that as the new flags.
> > > 
> > > Change the rmnet driver to follow the common scheme, before the
> > > introduction of IFLA_RMNET_FLAGS handling in iproute2 et al.
> > 
> > I like this a lot.  It should have been implemented this way
> > to begin with; there's not much point to have the mask if
> > it's only applied to the passed-in value.
> > 
> > KS, are you aware of *any* existing user space code that
> > would not work correctly if this were accepted?
> > 
> > I.e., the way it was (is), the value passed in *assigns*
> > the data format flags.  But with Bjorn's changes, the
> > data format flags would be *updated* (i.e., any bits not
> > set in the mask field would remain with their previous
> > value).
> > 
> > Reviewed-by: Alex Elder <elder@linaro.org>
> 
> What rmnet functionality which was broken without this change.
> That doesnt seem to be listed in this patch commit text.
> 

I recently posted a patch to iproute2 extending the rmnet link handling
to handle IFLA_RMNET_FLAGS, in the discussion that followed this subject
came up. So nothing is broken, it's just that the current logic doesn't
make sense and I wanted to attempt to fix it before we start to use it
commonly distributed userspace software (iproute2, libqmi etc)

> If this is an enhancement, then patch needs to be targeted to net-next
> instead of net

Okay, please let me know what hoops you want me to jump through. I just
want the subject concluded so that I can respin my iproute2 patch
according to what we decide here.

Regards,
Bjorn

Re: [PATCH] net: qualcomm: rmnet: Allow partial updates of IFLA_FLAGS

[subashab]

> I recently posted a patch to iproute2 extending the rmnet link handling
> to handle IFLA_RMNET_FLAGS, in the discussion that followed this 
> subject
> came up. So nothing is broken, it's just that the current logic doesn't
> make sense and I wanted to attempt to fix it before we start to use it
> commonly distributed userspace software (iproute2, libqmi etc)

With this patch, passing IFLA_RMNET_FLAGS in newlink vs changelink will 
have
different behavior. Is that inline with your expectations.

I checked VLAN and it seems to be using the same behavior for both the 
operations.
While the patch itself is fine, I don't think its right to have 
different
behavior for the operations.

> Okay, please let me know what hoops you want me to jump through. I just
> want the subject concluded so that I can respin my iproute2 patch
> according to what we decide here.

My suggestion is to have the subject prefix as [PATCH net-next] since 
this
is an enhancement rather than fixing something which is broken.