netdev - Re: [PATCH] Add Qualcomm Gobi 2000/3000 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] [day] [month] [year] [list]

Message-Id: <20110413.133723.70211873.davem@davemloft.net>
Date:	Wed, 13 Apr 2011 13:37:23 -0700 (PDT)
From:	David Miller <davem@...emloft.net>
To:	ellyjones@...gle.com
Cc:	netdev@...r.kernel.org, dcbw@...hat.com, mjg59@...hat.com,
	jglasgow@...gle.com, trond@...gle.com
Subject: Re: [PATCH] Add Qualcomm Gobi 2000/3000 driver.

From: Elly Jones <ellyjones@...gle.com>
Date: Wed, 13 Apr 2011 15:00:24 -0400

> +void qcusbnet_put(struct qcusbnet *dev)
> +{
> +	mutex_lock(&qcusbnet_lock);
> +	kref_put(&dev->refcount, free_dev);
> +	mutex_unlock(&qcusbnet_lock);
> +}

This locking looks excessive, and shouldn't be needed simply to
release a reference to an object.

> +int qc_suspend(struct usb_interface *iface, pm_message_t event)
> +{
> +	struct usbnet *usbnet;
> +	struct qcusbnet *dev;
> +
> +	if (!iface)
> +		return -ENOMEM;

When is qc_suspend() called with a NULL iface arguemnt?

> +static int qc_resume(struct usb_interface *iface)
> +{
> +	struct usbnet *usbnet;
> +	struct qcusbnet *dev;
> +	int ret;
> +	int oldstate;
> +
> +	if (iface == 0)
> +		return -ENOMEM;

Likewise, and if it is needed use consistent tests for NULL.  Testing
against the integer "0" is definitely the wrong way.

> +		if (usb_endpoint_dir_in(&endpoint->desc)
> +		&&  !usb_endpoint_xfer_int(&endpoint->desc)) {

Please do it like this:

	if (A &&
	    B) {

Not like:

	if (A
	&&  B

the latter looks awful at best.

> +	if (!usbnet || !usbnet->net) {
> +		DBG("failed to get usbnet device\n");
> +		return;
> +	}
> +
> +	dev = (struct qcusbnet *)usbnet->data[0];
> +	if (!dev) {
> +		DBG("failed to get QMIDevice\n");
> +		return;
> +	}

These NULL checks are everywhere!  Do we really _ever_ create a full
registered netdev with any of these things being NULL?  I severely
doubt it.

> +static int qcnet_worker(void *arg)
> +{
> +	struct list_head *node, *tmp;
> +	unsigned long activeflags, listflags;
> +	struct urbreq *req;
> +	int status;
> +	struct usb_device *usbdev;
> +	struct worker *worker = arg;
> +	if (!worker) {
> +		DBG("passed null pointer\n");
> +		return -EINVAL;
> +	}

This NULL check is impossible, you register the worker function with an
explicit &dev->worker argument, so seeing NULL here is impossible.

> +static int qcnet_startxmit(struct sk_buff *skb, struct net_device *netdev)
> +{
> +	unsigned long listflags;
> +	struct qcusbnet *dev;
> +	struct worker *worker;
> +	struct urbreq *req;
> +	void *data;
> +	struct usbnet *usbnet = netdev_priv(netdev);
> +
> +	DBG("\n");
> +
> +	if (!usbnet || !usbnet->net) {
> +		DBG("failed to get usbnet device\n");
> +		return NETDEV_TX_BUSY;
> +	}
> +
> +	dev = (struct qcusbnet *)usbnet->data[0];
> +	if (!dev) {

Again, kill this NULL check noise, all of it can't be necessary.

> +	netdev->trans_start = jiffies;

Setting netdev->trans_start in drivers is expensive and deprecated,
please set netdev_queue->trans_start instead.

> +static int qcnet_open(struct net_device *netdev)
> +{
> +	int status = 0;
> +	struct qcusbnet *dev;
> +	struct usbnet *usbnet = netdev_priv(netdev);
> +
> +	if (!usbnet) {
> +		DBG("failed to get usbnet device\n");
> +		return -ENXIO;
> +	}
> +
> +	dev = (struct qcusbnet *)usbnet->data[0];
> +	if (!dev) {
> +		DBG("failed to get QMIDevice\n");
> +		return -ENXIO;
> +	}

Again, excessive NULL checks.

> +int qcnet_stop(struct net_device *netdev)
> +{
> +	struct qcusbnet *dev;
> +	struct usbnet *usbnet = netdev_priv(netdev);
> +
> +	if (!usbnet || !usbnet->net) {
> +		DBG("failed to get netdevice\n");
> +		return -ENXIO;
> +	}
> +
> +	dev = (struct qcusbnet *)usbnet->data[0];
> +	if (!dev) {
> +		DBG("failed to get QMIDevice\n");
> +		return -ENXIO;
> +	}

Here too.

> +static u8 nibble(unsigned char c)
> +{
> +	if (likely(isdigit(c)))
> +		return c - '0';
> +	c = toupper(c);
> +	if (likely(isxdigit(c)))
> +		return 10 + c - 'A';
> +	return 0;
> +}

Remove this function and use hex_to_bin() instead.
--
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