| Message ID | 20210422182045.1040966-1-bjorn.andersson@linaro.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: qualcomm: rmnet: Allow partial updates of IFLA_FLAGS | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Guessed tree name to be net-next |
| netdev/subject_prefix | warning | Target tree name not specified in the subject |
| netdev/cc_maintainers | success | CCed 5 of 5 maintainers |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 9 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/header_inline | success | Link |
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; >
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
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
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
> 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.
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;
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(-)