Patchwork ixp4xx_hss: Little improvement in create_chan function

login
register
mail settings
Submitter =?ISO-8859-1?Q?Miguel_=C1ngel_=C1lvarez?=
Date Nov. 26, 2008, 5:38 p.m.
Message ID <b0438a630811260938n14835453jda2da43c6f1445f9@mail.gmail.com>
Download mbox | patch
Permalink /patch/10960/
State Not Applicable
Delegated to: David Miller
Headers show

Comments

Hi.

We are searching for the desired channel three times in the same list.
So I think we could erase two of them. What do you think?

Signed-off-by: Miguel Ángel Álvarez (gotzoncabanes@gmail.com)
---

Thanks

Miguel Ángel Álvarez
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Krzysztof Halasa - Dec. 4, 2008, 8:54 p.m.
"Miguel Ángel Álvarez" <gotzoncabanes@gmail.com> writes:

> We are searching for the desired channel three times in the same list.
> So I think we could erase two of them. What do you think?
>
> +++ ixp4xx_hss.c.create_chan	2008-11-26 18:34:23.000000000 +0100
>
>  	for (ch = 0; ch < MAX_CHANNELS; ch++)
> -		if (channels[ch] && port->channels[ch] != CHANNEL_UNUSED) {
> -			printk(KERN_DEBUG "Channel #%i already in use\n", ch);
> -			err = -EBUSY;
> -			goto free;

Well, the above checks if _all_ needed port->channels[] are unused.

> +		if (channels[ch]) {
> +		 	if (port->channels[ch] != CHANNEL_UNUSED) {
> +				printk(KERN_DEBUG "Channel #%i already in use\n", ch);
> +				err = -EBUSY;
> +				goto free;
> +			} else
> +				break;

We can't break here because we have to check all the remaining
channels (which are in channels[]).

>  		}
>

> -	for (ch = 0; ch < MAX_CHANNELS; ch++)
> -		if (channels[ch])
> -			break;
> -

This simply seeks the first channel (to pick up number for the
device name).

> -	for (ch = 0; ch < MAX_CHANNELS; ch++)
> -		if (channels[ch])
> -			port->channels[ch] = id;

This sets the ID of all requested channels (i.e., it's some sort of
a bitmap).

> +	port->channels[ch] = id;
>  	port->chan_devices[id] = chan_dev;
>  	dev_set_drvdata(chan_dev->dev, chan_dev);
>  	BUG_ON(device_create_file(chan_dev->dev, &chan_attr));

We need to
a) first check if all needed channels are free, then
b) mark the above as used (with our ID), and
c) note the first requested ID for the device name.

"c" could be optimized (in terms of execution speed), perhaps. Since
it's a slow path (device setup), I didn't bother, I just tried to use
something simple and readable.
Hi

You are absolutely right with create_chan function... I read it badly.

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

Patch

--- ixp4xx_hss.c.orig	2008-11-19 11:26:54.000000000 +0100
+++ ixp4xx_hss.c.create_chan	2008-11-26 18:34:23.000000000 +0100
@@ -2273,10 +2273,13 @@ 
 	}

 	for (ch = 0; ch < MAX_CHANNELS; ch++)
-		if (channels[ch] && port->channels[ch] != CHANNEL_UNUSED) {
-			printk(KERN_DEBUG "Channel #%i already in use\n", ch);
-			err = -EBUSY;
-			goto free;
+		if (channels[ch]) {
+		 	if (port->channels[ch] != CHANNEL_UNUSED) {
+				printk(KERN_DEBUG "Channel #%i already in use\n", ch);
+				err = -EBUSY;
+				goto free;
+			} else
+				break;
 		}

 	for (id = 0; id < MAX_CHAN_DEVICES; id++)
@@ -2288,10 +2291,6 @@ 
 		goto free;
 	}

-	for (ch = 0; ch < MAX_CHANNELS; ch++)
-		if (channels[ch])
-			break;
-
 	minor = port->id * MAX_CHAN_DEVICES + ch;
 	chan_dev->id = id;
 	chan_dev->port = port;
@@ -2307,9 +2306,7 @@ 
 	if ((err = cdev_add(&chan_dev->cdev, MKDEV(chan_major, minor), 1)))
 		goto destroy_device;

-	for (ch = 0; ch < MAX_CHANNELS; ch++)
-		if (channels[ch])
-			port->channels[ch] = id;
+	port->channels[ch] = id;
 	port->chan_devices[id] = chan_dev;
 	dev_set_drvdata(chan_dev->dev, chan_dev);
 	BUG_ON(device_create_file(chan_dev->dev, &chan_attr));