Message-ID: <CAGRyCJHjre7BByjcHt2XC5AH=Ta0jHGiPYAt=gDy71t=2PqOmw@mail.gmail.com>
Date:   Mon, 5 Dec 2016 14:04:24 +0100
From:   Daniele Palmas <dnlplm@...il.com>
To:     Bjørn Mork <bjorn@...k.no>
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

Hi,

2016-12-05 11:10 GMT+01:00 Bjørn Mork <bjorn@...k.no>:
> 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:
>

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

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

Thanks,
Daniele

> 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