DSS usage
Bjørn Mork
bjorn at mork.no
Wed May 7 06:48:53 PDT 2014
Dmytro Milinevskyy <dmilinevskyy at sequans.com> writes:
> diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
> index f6dce47..c4a5651 100644
> --- a/drivers/net/usb/usbnet.c
> +++ b/drivers/net/usb/usbnet.c
> @@ -465,8 +465,9 @@ static int rx_submit (struct usbnet *dev, struct urb *urb, gfp_t flags)
>
> spin_lock_irqsave (&dev->rxq.lock, lockflags);
>
> - if (netif_running (dev->net) &&
> - netif_device_present (dev->net) &&
> + if (((netif_running (dev->net) &&
> + netif_device_present (dev->net)) ||
> + (dev->driver_info->flags&FLAG_RX_NONSTOP)) &&
> !test_bit (EVENT_RX_HALT, &dev->flags) &&
> !test_bit (EVENT_DEV_ASLEEP, &dev->flags)) {
> switch (retval = usb_submit_urb (urb, GFP_ATOMIC)) {
Two comments on this:
1) practial: All these "is the device ready to recevie packets?" tests
in usbnet are already too complex. Could you wrap parts that always
go together into a small boolean helper function (which the compiler
hopefully will inline for us)? There also seems to be some
unexplainable variations between these tests. I wonder if those are
really bugs?
2) please make this feature non-static. We do NOT want all MBIM devices
always running RX, even when they are completely unmanaged. We should
only start RX if either
a) the netdev is running, and/or
b) at least one DSS chardev is created and opened for reading.
Maybe add another minidriver callback where usbnet can ask about
non-netdev RX instead of the new flag?
But as I said earlier: This is a discussion for Oliver and the other
usbnet developers.
> +void dss_get(struct mbim_dss_channel *c)
> +{
> + atomic_inc(&c->users);
> +}
> +EXPORT_SYMBOL(dss_get);
Who are the external users of this? External interfaces with no users
are usually rejected by the net maintainer, for good reasons.
In any case: This symbol name is too generic for an exported symbol.
Something like cdc_mbim_dss_get would be more appropriate.
> +void dss_put(struct mbim_dss_channel *c)
> +{
> + __dss_put(c, false);
> +}
> +EXPORT_SYMBOL(dss_put);
Likewise. And also the rest of these.
> @@ -70,6 +584,15 @@ static int cdc_mbim_bind(struct usbnet *dev, struct usb_interface *intf)
> int ret = -ENODEV;
> u8 data_altsetting = cdc_ncm_select_altsetting(dev, intf);
> struct cdc_mbim_state *info = (void *)&dev->data;
> + struct cdc_mbim_ext_state *ext;
> +
> + ext = kmalloc(sizeof *ext, GFP_ATOMIC);
> + if (!ext)
> + return -ENOMEM;
> +
> + ext->terminating = false;
> + atomic_set(&ext->users, 1);
> + init_waitqueue_head(&ext->users_wq);
>
> /* Probably NCM, defer for cdc_ncm_bind */
> if (!cdc_ncm_comm_intf_is_mbim(intf->cur_altsetting))
> @@ -97,20 +620,42 @@ static int cdc_mbim_bind(struct usbnet *dev, struct usb_interface *intf)
> dev->status = NULL;
> info->subdriver = subdriver;
>
> - /* MBIM cannot do ARP */
> - dev->net->flags |= IFF_NOARP;
> + info->ext = ext;
> + ext->info = info;
> +
> + spin_lock_bh(&mbim_device_lock);
> + list_add_tail(&ext->device_list, &mbim_device_list);
> + spin_unlock_bh(&mbim_device_lock);
>
> /* no need to put the VLAN tci in the packet headers */
> dev->net->features |= NETIF_F_HW_VLAN_CTAG_TX;
> +
> + return 0;
> err:
> + kfree(ext);
> return ret;
> }
Maybe factor out the DSS initialization in a separate function to
document the parts being specific to the DSS chardev support and keep
cdc_mbim_bind managable?
> static void cdc_mbim_unbind(struct usbnet *dev, struct usb_interface *intf)
> {
> struct cdc_mbim_state *info = (void *)&dev->data;
> + struct cdc_mbim_ext_state *ext = info->ext;
> struct cdc_ncm_ctx *ctx = info->ctx;
>
> + ext->terminating = true;
> + smp_wmb();
> +
> + mbim_exterminate_dss(info);
> +
> + spin_lock_bh(&mbim_device_lock);
> + list_del(&ext->device_list);
> + spin_unlock_bh(&mbim_device_lock);
> +
> + mbim_put(info);
> +
> + while (atomic_read(&ext->users))
> + wait_event(ext->users_wq, !atomic_read(&ext->users));
> +
> /* disconnect subdriver from control interface */
> if (info->subdriver && info->subdriver->disconnect)
> info->subdriver->disconnect(ctx->control);
And similar for the DSS destroy part?
> @@ -184,7 +728,44 @@ error:
> return NULL;
> }
>
> -static struct sk_buff *cdc_mbim_process_dgram(struct usbnet *dev, u8 *buf, size_t len, u16 tci)
> +static int mbim_process_dss(struct mbim_dss_channel *c, struct sk_buff *skb,
> + u32 offset, u32 len) {
> + struct dss_pkt *pkt;
> + unsigned long flags;
> + int ret = 0;
> +
> + pkt = kmalloc(sizeof(struct dss_pkt), GFP_ATOMIC);
> + if (!pkt) {
> + dev_err(mbim_dss_dev(c), "failed to allocate DSS packet\n");
> + ret = -ENOMEM;
> + goto out;
> + }
> +
> + pkt->skb = skb_clone(skb, GFP_ATOMIC);
> + if (!pkt->skb) {
> + dev_err(mbim_dss_dev(c), "failed to clone skb\n");
> + kfree(pkt);
> + ret = -ENOMEM;
> + goto out;
> + }
> +
> + pkt->buf = pkt->skb->data + offset;
> + pkt->len = len;
> + pkt->offset = 0;
> +
> + spin_lock_irqsave(&c->read_lock, flags);
> + list_add_tail(&pkt->list, &c->read_q);
> + spin_unlock_irqrestore(&c->read_lock, flags);
> +
> + wake_up(&c->read_wq);
So this creates an infinite list of skb's of up to 32kB (which is the
current NCM hard limit for dwNtbInMaxSize) skbufs? I think you should
somehow prevent buggy devices from causing problems here.
The Huawei E367 MBIM firmware I have will happily send small DSS session
0 packets forever (unsolicited vendor specific AT messages), even after
you do MBIM_CLOSE! And just to make sure we'll have problems, it uses a
crazy dwNtbInMaxSize of 131072 bytes. So if you happen to create a DSS
session 0 chardev for this modem, you are pretty much guaranteed trouble
unless you regularily read from it.
Would it be possible to limit the read_q list to something reasonable?
Or just drop the packets on the floor if there are no current readers?
> @@ -294,10 +875,19 @@ next_ndp:
> goto err_ndp;
> break;
> } else {
> - skb = cdc_mbim_process_dgram(dev, skb_in->data + offset, len, tci);
> - if (!skb)
> - goto error;
> - usbnet_skb_return(dev, skb);
> + struct mbim_dss_channel *dss;
> +
> + if (tci >= 256 && (dss = dss_find(info, tci-256))) {
> + int ret;
> + ret = mbim_process_dss(dss, skb_in, offset, len);
> + if (ret)
> + goto error;
> + } else {
> + skb = cdc_mbim_process_ips(dev, skb_in->data + offset, len, tci);
> + if (!skb)
> + goto error;
> + usbnet_skb_return(dev, skb);
> + }
> }
> }
> err_ndp:
So the idea is that if nothing has created the DSS chardev, then
dss_find() returns NULL and the DSS data is returned to the VLAN subdev
as before? I think that migh warrant a comment explaining here...
And as dss_find() looks somewhat expensive, I'd propose running it only
once per NDP. Yes, you can race against the device here, but it still
doesn't make any sense to switch from VLAN to chardev in the middle of a
multi-datagram NDP.
> @@ -362,7 +952,7 @@ err:
>
> static const struct driver_info cdc_mbim_info = {
> .description = "CDC MBIM",
> - .flags = FLAG_NO_SETINT | FLAG_MULTI_PACKET | FLAG_WWAN,
> + .flags = FLAG_NO_SETINT | FLAG_MULTI_PACKET | FLAG_WWAN | FLAG_NOARP | FLAG_RX_NONSTOP,
> .bind = cdc_mbim_bind,
> .unbind = cdc_mbim_unbind,
> .manage_power = cdc_mbim_manage_power,
Uhm, yes, I see that you've added the FLAG_NOARP usbnet flag instead of
configuring the netdev directly. I have mixed feelings about this
flag... It is completely unnecessary. There is no reason why usbnet
should manage the netdev flags on behalf of the minidrivers. This flag
was actually added to usbnet after the cdc_mbim driver started using the
feature. And there were already too many usbnet flags. But most of
them have some reason to exist. This one is pointless.
So I'd rather see a patch removing it altogether. But this is my
personal opinion only, and I haven't really bothered doing anything
about it. The usbnet developers might disagree.
I am a bit bothered by the addition here, though. The problem with the
number of usbnet flags is that the more you have, the less clear they
become. IMHO, noone understands that .flags field means anymore. It is
too long.
> @@ -377,7 +967,7 @@ static const struct driver_info cdc_mbim_info = {
> */
> static const struct driver_info cdc_mbim_info_zlp = {
> .description = "CDC MBIM",
> - .flags = FLAG_NO_SETINT | FLAG_MULTI_PACKET | FLAG_WWAN | FLAG_SEND_ZLP,
> + .flags = FLAG_NO_SETINT | FLAG_MULTI_PACKET | FLAG_WWAN | FLAG_SEND_ZLP | FLAG_NOARP | FLAG_RX_NONSTOP,
> .bind = cdc_mbim_bind,
> .unbind = cdc_mbim_unbind,
> .manage_power = cdc_mbim_manage_power,
Yes, and then it has to be mirrored to all driver_info instances. Yet
another reason to keep it as short as possible.
> +
> +static int __init mbim_module_init(void)
> +{
> + struct netlink_kernel_cfg cfg = {
> + .input = nl_mbim_rx_skb,
> + };
> + int err;
> +
> + nl_mbim.nls = netlink_kernel_create(&init_net, NETLINK_MBIM, &cfg);
> + if (!nl_mbim.nls) {
> + err= -EIO;
> + goto problem;
> + }
> +
> + return 0;
> +
> +problem:
> + panic("MBIM NL: cannot register controller: %d", err);
> +}
> +module_init(mbim_module_init);
> +
> +static void __exit mbim_module_exit(void)
> +{
> + netlink_kernel_release(nl_mbim.nls);
> +}
> +module_exit(mbim_module_exit);
> +
> +MODULE_AUTHOR("Dmytro Milinevskyy <dmilinevskyy at sequans.com>");
> +MODULE_DESCRIPTION("USB CDC MBIM host driver netlink interface");
> +MODULE_LICENSE("GPL");
Ah, so that's the user of those symbols. But IMHO the netlink inteface
should definitely not be a separate module from the DSS chardev
support. You might want to put them together in a separate module if
you like. But they should go together. There is no point in the
chardev support with no interface to manage it through, is there?
But this support should probably be a default integral part of the
cdc_mbim driver if it goes in, should it not? We do want it to always
be there.
And... I really don't know much about this, but I have gotten an
impression that new netlink families aren't really an option anymore.
"git grep netlink_kernel_create drivers/net" turns up exactly nothing.
I believe you will have to use the generic netlink family. Look at
e.g. the team driver or other drivers implementing their own netlink
based API.
But please verify this with some more competent source than me. I have
never really tried to submit any netlink based API myself.
Bjørn
More information about the libmbim-devel
mailing list