system-linux: fix autoneg for 2.5G/5G/10G #2
Conversation
There was a problem hiding this comment.
You have to sign off yout changes with your real name.
https://openwrt.org/submitting-patches
You could also wrap your commit body message.
/usr/bin/clang-15 -I/include -I/usr/include/libnl3 -g -Wall -Werror -Wextra -Werror=implicit-function-declaration -Wformat -Werror=format-security -Werror=format-nonliteral -Os --std=gnu99 -Wmissing-declarations -Wno-unused-parameter -Wno-unused-but-set-parameter -Wimplicit-fallthrough -MD -MT CMakeFiles/netifd.dir/system-linux.c.o -MF CMakeFiles/netifd.dir/system-linux.c.o.d -o CMakeFiles/netifd.dir/system-linux.c.o -c system-linux.c
system-linux.c:1754:14: error: incompatible pointer to integer conversion assigning to '__u32' (aka 'unsigned int') from '__u32 *' (aka 'unsigned int *'); remove & [-Werror,-Wint-conversion]
advertising = &ecmd.link_mode_data[nwords];
^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~
system-linux.c:1755:9: error: incompatible integer to pointer conversion passing '__u32' (aka 'unsigned int') to parameter of type 'void *' [-Werror,-Wint-conversion]
memset(advertising, 0, sizeof(__u32) * nwords);
^~~~~~~~~~~
/usr/include/string.h:60:28: note: passing argument to parameter '__s' here
extern void *memset (void *__s, int __c, size_t __n) __THROW __nonnull ((1));
^
system-linux.c:1760:35: error: incompatible integer to pointer conversion passing '__u32' (aka 'unsigned int') to parameter of type '__u32 *' (aka 'unsigned int *'); take the address with & [-Werror,-Wint-conversion]
ethtool_link_mode_set_bit(bit, advertising);
^~~~~~~~~~~
&
system-linux.c:1699:69: note: passing argument to parameter 'mask' here
static inline int ethtool_link_mode_set_bit(unsigned int nr, __u32 *mask) |
| { | ||
| struct { | ||
| struct ethtool_link_settings req; | ||
| __u32 link_mode_data[3 * 127]; |
There was a problem hiding this comment.
Is there any reason, why are you inventing this custom struct content, magic numbers instead of copy&pasting the struct from the kernel https://github.com/torvalds/linux/blob/fcd476ea6a888ef6e6627f4c21a2ea8cca3e9312/net/ethtool/ioctl.c#L428 ?
There was a problem hiding this comment.
Well, you can't do that. The number of words used for the three mode bitfields has to be queried from the kernel at runtime and hence also determines the starting offsets of two of the three fields. Hence the best you can do is allocate enough space (3 * INT8_MAX * sizeof(__u32)) and then do some pointer juggling once nwords has been returned from the first call to ETHTOOL_GLINKSETTINGS.
| return 4; | ||
|
|
||
| nwords = ecmd.req.link_mode_masks_nwords; | ||
| supported = &ecmd.link_mode_data[0]; |
There was a problem hiding this comment.
ecmd.link_modes.supported would make more sense here
|
|
||
| nwords = ecmd.req.link_mode_masks_nwords; | ||
| supported = &ecmd.link_mode_data[0]; | ||
| advertising = &ecmd.link_mode_data[nwords]; |
There was a problem hiding this comment.
ecmd.link_modes.advertising would make more sense here
Sorry, I made a stupid mistake. Because need to follow project's coding style, I changed it directly through Notepad. now corrected |
|
It would be great to get this merged as AQR PHY-s actually check whether certain speeds are allowed to be advertised so 2500BaseT and 5000BaseT are not currently advertised and this makes them allowed again. |
|
@crwnet Do you have time to do the modifications? If not, I can do them as this has been a quite long-standing issue. |
ETHTOOL_GSET / ETHTOOL_SSET API is deprecated, migrate to ETHTOOL_GLINKSETTINGS / ETHTOOL_SLINKSETTINGS API to handle auto-negotiation for higher bandwidth like 2.5G, 5G, 10G Signed-off-by: Ruiwei Chen <crwbak@gmail.com>
|
LGTM, so: |
|
Closing in favor of #9 which has been merged |
ETHTOOL_GSET / ETHTOOL_SSET API is deprecated, migrate to ETHTOOL_GLINKSETTINGS / ETHTOOL_SLINKSETTINGS API to handle auto-negotiation for higher bandwidth like 2.5G, 5G, 10G