Re: [PATCH 1/1] NET: usb: cdc_ncm: adding MBIM RESET_FUNCTION request and modifying ncm bind common code

2016-12-05 14:04 GMT+01:00 Daniele Palmas [off-list ref]:

Hi,

2016-12-05 11:10 GMT+01:00 Bjørn Mork [off-list ref]:

quoted

Daniele Palmas [off-list ref] writes:

quoted

I went back to this and further checking revealed that MBIM function
reset is not needed and the only problem is related to commit
48906f62c96cc2cd35753e59310cb70eb08cc6a5 cdc_ncm: toggle altsetting to
force reset before setup, that, however, is mandatory for some Huawei
modems.

Interesting.  That does sound like an odd firmware bug to me. I have a
bit of a hard time understanding how this can be.  Using data interface
altsetting 0 to reset the function is documented in the NCM spec:

Agree, this is likely a firmware bug and it is also proved by all the
other modems that accept the procedure without issues.

quoted

"
  7.2 Using Alternate Settings to Reset an NCM Function

  To place the network aspects of a function in a known state, the host
  shall:
  • select alternate setting 0 of the NCM Data Interface (this is the
    setting with no endpoints). This can be done explicitly using
    SetInterface, or implicitly using SetConfiguration. See [USB30] for
    details.
  • select the NCM operational parameters by sending commands to the NCM
    Communication Interface, then
  • select the second alternate interface setting of the NCM Data
    Interface (this is the setting with a bulk IN endpoint and a bulk
    OUT endpoint).

  Whenever alternate setting 0 is selected by the host, the function
  shall:
  • flush function buffers
  • reset the packet filter to its default state
  • clear all multicast address filters
  • clear all power filters set using
    SetEthernetPowerManagementPatternFilter
  • reset statistics counters to zero
  • restore its Ethernet address to its default state
  • reset its IN NTB size to the value given by field dwNtbInMaxSize
    from the NTB Parameter Struc- ture
  • reset the NTB format to NTB-16
  • reset the current Maximum Datagram Size to a function-specific
    default. If SetMaxDatagramSize is not supported, then the maximum
    datagram size shall be the same as the value in wMaxSegmentSize of
    the Ethernet Networking Functional Descriptor. If SetMaxDatagramSize
    is supported by the function, then the host must either query the
    function to determine the current effective maximum datagram size,
    or must explicitly set the maximum datagram size. If the host wishes
    to set the Maximum Datagram Size, it may do so prior to selecting
    the second alternate interface set- ting of the data
    interface. Doing so will ensure that the change takes effect prior
    to send or receiving data.
  • reset CRC mode so that the function will not append CRCs to
    datagrams sent on the IN pipe
  • reset NTB sequence numbers to zero

  When the host selects the second alternate interface setting of the
  NCM Data Interface, the function shall perform the following actions
  in the following order.
  • If connected to the network, the function shall send a
    ConnectionSpeedChange notification to the host indicating the
    current connection speed.
  • Whether connected or not, the function shall then send a
    NetworkConnection notification to the host, with wValue indicating
    the current state of network connectivity
"

The addition of the "RESET" request in MBIM did not change these
requirements AFAICS.  Supporting NCM style "altsetting 0 reset" is
required by default inheritance.  The description of dual NCM/MBIM
functions in the MBIM spec further emphasizes this:

"
  Regardless of the previous sequence of SET_INTERFACE commands targeting
  the Communication Interface, the host is able to put the function into
  a defined state by the following sequence:

  1. SET_INTERFACE(Data Interface, 0)
  2. SET_INTERFACE(Communication Interface, desired alternate setting)
  3. Sending the required class commands for the targeted alternate
    setting
  4. SET_INTERFACE(Data Interface, 1 if Communication Interface,0 and
    Data Interface,2 Communication Interface 1)
"


This, along with the lack of *any* sort of discussion of the
relation/interfaction between "MBIM RESET" and "data interface
altsetting 0" makes the MBIM RESET control request either ambigious or
redundant.  Or both...

FWIW, I've always considered RESET a symptom of the sloppy MBIM spec
development.  It did not exactly work to their advantage that the first
(and only?) published errata simply was an excuse to add new features
(the "MBIM extended functional descriptor" and "MBIM loopback testmode")
without updating the revision number.  Causing even more confusion,
since we now have two different MBIMv1.0 specs.

Well, whatever.  No need to get all frustrated about that. We have to
deal with whatever the firmware developers serve us.  And,
unfortunately, it is not surprising that unclear and ambigious specs
leads to incompatible firmware implementations.

I wonder, what happens if you unload the MBIM driver and let the USB
core reset the data interface to altsetting 0?  Will the device then
fail when the driver is loaded again?  If not, then where is the
difference?  And can we possibly reorder the driver set_interface
requests to make them similar to the USB core reset

I will check this.

I verified that the behavior does not change and data connection still
does not work, so I'm going to submit the new patch.

Regards,
Daniele

Thanks,
Daniele

quoted

Until now, I was under the impression that this was the
only documented way to reset both NCM and MBIM functions (since the
RESET is MBIM specific),

quoted

My proposal is to add something like
CDC_MBIM_FLAG_AVOID_ALTSETTING_TOGGLE quirk to avoid the toggle.

To have a conservative approach, I would add only Telit LE922 to this
quirk and keep also the usleep_range delay, since I do not have a
clear view on all the VIDs/PIDs affected by this quirk. Then other
modems, once tested, can be added to the quirk if the delay is not
enough.

If this approach, though ugly, has chances to be accepted I can write
a new patch.

Yes, that sounds like the best approach at the moment.  I have
unfortunately not as much time to experiment with this as I seem to
need.  And the EM7455 workaround (the delay) is a bit hard to verify,
since it is easily affected by small additional delays caused by the
debugging itself.  This can cause bogus test results.


Bjørn