Re: [RFC] USB: cdc-wdm: Extend and improve subdriver interface

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Oliver Neukum <oneukum@xxxxxxx> writes:
> On Tuesday 04 September 2012 15:45:36 Bjørn Mork wrote:
>
>> Yes, I expect that major changes to cdc_ncm will be necessary, and
>> pointing to it could be wrong from my side? It just seemed natural to
>> try to reuse any existing code.
>
> No, that is perfectly all right. It seems to me that the API between usbnet
> and its subdrivers is not powerful enough to support CDC-NCM well.

Maybe not.  But I assume that any shortcomings found could be integrated
in usbnet, if that is found to be the best solution.

So what we are looking for is somehow for a usbnet minidriver to receive
and submit single packet skbs and have usbnet handle all the logic
related to packing and unpacking the multi-packet URBs.  Should not be
impossible.

>> >> found that the cdc-wdm subdriver interface must be extended somewhat for
>> >> this to be possible.  At the very least, the main driver need to handle
>> >> some notifications which are ignored by cdc-wdm.  And depending on which
>> >
>> > Which ones?
>> 
>> The ones which are already handled by cdc_ncm I believe:
>> 
>> USB_CDC_NOTIFY_NETWORK_CONNECTION and USB_CDC_NOTIFY_SPEED_CHANGE.
>> cdc-wdm will just debug print USB_CDC_NOTIFY_NETWORK_CONNECTION and
>> ignore it, but will log an error if it sees USB_CDC_NOTIFY_SPEED_CHANGE
>
> So provide callbacks for them.

It seems a littly overkill to provide a separate callback for each of
these, so how about using the same callback but call it only for the
notifications we know a main driver may be interested in? I.e. something
along the lines


	switch (dr->bNotificationType) {
	case USB_CDC_NOTIFY_RESPONSE_AVAILABLE:
		dev_dbg(&desc->intf->dev,
			"NOTIFY_RESPONSE_AVAILABLE received: index %d len %d",
			dr->wIndex, dr->wLength);
		break;
	case USB_CDC_NOTIFY_NETWORK_CONNECTION:
	        if (desc->driver_info->status &&
	            !desc->driver_info->status(desc->intf, urb))
		        dev_dbg(&desc->intf->dev,
			        "NOTIFY_NETWORK_CONNECTION %s network",
			        dr->wValue ? "connected to" : "disconnected from");
		goto exit;
        case USB_CDC_NOTIFY_SPEED_CHANGE:
	        if (desc->driver_info->status &&
	            desc->driver_info->status(desc->intf, urb))
	                goto exit;
                /* else fallthrough */
	default:
		clear_bit(WDM_POLL_RUNNING, &desc->flags);
		dev_err(&desc->intf->dev,
			"unknown notification %d received: index %d len %d\n",
			dr->bNotificationType, dr->wIndex, dr->wLength);
		goto exit;
	}



>> yes.  Reading the spec terrified me.  Among other issues, it allows
>> multiplexing several IP connections over a single set of bulk in/out
>> endpoints, where an "IP connection" probably will have to map logically
>> to a network device.
>
> Yes, but given the capability of UMTS something like this would be needed.

I am not convinced about that..  Devices supporting more that 10
connections?  Well, the USB IF seems to think there will be, so you
are probably right...  I'd just like to see one of those devices :-)

>> I believe we can get away with the main driver only looking at messages
>> received from the device, without changing or filtering them.
>
> In hindsight, cdc-wdm should have used the tty layer. Then we'd now
> write a line discipline.

Too late to change that, I guess.  One of these days I'll have to find
out how all this tty magic works...
  
>> I see that I should have included the data interception hook, or not
>> have mentioned it at all. Sorry for any confusion.
>
> Very well.

Should I take it in again, and submit a new version for further
comments?  Still won't have any sample code using the API, I'm afraid.

Thanks for yet again giving very useful and constructive feedback.


Bjørn
--
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


[Index of Archives]     [Linux Media]     [Linux Input]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Old Linux USB Devel Archive]

  Powered by Linux