On Sat, Apr 23, 2011 at 12:05:39PM +0200, Hans Petter Selasky wrote:
> 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.
Why can't you send a patch based on the change of the above mentioned
#define?
thanks,
greg k-h
--
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
