Allocating unit from ird might return several error codes
not only -EAGAIN, so it should not be changed and returned
precisely. Same time unit release procedure should be invoked
only if device is unregistering.
Signed-off-by: Cyrill Gorcunov <[email protected]>
CC: Paul Mackerras <[email protected]>
---
Please review, thanks! (I've tested it a bit but more tests would
be really appreciated as well as comments)
drivers/net/ppp_generic.c | 43 ++++++++++++++++++++++---------------------
1 file changed, 22 insertions(+), 21 deletions(-)
Index: linux-2.6.git/drivers/net/ppp_generic.c
=====================================================================
--- linux-2.6.git.orig/drivers/net/ppp_generic.c
+++ linux-2.6.git/drivers/net/ppp_generic.c
@@ -2584,16 +2584,16 @@ ppp_create_interface(struct net *net, in
*/
dev_net_set(dev, net);
- ret = -EEXIST;
mutex_lock(&pn->all_ppp_mutex);
if (unit < 0) {
unit = unit_get(&pn->units_idr, ppp);
if (unit < 0) {
- *retp = unit;
+ ret = unit;
goto out2;
}
} else {
+ ret = -EEXIST;
if (unit_find(&pn->units_idr, unit))
goto out2; /* unit already exists */
/*
@@ -2668,10 +2668,10 @@ static void ppp_shutdown_interface(struc
ppp->closing = 1;
ppp_unlock(ppp);
unregister_netdev(ppp->dev);
+ unit_put(&pn->units_idr, ppp->file.index);
} else
ppp_unlock(ppp);
- unit_put(&pn->units_idr, ppp->file.index);
ppp->file.dead = 1;
ppp->owner = NULL;
wake_up_interruptible(&ppp->file.rwait);
@@ -2859,8 +2859,7 @@ static void __exit ppp_cleanup(void)
* by holding all_ppp_mutex
*/
-/* associate pointer with specified number */
-static int unit_set(struct idr *p, void *ptr, int n)
+static int __unit_alloc(struct idr *p, void *ptr, int n)
{
int unit, err;
@@ -2871,10 +2870,24 @@ again:
}
err = idr_get_new_above(p, ptr, n, &unit);
- if (err == -EAGAIN)
- goto again;
+ if (err < 0) {
+ if (err == -EAGAIN)
+ goto again;
+ return err;
+ }
+
+ return unit;
+}
+
+/* associate pointer with specified number */
+static int unit_set(struct idr *p, void *ptr, int n)
+{
+ int unit;
- if (unit != n) {
+ unit = __unit_alloc(p, ptr, n);
+ if (unit < 0)
+ return unit;
+ else if (unit != n) {
idr_remove(p, unit);
return -EINVAL;
}
@@ -2885,19 +2898,7 @@ again:
/* get new free unit number and associate pointer with it */
static int unit_get(struct idr *p, void *ptr)
{
- int unit, err;
-
-again:
- if (!idr_pre_get(p, GFP_KERNEL)) {
- printk(KERN_ERR "PPP: No free memory for idr\n");
- return -ENOMEM;
- }
-
- err = idr_get_new_above(p, ptr, 0, &unit);
- if (err == -EAGAIN)
- goto again;
-
- return unit;
+ return __unit_alloc(p, ptr, 0);
}
/* put unit number back to a pool */
On 2010-11-23 22:43, Cyrill Gorcunov wrote:
> Allocating unit from ird might return several error codes
> not only -EAGAIN, so it should not be changed and returned
> precisely. Same time unit release procedure should be invoked
> only if device is unregistering.
IMHO this unit release fix should be in a separate patch.
...
> @@ -2668,10 +2668,10 @@ static void ppp_shutdown_interface(struc
> ppp->closing = 1;
> ppp_unlock(ppp);
> unregister_netdev(ppp->dev);
> + unit_put(&pn->units_idr, ppp->file.index);
> } else
> ppp_unlock(ppp);
>
> - unit_put(&pn->units_idr, ppp->file.index);
> ppp->file.dead = 1;
> ppp->owner = NULL;
> wake_up_interruptible(&ppp->file.rwait);
Btw, it seems these last 3 lines could be moved similarly.
Jarek P.
On Wed, Nov 24, 2010 at 12:49:01PM +0000, Jarek Poplawski wrote:
> On 2010-11-23 22:43, Cyrill Gorcunov wrote:
> > Allocating unit from ird might return several error codes
> > not only -EAGAIN, so it should not be changed and returned
> > precisely. Same time unit release procedure should be invoked
> > only if device is unregistering.
>
> IMHO this unit release fix should be in a separate patch.
>
I thought about it, but still think it should be addressed at
same patch. Though if a separate would be preferred still --
I've no problem in making two patches instead.
> ...
> > @@ -2668,10 +2668,10 @@ static void ppp_shutdown_interface(struc
> > ppp->closing = 1;
> > ppp_unlock(ppp);
> > unregister_netdev(ppp->dev);
> > + unit_put(&pn->units_idr, ppp->file.index);
> > } else
> > ppp_unlock(ppp);
> >
> > - unit_put(&pn->units_idr, ppp->file.index);
> > ppp->file.dead = 1;
> > ppp->owner = NULL;
> > wake_up_interruptible(&ppp->file.rwait);
>
> Btw, it seems these last 3 lines could be moved similarly.
yup, at least ppp->file.dead and ppp->owner for sure, I wanted
make this patch 'unit' orientedc and do not touch anything aside,
it should be a separate change.
>
> Jarek P.
>
Thanks for comments Jarek!
Cyrill
From: Cyrill Gorcunov <[email protected]>
Date: Wed, 24 Nov 2010 00:43:44 +0300
> Allocating unit from ird might return several error codes
> not only -EAGAIN, so it should not be changed and returned
> precisely. Same time unit release procedure should be invoked
> only if device is unregistering.
>
> Signed-off-by: Cyrill Gorcunov <[email protected]>
Looks good to me, applied, thanks Cyrill.