Messages in this thread | | | Subject | Re: [PATCH] net: qualcomm: rmnet: Allow partial updates of IFLA_FLAGS | From | Alex Elder <> | Date | Thu, 22 Apr 2021 20:01:23 -0500 |
| |
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
| |