Re: Handling incoming ZLP in cdc-wdm

[Robert Hodaszi <robert.hodaszi@digi.com>]

On Wednesday, 26.03.2025 at 17:03 +0100, Hodaszi, Robert <robert.hodaszi@digi.com> wrote:
> Hi,
>
> (Sorry for the long mail, but want to describe exactly what is happening!)
>
> I'm having a weird error with a certain modem (Telit LE910C4) + ModemManager. In some circumstances (e.g. SIM switching while there are some other ongoing transactions already from ModemManager, or stopping ModemManager in the wrong moment (again, ongoing transactions)), I can make the qmi-proxy stuck, and can only SIGKILL it.
>
> qmi-proxy gets stuck in g_unix_input_stream_read() in glib.
>
> ModemManager tries to read an incoming message (/dev/cdc-wdm0), so it calls g_pollable_input_stream_default_read_nonblocking() (glib), which first checks if the stream is readable with g_poll(), and if there is, it reads the data in g_unix_input_stream_read() (glib).
>
> What g_unix_input_stream_read() does: it polls first (uninterruptible (EINTR) loop, this is where it gets stuck finally), then reads the data. If the read function returns with EINTR or EAGAIN, another loop starts, and goes back to poll().
>
> When the issue happens, the modem sends us a lot of zero-length packets. I see a 10+ INTERRUPT_IN URBs, without CONTROL_IN URB, because qmi-proxy is busy with close the connection (sending CONTROL_OUT URBs, and doing other things). In the INTERRUPT_IN URB's last 4 byte (cdc-wdm driver doesn't handle that), I can see the exact same number. I guess, this is the frame ID, as usually that gets incremented. So I think, modem tries to inform us about a pending message over-and-over. That's incrementing the desc->resp_count counter in the cdc-wdm driver.
>
> Finally qmi-proxy gets to the point to try to read in data (and call the aforementioned g_unix_input_stream_read()). But the modem is only sending back ZLPs, I suppose, because it informed us multiple times about the same pending packet, and it doesn't have anything more to send us (and it makes sense to send ZLP in this case).
>
> The problem is, wdm_poll() always return with EPOLLIN even when wdm_in_callback() receives a ZLP, as it sets WDM_READ. So it makes sense for glib to think, there's a pending packet. In wdm_read(), if the packet's length is 0 (desc->length = 0) and WDM_READ is set, we reach
>
>     if (!desc->length)
>
> line, where it puts out another URB (as the resp_count is not 0), clear WDM_READ and go back to "retry". The second time we test WDM_READ, it is obviously not set yet, and as we are reading non-blocking, the function returns with EAGAIN.
>
> And that is the issue here, as glib in this case thinks (with reason), that OK, it has to try to read the packet again, so it goes back to poll.
>
> Then another ZLP succeed, wdm_poll() returns with EPOLLIN, glib calls wdm_read(), which return EAGAIN, etc.
>
> Finally modem runs out from ZLPs as well, and has nothing to send us, so we just wait in wdm_poll(), and we cannot even interrupt this loop, as this is a non-blocking call, and EINTR is disabled.
>
> -----------------------------------
>
> I think, that should be fixed in cdc-wdm. So I'm wondering, what is the right approach here? Should we just return with success with a 0-length packet from wdm_read()? Consider ZLP as an error in wdm_in_callback, and schedule service_outs_intr work, like if desc->rerr is set? Other?
>
>
> Thanks,
> Robert Hodaszi
>
Following on this: returning 0 bytes for read would kill libqmi, as that handles that as an error as well ("connection broken").

So what about this patch?

    diff --git a/drivers/usb/class/cdc-wdm.c b/drivers/usb/class/cdc-wdm.c
    index 86ee39db013f..37873acd18f4 100644
    --- a/drivers/usb/class/cdc-wdm.c
    +++ b/drivers/usb/class/cdc-wdm.c
    @@ -214,6 +214,11 @@ static void wdm_in_callback(struct urb *urb)
            if (desc->rerr == 0 && status != -EPIPE)
                    desc->rerr = status;
     
    +       if (length == 0) {
    +               dev_dbg(&desc->intf->dev, "received ZLP\n");
    +               goto skip_error;
    +       }
    +
            if (length + desc->length > desc->wMaxCommand) {
                    /* The buffer would overflow */
                    set_bit(WDM_OVERFLOW, &desc->flags);
    @@ -227,10 +232,10 @@ static void wdm_in_callback(struct urb *urb)
            }
     skip_error:
     
    -       if (desc->rerr) {
    +       if (desc->rerr || length == 0) {
                    /*
    -                * Since there was an error, userspace may decide to not read
    -                * any data after poll'ing.
    +                * If there was a ZLP or an error, userspace may decide to not
    +                * read any data after poll'ing.
                     * We should respond to further attempts from the device to send
                     * data, so that we can get unstuck.
                     */

Best regards,
Robert Hodaszi