On Saturday 23 April 2011 09:36:09 Matthias Benesch wrote:
> Am 22.04.2011 22:45, schrieb Hans Petter Selasky:
> > On Friday 22 April 2011 16:32:42 Matthias Benesch wrote:
> >> Hello,
> >>
> >> I am testing the USB-CDC NCM host driver. One issue that was found by
> >> one USB device stack provider, is that the current implementation is not
> >> sending ZLP or short packets if the dwNtbOutMaxSize reported by the
> >> device is greater than CDC_NCM_NTB_MAX_SIZE_TX (=16383 bytes).
> >>
> >> As written in the specification, ZLP shall not be sent "If exactly
> >> dwNtbInMaxSize or dwNtbOutMaxSize bytes are sent, and the size is a
> >> multiple of wMaxPacketSize for the given pipe".
> >>
> >> This requirement is covered in cdc_ncm.c after zero padding (line 858):
> >> /* final zero padding */
> >> cdc_ncm_zero_fill(skb_out->data, last_offset, offset,
> >> ctx->tx_max);
> >>
> >> /* store last offset */
> >> last_offset = offset;
> >>
> >> if ((last_offset< ctx->tx_max)&& ((last_offset %
> >>
> >> le16_to_cpu(ctx->out_ep->desc.wMaxPacketSize)) == 0)) {
> >>
> >> /* force short packet */
> >> *(((u8 *)skb_out->data) + last_offset) = 0;
> >> last_offset++;
> >>
> >> }
> >>
> >> Unfortunately "ctx->tx_max" ist not equal to dwNtbOutMaxSize if
> >> dwNtbOutMaxSize is greater than CDC_NCM_NTB_MAX_SIZE_TX, because it will
> >> be reset to CDC_NCM_NTB_MAX_SIZE_TX in that case (see line 220& 289).
> >>
> >> ctx->tx_max = le32_to_cpu(ctx->ncm_parm.dwNtbOutMaxSize);
> >>
> >> ...
> >>
> >> /* verify maximum size of transmitted NTB in bytes */
> >> if ((ctx->tx_max<
> >>
> >> (CDC_NCM_MIN_HDR_SIZE + CDC_NCM_MIN_DATAGRAM_SIZE)) ||
> >> (ctx->tx_max> CDC_NCM_NTB_MAX_SIZE_TX)) {
> >> pr_debug("Using default maximum transmit length=%d\n",
> >>
> >> CDC_NCM_NTB_MAX_SIZE_TX);
> >>
> >> ctx->tx_max = CDC_NCM_NTB_MAX_SIZE_TX;
> >>
> >> }
> >>
> >> Therefore the short packet will not be sent although the USB device
> >> stack is still waiting for dwNtbOutMaxSize bytes of data to be filled by
> >> the host.
> >>
> >> In my opinion possible solutions are:
> >>
> >> 1. Set CDC_NCM_NTB_MAX_SIZE_TX to the maximum amount of bytes supported
> >> by NCM16 (=65535 bytes). In addition to that I disabled the zero
> >> padding, due to the big overhead of data. With this configuration I
> >> achieved a maximum stable throughput of 131 Mbit/s of transmitting data.
> >>
> >> 2. Enable "urb->transfer_flags |= URB_ZERO_PACKET;" within usbnet.c
> >> (line 1091) if ZLP shall be sent by the NCM implementation. This could
> >> be done by an additional flag which shall be set within the cdc_ncm.c if
> >> needed.
> >>
> >> /* don't assume the hardware handles USB_ZERO_PACKET
> >>
> >> * NOTE: strictly conforming cdc-ether devices should expect
> >> * the ZLP here, but ignore the one-byte packet.
> >> * NOTE2: CDC NCM specification is different from CDC ECM when
> >> * handling ZLP/short packets, so cdc_ncm driver will make short
> >> * packet itself if needed.
> >> */
> >>
> >> if (length % dev->maxpacket == 0) {
> >>
> >> if (!(info->flags& FLAG_SEND_ZLP)) {
> >>
> >> if (!(info->flags& FLAG_MULTI_PACKET)) {
> >>
> >> urb->transfer_buffer_length++;
> >> if (skb_tailroom(skb)) {
> >>
> >> skb->data[skb->len] = 0;
> >> __skb_put(skb, 1);
> >>
> >> }
> >>
> >> }
> >>
> >> } else
> >>
> >> urb->transfer_flags |= URB_ZERO_PACKET;
> >>
> >> }
> >
> > Hi Matthias,
> >
> > How about setting
> >
> > #define CDC_NCM_NTB_MAX_SIZE_TX 16384 /* bytes */
> >
> > to 16385 bytes. Then if the maximum is exceeded a short packet will
> > always be sent, without affecting performance with regard to zero
> > padding?
> >
> > --HPS
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> > the body of a message to majordomo@xxxxxxxxxxxxxxx
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
>
> Hi Hans Petter,
>
> Thanks for the tip. I have not thought about a easy solution like that.
> I just tested to set
>
> #define CDC_NCM_NTB_MAX_SIZE_TX 16384 /* bytes */
>
> to 16383 bytes.
>
> I have to say that I am not quite familar with the DMA engines of USB
> chipsets. But is it usefull to to pad zeros up to the 16385 bytes if the
> USB device has reported a dwNtbOutMaxSize greater than that? Because
> within the source code is written that "it would be more efficient for
> USB HS mobile device with DMA engine to receive a full size NTB, than
> canceling DMA transfer and receiving a short packet." But this will not
> be reached, due to the fact that we are not zero padding up to
> dwNtbOutMaxSize.
Hi Matthias,
The answer to your question depends on the hardware and software solution
used. The idea behind the comment is to reduce the number of IRQ's in the
hardware on the device side. Short packets are sometimes handled specially and
can be PIO'ed out of the FIFO instead of DMA'ed. Then it is better to PIO 0-4
bytes (16385 case) instead of 511 bytes (16383 case). Again, this depends on
the USB hardware and USB software used.
>
> What is the minimum usefull amount of zero padding in that case? Is it
> maybe possible to just fill zeros up to the multiple of wMaxPacketSize?
> Or to do not use zero padding in case of dwNtbOutMaxSize >
> CDC_NCM_NTB_MAX_SIZE_TX?
There are two ways to cause short termination:
a) ZLP feature
b) append a byte when the transfer length modulus wMaxPacket equals zero.
Solution b) was used because it is more generic, though a ZLP should also work
with most device side hardware.
I guess some more people will fill in details on this thread when they are
back from easter vacation, CC'ed, and submit a patch to fix this issue.
--HPS
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
