netdev - Re: [PATCH] USB host CDC Phonet network interface driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <200907151620.31509.oliver@neukum.org>
Date:	Wed, 15 Jul 2009 16:20:31 +0200
From:	Oliver Neukum <oliver@...kum.org>
To:	"Rémi Denis-Courmont" 
	<remi.denis-courmont@...ia.com>
Cc:	netdev@...r.kernel.org, linux-usb@...r.kernel.org
Subject: Re: [PATCH] USB host CDC Phonet network interface driver

Am Mittwoch, 15. Juli 2009 15:59:01 schrieb Rémi Denis-Courmont:

Hello,

> +	netif_stop_queue(dev);
> +	if (atomic_inc_return(&pnd->tx_queue) < dev->tx_queue_len)
> +		netif_wake_queue(dev);

This is unelegant.


> +	err = usb_set_interface(pnd->usb, num, pnd->active_setting);
> +	if (err)
> +		return err;
> +
> +	for (i = 0; i < rxq_size; i++) {
> +		struct urb *req = usb_alloc_urb(0, GFP_KERNEL);
> +
> +		if (req && rx_submit(pnd, req, GFP_KERNEL)) {
> +			usb_free_urb(req);
> +			req = NULL;
> +		}
> +		pnd->urbs[i] = req;
> +	}

You should fail if you cannot get a minimum number of URBs running.
> +
> +	netif_wake_queue(dev);
> +	return 0;
> +}
[..]
> +	/* Endpoints */
> +	if ((data_desc->endpoint[0].desc.bEndpointAddress & USB_DIR_IN) ==
> +			USB_DIR_IN) {

Please use the macro.

> +		pnd->rx_pipe = usb_rcvbulkpipe(usbdev,
> +			data_desc->endpoint[0].desc.bEndpointAddress);
> +		pnd->tx_pipe = usb_sndbulkpipe(usbdev,
> +			data_desc->endpoint[1].desc.bEndpointAddress);
> +	} else {
> +		pnd->rx_pipe = usb_rcvbulkpipe(usbdev,
> +			data_desc->endpoint[1].desc.bEndpointAddress);
> +		pnd->tx_pipe = usb_sndbulkpipe(usbdev,
> +			data_desc->endpoint[0].desc.bEndpointAddress);
> +	}
> +	pnd->active_setting = data_desc - data_intf->altsetting;
> +
> +	err = usb_driver_claim_interface(&usbpn_driver, data_intf, pnd);
> +	if (err)
> +		goto out;
> +
> +	/* Force inactive mode until the network device is brought UP */
> +	usb_set_interface(usbdev, union_header->bSlaveInterface0,
> +				!pnd->active_setting);
> +	usb_set_intfdata(intf, pnd);
> +	BUG_ON(!usb_get_intfdata(intf));

What for?

> +
> +	err = register_netdev(dev);
> +	if (err) {
> +		usb_driver_release_interface(&usbpn_driver, data_intf);
> +		goto out;
> +	}
> +
> +	dev_dbg(&dev->dev, "USB CDC Phonet device found\n");
> +	return 0;
> +
> +out:
> +	usb_set_intfdata(intf, NULL);
> +	free_netdev(dev);
> +	return err;
> +}
> +
> +static void usbpn_disconnect(struct usb_interface *intf)
> +{
> +	struct usbpn_dev *pnd = usb_get_intfdata(intf);
> +

What happens if this happens in the unexpected order?

> +	if (intf != pnd->data_intf) {
> +		usb_driver_release_interface(&usbpn_driver, pnd->data_intf);
> +		return;
> +	}
> +
> +	unregister_netdev(pnd->dev);
> +	usb_put_dev(pnd->usb);
> +}

	Regards
		Oliver

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html