Message-ID: <87a8ca1yat.fsf@miraculix.mork.no>
Date:   Mon, 05 Dec 2016 11:10:34 +0100
From:   Bjørn Mork <bjorn@...k.no>
To:     Daniele Palmas <dnlplm@...il.com>
Cc:     Oliver Neukum <oliver@...kum.org>,
        linux-usb <linux-usb@...r.kernel.org>, netdev@...r.kernel.org
Subject: Re: [PATCH 1/1] NET: usb: cdc_ncm: adding MBIM RESET_FUNCTION request and modifying ncm bind common code

Daniele Palmas <dnlplm@...il.com> writes:

> 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:

"
  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

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

> 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

Powered by blists - more mailing lists