atom feed22 messages in org.kernel.vger.kernel-janitorsRe: [PATCH 04/12] staging: usbip: stu...
FromSent OnAttachments
matt mooneyMay 19, 2011 9:36 pm 
matt mooneyMay 19, 2011 9:36 pm 
matt mooneyMay 19, 2011 9:36 pm 
matt mooneyMay 19, 2011 9:36 pm 
matt mooneyMay 19, 2011 9:36 pm 
matt mooneyMay 19, 2011 9:36 pm 
matt mooneyMay 19, 2011 9:37 pm 
matt mooneyMay 19, 2011 9:37 pm 
matt mooneyMay 19, 2011 9:37 pm 
matt mooneyMay 19, 2011 9:37 pm 
matt mooneyMay 19, 2011 9:37 pm 
matt mooneyMay 19, 2011 9:37 pm 
matt mooneyMay 19, 2011 9:37 pm 
Greg KHMay 19, 2011 9:59 pm 
matt mooneyMay 19, 2011 10:25 pm 
walter harmsMay 20, 2011 1:37 am 
walter harmsMay 20, 2011 2:08 am 
Greg KHMay 20, 2011 5:53 am 
matt mooneyMay 20, 2011 11:42 am 
matt mooneyMay 20, 2011 11:45 am 
matt mooneyMay 20, 2011 11:51 am 
walter harmsMay 21, 2011 4:45 am 
Subject:Re: [PATCH 04/12] staging: usbip: stub_main.c: code cleanup
From:matt mooney (mf@muteddisk.com)
Date:May 20, 2011 11:45:17 am
List:org.kernel.vger.kernel-janitors

On 11:08 Fri 20 May , walter harms wrote:

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 <mf@muteddisk.com>

--- 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 ??

I am not sure what you mean. It should be correct.

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; }