Hi Yauheni,
Some generic comments to the code submitted.
The code uses copied parts from other files, but it would be nice to
have unified data types through ncm gadget code u32, uint32_t, u16,
uint16_t, unsigned, etc. Since this is a new submission, could code
use Linux types u32/u16/u8? I assume we are not exporting any data
to user space directly, so these types could be used without limitation.
> +struct f_ncm {
> + struct gether port;
> + u8 ctrl_id, data_id;
> +
> + char ethaddr[14];
14 -> ETH_HLEN and some other places where '14' is used.
> +/* peak (theoretical) bulk transfer rate in bits-per-second */
> +static inline unsigned ncm_bitrate(struct usb_gadget *g)
> +{
> + if (gadget_is_dualspeed(g) && g->speed == USB_SPEED_HIGH)
> + return 13 * 512 * 8 * 1000 * 8;
> + else
> + return 19 * 64 * 1 * 1000 * 8;
> +}
If gadget is providing 2G/3G/LTE wireless connection to the USB host,
it would be better to know the max theoretical throughput for radio
link, not USB which is known to the host anyway. Since it is impossible
to get such information from radio modem API, a separate #define for
uplink and downlink would do the trick.
> +/*
> + * We cannot group frames so use just the minimal size which ok to put
> + * one max-size ethernet frame.
> + * If the host can group frames, allow it to do that, 16K is selected,
> + * because it's used by default by the current linux host driver
> + */
> +#define NTB_DEFAULT_IN_SIZE USB_CDC_NCM_NTB_MIN_IN_SIZE
> +#define NTB_OUT_SIZE 16384
Should constants have corresponding names, i.e. NTB_DEFAULT_OUT_SIZE?
If gadget is not designed for constant data throughput 100+Mbps I would
suggest to keep 8K instead of 16K to reduce the latency.
> +static struct usb_cdc_ncm_ntb_parameters ntb_parameters = {
> + .wLength = sizeof ntb_parameters,
> + .bmNtbFormatsSupported = cpu_to_le16(FORMATS_SUPPORTED),
> + .dwNtbInMaxSize = cpu_to_le32(NTB_DEFAULT_IN_SIZE),
> + .wNdpInDivisor = cpu_to_le16(4),
> + .wNdpInPayloadRemainder = cpu_to_le16(0),
> + .wNdpInAlignment = cpu_to_le16(4),
> +
> + .dwNtbOutMaxSize = cpu_to_le32(NTB_OUT_SIZE),
> + .wNdpOutDivisor = cpu_to_le16(4),
> + .wNdpOutPayloadRemainder = cpu_to_le16(0),
> + .wNdpOutAlignment = cpu_to_le16(4),
Divisor shall be based on HW needs for cache alignment. In might be safer
to use 32 instead or make a configuration define.
> +};
> +
> +/*
> + * Use wMaxPacketSize big enough to fit CDC_NOTIFY_SPEED_CHANGE in one
> + * packet, to simplify cancellation; and a big transfer interval, to
> + * waste less bandwidth.
> + */
> +
> +#define LOG2_STATUS_INTERVAL_MSEC 5 /* 1 << 5 == 32 msec */
> +#define NCM_STATUS_BYTECOUNT 16 /* 8 byte header + data */
Since notifications are often of 16 bytes size, could INT EP size defined
as 32 instead to avoid sending maxpacket?
> +
> +#define INIT_NDP16_OPTS { \
> + .nth_sign = USB_CDC_NCM_NTH16_SIGN, \
> + .ndp_sign = USB_CDC_NCM_NDP16_NOCRC_SIGN, \
> + .nth_size = sizeof(struct usb_cdc_ncm_nth16), \
> + .ndp_size = sizeof(struct usb_cdc_ncm_ndp16), \
> + .ndplen_align = 4, \
> + .dgram_item_len = 1, \
> + .block_length = 1, \
> + .fp_index = 1, \
> + .reserved1 = 0, \
> + .reserved2 = 0, \
> + .next_fp_index = 1, \
> + }
Redundant multiplication by 2 in several places and loops can be
avoided, if we use a real data size, i.e. sizeof(u16) instead of 1.
See comment for get_ncm()
> +
> +
> +#define INIT_NDP32_OPTS { \
> + .nth_sign = USB_CDC_NCM_NTH32_SIGN, \
> + .ndp_sign = USB_CDC_NCM_NDP32_NOCRC_SIGN, \
> + .nth_size = sizeof(struct usb_cdc_ncm_nth32), \
> + .ndp_size = sizeof(struct usb_cdc_ncm_ndp32), \
> + .ndplen_align = 8, \
> + .dgram_item_len = 2, \
> + .block_length = 2, \
> + .fp_index = 2, \
> + .reserved1 = 1, \
> + .reserved2 = 2, \
> + .next_fp_index = 2, \
Same comment as for INIT_NDP16_OPTS.
> +static inline void put_ncm(__le16 **p, unsigned size, unsigned val)
> +{
> + switch (size) {
> + case 1:
See comment for get_ncm()
> + put_unaligned_le16((u16)val, *p);
> + break;
> + case 2:
See comment for get_ncm()
> +static inline unsigned get_ncm(__le16 **p, unsigned size)
> +{
> + unsigned tmp;
> +
> + switch (size) {
> + case 1:
Redundant multiplication by 2 in several places and loops can be
avoided, if we use a real data size here:
case sizeof(u16):
> + tmp = get_unaligned_le16(*p);
> + break;
> + case 2:
> + tmp = get_unaligned_le32(*p);
> + break;
Same comment as above,
case sizeof(u32):
> + case NCM_NOTIFY_SPEED:
> + event->bNotificationType = USB_CDC_NOTIFY_SPEED_CHANGE;
> + event->wValue = cpu_to_le16(0);
> + event->wLength = cpu_to_le16(8);
> + req->length = NCM_STATUS_BYTECOUNT;
> +
> + /* SPEED_CHANGE data is up/down speeds in bits/sec */
> + data = req->buf + sizeof *event;
> + data[0] = cpu_to_le32(ncm_bitrate(cdev->gadget));
> + data[1] = data[0];
CDC specification, Revision 1.2. Table 21: ConnectionSpeedChange
Data Structure: should we put corresponding structure to the in cdc.h
header and use it the all CDC related code?
Current structure used in the cdc_ncm.c:
struct connection_speed_change {
__le32 USBitRate; /* holds 3GPP downlink value, bits per second */
__le32 DSBitRate; /* holds 3GPP uplink value, bits per second */
} __attribute__ ((packed));
> + case ((USB_DIR_IN | USB_TYPE_CLASS | USB_RECIP_INTERFACE) << 8)
> + | USB_CDC_GET_NTB_INPUT_SIZE:
> +
> + if (w_length < 4 || w_value != 0 || w_index != ncm-
> >ctrl_id)
> + goto invalid;
> + put_unaligned_le32(ncm->port.fixed_in_len, req->buf);
> + value = 4;
Use sizeof(ncm->port.fixed_in_len) instead as well as in other similar
places
> + case ((USB_DIR_IN | USB_TYPE_CLASS | USB_RECIP_INTERFACE) << 8)
> + | USB_CDC_GET_NTB_FORMAT:
> + {
> + uint16_t format;
> +
> + if (w_length < 2 || w_value != 0 || w_index != ncm-
> >ctrl_id)
> + goto invalid;
> + format = (ncm->parser_opts == &ndp16_opts) ? 0x0000 :
> 0x0001;
Use defines from cdc.h:
#define USB_CDC_NCM_NTB16_FORMAT 0x00
#define USB_CDC_NCM_NTB32_FORMAT 0x01
> + case ((USB_DIR_OUT | USB_TYPE_CLASS | USB_RECIP_INTERFACE) << 8)
> + | USB_CDC_SET_NTB_FORMAT:
> + {
> + if (w_length != 0 || w_index != ncm->ctrl_id)
> + goto invalid;
> + switch (w_value) {
> + case 0x0000:
...
> + case 0x0001:
Use defines from cdc.h
> + case ((USB_DIR_IN | USB_TYPE_CLASS | USB_RECIP_INTERFACE) << 8)
> + | USB_CDC_GET_CRC_MODE:
> + {
> + uint16_t is_crc;
> +
> + if (w_length < 2 || w_value != 0 || w_index != ncm-
> >ctrl_id)
> + goto invalid;
> + is_crc = ncm->is_crc ? 0x0001 : 0x0000;
Use defines from cdc.h:
#define USB_CDC_NCM_CRC_NOT_APPENDED 0x00
#define USB_CDC_NCM_CRC_APPENDED 0x01
> + case ((USB_DIR_OUT | USB_TYPE_CLASS | USB_RECIP_INTERFACE) << 8)
> + | USB_CDC_SET_CRC_MODE:
> + {
> + int ndp_hdr_crc = 0;
> +
> + if (w_length != 0 || w_index != ncm->ctrl_id)
> + goto invalid;
> + switch (w_value) {
> + case 0x0000:
...
> + case 0x0001:
Use defines from cdc.h
> + /* TODO */
> + /* Enable zlps by default for NCM conformance;
> + * override for musb_hdrc (avoids txdma ovhead)
> + */
Could you explain a passage regarding zlp and make some reference to
NCM specification? It looks confusing.
> +static struct sk_buff *ncm_wrap_ntb(struct gether *port,
> + struct sk_buff *skb)
> +{
> + struct f_ncm *ncm = func_to_ncm(&port->func);
> + struct sk_buff *skb2;
> + int ncb_len = 0;
Would it be better to use unsigned (u32) instead?
> + ncb_len += 2 * 2 * opts->dgram_item_len; /* Datagram entry */
> + ncb_len += 2 * 2 * opts->dgram_item_len; /* Zero datagram entry
One of 2* can be removed, if opts keeps the real size
> + do {
> + index = index2;
> + dg_len = dg_len2;
> + if (dg_len < 14 + crc_len) { /* ethernet header + crc */
14 -> ETH_HLEN
> + INFO(port->func.config->cdev, "Bad dgram length:
> + } while (ndp_len > 2 * (opts->dgram_item_len * 2)); /* zero entry
*2 can be removed, if opts keeps the real size
If by mistake host driver sending NTB without zero entry at the end,
skb is not freed
> + VDBG(port->func.config->cdev,
> + "Parsed NTB with %d frames\n", dgram_counter);
> + return 0;> +err:
> + skb_queue_purge(list);
If gadget managed to parse some datagrams successfully and encounter error
at some point later, could we save/keep IP packets in the queue, so at
least some IP packets will pass through?
Regards,
Alexey
--
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
