diff mbox series

media: don't drop front-end reference count for ->detach

Message ID 20180102095154.3424890-1-arnd@arndb.de
State Superseded
Headers show
Series media: don't drop front-end reference count for ->detach | expand

Commit Message

Arnd Bergmann Jan. 2, 2018, 9:48 a.m. UTC 
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 <max.kellermann@gmail.com>
Cc: Wolfgang Rohdewald <wolfgang@rohdewald.de>
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 <arnd@arndb.de>

---
 drivers/media/dvb-core/dvb_frontend.c | 4 +++-
 drivers/media/usb/dvb-usb/pctv452e.c  | 8 --------
 2 files changed, 3 insertions(+), 9 deletions(-)

-- 
2.9.0

Comments

Mauro Carvalho Chehab Jan. 3, 2018, 10:23 a.m. UTC | #1 
Em Tue,  2 Jan 2018 10:48:54 +0100
Arnd Bergmann <arnd@arndb.de> 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 <max.kellermann@gmail.com>

> Cc: Wolfgang Rohdewald <wolfgang@rohdewald.de>

> 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 <arnd@arndb.de>

> ---

>  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
Arnd Bergmann Jan. 3, 2018, 11:02 p.m. UTC | #2 
On Wed, Jan 3, 2018 at 11:23 AM, Mauro Carvalho Chehab
<mchehab@kernel.org> wrote:
> Em Tue,  2 Jan 2018 10:48:54 +0100

> Arnd Bergmann <arnd@arndb.de> 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
diff mbox series

Patch

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");