[review] aleksander/huawei-ndisdup-cdc-wdm-rebased-1.2

[Bjørn Mork]

Bjørn Mork <bjorn at mork.no> writes:

> [added Enrico to the Cc list]
>
> Dan Williams <dcbw at redhat.com> writes:
>
>> The rest looks fine; but my E3276 has some other issues that should get
>> fixed too...
>>
>> [1402602002.666216] [mm-port-serial.c:1237] mm_port_serial_open():
>> (cdc-wdm1) device open count is 3 (open)
>> [1402602002.666249] [mm-port-serial.c:1296] mm_port_serial_close():
>> (cdc-wdm1) device open count is 2 (close)
>> [1402602002.666283] [mm-port-serial-at.c:440] debug_log(): (cdc-wdm1):
>> --> 'AT^SYSCFGEX=?<CR><LF>'
>> [1402602002.670020] [mm-port-serial-at.c:440] debug_log(): (cdc-wdm1):
>> <-- '<CR><LF>^SYSCFGEX:
>> ("00","01","02","03","99"),((400380,"GSM900/GSM1800/WCDMA2100"),(6a80000,"GSM850/GSM1900/WCDMA850/AWS/WCDMA1900"),(3fffffff,"All bands")),(0-2),(0-4),((1081b,"LTE_B1/LTE_B2/LTE_B4/LTE_B5/LTE_B12/LTE_B17"),(7fffffffffffffff,"All bands"))<CR><LF><CR><LF>OK<CR>'
>> ModemManager[11313]: Error while checking ^SYSCFGEX format: Serial
>> command timed out
>> [1402602007.961186] [mm-port-serial-at.c:440] debug_log(): (cdc-wdm1):
>> --> 'AT^SYSCFG=?<CR><LF>'
>> [1402602007.962065] [mm-port-serial-at.c:440] debug_log(): (cdc-wdm1):
>> <-- '<LF>'
>> Error while checking ^SYSCFG format: Missing ^SYSCFG prefix
>>
>> Note the late-arriving <LF> that's meant for the ^SYSCFGEX; changing the
>> timeout doesn't help it arrive faster, for some reason the modem just
>> doesn't send the LF until later.  Is that a quirk of huawei_cdc_ncm,
>> that maybe it doesn't send the last byte until later or something?
>> Bjorn?
>
> Yes, that might actually be it.  I didn't think so when I started
> writing this reply, but then I checked the code :-)
>
> The huawei_cdc_ncm driver use cdc-wdm just like qmi_wwan and cdc_mbim.
> But failing to have any other guideline for minimum or maximum firmware
> message size, it uses 256 bytes buffers.  Which is just an arbitrary
> choice loosely based on the CDC-WMC spec, which requires wMaxCommand to
> be "at least 256 decimal (0x100)".
>
> And what do you know? Counting the bytes of that AT^SYSCFGEX=? reply I
> see exactly 257 bytes including the final <LF>....
>
> So, yes, it seems the driver will "mess up" this by splitting the read
> from the modem firmware into 2 transfers.  We just didn't imagine that
> would matter for AT commands, which by definition are supposed to be
> a stream of single bytes.
>
> The 256 byte choice is arbitrary as I said, so we can definitely change
> it if we have reason to believe some other value is better.  But I'm not
> sure this qualifies as a bug?  We will have to choose some maximum
> buffer size, and there is no way for us to know what the firmware is
> going to send.  So in theory we will have to cope with split messages,
> no matter how big we make the buffer.

Eyyh, thinking some more about this I wonder if cdc-wdm play well with
this strategy?  We cannot expect the firmware to notify us again about
the remaining bytes.  Which means that we won't read them until the next
time the firmware has something to tell us.  Which is probably what
screws up MM here?  The delay between the initial 256 bytes and the rest
is possibly infinite.

This is probably food for a generic cdc-wdm fixup: We should resubmit
the read URB if the last read returned a full buffer.

But I don't understand why the good USB-IF CDC committee members
couldn't just have put the message size into one of the unused fields of
the ResponseAvailable notification.  Both the wValue and the Data fields
are unused.  And the size would have fit nicely into wValue, so the size
of the notification wouldn't even have to increase.  It would have been
so much easier if we knew exactly what the firmware wanted to send us.
This would also have helped solve a problem I have with the EM7345,
which I currently don't think we can solve: It sometimes queues up
messages even if it cannot successfully notify the host, e.g. because
the driver decided to kill the interrupt URB just when the firmware was
about to send the notification.  Which may happen when userspace close
the /dev/cdc-wdmX device, but also during suspend - including
autosuspend. The result is that we start lagging behind, always reading
older messages than the ones we are notified about.  AFAICS, there is no
way for the driver to figure this out.  If we had the expected size then
we could have used that.  But as it is, we really do not know what to
expect, so we'll just have to trust that the firmware does everything
right.  And as I'm sure you are aware of: Firmware rarely does.


Bjørn