Patchwork [09/62] atm/nicstar: convert to idr_alloc()

login
register
mail settings
Submitter Tejun Heo
Date Feb. 3, 2013, 1:20 a.m.
Message ID <1359854463-2538-10-git-send-email-tj@kernel.org>
Download mbox | patch
Permalink /patch/217721/
State Not Applicable
Delegated to: David Miller
Headers show

Comments

Tejun Heo - Feb. 3, 2013, 1:20 a.m.
Convert to the much saner new idr interface.  The existing code looks
buggy to me - ID 0 is treated as no-ID but allocation specifies 0 as
lower limit and there's no error handling after parial success.  This
conversion keeps the bugs unchanged.

Only compile tested.

Signed-off-by: Tejun Heo <tj@kernel.org>
Cc: Chas Williams <chas@cmf.nrl.navy.mil>
Cc: netdev@vger.kernel.org
---
This patch depends on an earlier idr changes and I think it would be
best to route these together through -mm.  Please holler if there's
any objection.  Thanks.

 drivers/atm/nicstar.c | 27 ++++++++++++---------------
 1 file changed, 12 insertions(+), 15 deletions(-)
chas williams - CONTRACTOR - Feb. 4, 2013, 2:04 p.m.
I don't quite understand your comment.  idr_pre_get() returns 0 in the
case of failure.

On Sat,  2 Feb 2013 17:20:10 -0800
Tejun Heo <tj@kernel.org> wrote:

> Convert to the much saner new idr interface.  The existing code looks
> buggy to me - ID 0 is treated as no-ID but allocation specifies 0 as
> lower limit and there's no error handling after parial success.  This
> conversion keeps the bugs unchanged.
> 
> Only compile tested.
> 
> Signed-off-by: Tejun Heo <tj@kernel.org>
> Cc: Chas Williams <chas@cmf.nrl.navy.mil>
> Cc: netdev@vger.kernel.org
> ---
> This patch depends on an earlier idr changes and I think it would be
> best to route these together through -mm.  Please holler if there's
> any objection.  Thanks.
> 
>  drivers/atm/nicstar.c | 27 ++++++++++++---------------
>  1 file changed, 12 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/atm/nicstar.c b/drivers/atm/nicstar.c
> index 628787e..7fd6834 100644
> --- a/drivers/atm/nicstar.c
> +++ b/drivers/atm/nicstar.c
> @@ -1026,24 +1026,21 @@ static void push_rxbufs(ns_dev * card, struct sk_buff *skb)
>  				card->lbfqc += 2;
>  		}
>  
> -		do {
> -			if (!idr_pre_get(&card->idr, GFP_ATOMIC)) {
> -				printk(KERN_ERR
> -				       "nicstar%d: no free memory for idr\n",
> -				       card->index);
> +		if (!id1) {
> +			id1 = idr_alloc(&card->idr, handle1, 0, 0, GFP_ATOMIC);
> +			if (id1 < 0) {
> +				err = id1;
>  				goto out;
>  			}
> +		}
>  
> -			if (!id1)
> -				err = idr_get_new_above(&card->idr, handle1, 0, &id1);
> -
> -			if (!id2 && err == 0)
> -				err = idr_get_new_above(&card->idr, handle2, 0, &id2);
> -
> -		} while (err == -EAGAIN);
> -
> -		if (err)
> -			goto out;
> +		if (!id2) {
> +			id2 = idr_alloc(&card->idr, handle2, 0, 0, GFP_ATOMIC);
> +			if (id2 < 0) {
> +				err = id2;
> +				goto out;
> +			}
> +		}
>  
>  		spin_lock_irqsave(&card->res_lock, flags);
>  		while (CMD_BUSY(card)) ;

--
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
Tejun Heo - Feb. 4, 2013, 5:06 p.m.
Hello, Chas.

On Mon, Feb 04, 2013 at 09:04:10AM -0500, chas williams - CONTRACTOR wrote:
> I don't quite understand your comment.  idr_pre_get() returns 0 in the
> case of failure.

So, if you do the following,

	int id1 = 0, id2 = 0;

	if (!id1)
		err = idr_get_new_above(&card->idr, handle1, 0, &id1);
	if (!id2 && err == 0)
		err = idr_get_new_above(&card->idr, handle2, 0, &id2);
	if (err)
		goto out;
									    
You have no way of telling whether id1/2 are allocated or not.  0 is
the special "not allocated" value but it also is a valid ID.  The
error path should be freeing id1/2 if either of them has been
allocated but it doesn't and can't with 0 as the non-allocated value.

Thanks.
chas williams - CONTRACTOR - Feb. 4, 2013, 6:06 p.m.
On Mon, 4 Feb 2013 09:06:24 -0800
Tejun Heo <tj@kernel.org> wrote:

> Hello, Chas.
> 
> On Mon, Feb 04, 2013 at 09:04:10AM -0500, chas williams - CONTRACTOR wrote:
> > I don't quite understand your comment.  idr_pre_get() returns 0 in the
> > case of failure.
> 
> So, if you do the following,
> 
> 	int id1 = 0, id2 = 0;
> 
> 	if (!id1)
> 		err = idr_get_new_above(&card->idr, handle1, 0, &id1);
> 	if (!id2 && err == 0)
> 		err = idr_get_new_above(&card->idr, handle2, 0, &id2);
> 	if (err)
> 		goto out;
> 									    
> You have no way of telling whether id1/2 are allocated or not.  0 is
> the special "not allocated" value but it also is a valid ID.  The
> error path should be freeing id1/2 if either of them has been
> allocated but it doesn't and can't with 0 as the non-allocated value.

yeah, i see now.  it didn't understand what idr_get_new_above() was
doing with the start id.  i assumed that it would return something
greater than the start id.  i guess it never did return the starting id
(atleast after some initial failure).

additionally, yes, cleaning up if the second allocation failed was never
done.
--
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

diff --git a/drivers/atm/nicstar.c b/drivers/atm/nicstar.c
index 628787e..7fd6834 100644
--- a/drivers/atm/nicstar.c
+++ b/drivers/atm/nicstar.c
@@ -1026,24 +1026,21 @@  static void push_rxbufs(ns_dev * card, struct sk_buff *skb)
 				card->lbfqc += 2;
 		}
 
-		do {
-			if (!idr_pre_get(&card->idr, GFP_ATOMIC)) {
-				printk(KERN_ERR
-				       "nicstar%d: no free memory for idr\n",
-				       card->index);
+		if (!id1) {
+			id1 = idr_alloc(&card->idr, handle1, 0, 0, GFP_ATOMIC);
+			if (id1 < 0) {
+				err = id1;
 				goto out;
 			}
+		}
 
-			if (!id1)
-				err = idr_get_new_above(&card->idr, handle1, 0, &id1);
-
-			if (!id2 && err == 0)
-				err = idr_get_new_above(&card->idr, handle2, 0, &id2);
-
-		} while (err == -EAGAIN);
-
-		if (err)
-			goto out;
+		if (!id2) {
+			id2 = idr_alloc(&card->idr, handle2, 0, 0, GFP_ATOMIC);
+			if (id2 < 0) {
+				err = id2;
+				goto out;
+			}
+		}
 
 		spin_lock_irqsave(&card->res_lock, flags);
 		while (CMD_BUSY(card)) ;