| Message ID | 1413311335-25083-1-git-send-email-balbi@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi, On 14/10/14 21:28, Felipe Balbi wrote: > if we leave __exit annotation, driver can't be unbound > through sysfs. > > Signed-off-by: Felipe Balbi <balbi@ti.com> > --- > drivers/video/fbdev/omap2/omapfb/omapfb-main.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/video/fbdev/omap2/omapfb/omapfb-main.c b/drivers/video/fbdev/omap2/omapfb/omapfb-main.c > index ec2d132..9cbf1ce 100644 > --- a/drivers/video/fbdev/omap2/omapfb/omapfb-main.c > +++ b/drivers/video/fbdev/omap2/omapfb/omapfb-main.c > @@ -2619,7 +2619,7 @@ err0: > return r; > } > > -static int __exit omapfb_remove(struct platform_device *pdev) > +static int omapfb_remove(struct platform_device *pdev) > { > struct omapfb2_device *fbdev = platform_get_drvdata(pdev); > > @@ -2636,7 +2636,7 @@ static int __exit omapfb_remove(struct platform_device *pdev) > > static struct platform_driver omapfb_driver = { > .probe = omapfb_probe, > - .remove = __exit_p(omapfb_remove), > + .remove = omapfb_remove, > .driver = { > .name = "omapfb", > .owner = THIS_MODULE, Interesting. I don't know if I'm doing something funny, but without this patch, I can unbind omapfb, kind of. "echo omapfb > unbind" goes ok, but remove is obviously not called. Somehow omapfb device is still unbound from the driver, as I can then bind it again, causing probe to be called. Which breaks everything. I would've thought that unbinding is not possible if remove is missing, but that doesn't seem to be the case. I guess it just means that remove is not called when the driver & device are unbound. We have 18 __exit_p()s in omapdss and related drivers. I guess they are all broken the same way. Note that omapfb unbind & bind does not work even with this patch, but results in a crash as some old state is left into omapdss. The same happens also with unloading and loading omapfb module (but keeping omapdss module loaded). So there seems to be more issues around this. Tomi
Hi, On Wed, Oct 15, 2014 at 03:13:34PM +0300, Tomi Valkeinen wrote: > Hi, > > On 14/10/14 21:28, Felipe Balbi wrote: > > if we leave __exit annotation, driver can't be unbound > > through sysfs. > > > > Signed-off-by: Felipe Balbi <balbi@ti.com> > > --- > > drivers/video/fbdev/omap2/omapfb/omapfb-main.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/video/fbdev/omap2/omapfb/omapfb-main.c b/drivers/video/fbdev/omap2/omapfb/omapfb-main.c > > index ec2d132..9cbf1ce 100644 > > --- a/drivers/video/fbdev/omap2/omapfb/omapfb-main.c > > +++ b/drivers/video/fbdev/omap2/omapfb/omapfb-main.c > > @@ -2619,7 +2619,7 @@ err0: > > return r; > > } > > > > -static int __exit omapfb_remove(struct platform_device *pdev) > > +static int omapfb_remove(struct platform_device *pdev) > > { > > struct omapfb2_device *fbdev = platform_get_drvdata(pdev); > > > > @@ -2636,7 +2636,7 @@ static int __exit omapfb_remove(struct platform_device *pdev) > > > > static struct platform_driver omapfb_driver = { > > .probe = omapfb_probe, > > - .remove = __exit_p(omapfb_remove), > > + .remove = omapfb_remove, > > .driver = { > > .name = "omapfb", > > .owner = THIS_MODULE, > > Interesting. I don't know if I'm doing something funny, but without this > patch, I can unbind omapfb, kind of. > > "echo omapfb > unbind" goes ok, but remove is obviously not called. remove isn't called because it won't exist if it's built-in. Look at the definition of __exit_p() > Somehow omapfb device is still unbound from the driver, as I can then > bind it again, causing probe to be called. Which breaks everything. > > I would've thought that unbinding is not possible if remove is missing, > but that doesn't seem to be the case. I guess it just means that remove > is not called when the driver & device are unbound. if no remove it provided on platform_driver structure, platform bus assumes you have nothing to do on your ->remove(), so you end up leaking all resources you allocated on ->probe() (unless you *really* don't need to do anything on ->remove). > We have 18 __exit_p()s in omapdss and related drivers. I guess they are > all broken the same way. yup, I should've grepped. > Note that omapfb unbind & bind does not work even with this patch, but > results in a crash as some old state is left into omapdss. The same > happens also with unloading and loading omapfb module (but keeping > omapdss module loaded). It worked fine for me. I unbound and bound omapfb multiple times. > So there seems to be more issues around this. quite a few more, I'd say
On 15/10/14 17:41, Felipe Balbi wrote: >> Interesting. I don't know if I'm doing something funny, but without this >> patch, I can unbind omapfb, kind of. >> >> "echo omapfb > unbind" goes ok, but remove is obviously not called. > > remove isn't called because it won't exist if it's built-in. Look at the > definition of __exit_p() Yes, that's what I meant with "obviously". >> Somehow omapfb device is still unbound from the driver, as I can then >> bind it again, causing probe to be called. Which breaks everything. >> >> I would've thought that unbinding is not possible if remove is missing, >> but that doesn't seem to be the case. I guess it just means that remove >> is not called when the driver & device are unbound. > > if no remove it provided on platform_driver structure, platform bus > assumes you have nothing to do on your ->remove(), so you end up leaking > all resources you allocated on ->probe() (unless you *really* don't need > to do anything on ->remove). Yep. That's quite odd, still. grep shows quite many uses of __exit_p(), and all for remove callback. So, if you have something to release in remove(), you should set it always, for both module and built-in. And if you don't have anything to release, you would always just set .release to NULL. I mean, what's the use case for __exit_p()? With a quick glance, at least some of the other users also use __exit_p() the same way omapdss does (i.e. in the wrong way). >> We have 18 __exit_p()s in omapdss and related drivers. I guess they are >> all broken the same way. > > yup, I should've grepped. > >> Note that omapfb unbind & bind does not work even with this patch, but >> results in a crash as some old state is left into omapdss. The same >> happens also with unloading and loading omapfb module (but keeping >> omapdss module loaded). > > It worked fine for me. I unbound and bound omapfb multiple times. Hmm, ok. Odd, the bug was quite clear and I think it should happen every time. Well, I was using omap4. If you used AM4xx, that's basically omap3 DSS. Maybe there's a diff there. >> So there seems to be more issues around this. > > quite a few more, I'd say Yep, I'll have a look at this. Tomi
Hi, On Wed, Oct 15, 2014 at 06:43:40PM +0300, Tomi Valkeinen wrote: > >> Somehow omapfb device is still unbound from the driver, as I can then > >> bind it again, causing probe to be called. Which breaks everything. > >> > >> I would've thought that unbinding is not possible if remove is missing, > >> but that doesn't seem to be the case. I guess it just means that remove > >> is not called when the driver & device are unbound. > > > > if no remove it provided on platform_driver structure, platform bus > > assumes you have nothing to do on your ->remove(), so you end up leaking > > all resources you allocated on ->probe() (unless you *really* don't need > > to do anything on ->remove). > > Yep. That's quite odd, still. grep shows quite many uses of __exit_p(), > and all for remove callback. So, if you have something to release in > remove(), you should set it always, for both module and built-in. And if > you don't have anything to release, you would always just set .release > to NULL. > > I mean, what's the use case for __exit_p()? With a quick glance, at > least some of the other users also use __exit_p() the same way omapdss > does (i.e. in the wrong way). __exit_p() meant something else a few years back, perhaps those were left over from some tree-wide cleanups. > >> We have 18 __exit_p()s in omapdss and related drivers. I guess they are > >> all broken the same way. > > > > yup, I should've grepped. > > > >> Note that omapfb unbind & bind does not work even with this patch, but > >> results in a crash as some old state is left into omapdss. The same > >> happens also with unloading and loading omapfb module (but keeping > >> omapdss module loaded). > > > > It worked fine for me. I unbound and bound omapfb multiple times. > > Hmm, ok. Odd, the bug was quite clear and I think it should happen every > time. Well, I was using omap4. If you used AM4xx, that's basically omap3 > DSS. Maybe there's a diff there. could very well be :-) > >> So there seems to be more issues around this. > > > > quite a few more, I'd say > > Yep, I'll have a look at this. alright
diff --git a/drivers/video/fbdev/omap2/omapfb/omapfb-main.c b/drivers/video/fbdev/omap2/omapfb/omapfb-main.c index ec2d132..9cbf1ce 100644 --- a/drivers/video/fbdev/omap2/omapfb/omapfb-main.c +++ b/drivers/video/fbdev/omap2/omapfb/omapfb-main.c @@ -2619,7 +2619,7 @@ err0: return r; } -static int __exit omapfb_remove(struct platform_device *pdev) +static int omapfb_remove(struct platform_device *pdev) { struct omapfb2_device *fbdev = platform_get_drvdata(pdev); @@ -2636,7 +2636,7 @@ static int __exit omapfb_remove(struct platform_device *pdev) static struct platform_driver omapfb_driver = { .probe = omapfb_probe, - .remove = __exit_p(omapfb_remove), + .remove = omapfb_remove, .driver = { .name = "omapfb", .owner = THIS_MODULE,
if we leave __exit annotation, driver can't be unbound through sysfs. Signed-off-by: Felipe Balbi <balbi@ti.com> --- drivers/video/fbdev/omap2/omapfb/omapfb-main.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)