| Message ID | 20240809132804.2339243-1-david.marchand@redhat.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Ilya Maximets |
| Headers | show |
| Series | [ovs-dev] netdev: Fix full duplex capability for 25G ports. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/intel-ovs-compilation | fail | test: fail |
On Fri, Aug 9, 2024 at 9:28 AM David Marchand <david.marchand@redhat.com> wrote: > > As OVS does not know of the 25G speed, matching on a list of known speed > for deducing full duplex capability is broken. > Invert the test and assume full duplex is the default. > > Suggested-by: Adrian Moreno <amorenoz@redhat.com> > Signed-off-by: David Marchand <david.marchand@redhat.com> > --- Assuming full duplex seems a lot more reasonable than assuming half. Acked-by: Mike Pattrick <mkp@redhat.com>
On 8/9/24 15:28, David Marchand wrote: > As OVS does not know of the 25G speed, matching on a list of known speed > for deducing full duplex capability is broken. > Invert the test and assume full duplex is the default. Hi, David. Thanks for the patch. But I'm not sure about the approach. This may solve the issue for a particular card you have, but may introduce the opposite issue for some other card. There are few things we can do: 1. Report 'full', 'half', 'unknown', where 'unknown' is reported for NETDEV_F_OTHER (netdev-linux may need a fix to report it). The function is currently used only for sFlow and the link status in the database. sFlow supports the "unknown" value. Database schema may be extended to accept "unknown" as a value. 2. Go further and actually request the duplex configuration from the netdev layer for NETDEV_F_OTHER. We had the same issue with the speed reporting and ended up with a netdev-provider API to request the numerical value where it is possible, i.e. not reported via OpenFlow. So, we can either add a second API callback for duplex or add another "output" argument to the speed API, since the speed and the duplex are usually part of the same device info anyway. What do you think? Best regards, Ilya Maximets.
diff --git a/lib/netdev.c b/lib/netdev.c index f2d921ed63..bdacfe5c49 100644 --- a/lib/netdev.c +++ b/lib/netdev.c @@ -1285,14 +1285,14 @@ netdev_features_to_bps(enum netdev_features features, : default_bps); } -/* Returns true if any of the NETDEV_F_* bits that indicate a full-duplex link - * are set in 'features', otherwise false. */ +/* Returns true if any of the NETDEV_F_* bits that indicate a half-duplex link + * are unset in 'features', otherwise false. */ bool netdev_features_is_full_duplex(enum netdev_features features) { - return (features & (NETDEV_F_10MB_FD | NETDEV_F_100MB_FD | NETDEV_F_1GB_FD - | NETDEV_F_10GB_FD | NETDEV_F_40GB_FD - | NETDEV_F_100GB_FD | NETDEV_F_1TB_FD)) != 0; + + return (features & (NETDEV_F_10MB_HD | NETDEV_F_100MB_HD + | NETDEV_F_1GB_HD)) == 0; } /* Set the features advertised by 'netdev' to 'advertise'. Returns 0 if
As OVS does not know of the 25G speed, matching on a list of known speed for deducing full duplex capability is broken. Invert the test and assume full duplex is the default. Suggested-by: Adrian Moreno <amorenoz@redhat.com> Signed-off-by: David Marchand <david.marchand@redhat.com> --- lib/netdev.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)