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
F102934402: D30625.id.diff
Mon, Nov 18, 10:52 PM
Unknown Object (File)
Fri, Nov 15, 4:34 PM
Unknown Object (File)
Fri, Nov 15, 4:40 AM
Unknown Object (File)
Fri, Nov 15, 4:32 AM
Unknown Object (File)
Fri, Nov 15, 4:10 AM
Unknown Object (File)
Fri, Nov 15, 1:35 AM
Unknown Object (File)
Thu, Oct 31, 5:40 PM
Unknown Object (File)
Tue, Oct 29, 9:08 AM

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
rG FreeBSD src repository
Lint
Lint Not Applicable
Unit
Tests Not Applicable

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
815

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
1153

is actlen > sizeof buf ever expected to happen?

sys/dev/usb/net/if_cdce.c
1136–1146

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
815

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.

1136–1146

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).

1153

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
815

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

1153

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
1153

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
1153

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

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

As long as the compiler doesn't complain.

1136–1146

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
1136–1146

I have a feeling buf should be under

#ifdef USB_DEBUG
1152

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
815

As long as the compiler doesn't complain.

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

1136–1146

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.

1136–1146

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.