Re: [PATCH] backports: backport drvdata = NULL core driver fixes
On Fri, Jul 19, 2013 at 8:36 PM, Alan Stern wrote: > On Fri, 19 Jul 2013, Luis R. Rodriguez wrote: > >> Thanks Julia. In that case I'm going to just leave this in place given >> that if there's a bug upstream we'll get it fixed as soon as a >> respective patch gets upstream as well. That is, we are not using old >> drivers, we use the same upstream drivers so if a regression was found >> in backports the fix must go upstream s well. This is one of the >> benefits of backporting -- the range of users and testers increases >> and we still benefit from the upstream bandwagon. > > I don't understand. If you're not using old drivers, and you > incorporate all the upstream patches, then what's the difference > between a backport and the current kernel? The difference is that in older kernels driver_probe_device() and device_release_driver() wouldn't do the setting to NULL in case of probe fail or release, that's what the patch does. Given that the new drivers have the superfluous setting removed we'd need a way to get older kernels to clear it as well and there were two ways to do it -- revert the changes for all drivers this was cleared from or hack up the same callback used driver_probe_device() and device_release_driver() to do the appropriate clearing prior to calling the old kernel's routines. > In fact, if you're > incorporating all the upstream driver patches, then why haven't you > already got the drvdata change? Because older kernel's driver_probe_device() and device_release_driver() would be used. > As one example of the sort of subtle problem exposed by the drvdata > change, take a look at commit b2ca69907657. Understood. > For more examples, see commits bf90ff5f3b8f, 638b9e15233c, > 51ef847df746, 289b076f89c2, 53636555b919, 99a6f73c495c, 003615302a16, > 94ab71ce2889, 3124d1d71d3d, c27f3efc5608, 95940a04bfe8, 5c1a0f418d8d, > db5c8b52, 8bf769eb5f6e, 4295fe7791a1, fa919751a2d2, a9556040119a, > 7bdce71822f4, and a1028f0abfb3. Admittedly, these are all related > problems in a single subsystem, but it gives you a little idea of how > far this goes. Sure, I understand. By pushing the newer drivers as-is *and* by providing the driver_probe_device() and device_release_driver() respective changes for older kernels it should get us similar behavior on older kernels. Mind you, there is a small race in the way I implemented this on device_release_driver() but that seems like a reasonable tradeoff against all other alternatives I could come up with. As it is now, unless my port is incorrect, the newer drivers used on older systems should trigger a similar bug as if using the upstream kernel with the same drivers. Luis -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] backports: backport drvdata = NULL core driver fixes
On Fri, 19 Jul 2013, Luis R. Rodriguez wrote: > Thanks Julia. In that case I'm going to just leave this in place given > that if there's a bug upstream we'll get it fixed as soon as a > respective patch gets upstream as well. That is, we are not using old > drivers, we use the same upstream drivers so if a regression was found > in backports the fix must go upstream s well. This is one of the > benefits of backporting -- the range of users and testers increases > and we still benefit from the upstream bandwagon. I don't understand. If you're not using old drivers, and you incorporate all the upstream patches, then what's the difference between a backport and the current kernel? In fact, if you're incorporating all the upstream driver patches, then why haven't you already got the drvdata change? As one example of the sort of subtle problem exposed by the drvdata change, take a look at commit b2ca69907657. For more examples, see commits bf90ff5f3b8f, 638b9e15233c, 51ef847df746, 289b076f89c2, 53636555b919, 99a6f73c495c, 003615302a16, 94ab71ce2889, 3124d1d71d3d, c27f3efc5608, 95940a04bfe8, 5c1a0f418d8d, db5c8b52, 8bf769eb5f6e, 4295fe7791a1, fa919751a2d2, a9556040119a, 7bdce71822f4, and a1028f0abfb3. Admittedly, these are all related problems in a single subsystem, but it gives you a little idea of how far this goes. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] backports: backport drvdata = NULL core driver fixes
On Fri, Jul 19, 2013 at 2:27 PM, Julia Lawall wrote: > On Fri, 19 Jul 2013, Luis R. Rodriguez wrote: > >> On Fri, Jul 19, 2013 at 2:07 PM, Luis R. Rodriguez >> wrote: >> >> This is not a very good idea. Although setting drvdata to NULL allowed >> >> a lot of code to be removed, it also exposed a bunch of hidden bugs -- >> >> drivers were using the drvdata value even after their remove function >> >> returned. >> > >> > Eek, have we not SmPL'ify'd a proof yet to ensure code like this no >> > longer exists? Julia? :) >> >> Come to think of it, perhaps we should require *proof* with SmPL like >> this in future to avoid regressions ? > > Is it a concurrency problem? SmPL is not so good at that in the general > case. One would have to know a specific case where other functions of the > driver can be invoked after remove. Thanks Julia. In that case I'm going to just leave this in place given that if there's a bug upstream we'll get it fixed as soon as a respective patch gets upstream as well. That is, we are not using old drivers, we use the same upstream drivers so if a regression was found in backports the fix must go upstream s well. This is one of the benefits of backporting -- the range of users and testers increases and we still benefit from the upstream bandwagon. Luis -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] backports: backport drvdata = NULL core driver fixes
On Fri, 19 Jul 2013, Luis R. Rodriguez wrote: > On Fri, Jul 19, 2013 at 2:07 PM, Luis R. Rodriguez > wrote: > >> This is not a very good idea. Although setting drvdata to NULL allowed > >> a lot of code to be removed, it also exposed a bunch of hidden bugs -- > >> drivers were using the drvdata value even after their remove function > >> returned. > > > > Eek, have we not SmPL'ify'd a proof yet to ensure code like this no > > longer exists? Julia? :) > > Come to think of it, perhaps we should require *proof* with SmPL like > this in future to avoid regressions ? Is it a concurrency problem? SmPL is not so good at that in the general case. One would have to know a specific case where other functions of the driver can be invoked after remove. julia -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] backports: backport drvdata = NULL core driver fixes
On Fri, Jul 19, 2013 at 7:17 AM, Alan Stern wrote: > On Thu, 18 Jul 2013, Luis R. Rodriguez wrote: > >> From: "Luis R. Rodriguez" >> >> The Linux kernel had tons of code which at times cleared the >> drvdata upon probe failure or release. There are however a bunch >> of drivers that didn't clear this. >> >> Commit 0998d063 implmented clearing this upon device_release_driver() >> and dealt with probe failure on driver_probe_device(). After this the >> kernel was cleaned up separately with *tons* of patches to remove all >> these driver specific settings given that the clearing is now done >> internally by the device core. >> >> Instead of ifdef'ing code back in for older code where it was properly >> in place backport this by piggy backing the new required code upon the >> calls used in place. There is a small race here upon device_release_driver() >> but we can live with that theoretical race. >> >> Due to the way we hack this backport we can't use a separate namespace >> as we have with other symbols. >> >> mcgrof@frijol ~/linux-stable (git::master)$ git describe --contains \ >> 0998d0631001288a5974afc0b2a5f568bcdecb4d >> v3.6-rc1~99^2~14^2~17 >> >> commit 0998d0631001288a5974afc0b2a5f568bcdecb4d >> Author: Hans de Goede >> Date: Wed May 23 00:09:34 2012 +0200 >> >> device-core: Ensure drvdata = NULL when no driver is bound >> >> 1) drvdata is for a driver to store a pointer to driver specific data >> 2) If no driver is bound, there is no driver specific data associated >> with >>the device >> 3) Thus logically drvdata should be NULL if no driver is bound. >> >> But many drivers don't clear drvdata on device_release, or set drvdata >> early on in probe and leave it set on probe error. Both of which results >> in a dangling pointer in drvdata. >> >> This patch enforce for drvdata to be NULL after device_release or on >> probe >> failure. >> >> Signed-off-by: Hans de Goede >> Signed-off-by: Greg Kroah-Hartman > > This is not a very good idea. Although setting drvdata to NULL allowed > a lot of code to be removed, it also exposed a bunch of hidden bugs -- > drivers were using the drvdata value even after their remove function > returned. Eek, have we not SmPL'ify'd a proof yet to ensure code like this no longer exists? Julia? :) > Several commits have been applied to fix those bugs. I actualy found *tons* ! > Finding and backporting them and their dependencies will not be easy. May the SmPL proof be with us. > I suggest this patch not be applied. Thanks for the review Alan! Luis -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] backports: backport drvdata = NULL core driver fixes
On Fri, Jul 19, 2013 at 2:07 PM, Luis R. Rodriguez wrote: >> This is not a very good idea. Although setting drvdata to NULL allowed >> a lot of code to be removed, it also exposed a bunch of hidden bugs -- >> drivers were using the drvdata value even after their remove function >> returned. > > Eek, have we not SmPL'ify'd a proof yet to ensure code like this no > longer exists? Julia? :) Come to think of it, perhaps we should require *proof* with SmPL like this in future to avoid regressions ? Luis -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] backports: backport drvdata = NULL core driver fixes
On Thu, 18 Jul 2013, Luis R. Rodriguez wrote: > From: "Luis R. Rodriguez" > > The Linux kernel had tons of code which at times cleared the > drvdata upon probe failure or release. There are however a bunch > of drivers that didn't clear this. > > Commit 0998d063 implmented clearing this upon device_release_driver() > and dealt with probe failure on driver_probe_device(). After this the > kernel was cleaned up separately with *tons* of patches to remove all > these driver specific settings given that the clearing is now done > internally by the device core. > > Instead of ifdef'ing code back in for older code where it was properly > in place backport this by piggy backing the new required code upon the > calls used in place. There is a small race here upon device_release_driver() > but we can live with that theoretical race. > > Due to the way we hack this backport we can't use a separate namespace > as we have with other symbols. > > mcgrof@frijol ~/linux-stable (git::master)$ git describe --contains \ > 0998d0631001288a5974afc0b2a5f568bcdecb4d > v3.6-rc1~99^2~14^2~17 > > commit 0998d0631001288a5974afc0b2a5f568bcdecb4d > Author: Hans de Goede > Date: Wed May 23 00:09:34 2012 +0200 > > device-core: Ensure drvdata = NULL when no driver is bound > > 1) drvdata is for a driver to store a pointer to driver specific data > 2) If no driver is bound, there is no driver specific data associated with >the device > 3) Thus logically drvdata should be NULL if no driver is bound. > > But many drivers don't clear drvdata on device_release, or set drvdata > early on in probe and leave it set on probe error. Both of which results > in a dangling pointer in drvdata. > > This patch enforce for drvdata to be NULL after device_release or on probe > failure. > > Signed-off-by: Hans de Goede > Signed-off-by: Greg Kroah-Hartman This is not a very good idea. Although setting drvdata to NULL allowed a lot of code to be removed, it also exposed a bunch of hidden bugs -- drivers were using the drvdata value even after their remove function returned. Several commits have been applied to fix those bugs. Finding and backporting them and their dependencies will not be easy. I suggest this patch not be applied. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
