| Message ID | 20220816222920.1952936-2-vladimir.oltean@nxp.com (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | 802.1Q Frame Preemption and 802.3 MAC Merge support via ethtool | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/fixes_present | success | Fixes tag not required for -next series |
| netdev/subject_prefix | success | Link |
| netdev/cover_letter | success | Series has a cover letter |
| netdev/patch_count | success | Link |
| netdev/header_inline | success | No static functions without inline keyword in header files |
| netdev/build_32bit | success | Errors and warnings before: 2212 this patch: 2212 |
| netdev/cc_maintainers | success | CCed 6 of 6 maintainers |
| netdev/build_clang | success | Errors and warnings before: 5 this patch: 5 |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Signed-off-by tag matches author and committer |
| netdev/check_selftest | success | No net selftest shell script |
| netdev/verify_fixes | success | No Fixes tag |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 2212 this patch: 2212 |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 32 lines checked |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/source_inline | success | Was 0 now: 0 |
On Wed, Aug 17, 2022 at 01:29:14AM +0300, Vladimir Oltean wrote: > For a reason I can't really understand, ethnl_update_bool32() exists, > but the plain function that operates on a boolean value kept in an > actual u8 netlink attribute doesn't. I can explain that: at the moment these helpers were introduced, only members of traditional structures shared with ioctl interface were updated and all attributes which were booleans logically were represented as u32 in them so that no other helper was needed back then. > Introduce it; it's needed for > things like verify-disabled for the MAC merge configuration. > > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> > --- > net/ethtool/netlink.h | 26 ++++++++++++++++++++++++++ > 1 file changed, 26 insertions(+) > > diff --git a/net/ethtool/netlink.h b/net/ethtool/netlink.h > index c0d587611854..1653fd2cf0cf 100644 > --- a/net/ethtool/netlink.h > +++ b/net/ethtool/netlink.h > @@ -111,6 +111,32 @@ static inline void ethnl_update_u8(u8 *dst, const struct nlattr *attr, > *mod = true; > } > > +/** > + * ethnl_update_bool() - update bool from NLA_U8 attribute > + * @dst: value to update > + * @attr: netlink attribute with new value or null > + * @mod: pointer to bool for modification tracking > + * > + * Use the u8 value from NLA_U8 netlink attribute @attr to set bool variable > + * pointed to by @dst to false (if zero) or 1 (if not); do nothing if @attr is ^ true Looks good otherwise. Michal > + * null. Bool pointed to by @mod is set to true if this function changed the > + * logical value of *dst, otherwise it is left as is. > + */ > +static inline void ethnl_update_bool(bool *dst, const struct nlattr *attr, > + bool *mod) > +{ > + u8 val; > + > + if (!attr) > + return; > + val = !!nla_get_u8(attr); > + if (*dst == val) > + return; > + > + *dst = val; > + *mod = true; > +} > + > /** > * ethnl_update_bool32() - update u32 used as bool from NLA_U8 attribute > * @dst: value to update > -- > 2.34.1 >
On Wed, Aug 17, 2022 at 01:27:29PM +0200, Michal Kubecek wrote: > On Wed, Aug 17, 2022 at 01:29:14AM +0300, Vladimir Oltean wrote: > > For a reason I can't really understand, ethnl_update_bool32() exists, > > but the plain function that operates on a boolean value kept in an > > actual u8 netlink attribute doesn't. > > I can explain that: at the moment these helpers were introduced, only > members of traditional structures shared with ioctl interface were > updated and all attributes which were booleans logically were > represented as u32 in them so that no other helper was needed back then. Thanks, but the internal data structures of the kernel did not necessitate boolean netlink attributes to be promoted to u32 just because the ioctl interface did it that way; or did they? Or otherwise said, is there a technical requirement that if a boolean is passed to the kernel as u32 via ioctl, it should be passed as u32 via netlink too?
On Wed, Aug 17, 2022 at 02:52:56PM +0300, Vladimir Oltean wrote: > On Wed, Aug 17, 2022 at 01:27:29PM +0200, Michal Kubecek wrote: > > On Wed, Aug 17, 2022 at 01:29:14AM +0300, Vladimir Oltean wrote: > > > For a reason I can't really understand, ethnl_update_bool32() exists, > > > but the plain function that operates on a boolean value kept in an > > > actual u8 netlink attribute doesn't. > > > > I can explain that: at the moment these helpers were introduced, only > > members of traditional structures shared with ioctl interface were > > updated and all attributes which were booleans logically were > > represented as u32 in them so that no other helper was needed back then. > > Thanks, but the internal data structures of the kernel did not > necessitate boolean netlink attributes to be promoted to u32 just > because the ioctl interface did it that way; or did they? > > Or otherwise said, is there a technical requirement that if a boolean is > passed to the kernel as u32 via ioctl, it should be passed as u32 via > netlink too? Ah, don't mind me... By the time I wrote this email, I forgot that ethnl_update_bool32() also calls nla_get_u8(). All clear now.
On Wed, Aug 17, 2022 at 12:00:37PM +0000, Vladimir Oltean wrote: > On Wed, Aug 17, 2022 at 02:52:56PM +0300, Vladimir Oltean wrote: > > On Wed, Aug 17, 2022 at 01:27:29PM +0200, Michal Kubecek wrote: > > > On Wed, Aug 17, 2022 at 01:29:14AM +0300, Vladimir Oltean wrote: > > > > For a reason I can't really understand, ethnl_update_bool32() exists, > > > > but the plain function that operates on a boolean value kept in an > > > > actual u8 netlink attribute doesn't. > > > > > > I can explain that: at the moment these helpers were introduced, only > > > members of traditional structures shared with ioctl interface were > > > updated and all attributes which were booleans logically were > > > represented as u32 in them so that no other helper was needed back then. > > > > Thanks, but the internal data structures of the kernel did not > > necessitate boolean netlink attributes to be promoted to u32 just > > because the ioctl interface did it that way; or did they? > > > > Or otherwise said, is there a technical requirement that if a boolean is > > passed to the kernel as u32 via ioctl, it should be passed as u32 via > > netlink too? > > Ah, don't mind me... By the time I wrote this email, I forgot that > ethnl_update_bool32() also calls nla_get_u8(). All clear now. Yes, it expects a NLA_U8 attribute. But it does not really matter that much as even NLA_U8 still occupies 4 bytes in the message because of the padding. (Which is why I generally prefer using NLA_U32 attributes unless the value is naturally 8- or 16-bit and we are absolutely sure we will never need more.) Michal
diff --git a/net/ethtool/netlink.h b/net/ethtool/netlink.h index c0d587611854..1653fd2cf0cf 100644 --- a/net/ethtool/netlink.h +++ b/net/ethtool/netlink.h @@ -111,6 +111,32 @@ static inline void ethnl_update_u8(u8 *dst, const struct nlattr *attr, *mod = true; } +/** + * ethnl_update_bool() - update bool from NLA_U8 attribute + * @dst: value to update + * @attr: netlink attribute with new value or null + * @mod: pointer to bool for modification tracking + * + * Use the u8 value from NLA_U8 netlink attribute @attr to set bool variable + * pointed to by @dst to false (if zero) or 1 (if not); do nothing if @attr is + * null. Bool pointed to by @mod is set to true if this function changed the + * logical value of *dst, otherwise it is left as is. + */ +static inline void ethnl_update_bool(bool *dst, const struct nlattr *attr, + bool *mod) +{ + u8 val; + + if (!attr) + return; + val = !!nla_get_u8(attr); + if (*dst == val) + return; + + *dst = val; + *mod = true; +} + /** * ethnl_update_bool32() - update u32 used as bool from NLA_U8 attribute * @dst: value to update
For a reason I can't really understand, ethnl_update_bool32() exists, but the plain function that operates on a boolean value kept in an actual u8 netlink attribute doesn't. Introduce it; it's needed for things like verify-disabled for the MAC merge configuration. Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> --- net/ethtool/netlink.h | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+)