linux-media.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* Re: linux-next: Tree for Oct 8 (media/usb/gspca)
       [not found] <20141008174923.76786a03@canb.auug.org.au>
@ 2014-10-08 17:13 ` Randy Dunlap
  2014-10-08 18:31   ` Mauro Carvalho Chehab
  0 siblings, 1 reply; 8+ messages in thread
From: Randy Dunlap @ 2014-10-08 17:13 UTC (permalink / raw)
  To: Stephen Rothwell, linux-next; +Cc: linux-kernel, Hans de Goede, linux-media

On 10/07/14 23:49, Stephen Rothwell wrote:
> Hi all,
> 
> Please do not add any material intended for v3.19 to you linux-next
> included trees until after v3.18-rc1 has been released.
> 
> Changes since 20141007:
> 

I saw these build errors in gspca when CONFIG_INPUT=m but the gspca
sub-drivers are builtin:

drivers/built-in.o: In function `gspca_dev_probe2':
(.text+0x10ef43): undefined reference to `input_allocate_device'
drivers/built-in.o: In function `gspca_dev_probe2':
(.text+0x10efdd): undefined reference to `input_register_device'
drivers/built-in.o: In function `gspca_dev_probe2':
(.text+0x10f002): undefined reference to `input_free_device'
drivers/built-in.o: In function `gspca_dev_probe2':
(.text+0x10f0ac): undefined reference to `input_unregister_device'
drivers/built-in.o: In function `gspca_disconnect':
(.text+0x10f186): undefined reference to `input_unregister_device'
drivers/built-in.o: In function `sd_int_pkt_scan':
se401.c:(.text+0x11373d): undefined reference to `input_event'
se401.c:(.text+0x11374e): undefined reference to `input_event'
drivers/built-in.o: In function `sd_pkt_scan':
t613.c:(.text+0x119f0e): undefined reference to `input_event'
t613.c:(.text+0x119f1f): undefined reference to `input_event'
drivers/built-in.o: In function `sd_stopN':
t613.c:(.text+0x11a047): undefined reference to `input_event'
drivers/built-in.o:t613.c:(.text+0x11a058): more undefined references to `input_event' follow

These could be fixed in Kconfig by something like (for each sub-driver that tests
CONFIG_INPUT):

	depends on INPUT || INPUT=n

Do you have another preference for fixing this?

thanks,
-- 
~Randy

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: linux-next: Tree for Oct 8 (media/usb/gspca)
  2014-10-08 17:13 ` linux-next: Tree for Oct 8 (media/usb/gspca) Randy Dunlap
@ 2014-10-08 18:31   ` Mauro Carvalho Chehab
  2014-10-08 20:53     ` Randy Dunlap
  0 siblings, 1 reply; 8+ messages in thread
From: Mauro Carvalho Chehab @ 2014-10-08 18:31 UTC (permalink / raw)
  To: Randy Dunlap
  Cc: Stephen Rothwell, linux-next, linux-kernel, Hans de Goede,
	linux-media

Em Wed, 08 Oct 2014 10:13:39 -0700
Randy Dunlap <rdunlap@infradead.org> escreveu:

> On 10/07/14 23:49, Stephen Rothwell wrote:
> > Hi all,
> > 
> > Please do not add any material intended for v3.19 to you linux-next
> > included trees until after v3.18-rc1 has been released.
> > 
> > Changes since 20141007:
> > 
> 
> I saw these build errors in gspca when CONFIG_INPUT=m but the gspca
> sub-drivers are builtin:
> 
> drivers/built-in.o: In function `gspca_dev_probe2':
> (.text+0x10ef43): undefined reference to `input_allocate_device'
> drivers/built-in.o: In function `gspca_dev_probe2':
> (.text+0x10efdd): undefined reference to `input_register_device'
> drivers/built-in.o: In function `gspca_dev_probe2':
> (.text+0x10f002): undefined reference to `input_free_device'
> drivers/built-in.o: In function `gspca_dev_probe2':
> (.text+0x10f0ac): undefined reference to `input_unregister_device'
> drivers/built-in.o: In function `gspca_disconnect':
> (.text+0x10f186): undefined reference to `input_unregister_device'
> drivers/built-in.o: In function `sd_int_pkt_scan':
> se401.c:(.text+0x11373d): undefined reference to `input_event'
> se401.c:(.text+0x11374e): undefined reference to `input_event'
> drivers/built-in.o: In function `sd_pkt_scan':
> t613.c:(.text+0x119f0e): undefined reference to `input_event'
> t613.c:(.text+0x119f1f): undefined reference to `input_event'
> drivers/built-in.o: In function `sd_stopN':
> t613.c:(.text+0x11a047): undefined reference to `input_event'
> drivers/built-in.o:t613.c:(.text+0x11a058): more undefined references to `input_event' follow
> 
> These could be fixed in Kconfig by something like (for each sub-driver that tests
> CONFIG_INPUT):
> 
> 	depends on INPUT || INPUT=n
> 
> Do you have another preference for fixing this?

Hmm... The code at the gspca subdrivers looks like:

#if IS_ENABLED(CONFIG_INPUT)
		if (data[0] & 0x20) {
			input_report_key(gspca_dev->input_dev, KEY_CAMERA, 1);
			input_sync(gspca_dev->input_dev);
			input_report_key(gspca_dev->input_dev, KEY_CAMERA, 0);
			input_sync(gspca_dev->input_dev);
		}
#endif

As we never got any report about such bug, and this is there for a long
time, I suspect that maybe the IS_ENABLED() macro had some changes on
its behavior. So, IMHO, we should first check if something changed there.

>From gpsca's PoV, IMHO, it should be fine to disable the webcam buttons if
the webcam was compiled as builtin and the input subsystem is compiled as 
module. The core feature expected on a camera is to capture streams. 
Buttons are just a plus.

Also, most cams don't even have buttons. The gspca subdriver has support 
for buttons for the few models that have it.

So, IMHO, it should be ok to have GSPCA=y and INPUT=m, provided that 
the buttons will be disabled.

Regards,
Mauro

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: linux-next: Tree for Oct 8 (media/usb/gspca)
  2014-10-08 18:31   ` Mauro Carvalho Chehab
@ 2014-10-08 20:53     ` Randy Dunlap
  2014-10-09  1:50       ` Mauro Carvalho Chehab
  0 siblings, 1 reply; 8+ messages in thread
From: Randy Dunlap @ 2014-10-08 20:53 UTC (permalink / raw)
  To: Mauro Carvalho Chehab
  Cc: Stephen Rothwell, linux-next, linux-kernel, Hans de Goede,
	linux-media

On 10/08/14 11:31, Mauro Carvalho Chehab wrote:
> Em Wed, 08 Oct 2014 10:13:39 -0700
> Randy Dunlap <rdunlap@infradead.org> escreveu:
> 
>> On 10/07/14 23:49, Stephen Rothwell wrote:
>>> Hi all,
>>>
>>> Please do not add any material intended for v3.19 to you linux-next
>>> included trees until after v3.18-rc1 has been released.
>>>
>>> Changes since 20141007:
>>>
>>
>> I saw these build errors in gspca when CONFIG_INPUT=m but the gspca
>> sub-drivers are builtin:
>>
>> drivers/built-in.o: In function `gspca_dev_probe2':
>> (.text+0x10ef43): undefined reference to `input_allocate_device'
>> drivers/built-in.o: In function `gspca_dev_probe2':
>> (.text+0x10efdd): undefined reference to `input_register_device'
>> drivers/built-in.o: In function `gspca_dev_probe2':
>> (.text+0x10f002): undefined reference to `input_free_device'
>> drivers/built-in.o: In function `gspca_dev_probe2':
>> (.text+0x10f0ac): undefined reference to `input_unregister_device'
>> drivers/built-in.o: In function `gspca_disconnect':
>> (.text+0x10f186): undefined reference to `input_unregister_device'
>> drivers/built-in.o: In function `sd_int_pkt_scan':
>> se401.c:(.text+0x11373d): undefined reference to `input_event'
>> se401.c:(.text+0x11374e): undefined reference to `input_event'
>> drivers/built-in.o: In function `sd_pkt_scan':
>> t613.c:(.text+0x119f0e): undefined reference to `input_event'
>> t613.c:(.text+0x119f1f): undefined reference to `input_event'
>> drivers/built-in.o: In function `sd_stopN':
>> t613.c:(.text+0x11a047): undefined reference to `input_event'
>> drivers/built-in.o:t613.c:(.text+0x11a058): more undefined references to `input_event' follow
>>
>> These could be fixed in Kconfig by something like (for each sub-driver that tests
>> CONFIG_INPUT):
>>
>> 	depends on INPUT || INPUT=n
>>
>> Do you have another preference for fixing this?
> 
> Hmm... The code at the gspca subdrivers looks like:
> 
> #if IS_ENABLED(CONFIG_INPUT)

For builtin only, that should be

#if IS_BUILTIN(CONFIG_INPUT)

> 		if (data[0] & 0x20) {
> 			input_report_key(gspca_dev->input_dev, KEY_CAMERA, 1);
> 			input_sync(gspca_dev->input_dev);
> 			input_report_key(gspca_dev->input_dev, KEY_CAMERA, 0);
> 			input_sync(gspca_dev->input_dev);
> 		}
> #endif
> 
> As we never got any report about such bug, and this is there for a long
> time, I suspect that maybe the IS_ENABLED() macro had some changes on
> its behavior. So, IMHO, we should first check if something changed there.

I don't see any changes in <linux/kconfig.h>.

> From gpsca's PoV, IMHO, it should be fine to disable the webcam buttons if
> the webcam was compiled as builtin and the input subsystem is compiled as 
> module. The core feature expected on a camera is to capture streams. 
> Buttons are just a plus.
> 
> Also, most cams don't even have buttons. The gspca subdriver has support 
> for buttons for the few models that have it.
> 
> So, IMHO, it should be ok to have GSPCA=y and INPUT=m, provided that 
> the buttons will be disabled.

Then all of the sub-drivers that use IS_ENABLED(CONFIG_INPUT) should be
changed to use IS_BUILTIN(CONFIG_INPUT).

But that is too restrictive IMO.  The input subsystem will work fine when
CONFIG_INPUT=m and the GSPCA drivers are also loadable modules.
That's simple to express in Kconfig language but probly more messy in CPP.


Thanks.


-- 
~Randy

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: linux-next: Tree for Oct 8 (media/usb/gspca)
  2014-10-08 20:53     ` Randy Dunlap
@ 2014-10-09  1:50       ` Mauro Carvalho Chehab
  2014-10-09  6:45         ` Paul Bolle
  0 siblings, 1 reply; 8+ messages in thread
From: Mauro Carvalho Chehab @ 2014-10-09  1:50 UTC (permalink / raw)
  To: Randy Dunlap
  Cc: Stephen Rothwell, linux-next, linux-kernel, Hans de Goede,
	linux-media

Em Wed, 08 Oct 2014 13:53:33 -0700
Randy Dunlap <rdunlap@infradead.org> escreveu:

> On 10/08/14 11:31, Mauro Carvalho Chehab wrote:
> > Em Wed, 08 Oct 2014 10:13:39 -0700
> > Randy Dunlap <rdunlap@infradead.org> escreveu:
> > 
> >> On 10/07/14 23:49, Stephen Rothwell wrote:
> >>> Hi all,
> >>>
> >>> Please do not add any material intended for v3.19 to you linux-next
> >>> included trees until after v3.18-rc1 has been released.
> >>>
> >>> Changes since 20141007:
> >>>
> >>
> >> I saw these build errors in gspca when CONFIG_INPUT=m but the gspca
> >> sub-drivers are builtin:
> >>
> >> drivers/built-in.o: In function `gspca_dev_probe2':
> >> (.text+0x10ef43): undefined reference to `input_allocate_device'
> >> drivers/built-in.o: In function `gspca_dev_probe2':
> >> (.text+0x10efdd): undefined reference to `input_register_device'
> >> drivers/built-in.o: In function `gspca_dev_probe2':
> >> (.text+0x10f002): undefined reference to `input_free_device'
> >> drivers/built-in.o: In function `gspca_dev_probe2':
> >> (.text+0x10f0ac): undefined reference to `input_unregister_device'
> >> drivers/built-in.o: In function `gspca_disconnect':
> >> (.text+0x10f186): undefined reference to `input_unregister_device'
> >> drivers/built-in.o: In function `sd_int_pkt_scan':
> >> se401.c:(.text+0x11373d): undefined reference to `input_event'
> >> se401.c:(.text+0x11374e): undefined reference to `input_event'
> >> drivers/built-in.o: In function `sd_pkt_scan':
> >> t613.c:(.text+0x119f0e): undefined reference to `input_event'
> >> t613.c:(.text+0x119f1f): undefined reference to `input_event'
> >> drivers/built-in.o: In function `sd_stopN':
> >> t613.c:(.text+0x11a047): undefined reference to `input_event'
> >> drivers/built-in.o:t613.c:(.text+0x11a058): more undefined references to `input_event' follow
> >>
> >> These could be fixed in Kconfig by something like (for each sub-driver that tests
> >> CONFIG_INPUT):
> >>
> >> 	depends on INPUT || INPUT=n
> >>
> >> Do you have another preference for fixing this?
> > 
> > Hmm... The code at the gspca subdrivers looks like:
> > 
> > #if IS_ENABLED(CONFIG_INPUT)
> 
> For builtin only, that should be
> 
> #if IS_BUILTIN(CONFIG_INPUT)
> 
> > 		if (data[0] & 0x20) {
> > 			input_report_key(gspca_dev->input_dev, KEY_CAMERA, 1);
> > 			input_sync(gspca_dev->input_dev);
> > 			input_report_key(gspca_dev->input_dev, KEY_CAMERA, 0);
> > 			input_sync(gspca_dev->input_dev);
> > 		}
> > #endif
> > 
> > As we never got any report about such bug, and this is there for a long
> > time, I suspect that maybe the IS_ENABLED() macro had some changes on
> > its behavior. So, IMHO, we should first check if something changed there.
> 
> I don't see any changes in <linux/kconfig.h>.

Perhaps some changes at the Kbuild source code or scripts badly affected it.

> 
> > From gpsca's PoV, IMHO, it should be fine to disable the webcam buttons if
> > the webcam was compiled as builtin and the input subsystem is compiled as 
> > module. The core feature expected on a camera is to capture streams. 
> > Buttons are just a plus.
> > 
> > Also, most cams don't even have buttons. The gspca subdriver has support 
> > for buttons for the few models that have it.
> > 
> > So, IMHO, it should be ok to have GSPCA=y and INPUT=m, provided that 
> > the buttons will be disabled.
> 
> Then all of the sub-drivers that use IS_ENABLED(CONFIG_INPUT) should be
> changed to use IS_BUILTIN(CONFIG_INPUT).
> 
> But that is too restrictive IMO.  The input subsystem will work fine when
> CONFIG_INPUT=m and the GSPCA drivers are also loadable modules.

Agreed.

Maybe the solution would be something more complex like 
(for drivers/media/usb/gspca/zc3xx.c):

#if (IS_BUILTIN(CONFIG_INPUT)) || (IS_ENABLED(CONFIG_INPUT) && !IS_BUILTIN(CONFIG_USB_GSPCA_ZC3XX))

Probably the best would be to write another macro that would evaluate
like the above.

> That's simple to express in Kconfig language but probly more messy in CPP.
> 
> 
> Thanks.
> 
> 

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: linux-next: Tree for Oct 8 (media/usb/gspca)
  2014-10-09  1:50       ` Mauro Carvalho Chehab
@ 2014-10-09  6:45         ` Paul Bolle
  2014-10-09 10:30           ` Mauro Carvalho Chehab
  0 siblings, 1 reply; 8+ messages in thread
From: Paul Bolle @ 2014-10-09  6:45 UTC (permalink / raw)
  To: Mauro Carvalho Chehab
  Cc: Randy Dunlap, Stephen Rothwell, linux-next, linux-kernel,
	Hans de Goede, linux-media

On Wed, 2014-10-08 at 22:50 -0300, Mauro Carvalho Chehab wrote:
> Em Wed, 08 Oct 2014 13:53:33 -0700
> Randy Dunlap <rdunlap@infradead.org> escreveu:
> > On 10/08/14 11:31, Mauro Carvalho Chehab wrote:
> > > From gpsca's PoV, IMHO, it should be fine to disable the webcam buttons if
> > > the webcam was compiled as builtin and the input subsystem is compiled as 
> > > module. The core feature expected on a camera is to capture streams. 
> > > Buttons are just a plus.
> > > 
> > > Also, most cams don't even have buttons. The gspca subdriver has support 
> > > for buttons for the few models that have it.
> > > 
> > > So, IMHO, it should be ok to have GSPCA=y and INPUT=m, provided that 
> > > the buttons will be disabled.
> > 
> > Then all of the sub-drivers that use IS_ENABLED(CONFIG_INPUT) should be
> > changed to use IS_BUILTIN(CONFIG_INPUT).
> > 
> > But that is too restrictive IMO.  The input subsystem will work fine when
> > CONFIG_INPUT=m and the GSPCA drivers are also loadable modules.
> 
> Agreed.
> 
> Maybe the solution would be something more complex like 
> (for drivers/media/usb/gspca/zc3xx.c):
> 
> #if (IS_BUILTIN(CONFIG_INPUT)) || (IS_ENABLED(CONFIG_INPUT) && !IS_BUILTIN(CONFIG_USB_GSPCA_ZC3XX))

The above discussion meanders a bit, and I just stumbled onto it, but
would
    #if IS_BUILTIN(CONFIG_INPUT) || (IS_MODULE(CONFIG_INPUT) && defined(MODULE))

cover your requirements when using macros?

> Probably the best would be to write another macro that would evaluate
> like the above.


Paul Bolle


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: linux-next: Tree for Oct 8 (media/usb/gspca)
  2014-10-09  6:45         ` Paul Bolle
@ 2014-10-09 10:30           ` Mauro Carvalho Chehab
  2014-10-09 11:26             ` Paul Bolle
  0 siblings, 1 reply; 8+ messages in thread
From: Mauro Carvalho Chehab @ 2014-10-09 10:30 UTC (permalink / raw)
  To: Paul Bolle
  Cc: Randy Dunlap, Stephen Rothwell, linux-next, linux-kernel,
	Hans de Goede, linux-media

Em Thu, 09 Oct 2014 08:45:28 +0200
Paul Bolle <pebolle@tiscali.nl> escreveu:

> On Wed, 2014-10-08 at 22:50 -0300, Mauro Carvalho Chehab wrote:
> > Em Wed, 08 Oct 2014 13:53:33 -0700
> > Randy Dunlap <rdunlap@infradead.org> escreveu:
> > > On 10/08/14 11:31, Mauro Carvalho Chehab wrote:
> > > > From gpsca's PoV, IMHO, it should be fine to disable the webcam buttons if
> > > > the webcam was compiled as builtin and the input subsystem is compiled as 
> > > > module. The core feature expected on a camera is to capture streams. 
> > > > Buttons are just a plus.
> > > > 
> > > > Also, most cams don't even have buttons. The gspca subdriver has support 
> > > > for buttons for the few models that have it.
> > > > 
> > > > So, IMHO, it should be ok to have GSPCA=y and INPUT=m, provided that 
> > > > the buttons will be disabled.
> > > 
> > > Then all of the sub-drivers that use IS_ENABLED(CONFIG_INPUT) should be
> > > changed to use IS_BUILTIN(CONFIG_INPUT).
> > > 
> > > But that is too restrictive IMO.  The input subsystem will work fine when
> > > CONFIG_INPUT=m and the GSPCA drivers are also loadable modules.
> > 
> > Agreed.
> > 
> > Maybe the solution would be something more complex like 
> > (for drivers/media/usb/gspca/zc3xx.c):
> > 
> > #if (IS_BUILTIN(CONFIG_INPUT)) || (IS_ENABLED(CONFIG_INPUT) && !IS_BUILTIN(CONFIG_USB_GSPCA_ZC3XX))
> 
> The above discussion meanders a bit, and I just stumbled onto it, but
> would
>     #if IS_BUILTIN(CONFIG_INPUT) || (IS_MODULE(CONFIG_INPUT) && defined(MODULE))
> 
> cover your requirements when using macros?

No. What we need to do, for all gspca sub-drivers that have optional
support for buttons is to only enable the buttons support if:

	CONFIG_INPUT=y
or
	CONFIG_INPUT=m and CONFIG_USB_GSPCA_submodule=m

If we use a reverse logic, we need to disable the code if:
	# CONFIG_INPUT is not set
or
	CONFIG_INPUT=m and CONFIG_USB_GSPCA_submodule=y

The rationale for disabling the code on the last expression is that a
builtin code cannot call a function inside a module.

Also, as the submodule is already being compiled, we know that
CONFIG_USB_GSPCA_submodule is either module or builtin.

So, either one of those expressions should work:
	#if (IS_BUILTIN(CONFIG_INPUT)) || (IS_ENABLED(CONFIG_INPUT) && !IS_BUILTIN(CONFIG_USB_GSPCA_submodule))
or
	#if (IS_BUILTIN(CONFIG_INPUT)) || (IS_MODULE(CONFIG_INPUT) && IS_MODULE(CONFIG_USB_GSPCA_submodule) && defined(MODULE))
or
	#if (IS_BUILTIN(CONFIG_INPUT)) || (IS_ENABLED(CONFIG_INPUT) && IS_MODULE(CONFIG_USB_GSPCA_submodule))

> 
> > Probably the best would be to write another macro that would evaluate
> > like the above.
> 
> 
> Paul Bolle
> 

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: linux-next: Tree for Oct 8 (media/usb/gspca)
  2014-10-09 10:30           ` Mauro Carvalho Chehab
@ 2014-10-09 11:26             ` Paul Bolle
  2014-10-09 11:52               ` Mauro Carvalho Chehab
  0 siblings, 1 reply; 8+ messages in thread
From: Paul Bolle @ 2014-10-09 11:26 UTC (permalink / raw)
  To: Mauro Carvalho Chehab
  Cc: Randy Dunlap, Stephen Rothwell, linux-next, linux-kernel,
	Hans de Goede, linux-media

On Thu, 2014-10-09 at 07:30 -0300, Mauro Carvalho Chehab wrote:
> Em Thu, 09 Oct 2014 08:45:28 +0200
> Paul Bolle <pebolle@tiscali.nl> escreveu:
> > The above discussion meanders a bit, and I just stumbled onto it, but
> > would
> >     #if IS_BUILTIN(CONFIG_INPUT) || (IS_MODULE(CONFIG_INPUT) && defined(MODULE))
> > 
> > cover your requirements when using macros?
> 
> No. What we need to do, for all gspca sub-drivers that have optional
> support for buttons is to only enable the buttons support if:
> 
> 	CONFIG_INPUT=y
> or
> 	CONFIG_INPUT=m and CONFIG_USB_GSPCA_submodule=m
> 
> If we use a reverse logic, we need to disable the code if:
> 	# CONFIG_INPUT is not set
> or
> 	CONFIG_INPUT=m and CONFIG_USB_GSPCA_submodule=y
> 
> The rationale for disabling the code on the last expression is that a
> builtin code cannot call a function inside a module.
> 
> Also, as the submodule is already being compiled, we know that
> CONFIG_USB_GSPCA_submodule is either module or builtin.
> 
> So, either one of those expressions should work:
> 	#if (IS_BUILTIN(CONFIG_INPUT)) || (IS_ENABLED(CONFIG_INPUT) && !IS_BUILTIN(CONFIG_USB_GSPCA_submodule))
> or
> 	#if (IS_BUILTIN(CONFIG_INPUT)) || (IS_MODULE(CONFIG_INPUT) && IS_MODULE(CONFIG_USB_GSPCA_submodule) && defined(MODULE))

I thought MODULE was only defined for code that will be part of a
module. So "IS_MODULE(CONFIG_USB_GSPCA_submodule)" and "defined(MODULE)"
should be equal when used _inside_ [...]/usb/gspca/that_submodule.c,
shouldn't they? That would make this option basically identical to my
suggestion. Or are you thinking about using these tests outside of these
submodules themselves?

> or
> 	#if (IS_BUILTIN(CONFIG_INPUT)) || (IS_ENABLED(CONFIG_INPUT) && IS_MODULE(CONFIG_USB_GSPCA_submodule))

I think it's clearer to use
    IS_BUILTIN(CONFIG_FOO) || (IS_MODULE(CONFIG_FOO) && [...])

Ditto above. Perhaps just a matter of taste.

(Depending on INPUT is apparently not possible for these submodules. So
obviously any solution needs to check whether input is available, say
like
    if (IS_MODULE(CONFIG_INPUT))
        if (!is_input_loaded())
            goto no_input;

Doesn't it?)

Thanks,


Paul Bolle


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: linux-next: Tree for Oct 8 (media/usb/gspca)
  2014-10-09 11:26             ` Paul Bolle
@ 2014-10-09 11:52               ` Mauro Carvalho Chehab
  0 siblings, 0 replies; 8+ messages in thread
From: Mauro Carvalho Chehab @ 2014-10-09 11:52 UTC (permalink / raw)
  To: Paul Bolle
  Cc: Randy Dunlap, Stephen Rothwell, linux-next, linux-kernel,
	Hans de Goede, linux-media

Em Thu, 09 Oct 2014 13:26:10 +0200
Paul Bolle <pebolle@tiscali.nl> escreveu:

> On Thu, 2014-10-09 at 07:30 -0300, Mauro Carvalho Chehab wrote:
> > Em Thu, 09 Oct 2014 08:45:28 +0200
> > Paul Bolle <pebolle@tiscali.nl> escreveu:
> > > The above discussion meanders a bit, and I just stumbled onto it, but
> > > would
> > >     #if IS_BUILTIN(CONFIG_INPUT) || (IS_MODULE(CONFIG_INPUT) && defined(MODULE))
> > > 
> > > cover your requirements when using macros?
> > 
> > No. What we need to do, for all gspca sub-drivers that have optional
> > support for buttons is to only enable the buttons support if:
> > 
> > 	CONFIG_INPUT=y
> > or
> > 	CONFIG_INPUT=m and CONFIG_USB_GSPCA_submodule=m
> > 
> > If we use a reverse logic, we need to disable the code if:
> > 	# CONFIG_INPUT is not set
> > or
> > 	CONFIG_INPUT=m and CONFIG_USB_GSPCA_submodule=y
> > 
> > The rationale for disabling the code on the last expression is that a
> > builtin code cannot call a function inside a module.
> > 
> > Also, as the submodule is already being compiled, we know that
> > CONFIG_USB_GSPCA_submodule is either module or builtin.
> > 
> > So, either one of those expressions should work:
> > 	#if (IS_BUILTIN(CONFIG_INPUT)) || (IS_ENABLED(CONFIG_INPUT) && !IS_BUILTIN(CONFIG_USB_GSPCA_submodule))
> > or
> > 	#if (IS_BUILTIN(CONFIG_INPUT)) || (IS_MODULE(CONFIG_INPUT) && IS_MODULE(CONFIG_USB_GSPCA_submodule) && defined(MODULE))
> 
> I thought MODULE was only defined for code that will be part of a
> module. So "IS_MODULE(CONFIG_USB_GSPCA_submodule)" and "defined(MODULE)"
> should be equal when used _inside_ [...]/usb/gspca/that_submodule.c,

Ah, I didn't know that. In such case, your suggestion is indeed better,
as it is easier to write the patch.

> shouldn't they? That would make this option basically identical to my
> suggestion. Or are you thinking about using these tests outside of these
> submodules themselves?

> 
> > or
> > 	#if (IS_BUILTIN(CONFIG_INPUT)) || (IS_ENABLED(CONFIG_INPUT) && IS_MODULE(CONFIG_USB_GSPCA_submodule))
> 
> I think it's clearer to use
>     IS_BUILTIN(CONFIG_FOO) || (IS_MODULE(CONFIG_FOO) && [...])
> 
> Ditto above. Perhaps just a matter of taste.
> 
> (Depending on INPUT is apparently not possible for these submodules. 

No, because the main functionality (webcam support) doesn't depend on
INPUT.

> So
> obviously any solution needs to check whether input is available, say
> like
>     if (IS_MODULE(CONFIG_INPUT))
>         if (!is_input_loaded())
>             goto no_input;
> 
> Doesn't it?)

Yeah, but the thing is that it breaks at compilation time, because
the builtin module would try to use some symbols that are defined only
at runtime.

So, the above won't solve the issue.

One possibility for future Kernels would be to add some logic that would
automatically load the module if a call to one of those symbols defined
inside a module happens inside a builtin module.

The DVB subsystem actually does that for I2C frontend drivers, 
using those macros:

#define dvb_attach(FUNCTION, ARGS...) ({ \
	void *__r = NULL; \
	typeof(&FUNCTION) __a = symbol_request(FUNCTION); \
	if (__a) { \
		__r = (void *) __a(ARGS); \
		if (__r == NULL) \
			symbol_put(FUNCTION); \
	} else { \
		printk(KERN_ERR "DVB: Unable to find symbol "#FUNCTION"()\n"); \
	} \
	__r; \
})

#define dvb_detach(FUNC)	symbol_put_addr(FUNC)

The above works reasonably fine when there's just one symbol needed
from the module driver.

There are, however, some issues with such approach:

1) as symbol_request increments refcount, this works very badly when
   there's more than one symbol needed, as symbol_put() would need
   to be called as many times as symbol_request() is called;

2) lsmod doesn't list what module is actually requesting such symbol
   (if the caller is also a module). It just increments refcounting.
    There were some patches meant to fix that, send a long time ago,
    but were never merged. Can't remember why:
	http://linuxtv.org/pipermail/linux-dvb/2006-August/012322.html

Due to the above issues, and because we want to better use the I2C
binding model, we're currently trying to get rid of such approach for
the DVB subsystem, but there are too many changes to get rid of it.
So, the migration is slow.

Anyway, IMHO, having such sort of logic would help to solve the issues
with some weird configs like INPUT=m.

Regards,
Mauro

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2014-10-09 11:52 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
     [not found] <20141008174923.76786a03@canb.auug.org.au>
2014-10-08 17:13 ` linux-next: Tree for Oct 8 (media/usb/gspca) Randy Dunlap
2014-10-08 18:31   ` Mauro Carvalho Chehab
2014-10-08 20:53     ` Randy Dunlap
2014-10-09  1:50       ` Mauro Carvalho Chehab
2014-10-09  6:45         ` Paul Bolle
2014-10-09 10:30           ` Mauro Carvalho Chehab
2014-10-09 11:26             ` Paul Bolle
2014-10-09 11:52               ` Mauro Carvalho Chehab

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).