| Message ID | 1424798958-2819-1-git-send-email-dheitmueller@kernellabs.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 02/24/2015 10:29 AM, Devin Heitmueller wrote: > This patch addresses a regression introduced in the following patch: > > commit 5264a522a597032c009f9143686ebf0fa4e244fb > Author: Shuah Khan <shuahkh@osg.samsung.com> > Date: Mon Sep 22 21:30:46 2014 -0300 > [media] media: tuner xc5000 - release firmwware from xc5000_release() > > The "priv" struct is actually reference counted, so the xc5000_release() > function gets called multiple times for hybrid devices. Because > release_firmware() was always being called, it would work fine as expected > on the first call but then the second call would corrupt aribtrary memory. > > Set the pointer to NULL after releasing so that we don't call > release_firmware() twice. > > This problem was detected in the HVR-950q where plugging/unplugging the > device multiple times would intermittently show panics in completely > unrelated areas of the kernel. Thanks for finding and fixing the problem. > > Signed-off-by: Devin Heitmueller <dheitmueller@kernellabs.com> > Cc: Shuah Khan <shuahkh@osg.samsung.com> > --- > drivers/media/tuners/xc5000.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/drivers/media/tuners/xc5000.c b/drivers/media/tuners/xc5000.c > index 40f9db6..74b2092 100644 > --- a/drivers/media/tuners/xc5000.c > +++ b/drivers/media/tuners/xc5000.c > @@ -1314,7 +1314,10 @@ static int xc5000_release(struct dvb_frontend *fe) > > if (priv) { > cancel_delayed_work(&priv->timer_sleep); > - release_firmware(priv->firmware); I would request you to add a comment here indicating the hybrid case scenario to avoid any future cleanup type work deciding there is no need to set priv->firmware to null since priv gets released in hybrid_tuner_release_state(priv); > + if (priv->firmware) { > + release_firmware(priv->firmware); > + priv->firmware = NULL; > + } > hybrid_tuner_release_state(priv); > } > > Adding Mauro as will to the thread. This should go into stable as well. thanks, -- Shuah
> I would request you to add a comment here indicating the > hybrid case scenario to avoid any future cleanup type work > deciding there is no need to set priv->firmware to null > since priv gets released in hybrid_tuner_release_state(priv); No, I'm not going to rebase my tree and regenerate the patch just to add a comment explaining how hybrid_tuner_[request/release]_state() works (which, btw, is how it works in all hybrid tuner drivers). I already wasted enough of my time tracking down the source of the memory corruption and providing a fix for this regression. If you want to submit a subsequent patch with a comment, be my guest. Devin
On 02/25/2015 07:56 PM, Devin Heitmueller wrote: >> I would request you to add a comment here indicating the >> hybrid case scenario to avoid any future cleanup type work >> deciding there is no need to set priv->firmware to null >> since priv gets released in hybrid_tuner_release_state(priv); > > No, I'm not going to rebase my tree and regenerate the patch just to > add a comment explaining how hybrid_tuner_[request/release]_state() > works (which, btw, is how it works in all hybrid tuner drivers). I > already wasted enough of my time tracking down the source of the > memory corruption and providing a fix for this regression. If you > want to submit a subsequent patch with a comment, be my guest. These are just the issues I would like to implement drivers as standard I2C driver model =) Attaching driver for one chip twice is ugly hack! regards Antti
> These are just the issues I would like to implement drivers as standard I2C > driver model =) Attaching driver for one chip twice is ugly hack! While I'm not arguing the merits of using the standard I2C driver model, it won't actually help in this case since we would still need a structure representing shared state accessible by both the DVB and V4L2 subsystems. And that struct will need to be referenced counted, which is exactly what hybrid_tuner_request_state() does. In short, what you're proposing doesn't have any relevance to this case - it just moves the problem to some other mechanism for sharing private state between two drivers and having to reference count it. And at least in this case it's done the same way for all the tuner drivers (as opposed to different tuners re-inventing their own mechanism for sharing state between the different consumers). If you ever get around to implementing support for a hybrid device (where you actually have to worry about how both digital and analog share a single tuner), you'll appreciate some of these challenges and why what was done was not as bad/crazy as you might think. If anything, this little regression is yet another point of evidence that innocent refactoring and "cleanup" of existing code without really understanding what it does and/or performing significant testing can leave everybody worse off than if the well-intentioned committer had done nothing at all. Devin
Em Wed, 25 Feb 2015 13:37:07 -0500 Devin Heitmueller <dheitmueller@kernellabs.com> escreveu: > > These are just the issues I would like to implement drivers as standard I2C > > driver model =) Attaching driver for one chip twice is ugly hack! > > While I'm not arguing the merits of using the standard I2C driver > model, it won't actually help in this case since we would still need a > structure representing shared state accessible by both the DVB and > V4L2 subsystems. And that struct will need to be referenced counted, > which is exactly what hybrid_tuner_request_state() does. ... > If you ever get around to implementing support for a hybrid device > (where you actually have to worry about how both digital and analog > share a single tuner), you'll appreciate some of these challenges and > why what was done was not as bad/crazy as you might think. The I2C model that Antti is proposing may work, but, for that, we'll very likely need to re-write the tuner core. Currently, the binding model is: for analog: tuner driver -> tuner-core module <==> V4L2 driver The interface between V4L2 driver and tuner core is the I2C high level API. for digital tuner driver <==> DVB driver The interface between the tuner driver and the DVB driver is the I2C low level API. Antti's proposal makes the DVB driver to use the high level I2C API, with makes the DVB driver a little closer to what V4L2 does. Yet, for V4L2, the tuner-core module is required. The binding code at the tuner-core is very complex, as it needs to talk both V4L2 and DVB "dialogs". This should be simplified. In other words, If we want to use the same model for all tuners, we'll need to rewrite the binding schema to avoid the need of a tuner core module, or to replace it by something better/simpler. For locking the tuner between analog/digital usage, the best approach seems to be to use the media controller. Regards, Mauro -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/media/tuners/xc5000.c b/drivers/media/tuners/xc5000.c index 40f9db6..74b2092 100644 --- a/drivers/media/tuners/xc5000.c +++ b/drivers/media/tuners/xc5000.c @@ -1314,7 +1314,10 @@ static int xc5000_release(struct dvb_frontend *fe) if (priv) { cancel_delayed_work(&priv->timer_sleep); - release_firmware(priv->firmware); + if (priv->firmware) { + release_firmware(priv->firmware); + priv->firmware = NULL; + } hybrid_tuner_release_state(priv); }
This patch addresses a regression introduced in the following patch: commit 5264a522a597032c009f9143686ebf0fa4e244fb Author: Shuah Khan <shuahkh@osg.samsung.com> Date: Mon Sep 22 21:30:46 2014 -0300 [media] media: tuner xc5000 - release firmwware from xc5000_release() The "priv" struct is actually reference counted, so the xc5000_release() function gets called multiple times for hybrid devices. Because release_firmware() was always being called, it would work fine as expected on the first call but then the second call would corrupt aribtrary memory. Set the pointer to NULL after releasing so that we don't call release_firmware() twice. This problem was detected in the HVR-950q where plugging/unplugging the device multiple times would intermittently show panics in completely unrelated areas of the kernel. Signed-off-by: Devin Heitmueller <dheitmueller@kernellabs.com> Cc: Shuah Khan <shuahkh@osg.samsung.com> --- drivers/media/tuners/xc5000.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)