| Submitter | Cyrill Gorcunov |
|---|---|
| Date | Nov. 23, 2010, 9:43 p.m. |
| Message ID | <20101123214344.GB1839@lenovo> |
| Download | mbox | patch |
| Permalink | /patch/72764/ |
| State | Accepted |
| Delegated to: | David Miller |
| Headers | show |
Comments
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. -- 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
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 -- 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
From: Cyrill Gorcunov <gorcunov@gmail.com> 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 <gorcunov@openvz.org> Looks good to me, applied, thanks Cyrill. -- 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
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 */
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 <gorcunov@openvz.org> CC: Paul Mackerras <paulus@samba.org> --- 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(-) -- 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