| Message ID | 1515110659-20145-8-git-send-email-brad@nextdimension.cc (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Jan 4, 2018 at 7:04 PM, Brad Love <brad@nextdimension.cc> wrote: > If release is part of frontend ops then it is called in the > course of dvb_frontend_detach. The process also decrements > the module usage count. The problem is if the lgdt3306a > driver is reached via i2c_new_device, then when it is > eventually destroyed remove is called, which further > decrements the module usage count to negative. After this > occurs the driver is in a bad state and no longer works. > Also fixed by NULLing out the release callback is a double > kfree of state, which introduces arbitrary oopses/GPF. > This problem is only currently reachable via the em28xx driver. > > On disconnect of Hauppauge SoloHD before: > > lsmod | grep lgdt3306a > lgdt3306a 28672 -1 > i2c_mux 16384 1 lgdt3306a > > On disconnect of Hauppauge SoloHD after: > > lsmod | grep lgdt3306a > lgdt3306a 28672 0 > i2c_mux 16384 1 lgdt3306a > > Signed-off-by: Brad Love <brad@nextdimension.cc> > --- > drivers/media/dvb-frontends/lgdt3306a.c | 1 + > 1 file changed, 1 insertion(+) > Brad, We won't be able to apply this one. The symptom that you're trying to fix is indicative of some other problem, probably in the em28xx driver. NULL'ing the release callback is not the right thing to do. -Mike Krufky > diff --git a/drivers/media/dvb-frontends/lgdt3306a.c b/drivers/media/dvb-frontends/lgdt3306a.c > index 6356815..d2477ed 100644 > --- a/drivers/media/dvb-frontends/lgdt3306a.c > +++ b/drivers/media/dvb-frontends/lgdt3306a.c > @@ -2177,6 +2177,7 @@ static int lgdt3306a_probe(struct i2c_client *client, > > i2c_set_clientdata(client, fe->demodulator_priv); > state = fe->demodulator_priv; > + state->frontend.ops.release = NULL; > > /* create mux i2c adapter for tuner */ > state->muxc = i2c_mux_alloc(client->adapter, &client->dev, > -- > 2.7.4 >
On 2018-01-04 18:19, Michael Ira Krufky wrote: > On Thu, Jan 4, 2018 at 7:04 PM, Brad Love <brad@nextdimension.cc> wrote: >> If release is part of frontend ops then it is called in the >> course of dvb_frontend_detach. The process also decrements >> the module usage count. The problem is if the lgdt3306a >> driver is reached via i2c_new_device, then when it is >> eventually destroyed remove is called, which further >> decrements the module usage count to negative. After this >> occurs the driver is in a bad state and no longer works. >> Also fixed by NULLing out the release callback is a double >> kfree of state, which introduces arbitrary oopses/GPF. >> This problem is only currently reachable via the em28xx driver. >> >> On disconnect of Hauppauge SoloHD before: >> >> lsmod | grep lgdt3306a >> lgdt3306a 28672 -1 >> i2c_mux 16384 1 lgdt3306a >> >> On disconnect of Hauppauge SoloHD after: >> >> lsmod | grep lgdt3306a >> lgdt3306a 28672 0 >> i2c_mux 16384 1 lgdt3306a >> >> Signed-off-by: Brad Love <brad@nextdimension.cc> >> --- >> drivers/media/dvb-frontends/lgdt3306a.c | 1 + >> 1 file changed, 1 insertion(+) >> > Brad, > > We won't be able to apply this one. The symptom that you're trying to > fix is indicative of some other problem, probably in the em28xx > driver. NULL'ing the release callback is not the right thing to do. > > -Mike Krufky Hey Mike, Redacting this patch to deal with individually. I will separately send to the list an example of another bridge driver which exhibits the same issue, when using the lgdt3306a driver similarly. I don't think this is solely an em28xx issue. I will also send a patch fixing only the double free, as that seems to be most important. Cheers, Brad >> diff --git a/drivers/media/dvb-frontends/lgdt3306a.c b/drivers/media/dvb-frontends/lgdt3306a.c >> index 6356815..d2477ed 100644 >> --- a/drivers/media/dvb-frontends/lgdt3306a.c >> +++ b/drivers/media/dvb-frontends/lgdt3306a.c >> @@ -2177,6 +2177,7 @@ static int lgdt3306a_probe(struct i2c_client *client, >> >> i2c_set_clientdata(client, fe->demodulator_priv); >> state = fe->demodulator_priv; >> + state->frontend.ops.release = NULL; >> >> /* create mux i2c adapter for tuner */ >> state->muxc = i2c_mux_alloc(client->adapter, &client->dev, >> -- >> 2.7.4 >>
Am 05.01.2018 um 01:19 schrieb Michael Ira Krufky: > On Thu, Jan 4, 2018 at 7:04 PM, Brad Love <brad@nextdimension.cc> wrote: >> If release is part of frontend ops then it is called in the >> course of dvb_frontend_detach. The process also decrements >> the module usage count. The problem is if the lgdt3306a >> driver is reached via i2c_new_device, then when it is >> eventually destroyed remove is called, which further >> decrements the module usage count to negative. After this >> occurs the driver is in a bad state and no longer works. >> Also fixed by NULLing out the release callback is a double >> kfree of state, which introduces arbitrary oopses/GPF. >> This problem is only currently reachable via the em28xx driver. >> >> On disconnect of Hauppauge SoloHD before: >> >> lsmod | grep lgdt3306a >> lgdt3306a 28672 -1 >> i2c_mux 16384 1 lgdt3306a >> >> On disconnect of Hauppauge SoloHD after: >> >> lsmod | grep lgdt3306a >> lgdt3306a 28672 0 >> i2c_mux 16384 1 lgdt3306a >> >> Signed-off-by: Brad Love <brad@nextdimension.cc> >> --- >> drivers/media/dvb-frontends/lgdt3306a.c | 1 + >> 1 file changed, 1 insertion(+) >> > > Brad, > > We won't be able to apply this one. The symptom that you're trying to > fix is indicative of some other problem, probably in the em28xx > driver. NULL'ing the release callback is not the right thing to do. > Hi Mike, Why do you nak this perfectly fine patch? Let me start to explain. dvb_attach style drivers have an attach function and for unloading a .release callback. i2c-style drivers have a probe and a remove function. Mixed style drivers must be constructed, that either release or remove is called, never both. They both do the same thing, but with different signature. Now checking lgdt3306a driver: dvb attach style: In lgdt3306a_attach the release callback is set to lgdt3306a_release and no remove exists. Fine. i2c style probe: struct i2c_driver contains lgdt3306a_probe and lgdt3306a_remove. lgdt3306a_probe shares code and calls lgdt3306a_attach, but afterwards the release callback field must be set to NULL. This is/was done exactly like this in multiple other drivers as long as they have been multiple style attachable. Regards Matthias > >> diff --git a/drivers/media/dvb-frontends/lgdt3306a.c b/drivers/media/dvb-frontends/lgdt3306a.c >> index 6356815..d2477ed 100644 >> --- a/drivers/media/dvb-frontends/lgdt3306a.c >> +++ b/drivers/media/dvb-frontends/lgdt3306a.c >> @@ -2177,6 +2177,7 @@ static int lgdt3306a_probe(struct i2c_client *client, >> >> i2c_set_clientdata(client, fe->demodulator_priv); >> state = fe->demodulator_priv; >> + state->frontend.ops.release = NULL; >> >> /* create mux i2c adapter for tuner */ >> state->muxc = i2c_mux_alloc(client->adapter, &client->dev, >> -- >> 2.7.4 >> >
On Tue, Jan 9, 2018 at 12:17 AM, Matthias Schwarzott <zzam@gentoo.org> wrote: > Am 05.01.2018 um 01:19 schrieb Michael Ira Krufky: >> On Thu, Jan 4, 2018 at 7:04 PM, Brad Love <brad@nextdimension.cc> wrote: >>> If release is part of frontend ops then it is called in the >>> course of dvb_frontend_detach. The process also decrements >>> the module usage count. The problem is if the lgdt3306a >>> driver is reached via i2c_new_device, then when it is >>> eventually destroyed remove is called, which further >>> decrements the module usage count to negative. After this >>> occurs the driver is in a bad state and no longer works. >>> Also fixed by NULLing out the release callback is a double >>> kfree of state, which introduces arbitrary oopses/GPF. >>> This problem is only currently reachable via the em28xx driver. >>> >>> On disconnect of Hauppauge SoloHD before: >>> >>> lsmod | grep lgdt3306a >>> lgdt3306a 28672 -1 >>> i2c_mux 16384 1 lgdt3306a >>> >>> On disconnect of Hauppauge SoloHD after: >>> >>> lsmod | grep lgdt3306a >>> lgdt3306a 28672 0 >>> i2c_mux 16384 1 lgdt3306a >>> >>> Signed-off-by: Brad Love <brad@nextdimension.cc> >>> --- >>> drivers/media/dvb-frontends/lgdt3306a.c | 1 + >>> 1 file changed, 1 insertion(+) >>> >> >> Brad, >> >> We won't be able to apply this one. The symptom that you're trying to >> fix is indicative of some other problem, probably in the em28xx >> driver. NULL'ing the release callback is not the right thing to do. >> > > Hi Mike, > Why do you nak this perfectly fine patch? > Let me start to explain. > > dvb_attach style drivers have an attach function and for unloading a > .release callback. > > i2c-style drivers have a probe and a remove function. > > Mixed style drivers must be constructed, that either release or remove > is called, never both. > They both do the same thing, but with different signature. > > > Now checking lgdt3306a driver: > > dvb attach style: > In lgdt3306a_attach the release callback is set to lgdt3306a_release and > no remove exists. Fine. > > i2c style probe: > struct i2c_driver contains lgdt3306a_probe and lgdt3306a_remove. > lgdt3306a_probe shares code and calls lgdt3306a_attach, but afterwards > the release callback field must be set to NULL. > > This is/was done exactly like this in multiple other drivers as long as > they have been multiple style attachable. > > Regards > Matthias > >> >>> diff --git a/drivers/media/dvb-frontends/lgdt3306a.c b/drivers/media/dvb-frontends/lgdt3306a.c >>> index 6356815..d2477ed 100644 >>> --- a/drivers/media/dvb-frontends/lgdt3306a.c >>> +++ b/drivers/media/dvb-frontends/lgdt3306a.c >>> @@ -2177,6 +2177,7 @@ static int lgdt3306a_probe(struct i2c_client *client, >>> >>> i2c_set_clientdata(client, fe->demodulator_priv); >>> state = fe->demodulator_priv; >>> + state->frontend.ops.release = NULL; >>> >>> /* create mux i2c adapter for tuner */ >>> state->muxc = i2c_mux_alloc(client->adapter, &client->dev, >>> -- >>> 2.7.4 >>> >> > Brad & Matthias, It makes sense. This is just a result of a transitional period where there are multiple APIs for attaching the driver. Hopefully we can consolidate soon. I will look at the other drivers when I have time later on, but you're probably right, and we will likely end up merging this one after all. I'll try to get a PR out to Mauro by the weekend. -Mike
diff --git a/drivers/media/dvb-frontends/lgdt3306a.c b/drivers/media/dvb-frontends/lgdt3306a.c index 6356815..d2477ed 100644 --- a/drivers/media/dvb-frontends/lgdt3306a.c +++ b/drivers/media/dvb-frontends/lgdt3306a.c @@ -2177,6 +2177,7 @@ static int lgdt3306a_probe(struct i2c_client *client, i2c_set_clientdata(client, fe->demodulator_priv); state = fe->demodulator_priv; + state->frontend.ops.release = NULL; /* create mux i2c adapter for tuner */ state->muxc = i2c_mux_alloc(client->adapter, &client->dev,
If release is part of frontend ops then it is called in the course of dvb_frontend_detach. The process also decrements the module usage count. The problem is if the lgdt3306a driver is reached via i2c_new_device, then when it is eventually destroyed remove is called, which further decrements the module usage count to negative. After this occurs the driver is in a bad state and no longer works. Also fixed by NULLing out the release callback is a double kfree of state, which introduces arbitrary oopses/GPF. This problem is only currently reachable via the em28xx driver. On disconnect of Hauppauge SoloHD before: lsmod | grep lgdt3306a lgdt3306a 28672 -1 i2c_mux 16384 1 lgdt3306a On disconnect of Hauppauge SoloHD after: lsmod | grep lgdt3306a lgdt3306a 28672 0 i2c_mux 16384 1 lgdt3306a Signed-off-by: Brad Love <brad@nextdimension.cc> --- drivers/media/dvb-frontends/lgdt3306a.c | 1 + 1 file changed, 1 insertion(+)