Date:   Wed, 7 Dec 2016 13:32:53 +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

2016-12-05 14:04 GMT+01:00 Daniele Palmas <dnlplm@...il.com>:
> 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.
>

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