diff mbox

[bug-report] unconditionally calling cxd2820r_get_tuner_i2c_adapter() from em28xx-dvb.c creates a hard module dependency

Message ID 87mxi1n7ql.fsf@nemi.mork.no (mailing list archive)
State RFC
Headers show

Commit Message

Bjørn Mork June 1, 2011, 10:53 a.m. UTC 
Another possible solution...  This is my last one, I promise :-)

I looked at dvb_attach() and realized that you can ab^H^Hreuse it.  This
makes the patch tiny, and allow you to continue hiding 
struct cxd2820r_priv.


Bjørn

Comments

Bjørn Mork June 1, 2011, 5:18 p.m. UTC | #1 
Bjørn Mork <bjorn@mork.no> writes:

> diff --git a/drivers/media/video/em28xx/em28xx-dvb.c b/drivers/media/video/em28xx/em28xx-dvb.c
> index 7904ca4..d994592 100644
> --- a/drivers/media/video/em28xx/em28xx-dvb.c
> +++ b/drivers/media/video/em28xx/em28xx-dvb.c
> @@ -669,7 +669,8 @@ static int dvb_init(struct em28xx *dev)
>  			&em28xx_cxd2820r_config, &dev->i2c_adap, NULL);
>  		if (dvb->fe[0]) {
>  			struct i2c_adapter *i2c_tuner;
> -			i2c_tuner = cxd2820r_get_tuner_i2c_adapter(dvb->fe[0]);
> +			/* we don't really attach i2c_tuner.  Just reusing the symbol logic */
> +			i2c_tuner = dvb_attach(cxd2820r_get_tuner_i2c_adapter, dvb->fe[0]);

Except that this really messes up the reference count, and need to have
a matching symbol_put...  So you should probably code it with
symbol_request()/symbol_put() if you want to go this way instead of
the dvb_attach shortcut .


Bjørn 
--
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
Antti Palosaari June 3, 2011, 12:21 p.m. UTC | #2 
On 06/01/2011 08:18 PM, Bjørn Mork wrote:
> Bjørn Mork<bjorn@mork.no>  writes:
>
>> diff --git a/drivers/media/video/em28xx/em28xx-dvb.c b/drivers/media/video/em28xx/em28xx-dvb.c
>> index 7904ca4..d994592 100644
>> --- a/drivers/media/video/em28xx/em28xx-dvb.c
>> +++ b/drivers/media/video/em28xx/em28xx-dvb.c
>> @@ -669,7 +669,8 @@ static int dvb_init(struct em28xx *dev)
>>   			&em28xx_cxd2820r_config,&dev->i2c_adap, NULL);
>>   		if (dvb->fe[0]) {
>>   			struct i2c_adapter *i2c_tuner;
>> -			i2c_tuner = cxd2820r_get_tuner_i2c_adapter(dvb->fe[0]);
>> +			/* we don't really attach i2c_tuner.  Just reusing the symbol logic */
>> +			i2c_tuner = dvb_attach(cxd2820r_get_tuner_i2c_adapter, dvb->fe[0]);
>
> Except that this really messes up the reference count, and need to have
> a matching symbol_put...  So you should probably code it with
> symbol_request()/symbol_put() if you want to go this way instead of
> the dvb_attach shortcut .


There is some other FEs having also I2C adapter, I wonder how those 
handle this situation. I looked example from cx24123 and s5h1420 
drivers, both used by flexcop.

Did you see what is magic used those devices?


best regards,
Antti
Bjørn Mork June 3, 2011, 12:50 p.m. UTC | #3 
Antti Palosaari <crope@iki.fi> writes:

> There is some other FEs having also I2C adapter, I wonder how those
> handle this situation. I looked example from cx24123 and s5h1420
> drivers, both used by flexcop.
>
> Did you see what is magic used those devices?

None.  They have the same problem, creating hard module dependencies
even if they use dvb_attach() and CONFIG_MEDIA_ATTACH is set:

bjorn@canardo:~$ modinfo b2c2-flexcop
filename:       /lib/modules/2.6.32-5-amd64/kernel/drivers/media/dvb/b2c2/b2c2-flexcop.ko
license:        GPL
description:    B2C2 FlexcopII/II(b)/III digital TV receiver chip
author:         Patrick Boettcher <patrick.boettcher@desy.de
depends:        s5h1420,dvb-core,cx24113,cx24123,i2c-core
vermagic:       2.6.32-5-amd64 SMP mod_unload modversions 
parm:           debug:set debug level (1=info,2=tuner,4=i2c,8=ts,16=sram,32=reg (|-able)). (debugging is not enabled) (int)
parm:           adapter_nr:DVB adapter numbers (array of short)



This probably means that a generic i2c_tuner wrapper, similar to
dvb_attach, would be useful.



Bjørn
--
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
Antti Palosaari June 3, 2011, 12:59 p.m. UTC | #4 
On 06/03/2011 03:50 PM, Bjørn Mork wrote:
> Antti Palosaari<crope@iki.fi>  writes:
>
>> There is some other FEs having also I2C adapter, I wonder how those
>> handle this situation. I looked example from cx24123 and s5h1420
>> drivers, both used by flexcop.
>>
>> Did you see what is magic used those devices?
>
> None.  They have the same problem, creating hard module dependencies
> even if they use dvb_attach() and CONFIG_MEDIA_ATTACH is set:
>
> bjorn@canardo:~$ modinfo b2c2-flexcop
> filename:       /lib/modules/2.6.32-5-amd64/kernel/drivers/media/dvb/b2c2/b2c2-flexcop.ko
> license:        GPL
> description:    B2C2 FlexcopII/II(b)/III digital TV receiver chip
> author:         Patrick Boettcher<patrick.boettcher@desy.de
> depends:        s5h1420,dvb-core,cx24113,cx24123,i2c-core
> vermagic:       2.6.32-5-amd64 SMP mod_unload modversions
> parm:           debug:set debug level (1=info,2=tuner,4=i2c,8=ts,16=sram,32=reg (|-able)). (debugging is not enabled) (int)
> parm:           adapter_nr:DVB adapter numbers (array of short)
>
>
>
> This probably means that a generic i2c_tuner wrapper, similar to
> dvb_attach, would be useful.

For the cxd2820r it is also possible to return I2C adapter as pointer 
from dvb_attach like pointer to FE0 is carried for FE1 dvb_attach. What 
you think about that?

regards,
Antti
Bjørn Mork June 3, 2011, 1:20 p.m. UTC | #5 
Antti Palosaari <crope@iki.fi> writes:
> On 06/03/2011 03:50 PM, Bjørn Mork wrote:
>
>> This probably means that a generic i2c_tuner wrapper, similar to
>> dvb_attach, would be useful.
>
> For the cxd2820r it is also possible to return I2C adapter as pointer
> from dvb_attach like pointer to FE0 is carried for FE1
> dvb_attach. What you think about that?

I don't feel competent to answer that at all.  It does seem like
overloading an existing interface, but it might be OK.

I just grepped a bit around for EXPORT_SYMBOL of anything except
foo_attach, and found that there are a few frontend drivers which
exports multiple symbols:

bjorn@canardo:/usr/local/src/git/linux-2.6$ grep EXPORT_SYMBOL drivers/media/dvb/frontends/*.c|grep -v _attach
drivers/media/dvb/frontends/cx24113.c:EXPORT_SYMBOL(cx24113_agc_callback);
drivers/media/dvb/frontends/cx24123.c:EXPORT_SYMBOL(cx24123_get_tuner_i2c_adapter);
drivers/media/dvb/frontends/cxd2820r_core.c:EXPORT_SYMBOL(cxd2820r_get_tuner_i2c_adapter);
drivers/media/dvb/frontends/dib0070.c:EXPORT_SYMBOL(dib0070_ctrl_agc_filter);
drivers/media/dvb/frontends/dib0070.c:EXPORT_SYMBOL(dib0070_get_rf_output);
drivers/media/dvb/frontends/dib0070.c:EXPORT_SYMBOL(dib0070_set_rf_output);
drivers/media/dvb/frontends/dib0070.c:EXPORT_SYMBOL(dib0070_wbd_offset);
drivers/media/dvb/frontends/dib0090.c:EXPORT_SYMBOL(dib0090_dcc_freq);
drivers/media/dvb/frontends/dib0090.c:EXPORT_SYMBOL(dib0090_pwm_gain_reset);
drivers/media/dvb/frontends/dib0090.c:EXPORT_SYMBOL(dib0090_gain_control);
drivers/media/dvb/frontends/dib0090.c:EXPORT_SYMBOL(dib0090_get_current_gain);
drivers/media/dvb/frontends/dib0090.c:EXPORT_SYMBOL(dib0090_get_wbd_offset);
drivers/media/dvb/frontends/dib0090.c:EXPORT_SYMBOL(dib0090_get_tune_state);
drivers/media/dvb/frontends/dib0090.c:EXPORT_SYMBOL(dib0090_set_tune_state);
drivers/media/dvb/frontends/dib0090.c:EXPORT_SYMBOL(dib0090_register);
drivers/media/dvb/frontends/dib0090.c:EXPORT_SYMBOL(dib0090_fw_register);
drivers/media/dvb/frontends/dib3000mc.c:EXPORT_SYMBOL(dib3000mc_get_tuner_i2c_master);
drivers/media/dvb/frontends/dib3000mc.c:EXPORT_SYMBOL(dib3000mc_pid_control);
drivers/media/dvb/frontends/dib3000mc.c:EXPORT_SYMBOL(dib3000mc_pid_parse);
drivers/media/dvb/frontends/dib3000mc.c:EXPORT_SYMBOL(dib3000mc_set_config);
drivers/media/dvb/frontends/dib3000mc.c:EXPORT_SYMBOL(dib3000mc_i2c_enumeration);
drivers/media/dvb/frontends/dib7000m.c:EXPORT_SYMBOL(dib7000m_get_i2c_master);
drivers/media/dvb/frontends/dib7000m.c:EXPORT_SYMBOL(dib7000m_pid_filter_ctrl);
drivers/media/dvb/frontends/dib7000m.c:EXPORT_SYMBOL(dib7000m_pid_filter);
drivers/media/dvb/frontends/dib7000m.c:EXPORT_SYMBOL(dib7000m_i2c_enumeration);
drivers/media/dvb/frontends/dib7000p.c:EXPORT_SYMBOL(dib7000p_set_wbd_ref);
drivers/media/dvb/frontends/dib7000p.c:EXPORT_SYMBOL(dib7000p_update_pll);
drivers/media/dvb/frontends/dib7000p.c:EXPORT_SYMBOL(dib7000p_set_gpio);
drivers/media/dvb/frontends/dib7000p.c:EXPORT_SYMBOL(dib7000p_ctrl_timf);
drivers/media/dvb/frontends/dib7000p.c:EXPORT_SYMBOL(dib7000pc_detection);
drivers/media/dvb/frontends/dib7000p.c:EXPORT_SYMBOL(dib7000p_get_i2c_master);
drivers/media/dvb/frontends/dib7000p.c:EXPORT_SYMBOL(dib7000p_pid_filter_ctrl);
drivers/media/dvb/frontends/dib7000p.c:EXPORT_SYMBOL(dib7000p_pid_filter);
drivers/media/dvb/frontends/dib7000p.c:EXPORT_SYMBOL(dib7000p_i2c_enumeration);
drivers/media/dvb/frontends/dib7000p.c:EXPORT_SYMBOL(dib7090_get_i2c_tuner);
drivers/media/dvb/frontends/dib7000p.c:EXPORT_SYMBOL(dib7090_tuner_sleep);
drivers/media/dvb/frontends/dib7000p.c:EXPORT_SYMBOL(dib7090_agc_restart);
drivers/media/dvb/frontends/dib7000p.c:EXPORT_SYMBOL(dib7090_get_adc_power);
drivers/media/dvb/frontends/dib7000p.c:EXPORT_SYMBOL(dib7090_slave_reset);
drivers/media/dvb/frontends/dib8000.c:EXPORT_SYMBOL(dib8000_set_wbd_ref);
drivers/media/dvb/frontends/dib8000.c:EXPORT_SYMBOL(dib8000_set_gpio);
drivers/media/dvb/frontends/dib8000.c:EXPORT_SYMBOL(dib8000_pwm_agc_reset);
drivers/media/dvb/frontends/dib8000.c:EXPORT_SYMBOL(dib8000_get_adc_power);
drivers/media/dvb/frontends/dib8000.c:EXPORT_SYMBOL(dib8000_get_tune_state);
drivers/media/dvb/frontends/dib8000.c:EXPORT_SYMBOL(dib8000_set_tune_state);
drivers/media/dvb/frontends/dib8000.c:EXPORT_SYMBOL(dib8000_set_slave_frontend);
drivers/media/dvb/frontends/dib8000.c:EXPORT_SYMBOL(dib8000_remove_slave_frontend);
drivers/media/dvb/frontends/dib8000.c:EXPORT_SYMBOL(dib8000_get_slave_frontend);
drivers/media/dvb/frontends/dib8000.c:EXPORT_SYMBOL(dib8000_i2c_enumeration);
drivers/media/dvb/frontends/dib8000.c:EXPORT_SYMBOL(dib8000_get_i2c_master);
drivers/media/dvb/frontends/dib8000.c:EXPORT_SYMBOL(dib8000_pid_filter_ctrl);
drivers/media/dvb/frontends/dib8000.c:EXPORT_SYMBOL(dib8000_pid_filter);
drivers/media/dvb/frontends/dib9000.c:EXPORT_SYMBOL(dib9000_fw_set_component_bus_speed);
drivers/media/dvb/frontends/dib9000.c:EXPORT_SYMBOL(dib9000_get_tuner_interface);
drivers/media/dvb/frontends/dib9000.c:EXPORT_SYMBOL(dib9000_get_component_bus_interface);
drivers/media/dvb/frontends/dib9000.c:EXPORT_SYMBOL(dib9000_get_i2c_master);
drivers/media/dvb/frontends/dib9000.c:EXPORT_SYMBOL(dib9000_set_i2c_adapter);
drivers/media/dvb/frontends/dib9000.c:EXPORT_SYMBOL(dib9000_set_gpio);
drivers/media/dvb/frontends/dib9000.c:EXPORT_SYMBOL(dib9000_fw_pid_filter_ctrl);
drivers/media/dvb/frontends/dib9000.c:EXPORT_SYMBOL(dib9000_fw_pid_filter);
drivers/media/dvb/frontends/dib9000.c:EXPORT_SYMBOL(dib9000_firmware_post_pll_init);
drivers/media/dvb/frontends/dib9000.c:EXPORT_SYMBOL(dib9000_i2c_enumeration);
drivers/media/dvb/frontends/dib9000.c:EXPORT_SYMBOL(dib9000_set_slave_frontend);
drivers/media/dvb/frontends/dib9000.c:EXPORT_SYMBOL(dib9000_remove_slave_frontend);
drivers/media/dvb/frontends/dib9000.c:EXPORT_SYMBOL(dib9000_get_slave_frontend);
drivers/media/dvb/frontends/dibx000_common.c:EXPORT_SYMBOL(dibx000_i2c_set_speed);
drivers/media/dvb/frontends/dibx000_common.c:EXPORT_SYMBOL(dibx000_get_i2c_adapter);
drivers/media/dvb/frontends/dibx000_common.c:EXPORT_SYMBOL(dibx000_reset_i2c_master);
drivers/media/dvb/frontends/dibx000_common.c:EXPORT_SYMBOL(dibx000_init_i2c_master);
drivers/media/dvb/frontends/dibx000_common.c:EXPORT_SYMBOL(dibx000_exit_i2c_master);
drivers/media/dvb/frontends/dibx000_common.c:EXPORT_SYMBOL(systime);
drivers/media/dvb/frontends/drxd_hard.c:EXPORT_SYMBOL(drxd_config_i2c);
drivers/media/dvb/frontends/s5h1420.c:EXPORT_SYMBOL(s5h1420_get_tuner_i2c_adapter);
drivers/media/dvb/frontends/stv090x.c:EXPORT_SYMBOL(stv090x_set_gpio);


Which does show up as hard dependencies on your typical Debian
installation (which does set CONFIG_MEDIA_ATTACH):

 bjorn@nemi:~$ modinfo -F depends dvb-usb-dib0700
 dib8000,dvb-usb,i2c-core,dib0070,dib3000mc,usbcore,dib7000p,dib7000m


So it looks like this problem is much more widespread than I initially
thought, and it may therefore be perfectly OK to do what you did.
However, the em28xx-dvb driver can be used with a large number of
frontends and we certainly don't want them all as hard dependencies.


I think it's time for someone with authority to state what is acceptable
and what is not, with regard to frontends, tuners, dvb_attach(),
EXPORT_SYMBOL and CONFIG_MEDIA_ATTACH. 

For all I know, this may already be documented somewhere.  If so, please
point me there.



Bjørn
--
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 mbox

Patch

From b570bbad12c1d164ed92c6711a1775db29c4c0a7 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Bj=C3=B8rn=20Mork?= <bjorn@mork.no>
Date: Wed, 1 Jun 2011 12:48:25 +0200
Subject: [PATCH] em28xx-dvb: Use dvb_attach to call cxd2820r_get_tuner_i2c_adapter()
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

This avoids a hard module dependency on cxd2820r.  Even though we
don't really attach anything in this call, we can stil reuse
dvb_attach since the called function is expected to return a
pointer.

Signed-off-by: Bjørn Mork <bjorn@mork.no>
---
 drivers/media/video/em28xx/em28xx-dvb.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/drivers/media/video/em28xx/em28xx-dvb.c b/drivers/media/video/em28xx/em28xx-dvb.c
index 7904ca4..d994592 100644
--- a/drivers/media/video/em28xx/em28xx-dvb.c
+++ b/drivers/media/video/em28xx/em28xx-dvb.c
@@ -669,7 +669,8 @@  static int dvb_init(struct em28xx *dev)
 			&em28xx_cxd2820r_config, &dev->i2c_adap, NULL);
 		if (dvb->fe[0]) {
 			struct i2c_adapter *i2c_tuner;
-			i2c_tuner = cxd2820r_get_tuner_i2c_adapter(dvb->fe[0]);
+			/* we don't really attach i2c_tuner.  Just reusing the symbol logic */
+			i2c_tuner = dvb_attach(cxd2820r_get_tuner_i2c_adapter, dvb->fe[0]);
 			/* FE 0 attach tuner */
 			if (!dvb_attach(tda18271_attach, dvb->fe[0], 0x60,
 				i2c_tuner, &em28xx_cxd2820r_tda18271_config)) {
-- 
1.7.2.5