Page MenuHomeFreeBSD

add support for media and link change events to CDCE.
AcceptedPublic

Authored by jmg on Thu, Jun 3, 6:05 AM.

Details

Reviewers
hselasky
Summary

This adds support for NetworkConnection (connection state), and
ConnectionSpeedChange (speed). Both of these are defined in CDC
1.2, 6.3. Both of these are required messages per ECM v1.2, so
this shouldn't break anything as ALL CDCE devices should support
these messages.

This means that devd now will properly run dhclient for configuration
of these devices, as devd only launches on link up events for
ethernet interfaces, which require the media and link support that
was added.

I have tested this w/ RealTek 2.5G devices that aren't supported
by the if_ure driver yet, and so attach to the cdce driver.

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 39677
Build 36566: arc lint + arc unit

Event Timeline

jmg requested review of this revision.Thu, Jun 3, 6:05 AM
hselasky added a subscriber: hselasky.
hselasky added inline comments.
sys/dev/usb/net/if_cdce.c
802

no need to set error = 0, when error is set in all cases below.

This revision is now accepted and ready to land.Thu, Jun 3, 7:31 AM
emaste added inline comments.
sys/dev/usb/net/if_cdce.c
1140

is actlen > sizeof buf ever expected to happen?

sys/dev/usb/net/if_cdce.c
1123

If you use:

buf[CDCE_IND_SIZE_MAX]

The MIN() check below won't be needed.

--HPS

jmg marked an inline comment as done.Thu, Jun 3, 5:57 PM

Let me know more about the CDCE_IND_SIZE_MAX constant.

sys/dev/usb/net/if_cdce.c
802

Combo of copying what other _ioctl's routines do (if_ure) and not noticing this. I'd prefer to keep it, to match a few others.

Appears we have three different styles in the various network drivers, one is this, if cases, and direct return of value.

If I remove it, it'd deviate and create a new pattern. If someone wants to standardize how the _ioctl functions run, they are free to do it here.

1123

Do you know where this constant came from? I can't find it in the CDC spec. I GUESS if all three messages specified by the spec were sent and sent only once, that would add up to 32, BUT this prevents future other messages from being sent (I didn't check a newer version of the CDCE spec).

Hmm.. I'm split on this. the max size for the function is a bit divorced and so not immediately clear, and I'm more inclined to write defensively, especially when small checks like these will not significantly impact performance (re MIN usage).

1140

per previous, I don't know, and prefer not to assume things that could end up causing buffer overruns.

sys/dev/usb/net/if_cdce.c
802

Personally I'd like to adopt early-return in style(9) but that's certainly a bigger discussion than this driver.

1140

Yes, definitely do not want a buffer overrun due to an assumption. But, I mean should we emit an error? Should we discard the buffer rather than processing part of it?

jmg marked an inline comment as done.Thu, Jun 3, 7:43 PM
jmg added inline comments.
sys/dev/usb/net/if_cdce.c
1140

I think you're missing that this is a debug statement. The user should know/understand when the bytes is larger than the output.

sys/dev/usb/net/if_cdce.c
1140

Ah, indeed. I noticed the DPRINTF but missed the USB_DEBUG_VAR above somehow

sys/dev/usb/net/if_cdce.c
802

As long as the compiler doesn't complain.

1123

This is just a software value. I think USB does not define this size.