A bugfix introduce a link failure in configurations without CONFIG_MODULES:
In file included from drivers/media/usb/dvb-usb/pctv452e.c:20:0:
drivers/media/usb/dvb-usb/pctv452e.c: In function 'pctv452e_frontend_attach':
drivers/media/dvb-frontends/stb0899_drv.h:151:36: error: weak declaration of 'stb0899_attach' being applied to a already existing, static definition
The problem is that the !IS_REACHABLE() declaration of stb0899_attach()
is a 'static inline' definition that clashes with the weak definition.
I further observed that the bugfix was only done for one of the five users
of stb0899_attach(), the other four still have the problem. This reverts
the bugfix and instead addresses the problem by not dropping the reference
count when calling '->detach()', instead we call this function directly
in dvb_frontend_put() before dropping the kref on the front-end.
Cc: Max Kellermann <[email protected]>
Cc: Wolfgang Rohdewald <[email protected]>
Fixes: f686c14364ad ("[media] stb0899: move code to "detach" callback")
Fixes: 6cdeaed3b142 ("media: dvb_usb_pctv452e: module refcount changes were unbalanced")
Signed-off-by: Arnd Bergmann <[email protected]>
---
drivers/media/dvb-core/dvb_frontend.c | 4 +++-
drivers/media/usb/dvb-usb/pctv452e.c | 8 --------
2 files changed, 3 insertions(+), 9 deletions(-)
diff --git a/drivers/media/dvb-core/dvb_frontend.c b/drivers/media/dvb-core/dvb_frontend.c
index 87fc1bcae5ae..fe10b6f4d3e0 100644
--- a/drivers/media/dvb-core/dvb_frontend.c
+++ b/drivers/media/dvb-core/dvb_frontend.c
@@ -164,6 +164,9 @@ static void dvb_frontend_free(struct kref *ref)
static void dvb_frontend_put(struct dvb_frontend *fe)
{
+ /* call detach before dropping the reference count */
+ if (fe->ops.detach)
+ fe->ops.detach(fe);
/*
* Check if the frontend was registered, as otherwise
* kref was not initialized yet.
@@ -2965,7 +2968,6 @@ void dvb_frontend_detach(struct dvb_frontend* fe)
dvb_frontend_invoke_release(fe, fe->ops.release_sec);
dvb_frontend_invoke_release(fe, fe->ops.tuner_ops.release);
dvb_frontend_invoke_release(fe, fe->ops.analog_ops.release);
- dvb_frontend_invoke_release(fe, fe->ops.detach);
dvb_frontend_put(fe);
}
EXPORT_SYMBOL(dvb_frontend_detach);
diff --git a/drivers/media/usb/dvb-usb/pctv452e.c b/drivers/media/usb/dvb-usb/pctv452e.c
index 0af74383083d..ae793dac4964 100644
--- a/drivers/media/usb/dvb-usb/pctv452e.c
+++ b/drivers/media/usb/dvb-usb/pctv452e.c
@@ -913,14 +913,6 @@ static int pctv452e_frontend_attach(struct dvb_usb_adapter *a)
&a->dev->i2c_adap);
if (!a->fe_adap[0].fe)
return -ENODEV;
-
- /*
- * dvb_frontend will call dvb_detach for both stb0899_detach
- * and stb0899_release but we only do dvb_attach(stb0899_attach).
- * Increment the module refcount instead.
- */
- symbol_get(stb0899_attach);
-
if ((dvb_attach(lnbp22_attach, a->fe_adap[0].fe,
&a->dev->i2c_adap)) == NULL)
err("Cannot attach lnbp22\n");
--
2.9.0
Em Tue, 2 Jan 2018 10:48:54 +0100
Arnd Bergmann <[email protected]> escreveu:
> A bugfix introduce a link failure in configurations without CONFIG_MODULES:
>
> In file included from drivers/media/usb/dvb-usb/pctv452e.c:20:0:
> drivers/media/usb/dvb-usb/pctv452e.c: In function 'pctv452e_frontend_attach':
> drivers/media/dvb-frontends/stb0899_drv.h:151:36: error: weak declaration of 'stb0899_attach' being applied to a already existing, static definition
>
> The problem is that the !IS_REACHABLE() declaration of stb0899_attach()
> is a 'static inline' definition that clashes with the weak definition.
>
> I further observed that the bugfix was only done for one of the five users
> of stb0899_attach(), the other four still have the problem. This reverts
> the bugfix and instead addresses the problem by not dropping the reference
> count when calling '->detach()', instead we call this function directly
> in dvb_frontend_put() before dropping the kref on the front-end.
>
> Cc: Max Kellermann <[email protected]>
> Cc: Wolfgang Rohdewald <[email protected]>
> Fixes: f686c14364ad ("[media] stb0899: move code to "detach" callback")
> Fixes: 6cdeaed3b142 ("media: dvb_usb_pctv452e: module refcount changes were unbalanced")
> Signed-off-by: Arnd Bergmann <[email protected]>
> ---
> drivers/media/dvb-core/dvb_frontend.c | 4 +++-
> drivers/media/usb/dvb-usb/pctv452e.c | 8 --------
> 2 files changed, 3 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/media/dvb-core/dvb_frontend.c b/drivers/media/dvb-core/dvb_frontend.c
> index 87fc1bcae5ae..fe10b6f4d3e0 100644
> --- a/drivers/media/dvb-core/dvb_frontend.c
> +++ b/drivers/media/dvb-core/dvb_frontend.c
> @@ -164,6 +164,9 @@ static void dvb_frontend_free(struct kref *ref)
>
> static void dvb_frontend_put(struct dvb_frontend *fe)
> {
> + /* call detach before dropping the reference count */
> + if (fe->ops.detach)
> + fe->ops.detach(fe);
> /*
> * Check if the frontend was registered, as otherwise
> * kref was not initialized yet.
> @@ -2965,7 +2968,6 @@ void dvb_frontend_detach(struct dvb_frontend* fe)
> dvb_frontend_invoke_release(fe, fe->ops.release_sec);
> dvb_frontend_invoke_release(fe, fe->ops.tuner_ops.release);
> dvb_frontend_invoke_release(fe, fe->ops.analog_ops.release);
> - dvb_frontend_invoke_release(fe, fe->ops.detach);
> dvb_frontend_put(fe);
Hmm... stb0899 is not the only driver using detach:
drivers/media/dvb-frontends/stb0899_drv.c: .detach = stb0899_detach,
drivers/media/pci/saa7146/hexium_gemini.c: .detach = hexium_detach,
drivers/media/pci/saa7146/hexium_orion.c: .detach = hexium_detach,
drivers/media/pci/saa7146/mxb.c: .detach = mxb_detach,
drivers/media/pci/ttpci/av7110.c: .detach = av7110_detach,
drivers/media/pci/ttpci/budget-av.c: .detach = budget_av_detach,
drivers/media/pci/ttpci/budget-ci.c: .detach = budget_ci_detach,
drivers/media/pci/ttpci/budget-patch.c: .detach = budget_patch_detach,
drivers/media/pci/ttpci/budget.c: .detach = budget_detach,
Unfortunately, I don't have any device that would be affected by
this change, but it sounds risky to not call this code anymore:
#ifdef CONFIG_MEDIA_ATTACH
dvb_detach(release);
#endif
for .detach ops, as it has the potential of preventing unbind on
those drivers.
> }
> EXPORT_SYMBOL(dvb_frontend_detach);
> diff --git a/drivers/media/usb/dvb-usb/pctv452e.c b/drivers/media/usb/dvb-usb/pctv452e.c
> index 0af74383083d..ae793dac4964 100644
> --- a/drivers/media/usb/dvb-usb/pctv452e.c
> +++ b/drivers/media/usb/dvb-usb/pctv452e.c
> @@ -913,14 +913,6 @@ static int pctv452e_frontend_attach(struct dvb_usb_adapter *a)
> &a->dev->i2c_adap);
> if (!a->fe_adap[0].fe)
> return -ENODEV;
> -
> - /*
> - * dvb_frontend will call dvb_detach for both stb0899_detach
> - * and stb0899_release but we only do dvb_attach(stb0899_attach).
> - * Increment the module refcount instead.
> - */
> - symbol_get(stb0899_attach);
IMHO, the safest fix would be, instead, to do:
#ifdef CONFIG_MEDIA_ATTACH
symbol_get(stb0899_attach);
#endif
Btw, we have some code similar to that on other drivers
with either symbol_get() or symbol_put().
Yeah, I agree that this sucks. The right fix here is to use i2c high level
interfaces, binding it via i2c bus, instead of using the symbol_get()
based dvb_attach() macro.
We're (very) slowing doing such changes along the media subsystem.
Thanks,
Mauro
On Wed, Jan 3, 2018 at 11:23 AM, Mauro Carvalho Chehab
<[email protected]> wrote:
> Em Tue, 2 Jan 2018 10:48:54 +0100
> Arnd Bergmann <[email protected]> escreveu:
>> @@ -2965,7 +2968,6 @@ void dvb_frontend_detach(struct dvb_frontend* fe)
>> dvb_frontend_invoke_release(fe, fe->ops.release_sec);
>> dvb_frontend_invoke_release(fe, fe->ops.tuner_ops.release);
>> dvb_frontend_invoke_release(fe, fe->ops.analog_ops.release);
>> - dvb_frontend_invoke_release(fe, fe->ops.detach);
>> dvb_frontend_put(fe);
>
> Hmm... stb0899 is not the only driver using detach:
>
> drivers/media/dvb-frontends/stb0899_drv.c: .detach = stb0899_detach,
> drivers/media/pci/saa7146/hexium_gemini.c: .detach = hexium_detach,
> drivers/media/pci/saa7146/hexium_orion.c: .detach = hexium_detach,
> drivers/media/pci/saa7146/mxb.c: .detach = mxb_detach,
> drivers/media/pci/ttpci/av7110.c: .detach = av7110_detach,
> drivers/media/pci/ttpci/budget-av.c: .detach = budget_av_detach,
> drivers/media/pci/ttpci/budget-ci.c: .detach = budget_ci_detach,
> drivers/media/pci/ttpci/budget-patch.c: .detach = budget_patch_detach,
> drivers/media/pci/ttpci/budget.c: .detach = budget_detach,
I'm pretty sure I checked this before and found stb0899_detach to be the
only implementation of dvb_frontend_ops:detach.
The other eight you quoted are all setting the member in struct
saa7146_extension, not struct dvb_frontend_ops, so they are
not called using the same method.
> Unfortunately, I don't have any device that would be affected by
> this change, but it sounds risky to not call this code anymore:
>
> #ifdef CONFIG_MEDIA_ATTACH
> dvb_detach(release);
> #endif
>
> for .detach ops, as it has the potential of preventing unbind on
> those drivers.
The problem that Wolfgang reported originally was specifically that
this dvb_detach() call was one more than there should be,
leading to a negative reference count. As far as I can tell, this
is true for every user of the 'detach' callback.
>> diff --git a/drivers/media/usb/dvb-usb/pctv452e.c b/drivers/media/usb/dvb-usb/pctv452e.c
>> index 0af74383083d..ae793dac4964 100644
>> --- a/drivers/media/usb/dvb-usb/pctv452e.c
>> +++ b/drivers/media/usb/dvb-usb/pctv452e.c
>> @@ -913,14 +913,6 @@ static int pctv452e_frontend_attach(struct dvb_usb_adapter *a)
>> &a->dev->i2c_adap);
>> if (!a->fe_adap[0].fe)
>> return -ENODEV;
>> -
>> - /*
>> - * dvb_frontend will call dvb_detach for both stb0899_detach
>> - * and stb0899_release but we only do dvb_attach(stb0899_attach).
>> - * Increment the module refcount instead.
>> - */
>> - symbol_get(stb0899_attach);
>
>
> IMHO, the safest fix would be, instead, to do:
>
> #ifdef CONFIG_MEDIA_ATTACH
> symbol_get(stb0899_attach);
> #endif
>
> Btw, we have some code similar to that on other drivers
> with either symbol_get() or symbol_put().
This would work if we do it for every user of a driver that
has a detach callback in dvb_frontend_ops, but I still think
my suggested patch is correct and less error-prone as a
workaround here.
> Yeah, I agree that this sucks. The right fix here is to use i2c high level
> interfaces, binding it via i2c bus, instead of using the symbol_get()
> based dvb_attach() macro.
>
> We're (very) slowing doing such changes along the media subsystem.
Good to know there is a proper solution already. I had some ideas
for how to change it, but the i2c bus method is clearly better than
whatever I had in mind there.
Arnd