Page MenuHomeFreeBSD

add support for media and link change events to CDCE.
Needs ReviewPublic

Authored by jmg on Jun 3 2021, 6:05 AM.
Tags
None
Referenced Files
F102629835: D30625.diff
Fri, Nov 15, 1:35 AM
Unknown Object (File)
Thu, Oct 31, 5:40 PM
Unknown Object (File)
Tue, Oct 29, 9:08 AM
Unknown Object (File)
Tue, Oct 22, 1:11 AM
Unknown Object (File)
Sep 26 2024, 7:00 PM
Unknown Object (File)
Sep 19 2024, 2:17 AM
Unknown Object (File)
Sep 13 2024, 3:40 AM
Unknown Object (File)
Sep 12 2024, 7:33 PM

Details

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 Passed
Unit
No Test Coverage
Build Status
Buildable 39677
Build 36566: arc lint + arc unit

Event Timeline

jmg requested review of this revision.Jun 3 2021, 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.Jun 3 2021, 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.Jun 3 2021, 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.Jun 3 2021, 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.

update to support VLAN MTU...

This revision now requires review to proceed.Jun 24 2021, 6:06 AM
sys/dev/usb/net/if_cdce.c
1123–1133

I have a feeling buf should be under

#ifdef USB_DEBUG
1139

Ditto.

  • make compile w/o USB_DEBUG defined...
jmg marked 9 inline comments as done.Jun 24 2021, 10:33 PM

Thanks for catching the compile issue. I hacked the modules to compile w/ debug, so missed the bug compiling w/o.

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

As long as the compiler doesn't complain.

yeah, doesn't seem to generate any warnings/errors.

1123

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

ok, yeah, that's what I thought. I have a feeling this might need to be increased if we start sending commands and getting data back from it, which may be necessary if we want to do additional things w/ it.

1123–1133

I have a feeling buf should be under

#ifdef USB_DEBUG

yes, it was. I did an #else and defined cdce_debug to 0 instead of putting ifdef in the body of the code. IMO, having the compiler eliminate the dead code makes the code easier to read than adding a bunch of ifdef's (one to define the var buf, and a second for the actual code.

This revision is now accepted and ready to land.Jun 25 2021, 8:03 AM

This breaks Sierra Wireless 3G/4G cellular modems:
MDM6200
MDM6270
MDM8200
MDM8200A
MDM8220
MDM9200
MSM6290
QSC6270

Tested not working on MC7710.

As stated in the "USB Driver Developer’s Guide" Rev 5 2130634 (November 2011) @ page 102 table B-2 CDC commands NETWORK_CONNECTION and CONNECTION_SPEED_CHANGE are not supported. Given the lack of QMI/MBIM support for "newer" 4G modems in FreeBSD, this makes also older DirectIP ECM devices like the MC77xx not usable outside the stupid and slow PPP mode. not suitable for 4G speeds.