Hi Oliver,
Oliver Neukum wrote:
>> +cdc_ncm_setup(struct cdc_ncm_softc *sc)
>> +{
>> + struct cdc_ncm_parameters temp;
>> + struct cdc_ncm_device_request req;
>> + u32 val;
>> + u8 flags;
>> + u8 value[8];
>> + u8 iface_no;
>> + int err;
>> +
>> + iface_no = sc->sc_control->cur_altsetting-
>>desc.bInterfaceNumber;
>> +
>> + req.bmRequestType = USB_TYPE_CLASS | USB_DIR_IN |USB_RECIP_INTERFACE;
>> + req.bRequest = CDC_NCM_GET_NTB_PARAMETERS;
>> + put_unaligned_le16(0, req.wValue);
>> + req.wIndex[0] = iface_no;
>> + req.wIndex[1] = 0;
>> + put_unaligned_le16(sizeof(temp), req.wLength);
>> +
>> + err = cdc_ncm_do_request(sc, &req, &temp, 0, NULL, 1000 /* ms */);
> DMA on stack. You must alloc buffers with kmalloc.
OK, thanks we've fixed this.
...
>> +
>> + /* Additional configuration */
>> +
>> + req.bmRequestType = USB_TYPE_CLASS | USB_DIR_OUT |USB_RECIP_INTERFACE;
>> + req.bRequest = CDC_NCM_SET_NTB_INPUT_SIZE;
>> + put_unaligned_le16(0, req.wValue);
>> + req.wIndex[0] = iface_no;
> endianness?
This should be ok, as the wIndex is decoded as little endian in function cdc_ncm_do_request.
>> + req.wIndex[1] = 0;
>> +
>> + if (flags & CDC_NCM_CAP_NTBINPUTSIZE) {
>> + put_unaligned_le16(8, req.wLength);
>> + put_unaligned_le32(sc->sc_rx_ncm.rx_max, value + 0);
>> + put_unaligned_le16(CDC_NCM_DPT_DATAGRAMS_MAX, value + 4);
>> + put_unaligned_le16(0, value + 6);
>> + } else {
>> + put_unaligned_le16(4, req.wLength);
>> + put_unaligned_le32(sc->sc_rx_ncm.rx_max, value);
>> + }
>> +
>> + err = cdc_ncm_do_request(sc, &req, &value, 0, NULL, 1000 /* ms */);
>
> DMA on stack.
OK, thanks we've fixed this.
>> +
>> +static void
>> +cdc_ncm_softc_free(struct cdc_ncm_softc *sc)
>> +{
>> + if (sc == NULL)
>> + return;
>> +
>> + del_timer(&sc->sc_tx_timer);
>
> Races with kfree. How do you prevent the timeout handler from using
> the memory you kfree? I suggest del_timer_sync().
OK, thanks we've fixed this as you have suggested.
>
> [..]
>> + kfree(sc);
>> +}
>> +static void
>> +cdc_ncm_tx_timeout_start(struct cdc_ncm_softc *sc)
>> +{
>> + if (timer_pending(&sc->sc_tx_timer) == 0) {
>
> Arg. What is this supposed to test for?
At every TX we start a timer unless it's already started.
We need a timeout when to send out the USB frame, because we are accumulating Ethernet packets and don't know when we have got the last one in the outgoing network queue. The cdc_ncm_tx_timeout_start() function is called at every TX, but we don't reset the timer at every TX, which means we have to check timer_pending() to avoid re-starting the timer before it has elapsed.
>> + sc->sc_tx_timer.function = &cdc_ncm_tx_timeout;
>> + sc->sc_tx_timer.data = (unsigned long)sc;
>> + sc->sc_tx_timer.expires = jiffies + ((HZ + 999) /1000);
>> + add_timer(&sc->sc_tx_timer);
>> + }
>> +}
>> +
>
>> +static struct sk_buff *
>> +cdc_ncm_tx_fixup(struct usbnet *dev, struct sk_buff *skb, gfp_t flags)
>> +{
>> + struct sk_buff *skb_out;
>> + struct cdc_ncm_softc *sc;
>> +
>> + u8 need_timer;
>> +
>> + /*
>> + * The ethernet API we are using does not support transmitting
>> + * multiple ethernet frames in a single call. This driver will
>> + * accumulate multiple ethernet frames and send out a larger
>> + * USB frame when the USB buffer is full, or a single jiffies
>> + * timeout happens.
>> + */
>
> So you send out a partially filled packet even if another packet is in
> flight?
Yes, we send out packets either when queue if full or at timeout.
>
>
> > +static struct usb_driver cdc_ncm_driver = {
> > + .name = "cdc_ncm",
> > + .id_table = cdc_devs,
> > + .probe = cdc_ncm_probe,
> > + .disconnect = cdc_ncm_disconnect,
> > + .suspend = cdc_ncm_suspend,
> > + .resume = cdc_ncm_resume,
> > + .supports_autosuspend = 1,
>
> Where do you do that?
Are you referring to "supports_autosuspend"? Could you elaborate your question please.
Regards
Sjur Brændeland
Hans Petter Selasky
--
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
