Bjørn Mork <bjorn@xxxxxxx> writes: > Oliver Neukum <oliver@xxxxxxxxxx> writes: >> On Tuesday 04 September 2012 17:32:17 Bjørn Mork wrote: >>> Oliver Neukum <oneukum@xxxxxxx> writes: >>> > On Tuesday 04 September 2012 15:45:36 Bjørn Mork wrote: >> >>> >> 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 >> >> No. Once we've decided that multiple callbacks are needed, there's no use in >> limiting their number. It is importantant that they get clear semantics and >> reasonable names. You've already introduced a structure for them. So beef it up. >> >>> 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. >> >> Please propose a version with the callbacks you'll need all separated. > > Just a FYI in case you wonder where this went... > > Re-reading the MBIM specification it became clear that the need for > USB_CDC_NOTIFY_NETWORK_CONNECTION and USB_CDC_NOTIFY_SPEED_CHANGE > callbacks was a misunderstanding. These are used by NCM but not > required for MBIM. So they can be dropped. > > And after implementing MBIM control channel snooping, doing magic device > creation based on the snooped messages, I realized that those who warned > me about doing that were right. The idea is currently dropped, and with > it the need for any cdc_wdm data snooping. > > So it seems we can do MBIM just fine with the existing cdc_wdm subdriver > API. Sorry for the unnecessary noise this discussion caused, and thanks > for all the valuable feedback as usual. Resurrecting a very old thread... Those with a more limited linux-usb archive can find the old discussion here: http://comments.gmane.org/gmane.linux.usb.general/70395 The first MBIM device sending USB_CDC_NOTIFY_SPEED_CHANGE notifications has now appeared, causing unwanted error messages: [66255.801874] cdc_mbim 1-3:1.5: unknown notification 42 received: index 5 len 8 [66779.464738] cdc_mbim 1-3:1.5: unknown notification 42 received: index 5 len 8 Ref http://ubuntuforums.org/showthread.php?t=2165362&p=12762195#post12762195 This is of course harmless, and there is absolutely nothing useful we can do about the notification. So the real question is actually how we prefer to silence the error. Reading the old thread, the proper way would be to extend the subdriver API, adding a callback into cdc_mbim specifically for this notification. But I hesitate to do that, because the notification is completely useless to cdc_mbim. If we look at the three CDC host drivers explicitly handling this notification, we have: cdc_ether: logs the speeds with netif_info() (i.e. depending on the netdev msglvl): netif_info(dev, timer, dev->net, "link speeds: %u kbps up, %u kbps down\n", __le32_to_cpu(speeds[0]) / 1000, __le32_to_cpu(speeds[1]) / 1000); cdc-ncm: saves the speeds in its private context (never used) and logs with printk(KERN_INFO, ...): if ((tx_speed > 1000000) && (rx_speed > 1000000)) { printk(KERN_INFO KBUILD_MODNAME ": %s: %u mbit/s downlink " "%u mbit/s uplink\n", ctx->netdev->name, (unsigned int)(rx_speed / 1000000U), (unsigned int)(tx_speed / 1000000U)); } else { printk(KERN_INFO KBUILD_MODNAME ": %s: %u kbit/s downlink " "%u kbit/s uplink\n", ctx->netdev->name, (unsigned int)(rx_speed / 1000U), (unsigned int)(tx_speed / 1000U)); } sierra_net: silently ignores it along with its NETWORK_CONNECTION companion notification Of these, I personally prefer the sierra_net approach. There is little, if any, value in the log messages. And when it comes to MBIM, we do have a standardized management protocol handled by userspace, providing a standard way to retrieve the same data if/when required: bjorn@nemi:~$ mbimcli -d /dev/cdc-wdm0 --query-packet-service-state [/dev/cdc-wdm0] Packet service status: Network error: 'unknown' Packet service state: 'attached' Available data classes: 'lte' Uplink speed: '50000000 bps' Downlink speed: '100000000 bps' Now, I'd really prefer not to have to change the cdc-wdm subdriver API adding hooks for this case, just to be able to add a "silently ignore" hook. I'd much rather just add the ignore statement to cdc-wdm, like for NETWORK_CONNECTION. Which has been handled/ignored by cdc-wdm from the beginning, although it isn't part of the CDC WMC Device Managent spec either. The only supported notification is RESPONSE_AVAILABLE according to the WMC Revision 1.1 (Errata 1) spec: 6.6.4 Notifications The only notifications supported by the Device Management class are: ResponseAvailable (mandatory) So I propose that we add something like this to cdc-wdm instead of extending the API: case USB_CDC_NOTIFY_SPEED_CHANGE: dev_dbg(&desc->intf->dev, "SPEED_CHANGE received (len %d)", urb->actual_length); goto exit; Alternatively, I propose to demote the default "unknown notification" message to debug level. I fail to see how it is useful to any end user. 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
