Re: [PATCH 04/12] staging: usbip: stub_main.c: code cleanup

[walter harms]

Am 20.05.2011 06:36, schrieb matt mooney:
> Remove match_find() and replace with get_busid_idx(); change
> get_busid_priv(), add_match_busid(), and del_match_busid() to use
> get_busid_idx(); and cleanup code in the other functions.
> 
> Signed-off-by: matt mooney <mfm@xxxxxxxxxxxxx>
> ---
>  drivers/staging/usbip/stub_main.c |  147 ++++++++++++++++++-------------------
>  1 files changed, 73 insertions(+), 74 deletions(-)
> 
> diff --git a/drivers/staging/usbip/stub_main.c b/drivers/staging/usbip/stub_main.c
> index 0ca1462..00398a6 100644
> --- a/drivers/staging/usbip/stub_main.c
> +++ b/drivers/staging/usbip/stub_main.c
> @@ -50,82 +50,90 @@ static void init_busid_table(void)
>  	spin_lock_init(&busid_table_lock);
>  }
>  
> -int match_busid(const char *busid)
> +/*
> + * Find the index of the busid by name.
> + * Must be called with busid_table_lock held.
> + */
> +static int get_busid_idx(const char *busid)
>  {
>  	int i;
> +	int idx = -1;
>  
> -	spin_lock(&busid_table_lock);
>  	for (i = 0; i < MAX_BUSID; i++)
>  		if (busid_table[i].name[0])
>  			if (!strncmp(busid_table[i].name, busid, BUSID_SIZE)) {
> -				/* already registerd */
> -				spin_unlock(&busid_table_lock);
> -				return 0;
> +				idx = i;
> +				break;
>  			}
> -	spin_unlock(&busid_table_lock);
> -
> -	return 1;
> +	return idx;
>  }
>  
>  struct bus_id_priv *get_busid_priv(const char *busid)
>  {
> -	int i;
> +	int idx;
> +	struct bus_id_priv *bid = NULL;
>  
>  	spin_lock(&busid_table_lock);
> -	for (i = 0; i < MAX_BUSID; i++)
> -		if (busid_table[i].name[0])
> -			if (!strncmp(busid_table[i].name, busid, BUSID_SIZE)) {
> -				/* already registerd */
> -				spin_unlock(&busid_table_lock);
> -				return &(busid_table[i]);
> -			}
> +	idx = get_busid_idx(busid);
> +	if (idx >= 0)
> +		bid = &(busid_table[idx]);
>  	spin_unlock(&busid_table_lock);
>  
> -	return NULL;
> +	return bid;
>  }
>  
>  static int add_match_busid(char *busid)
>  {
>  	int i;
> -
> -	if (!match_busid(busid))
> -		return 0;
> +	int ret = -1;
>  
>  	spin_lock(&busid_table_lock);
> +	/* already registered? */
> +	if (get_busid_idx(busid) >= 0) {
> +		ret = 0;
> +		goto out;
> +	}
> +
>  	for (i = 0; i < MAX_BUSID; i++)
>  		if (!busid_table[i].name[0]) {
>  			strncpy(busid_table[i].name, busid, BUSID_SIZE);

i am missing an if() here ??


>  			if ((busid_table[i].status != STUB_BUSID_ALLOC) &&
>  			    (busid_table[i].status != STUB_BUSID_REMOV))
>  				busid_table[i].status = STUB_BUSID_ADDED;
> -			spin_unlock(&busid_table_lock);
> -			return 0;
> +			ret = 0;
> +			break;
>  		}
> +





> +out:
>  	spin_unlock(&busid_table_lock);
>  
> -	return -1;
> +	return ret;
>  }
>  
>  int del_match_busid(char *busid)
>  {
> -	int i;
> +	int idx;
> +	int ret = -1;
>  
>  	spin_lock(&busid_table_lock);
> -	for (i = 0; i < MAX_BUSID; i++)
> -		if (!strncmp(busid_table[i].name, busid, BUSID_SIZE)) {
> -			/* found */
> -			if (busid_table[i].status == STUB_BUSID_OTHER)
> -				memset(busid_table[i].name, 0, BUSID_SIZE);
> -			if ((busid_table[i].status != STUB_BUSID_OTHER) &&
> -			    (busid_table[i].status != STUB_BUSID_ADDED)) {
> -				busid_table[i].status = STUB_BUSID_REMOV;
> -			}
> -			spin_unlock(&busid_table_lock);
> -			return 0;
> -		}
> +	idx = get_busid_idx(busid);
> +	if (idx < 0)
> +		goto out;
> +
> +	/* found */
> +	ret = 0;
> +
> +	if (busid_table[idx].status == STUB_BUSID_OTHER)
> +		memset(busid_table[idx].name, 0, BUSID_SIZE);
> +
> +	if ((busid_table[idx].status != STUB_BUSID_OTHER) &&
> +	    (busid_table[idx].status != STUB_BUSID_ADDED))
> +		busid_table[idx].status = STUB_BUSID_REMOV;
> +
> +out:
>  	spin_unlock(&busid_table_lock);
>  
> -	return -1;
> +	return ret;
>  }
>  
>  static ssize_t show_match_busid(struct device_driver *drv, char *buf)
> @@ -138,8 +146,8 @@ static ssize_t show_match_busid(struct device_driver *drv, char *buf)
>  		if (busid_table[i].name[0])
>  			out += sprintf(out, "%s ", busid_table[i].name);
>  	spin_unlock(&busid_table_lock);
> -
>  	out += sprintf(out, "\n");
> +
>  	return out - buf;
>  }
>  
> @@ -162,23 +170,24 @@ static ssize_t store_match_busid(struct device_driver *dev, const char *buf,
>  	strncpy(busid, buf + 4, BUSID_SIZE);
>  
>  	if (!strncmp(buf, "add ", 4)) {
> -		if (add_match_busid(busid) < 0)
> +		if (add_match_busid(busid) < 0) {
>  			return -ENOMEM;
> -		else {
> +		} else {
>  			pr_debug("add busid %s\n", busid);
>  			return count;
>  		}
>  	} else if (!strncmp(buf, "del ", 4)) {
> -		if (del_match_busid(busid) < 0)
> +		if (del_match_busid(busid) < 0) {
>  			return -ENODEV;
> -		else {
> +		} else {
>  			pr_debug("del busid %s\n", busid);
>  			return count;
>  		}
> -	} else
> +	} else {
>  		return -EINVAL;
> +	}
>  }
> -static DRIVER_ATTR(match_busid, S_IRUSR|S_IWUSR, show_match_busid,
> +static DRIVER_ATTR(match_busid, S_IRUSR | S_IWUSR, show_match_busid,
>  		   store_match_busid);
>  
>  static struct stub_priv *stub_priv_pop_from_listhead(struct list_head *listhead)
> @@ -201,36 +210,30 @@ static struct stub_priv *stub_priv_pop(struct stub_device *sdev)
>  	spin_lock_irqsave(&sdev->priv_lock, flags);
>  
>  	priv = stub_priv_pop_from_listhead(&sdev->priv_init);
> -	if (priv) {
> -		spin_unlock_irqrestore(&sdev->priv_lock, flags);
> -		return priv;
> -	}
> +	if (priv)
> +		goto done;
>  
>  	priv = stub_priv_pop_from_listhead(&sdev->priv_tx);
> -	if (priv) {
> -		spin_unlock_irqrestore(&sdev->priv_lock, flags);
> -		return priv;
> -	}
> +	if (priv)
> +		goto done;
>  
>  	priv = stub_priv_pop_from_listhead(&sdev->priv_free);
> -	if (priv) {
> -		spin_unlock_irqrestore(&sdev->priv_lock, flags);
> -		return priv;
> -	}
>  
> +done:
>  	spin_unlock_irqrestore(&sdev->priv_lock, flags);
> -	return NULL;
> +
> +	return priv;
>  }
>  
>  void stub_device_cleanup_urbs(struct stub_device *sdev)
>  {
>  	struct stub_priv *priv;
> +	struct urb *urb;
>  
>  	dev_dbg(&sdev->udev->dev, "free sdev %p\n", sdev);
>  
>  	while ((priv = stub_priv_pop(sdev))) {
> -		struct urb *urb = priv->urb;
> -
> +		urb = priv->urb;
>  		dev_dbg(&sdev->udev->dev, "free urb %p\n", urb);
>  		usb_kill_urb(urb);
>  
> @@ -238,7 +241,6 @@ void stub_device_cleanup_urbs(struct stub_device *sdev)
>  
>  		kfree(urb->transfer_buffer);
>  		kfree(urb->setup_packet);
> -
>  		usb_free_urb(urb);
>  	}
>  }
> @@ -250,34 +252,31 @@ static int __init usb_stub_init(void)
>  	stub_priv_cache = kmem_cache_create("stub_priv",
>  					    sizeof(struct stub_priv), 0,
>  					    SLAB_HWCACHE_ALIGN, NULL);
> -
>  	if (!stub_priv_cache) {
> -		pr_err("create stub_priv_cache error\n");
> +		pr_err("kmem_cache_create failed\n");
>  		return -ENOMEM;
>  	}
>  
>  	ret = usb_register(&stub_driver);
> -	if (ret) {
> +	if (ret < 0) {
>  		pr_err("usb_register failed %d\n", ret);
> -		goto error_usb_register;
> +		goto err_usb_register;
>  	}
>  
> -	pr_info(DRIVER_DESC " " USBIP_VERSION "\n");
> -
> -	init_busid_table();
> -
>  	ret = driver_create_file(&stub_driver.drvwrap.driver,
>  				 &driver_attr_match_busid);
> -
> -	if (ret) {
> -		pr_err("create driver sysfs\n");
> -		goto error_create_file;
> +	if (ret < 0) {
> +		pr_err("driver_create_file failed\n");
> +		goto err_create_file;
>  	}
>  
> +	init_busid_table();
> +	pr_info(DRIVER_DESC " v" USBIP_VERSION "\n");
>  	return ret;
> -error_create_file:
> +
> +err_create_file:
>  	usb_deregister(&stub_driver);
> -error_usb_register:
> +err_usb_register:
>  	kmem_cache_destroy(stub_priv_cache);
>  	return ret;
>  }
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/