Message ID | 5664545C.90607@imap.cc |
---|---|
State | RFC, archived |
Delegated to: | David Miller |
Headers | show |
On zo, 2015-12-06 at 16:29 +0100, Tilman Schmidt wrote: > So the solution might be as simple as moving the kfree() call from > gigaset_freecshw() to gigaset_device_release(). Something like this: > > --- a/drivers/isdn/gigaset/ser-gigaset.c > +++ b/drivers/isdn/gigaset/ser-gigaset.c > @@ -370,19 +370,18 @@ static void gigaset_freecshw(struct cardstate > *cs) > tasklet_kill(&cs->write_tasklet); > if (!cs->hw.ser) > return; > - dev_set_drvdata(&cs->hw.ser->dev.dev, NULL); > platform_device_unregister(&cs->hw.ser->dev); > - kfree(cs->hw.ser); > - cs->hw.ser = NULL; > } > > static void gigaset_device_release(struct device *dev) > { > - struct platform_device *pdev = to_platform_device(dev); > + struct cardstate *cs = dev_get_drvdata(dev); > > - /* adapted from platform_device_release() in > drivers/base/platform.c */ > - kfree(dev->platform_data); > - kfree(pdev->resource); > + if (!cs) > + return; > + dev_set_drvdata(dev, NULL); > + kfree(cs->hw.ser); > + cs->hw.ser = NULL; > } This solution assumes that the struct platform_device is moved out of the struct ser_cardstate, doesn't it? In other words, this is something to do on top of my (draft) patch. Otherwise we'd still be freeing memory managed through reference counting. Thanks, Paul Bolle -- 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
Am 06.12.2015 um 21:12 schrieb Paul Bolle: > On zo, 2015-12-06 at 16:29 +0100, Tilman Schmidt wrote: >> So the solution might be as simple as moving the kfree() call from >> gigaset_freecshw() to gigaset_device_release(). Something like this: >> >> --- a/drivers/isdn/gigaset/ser-gigaset.c >> +++ b/drivers/isdn/gigaset/ser-gigaset.c >> @@ -370,19 +370,18 @@ static void gigaset_freecshw(struct cardstate >> *cs) >> tasklet_kill(&cs->write_tasklet); >> if (!cs->hw.ser) >> return; >> - dev_set_drvdata(&cs->hw.ser->dev.dev, NULL); >> platform_device_unregister(&cs->hw.ser->dev); >> - kfree(cs->hw.ser); >> - cs->hw.ser = NULL; >> } >> >> static void gigaset_device_release(struct device *dev) >> { >> - struct platform_device *pdev = to_platform_device(dev); >> + struct cardstate *cs = dev_get_drvdata(dev); >> >> - /* adapted from platform_device_release() in drivers/base/platform.c */ >> - kfree(dev->platform_data); >> - kfree(pdev->resource); >> + if (!cs) >> + return; >> + dev_set_drvdata(dev, NULL); >> + kfree(cs->hw.ser); >> + cs->hw.ser = NULL; >> } > > This solution assumes that the struct platform_device is moved out of > the struct ser_cardstate, doesn't it? In other words, this is something > to do on top of my (draft) patch. No, that wasn't my intention. I thought of that solution as an alternative, not an increment to your patch. > Otherwise we'd still be freeing memory > managed through reference counting. Now I#m confused. I thought by following Peter's suggestion to put the kfree() in the release method we'd avoid just that. Regards, Tilman
[Re-added mailinglist that got dropped somehow.] On ma, 2015-12-07 at 10:27 +0100, Tilman Schmidt wrote: > Am 06.12.2015 um 21:12 schrieb Paul Bolle: > > This solution assumes that the struct platform_device is moved out > > of > > the struct ser_cardstate, doesn't it? In other words, this is > > something > > to do on top of my (draft) patch. > > No, that wasn't my intention. I thought of that solution as an > alternative, not an increment to your patch. > > > Otherwise we'd still be freeing memory > > managed through reference counting. > > Now I#m confused. I thought by following Peter's suggestion to put the > kfree() in the release method we'd avoid just that. (Your patch compiles just fine.) Apparently it does, because I can't trigger the WARNING we're discussing here with your patch applied. I'll have to dive into this stuff again, because apparently my mental model of what's going on is incomplete at best. In the mean time you might want to turn your patch into something that can actually be applied (with or without my Sign-off or Ack; I don't care how it finds its way into the tree). Please add add Fixes: 2869b23e4b95 ("[PATCH] drivers/isdn/gigaset: new M101 driver (v2)") (Perhaps with a comment that v2.6.32 needs a trivial context change; I'm not sure how that needs to be communicated.) But I'm fine with cobbling together a commit explanation myself if you're too busy right now. Thanks, Paul Bolle -- 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
Am 07.12.2015 um 13:25 schrieb Paul Bolle: >>> Otherwise we'd still be freeing memory >>> managed through reference counting. >> >> Now I#m confused. I thought by following Peter's suggestion to put the >> kfree() in the release method we'd avoid just that. > > Apparently it does, because I can't trigger the WARNING we're discussing > here with your patch applied. Nice. > I'll have to dive into this stuff again, > because apparently my mental model of what's going on is incomplete at > best. I won't claim anything like completeness for mine. > In the mean time you might want to turn your patch into something that > can actually be applied (with or without my Sign-off or Ack; I don't > care how it finds its way into the tree). Please add add > Fixes: 2869b23e4b95 ("[PATCH] drivers/isdn/gigaset: new M101 driver (v2)") Will do. (Not today, though.) Regards, Tilman
--- a/drivers/isdn/gigaset/ser-gigaset.c +++ b/drivers/isdn/gigaset/ser-gigaset.c @@ -370,19 +370,18 @@ static void gigaset_freecshw(struct cardstate *cs) tasklet_kill(&cs->write_tasklet); if (!cs->hw.ser) return; - dev_set_drvdata(&cs->hw.ser->dev.dev, NULL); platform_device_unregister(&cs->hw.ser->dev); - kfree(cs->hw.ser); - cs->hw.ser = NULL; } static void gigaset_device_release(struct device *dev) { - struct platform_device *pdev = to_platform_device(dev); + struct cardstate *cs = dev_get_drvdata(dev); - /* adapted from platform_device_release() in drivers/base/platform.c */ - kfree(dev->platform_data); - kfree(pdev->resource); + if (!cs) + return; + dev_set_drvdata(dev, NULL); + kfree(cs->hw.ser); + cs->hw.ser = NULL; }