Re: [PATCH net] cdc_ether: Fix handling connection notification

Kristian Evensen [off-list ref] writes:

+void usbnet_cdc_zte_status(struct usbnet *dev, struct urb *urb)
+{
+	struct usb_cdc_notification *event;
+
+	if (urb->actual_length < sizeof(*event))
+		return;
+
+	event = urb->transfer_buffer;
+
+	if (event->bNotificationType != USB_CDC_NOTIFY_NETWORK_CONNECTION) {
+		usbnet_cdc_status(dev, urb);
+		return;
+	}
+
+	netif_dbg(dev, timer, dev->net, "CDC: carrier %s\n",
+		  event->wValue ? "on" : "off");
+
+	if (event->wValue &&
+	    !test_bit(__LINK_STATE_NOCARRIER, &dev->net->state))
+		usbnet_link_change(dev, 0, 0);
+
+	usbnet_link_change(dev, !!event->wValue, 0);
+}

As Henning said: Use netif_carrier_ok instead of open coding it.

But I also think you need to replace the first usbnet_link_change() with
a plain netif_carrier_off(dev->net).  Calling usbnet_link_change() twice
here is only going to set off the "kevent XX may have been dropped"
message since you call schedule_work() twice without giving the work
queue a chance to be processed.  No need to do that.


Bjørn

On Wed, Nov 30, 2016 at 10:51 PM, Bjørn Mork [off-list ref] wrote:

Kristian Evensen [off-list ref] writes:

quoted

+void usbnet_cdc_zte_status(struct usbnet *dev, struct urb *urb)
+{
+     struct usb_cdc_notification *event;
+
+     if (urb->actual_length < sizeof(*event))
+             return;
+
+     event = urb->transfer_buffer;
+
+     if (event->bNotificationType != USB_CDC_NOTIFY_NETWORK_CONNECTION) {
+             usbnet_cdc_status(dev, urb);
+             return;
+     }
+
+     netif_dbg(dev, timer, dev->net, "CDC: carrier %s\n",
+               event->wValue ? "on" : "off");
+
+     if (event->wValue &&
+         !test_bit(__LINK_STATE_NOCARRIER, &dev->net->state))
+             usbnet_link_change(dev, 0, 0);
+
+     usbnet_link_change(dev, !!event->wValue, 0);
+}

As Henning said: Use netif_carrier_ok instead of open coding it.

But I also think you need to replace the first usbnet_link_change() with
a plain netif_carrier_off(dev->net).  Calling usbnet_link_change() twice
here is only going to set off the "kevent XX may have been dropped"
message since you call schedule_work() twice without giving the work
queue a chance to be processed.  No need to do that.

Thanks for the feedback and agree. Will submit a v2 tomorrow.

-Kristian