From: matt mooney <mfm@muteddisk.com>
To: walter harms <wharms@bfs.de>
Cc: Greg Kroah-Hartman <greg@kroah.com>,
linux-kernel@vger.kernel.org, kernel-janitors@vger.kernel.org
Subject: Re: [PATCH 04/12] staging: usbip: stub_main.c: code cleanup
Date: Fri, 20 May 2011 11:45:17 -0700 [thread overview]
Message-ID: <20110520184517.GB83316@haskell.muteddisk.com> (raw)
In-Reply-To: <4DD62F8A.90208@bfs.de>
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 <mfm@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;
> > }
>
next prev parent reply other threads:[~2011-05-20 18:52 UTC|newest]
Thread overview: 20+ messages / expand[flat|nested] mbox.gz Atom feed top
2011-05-20 4:36 [PATCH 00/12] staging: usbip matt mooney
2011-05-20 4:36 ` [PATCH 01/12] staging: usbip: replace usbip_u{dbg,err,info} and printk with dev_ and pr_ matt mooney
2011-05-20 5:01 ` Greg KH
2011-05-20 5:25 ` matt mooney
2011-05-20 12:53 ` Greg KH
2011-05-20 18:51 ` matt mooney
2011-05-20 4:36 ` [PATCH 02/12] staging: usbip: remove unnecessary lines and extra return statements matt mooney
2011-05-20 4:36 ` [PATCH 03/12] staging: usbip: stub_main.c: reorder functions matt mooney
2011-05-20 4:36 ` [PATCH 04/12] staging: usbip: stub_main.c: code cleanup matt mooney
2011-05-20 9:08 ` walter harms
2011-05-20 18:45 ` matt mooney [this message]
2011-05-21 11:45 ` walter harms
2011-05-20 4:36 ` [PATCH 05/12] staging: usbip: stub_main.c: rename init and exit functions matt mooney
2011-05-20 4:37 ` [PATCH 06/12] staging: usbip: stub_main.c: use KMEM_CACHE macro matt mooney
2011-05-20 4:37 ` [PATCH 07/12] staging: usbip: usbip_common.c: fix misspelled function name matt mooney
2011-05-20 4:37 ` [PATCH 08/12] staging: usbip: usbip_common.h: reorganize and document request headers matt mooney
2011-05-20 4:37 ` [PATCH 09/12] staging: usbip: stub_dev.c: move stub_driver definition and update driver name matt mooney
2011-05-20 4:37 ` [PATCH 10/12] staging: usbip: userspace: bind_driver.c: update kernel module name matt mooney
2011-05-20 4:37 ` [PATCH 11/12] staging: usbip: usbip_common.c: rename init and exit functions matt mooney
2011-05-20 4:37 ` [PATCH 12/12] staging: usbip: vhci_hcd.c: " matt mooney
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20110520184517.GB83316@haskell.muteddisk.com \
--to=mfm@muteddisk.com \
--cc=greg@kroah.com \
--cc=kernel-janitors@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=wharms@bfs.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox