Message ID | 87k2le815w.fsf_-_@nemi.mork.no |
---|---|
State | Accepted, archived |
Delegated to: | David Miller |
Headers | show |
From: Bjørn Mork <bjorn@mork.no> Date: Mon, 7 Mar 2016 21:15:36 +0100 > usbnet_link_change will call schedule_work and should be > avoided if bind is failing. Otherwise we will end up with > scheduled work referring to a netdev which has gone away. > > Instead of making the call conditional, we can just defer > it to usbnet_probe, using the driver_info flag made for > this purpose. > > Fixes: 8a34b0ae8778 ("usbnet: cdc_ncm: apply usbnet_link_change") > Reported-by: Andrey Konovalov <andreyknvl@gmail.com> > Suggested-by: Linus Torvalds <torvalds@linux-foundation.org> > Signed-off-by: Bjørn Mork <bjorn@mork.no> ... > Even with Oliver's generic fix we should still fix the inconsistency > in cdc_ncm, as pointed out by Linus. > > This is a slightly different approach than the patch proposed by Linus. > When I started looking at this I couldn't figure out why we were doing > this differently in this driver from all the other usbnet drivers > disabling the link at probe time. So let's make it consistent. Then at > least we get consistent bugs :) Fair enough, applied and queued up for -stable. Thanks.
On Mon, Mar 7, 2016 at 12:15 PM, Bjørn Mork <bjorn@mork.no> wrote: > usbnet_link_change will call schedule_work and should be > avoided if bind is failing. Otherwise we will end up with > scheduled work referring to a netdev which has gone away. > > Instead of making the call conditional, we can just defer > it to usbnet_probe, using the driver_info flag made for > this purpose. So looking at this, I wonder... Why is that FLAG_LINK_INTR thing not just always used? The _only_ thing that FLAG_LINK_INTR does is to cause usbnet_link_change(dev, 0, 0); to be called after network device attach. That doesn't seem to be controversial. Looking at some examples, we have ax88179_178a.c that doesn't set the flag, but instead does that usbnet_link_change() call at the end of ax88179_bind(). There are a few drivers that seem to never call that usbnet_link_change() at all, and don't have that FLAG_LINK_INTR flag. Would they break? Stupid grep: git grep -lw FLAG_ETHER | xargs grep -L FLAG_LINK_INTR | xargs grep -L usbnet_link_change | sed 's:drivers/net/usb/::' gives cdc_eem.c ch9200.c cx82310_eth.c int51x1.c rndis_host.c so maybe that FLAG_LINK_INTR si required. Why is it called "FLAG_LINK_INTR" anyway? There doesn't seem to be anything "INTR" about it. Linus
On Tue, 2016-03-08 at 11:43 -0800, Linus Torvalds wrote: > Why is that FLAG_LINK_INTR thing not just always used? > > The _only_ thing that FLAG_LINK_INTR does is to cause > > usbnet_link_change(dev, 0, 0); > > to be called after network device attach. That doesn't seem to be > controversial. It depends on the devices' capabilities. For a few old devices that would be deadly, because they cannot indicate that a link goes up again. > Looking at some examples, we have ax88179_178a.c that doesn't set the > flag, but instead does that usbnet_link_change() call at the end of > ax88179_bind(). > > There are a few drivers that seem to never call that > usbnet_link_change() at all, and don't have that FLAG_LINK_INTR flag. > Would they break? Yes. If we did the call unconditionally. We could not do it, then we'd see some spurious detection of interfaces being up, but something breaks. Today I'd probably require a flag for those cases that do not want the call to be made, but the distinction as such is necessary. Regards Oliver
Linus Torvalds <torvalds@linux-foundation.org> writes: > So looking at this, I wonder... > > Why is that FLAG_LINK_INTR thing not just always used? > > The _only_ thing that FLAG_LINK_INTR does is to cause > > usbnet_link_change(dev, 0, 0); > > to be called after network device attach. That doesn't seem to be controversial. Not all usbnet drivers support carrier detection, which is required to ever bring the link up again. > Looking at some examples, we have ax88179_178a.c that doesn't set the > flag, but instead does that usbnet_link_change() call at the end of > ax88179_bind(). > > There are a few drivers that seem to never call that > usbnet_link_change() at all, and don't have that FLAG_LINK_INTR flag. > Would they break? Yes. Drivers without carrier detection will be "down" forever. > Why is it called "FLAG_LINK_INTR" anyway? There doesn't seem to be > anything "INTR" about it. Beats me. I can only say that I always find naming difficult... We could ask Ben, who introduced it in: commit 37e8273cd30592d3a82bcb70cbb1bdc4eaeb6b71 Author: Ben Hutchings <ben@decadent.org.uk> Date: Wed Nov 4 15:29:52 2009 +0000 usbnet: Set link down initially for drivers that update link state Some usbnet drivers update link state while others do not due to hardware limitations. Add a flag to distinguish those that do, and set the link down initially for their devices. This is intended to fix this bug: http://bugs.debian.org/444043 Signed-off-by: Ben Hutchings <ben@decadent.org.uk> Signed-off-by: David S. Miller <davem@davemloft.net> But I guess it doesn't really matter. Bjørn
On Tue, 2016-03-08 at 21:18 +0100, Bjørn Mork wrote: > > Why is it called "FLAG_LINK_INTR" anyway? There doesn't seem to be > > anything "INTR" about it. > > Beats me. I can only say that I always find naming difficult... > We could ask Ben, who introduced it in: It used to be done over USB interrupt endpoints. Regards Oliver
On Tue, 2016-03-08 at 21:18 +0100, Bjørn Mork wrote: > Linus Torvalds <torvalds@linux-foundation.org> writes: > > > > > So looking at this, I wonder... > > > > Why is that FLAG_LINK_INTR thing not just always used? > > > > The _only_ thing that FLAG_LINK_INTR does is to cause > > > > usbnet_link_change(dev, 0, 0); > > > > to be called after network device attach. That doesn't seem to be controversial. > Not all usbnet drivers support carrier detection, which is required to > ever bring the link up again. > > > > > Looking at some examples, we have ax88179_178a.c that doesn't set the > > flag, but instead does that usbnet_link_change() call at the end of > > ax88179_bind(). > > > > There are a few drivers that seem to never call that > > usbnet_link_change() at all, and don't have that FLAG_LINK_INTR flag. > > Would they break? > Yes. Drivers without carrier detection will be "down" forever. > > > > > Why is it called "FLAG_LINK_INTR" anyway? There doesn't seem to be > > anything "INTR" about it. > Beats me. I can only say that I always find naming difficult... > We could ask Ben, who introduced it in: [...] It is supposed to imply that the device generates link-change interrupts. Of course it is also possible for a device driver to satisfy the requirement by polling the link state. Ben.
diff --git a/drivers/net/usb/cdc_ncm.c b/drivers/net/usb/cdc_ncm.c index be927964375b..86ba30ba35e8 100644 --- a/drivers/net/usb/cdc_ncm.c +++ b/drivers/net/usb/cdc_ncm.c @@ -988,8 +988,6 @@ EXPORT_SYMBOL_GPL(cdc_ncm_select_altsetting); static int cdc_ncm_bind(struct usbnet *dev, struct usb_interface *intf) { - int ret; - /* MBIM backwards compatible function? */ if (cdc_ncm_select_altsetting(intf) != CDC_NCM_COMM_ALTSETTING_NCM) return -ENODEV; @@ -998,16 +996,7 @@ static int cdc_ncm_bind(struct usbnet *dev, struct usb_interface *intf) * Additionally, generic NCM devices are assumed to accept arbitrarily * placed NDP. */ - ret = cdc_ncm_bind_common(dev, intf, CDC_NCM_DATA_ALTSETTING_NCM, 0); - - /* - * We should get an event when network connection is "connected" or - * "disconnected". Set network connection in "disconnected" state - * (carrier is OFF) during attach, so the IP network stack does not - * start IPv6 negotiation and more. - */ - usbnet_link_change(dev, 0, 0); - return ret; + return cdc_ncm_bind_common(dev, intf, CDC_NCM_DATA_ALTSETTING_NCM, 0); } static void cdc_ncm_align_tail(struct sk_buff *skb, size_t modulus, size_t remainder, size_t max) @@ -1590,7 +1579,8 @@ static void cdc_ncm_status(struct usbnet *dev, struct urb *urb) static const struct driver_info cdc_ncm_info = { .description = "CDC NCM", - .flags = FLAG_POINTTOPOINT | FLAG_NO_SETINT | FLAG_MULTI_PACKET, + .flags = FLAG_POINTTOPOINT | FLAG_NO_SETINT | FLAG_MULTI_PACKET + | FLAG_LINK_INTR, .bind = cdc_ncm_bind, .unbind = cdc_ncm_unbind, .manage_power = usbnet_manage_power, @@ -1603,7 +1593,7 @@ static const struct driver_info cdc_ncm_info = { static const struct driver_info wwan_info = { .description = "Mobile Broadband Network Device", .flags = FLAG_POINTTOPOINT | FLAG_NO_SETINT | FLAG_MULTI_PACKET - | FLAG_WWAN, + | FLAG_LINK_INTR | FLAG_WWAN, .bind = cdc_ncm_bind, .unbind = cdc_ncm_unbind, .manage_power = usbnet_manage_power, @@ -1616,7 +1606,7 @@ static const struct driver_info wwan_info = { static const struct driver_info wwan_noarp_info = { .description = "Mobile Broadband Network Device (NO ARP)", .flags = FLAG_POINTTOPOINT | FLAG_NO_SETINT | FLAG_MULTI_PACKET - | FLAG_WWAN | FLAG_NOARP, + | FLAG_LINK_INTR | FLAG_WWAN | FLAG_NOARP, .bind = cdc_ncm_bind, .unbind = cdc_ncm_unbind, .manage_power = usbnet_manage_power,