| Message ID | 20231026-ethtool_puts_impl-v2-1-0d67cbdd0538@google.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | ethtool: Add ethtool_puts() | expand |
On Thu, Oct 26, 2023 at 3:02 PM Vladimir Oltean <olteanv@gmail.com> wrote: > > Hi Justin, > > On Thu, Oct 26, 2023 at 09:56:07PM +0000, Justin Stitt wrote: > > Use strscpy() to implement ethtool_puts(). > > > > Functionally the same as ethtool_sprintf() when it's used with two > > arguments or with just "%s" format specifier. > > > > Signed-off-by: Justin Stitt <justinstitt@google.com> > > --- > > include/linux/ethtool.h | 34 +++++++++++++++++++++++----------- > > net/ethtool/ioctl.c | 7 +++++++ > > 2 files changed, 30 insertions(+), 11 deletions(-) > > > > diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h > > index 226a36ed5aa1..7129dd2e227c 100644 > > --- a/include/linux/ethtool.h > > +++ b/include/linux/ethtool.h > > @@ -1053,22 +1053,34 @@ static inline int ethtool_mm_frag_size_min_to_add(u32 val_min, u32 *val_add, > > */ > > extern __printf(2, 3) void ethtool_sprintf(u8 **data, const char *fmt, ...); > > > > +/** > > + * ethtool_puts - Write string to ethtool string data > > + * @data: Pointer to start of string to update > > + * @str: String to write > > + * > > + * Write string to data. Update data to point at start of next > > + * string. > > + * > > + * Prefer this function to ethtool_sprintf() when given only > > + * two arguments or if @fmt is just "%s". > > + */ > > +extern void ethtool_puts(u8 **data, const char *str); > > + > > /* Link mode to forced speed capabilities maps */ > > struct ethtool_forced_speed_map { > > - u32 speed; > > + u32 speed; > > __ETHTOOL_DECLARE_LINK_MODE_MASK(caps); > > > > - const u32 *cap_arr; > > - u32 arr_size; > > + const u32 *cap_arr; > > + u32 arr_size; > > }; > > > > -#define ETHTOOL_FORCED_SPEED_MAP(prefix, value) \ > > -{ \ > > - .speed = SPEED_##value, \ > > - .cap_arr = prefix##_##value, \ > > - .arr_size = ARRAY_SIZE(prefix##_##value), \ > > -} > > +#define ETHTOOL_FORCED_SPEED_MAP(prefix, value) \ > > + { \ > > + .speed = SPEED_##value, .cap_arr = prefix##_##value, \ > > + .arr_size = ARRAY_SIZE(prefix##_##value), \ > > + } > > > > -void > > -ethtool_forced_speed_maps_init(struct ethtool_forced_speed_map *maps, u32 size); > > +void ethtool_forced_speed_maps_init(struct ethtool_forced_speed_map *maps, > > + u32 size); > > #endif /* _LINUX_ETHTOOL_H */ > > Maybe this is due to an incorrect rebase conflict resolution, but you > shouldn't have touched any of the ethtool force speed maps. Ah, I did have a conflict and resolved by simply moving the hunks out of each other's way. Trivial resolution. Should I undo this? I want my patch against next since it's targeting some stuff in-flight over there. BUT, I also want ethtool_puts() to be directly below ethtool_sprintf() in the source code. What to do? > > Please wait for at least 24 hours to pass before posting a new version, > to allow for more comments to come in. Ok :) Thanks Justin
On Thu, Oct 26, 2023 at 3:09 PM Justin Stitt <justinstitt@google.com> wrote: > > On Thu, Oct 26, 2023 at 3:02 PM Vladimir Oltean <olteanv@gmail.com> wrote: > > > > Hi Justin, > > > > On Thu, Oct 26, 2023 at 09:56:07PM +0000, Justin Stitt wrote: > > > Use strscpy() to implement ethtool_puts(). > > > > > > Functionally the same as ethtool_sprintf() when it's used with two > > > arguments or with just "%s" format specifier. > > > > > > Signed-off-by: Justin Stitt <justinstitt@google.com> > > > --- > > > include/linux/ethtool.h | 34 +++++++++++++++++++++++----------- > > > net/ethtool/ioctl.c | 7 +++++++ > > > 2 files changed, 30 insertions(+), 11 deletions(-) > > > > > > diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h > > > index 226a36ed5aa1..7129dd2e227c 100644 > > > --- a/include/linux/ethtool.h > > > +++ b/include/linux/ethtool.h > > > @@ -1053,22 +1053,34 @@ static inline int ethtool_mm_frag_size_min_to_add(u32 val_min, u32 *val_add, > > > */ > > > extern __printf(2, 3) void ethtool_sprintf(u8 **data, const char *fmt, ...); > > > > > > +/** > > > + * ethtool_puts - Write string to ethtool string data > > > + * @data: Pointer to start of string to update > > > + * @str: String to write > > > + * > > > + * Write string to data. Update data to point at start of next > > > + * string. > > > + * > > > + * Prefer this function to ethtool_sprintf() when given only > > > + * two arguments or if @fmt is just "%s". > > > + */ > > > +extern void ethtool_puts(u8 **data, const char *str); > > > + > > > /* Link mode to forced speed capabilities maps */ > > > struct ethtool_forced_speed_map { > > > - u32 speed; > > > + u32 speed; > > > __ETHTOOL_DECLARE_LINK_MODE_MASK(caps); > > > > > > - const u32 *cap_arr; > > > - u32 arr_size; > > > + const u32 *cap_arr; > > > + u32 arr_size; > > > }; > > > > > > -#define ETHTOOL_FORCED_SPEED_MAP(prefix, value) \ > > > -{ \ > > > - .speed = SPEED_##value, \ > > > - .cap_arr = prefix##_##value, \ > > > - .arr_size = ARRAY_SIZE(prefix##_##value), \ > > > -} > > > +#define ETHTOOL_FORCED_SPEED_MAP(prefix, value) \ > > > + { \ > > > + .speed = SPEED_##value, .cap_arr = prefix##_##value, \ > > > + .arr_size = ARRAY_SIZE(prefix##_##value), \ > > > + } > > > > > > -void > > > -ethtool_forced_speed_maps_init(struct ethtool_forced_speed_map *maps, u32 size); > > > +void ethtool_forced_speed_maps_init(struct ethtool_forced_speed_map *maps, > > > + u32 size); > > > #endif /* _LINUX_ETHTOOL_H */ > > > > Maybe this is due to an incorrect rebase conflict resolution, but you > > shouldn't have touched any of the ethtool force speed maps. > > Ah, I did have a conflict and resolved by simply moving the hunks > out of each other's way. Trivial resolution. > > Should I undo this? I want my patch against next since it's targeting > some stuff in-flight over there. BUT, I also want ethtool_puts() to be > directly below ethtool_sprintf() in the source code. What to do? Oh, I just realized my auto formatter had a field day with that function. I will rectify this in a new version after waiting 24hrs for comments to trickle in as well. > > > > > Please wait for at least 24 hours to pass before posting a new version, > > to allow for more comments to come in. > > Ok :) > > Thanks > Justin Thanks Justin
On Thu, Oct 26, 2023 at 3:25 PM Vladimir Oltean <olteanv@gmail.com> wrote: > > On Thu, Oct 26, 2023 at 03:09:59PM -0700, Justin Stitt wrote: > > Should I undo this? I want my patch against next since it's targeting > > some stuff in-flight over there. BUT, I also want ethtool_puts() to be > > directly below ethtool_sprintf() in the source code. What to do? > > (removing everyone except the lists from CC, I don't want to go to email > arest because of spamming too many recipients) > > What is the stuff in-flight in next that this is targeting? > > And why would anything prevent you from putting ethtool_puts() directly > below ethtool_sprintf()? The in-flight stuff consists of patches I sent changing some strncpy() usage to ethtool_sprintf(&data, "%s", something[i].name); We can see them here [1]. I went for this approach initially but then discussion came up about introducing ethtool_puts() which now made my patches (some accepted into next already) semi-outdated and in need of another swap from sprintf->puts() -- hence this series. As far as the rebase, I simply took my commits and placed them on top of next/master and got merge conflicts when ethtool_puts() was placed below ethtool_sprintf(). All I have to do is move the hunks around but since I formatted the file it's appearing in the diff. v3 will be a clean diff. [1]: https://lore.kernel.org/all/?q=dfb:ethtool_sprintf%20AND%20f:justinstitt Thanks Justin
diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h index 226a36ed5aa1..7129dd2e227c 100644 --- a/include/linux/ethtool.h +++ b/include/linux/ethtool.h @@ -1053,22 +1053,34 @@ static inline int ethtool_mm_frag_size_min_to_add(u32 val_min, u32 *val_add, */ extern __printf(2, 3) void ethtool_sprintf(u8 **data, const char *fmt, ...); +/** + * ethtool_puts - Write string to ethtool string data + * @data: Pointer to start of string to update + * @str: String to write + * + * Write string to data. Update data to point at start of next + * string. + * + * Prefer this function to ethtool_sprintf() when given only + * two arguments or if @fmt is just "%s". + */ +extern void ethtool_puts(u8 **data, const char *str); + /* Link mode to forced speed capabilities maps */ struct ethtool_forced_speed_map { - u32 speed; + u32 speed; __ETHTOOL_DECLARE_LINK_MODE_MASK(caps); - const u32 *cap_arr; - u32 arr_size; + const u32 *cap_arr; + u32 arr_size; }; -#define ETHTOOL_FORCED_SPEED_MAP(prefix, value) \ -{ \ - .speed = SPEED_##value, \ - .cap_arr = prefix##_##value, \ - .arr_size = ARRAY_SIZE(prefix##_##value), \ -} +#define ETHTOOL_FORCED_SPEED_MAP(prefix, value) \ + { \ + .speed = SPEED_##value, .cap_arr = prefix##_##value, \ + .arr_size = ARRAY_SIZE(prefix##_##value), \ + } -void -ethtool_forced_speed_maps_init(struct ethtool_forced_speed_map *maps, u32 size); +void ethtool_forced_speed_maps_init(struct ethtool_forced_speed_map *maps, + u32 size); #endif /* _LINUX_ETHTOOL_H */ diff --git a/net/ethtool/ioctl.c b/net/ethtool/ioctl.c index 0b0ce4f81c01..abdf05edf804 100644 --- a/net/ethtool/ioctl.c +++ b/net/ethtool/ioctl.c @@ -1991,6 +1991,13 @@ __printf(2, 3) void ethtool_sprintf(u8 **data, const char *fmt, ...) } EXPORT_SYMBOL(ethtool_sprintf); +void ethtool_puts(u8 **data, const char *str) +{ + strscpy(*data, str, ETH_GSTRING_LEN); + *data += ETH_GSTRING_LEN; +} +EXPORT_SYMBOL(ethtool_puts); + static int ethtool_phys_id(struct net_device *dev, void __user *useraddr) { struct ethtool_value id;
Use strscpy() to implement ethtool_puts(). Functionally the same as ethtool_sprintf() when it's used with two arguments or with just "%s" format specifier. Signed-off-by: Justin Stitt <justinstitt@google.com> --- include/linux/ethtool.h | 34 +++++++++++++++++++++++----------- net/ethtool/ioctl.c | 7 +++++++ 2 files changed, 30 insertions(+), 11 deletions(-)