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

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

[Tejun Heo]

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)) ;
-- 
1.8.1

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

[chas williams - CONTRACTOR]

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

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

[Tejun Heo]

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.

-- 
tejun

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

[chas williams - CONTRACTOR]

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.

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

[Tejun Heo]

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 partial success.  This
conversion keeps the bugs unchanged.

Only compile tested.

v2: id1 and id2 are now directly used for -errno return and thus
    should be signed.  Make them int instead of u32.  This was spotted
    by kbuild test robot.

v3: Further simplify as suggested by Chas Williams.

Signed-off-by: Tejun Heo <tj@kernel.org>
Acked-by: Chas Williams <chas@cmf.nrl.navy.mil>
Reported-by: kbuild test robot <fengguang.wu@intel.com>
Cc: netdev@vger.kernel.org
---
 drivers/atm/nicstar.c |   24 ++++++------------------
 1 file changed, 6 insertions(+), 18 deletions(-)

--- a/drivers/atm/nicstar.c
+++ b/drivers/atm/nicstar.c
@@ -949,11 +949,10 @@ static void free_scq(ns_dev *card, scq_i
 static void push_rxbufs(ns_dev * card, struct sk_buff *skb)
 {
 	struct sk_buff *handle1, *handle2;
-	u32 id1 = 0, id2 = 0;
+	int id1, id2;
 	u32 addr1, addr2;
 	u32 stat;
 	unsigned long flags;
-	int err;
 
 	/* *BARF* */
 	handle2 = NULL;
@@ -1026,23 +1025,12 @@ static void push_rxbufs(ns_dev * card, s
 				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);
-				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);
+		id1 = idr_alloc(&card->idr, handle1, 0, 0, GFP_ATOMIC);
+		if (id1 < 0)
+			goto out;
 
-		if (err)
+		id2 = idr_alloc(&card->idr, handle2, 0, 0, GFP_ATOMIC);
+		if (id2 < 0)
 			goto out;
 
 		spin_lock_irqsave(&card->res_lock, flags);