login
Search
The mm-of-the-moment snapshot 2010-04-05-16-09 has been uploaded to http://userweb.kernel.org/~akpm/mmotm/ and will soon be available at git://zen-kernel.org/kernel/mmotm.git It contains the following patches against ...
From: Randy Dunlap <randy.dunlap@oracle.com> HID_PICOLCD should depend on LCD_CLASS_DEVICE, otherwise the build fails when HID_PICOLCD=y and LCD_CLASS_DEVICE=m: hid-picolcd.c:(.text+0x84523f): undefined reference to `lcd_device_unregister' hid-picolcd.c:(.text+0x8478ab): undefined reference to `lcd_device_register' hid-picolcd.c:(.text+0x84c15f): undefined reference to `lcd_device_unregister' Signed-off-by: Randy Dunlap <randy.dunlap@oracle.com> --- drivers/hid/Kconfig | 1 + 1 file changed, 1 insertion(+) --- mmotm-2010-0405-1609.orig/drivers/hid/Kconfig +++ mmotm-2010-0405-1609/drivers/hid/Kconfig @@ -265,6 +265,7 @@ config HID_PETALYNX config HID_PICOLCD tristate "PicoLCD (graphic version)" depends on USB_HID + depends on LCD_CLASS_DEVICE select FB_DEFERRED_IO if FB select FB_SYS_FILLRECT if FB select FB_SYS_COPYAREA if FB --
[ adding Bruno to CC ] Thanks Randy. We'll have to take care of the other dependencies as well though (CONFIG_LCD_CLASS_DEVICE, CONFIG_LEDS_CLASS). -- Jiri Kosina SUSE Labs, Novell Inc. --
That is weird, the #if defined(CONFIG_LCD_CLASS_DEVICE) || defined(CONFIG_LCD_CLASS_DEVICE_MODULE) feature support code #else empty stubs #endif blocks should have prevented LCD_CLASS support from being built if it was not enabled in configuration. Do you have the .config matching your build? When I did my test-build with LCD support enabled/disabled I didn't get any linker errors as those mentioned above. Thanks, --
Thanks, here is the extract (only the pertinent items):
CONFIG_BACKLIGHT_LCD_SUPPORT=y
CONFIG_LCD_CLASS_DEVICE=m
CONFIG_BACKLIGHT_CLASS_DEVICE=y
CONFIG_HID_PICOLCD=y
It triggers the issue by having PicoLCD built-in while one of the
optional dependencies as a module.
Any idea of how this can be solved with kbuild in order to keep the
dependencies optional?
Something that would satisfy the following pseudocode:
if CONFIG_HID_PICOLCD == y
assert(CONFIG_LCD_CLASS_DEVICE != m)
One of my attempts did end up with a circular loop with regard to FB
(some of the FB drivers did select INPUT)?
I had something like:
config HID_PICOLCD
tristate ...
config HID_PICOLCD_FB
bool ...
depends on HID_PICOLCD
select FB
select FB_...
...
Then in the code I checked for CONFIG_HID_PICOLCD_FB instead of
(CONFIG_FB or CONFIG_FB_MODULE).
Thanks,
Bruno
--
If you don't want the kconfig machinery to do that (it appears that you don't), then I guess that you'll need to expand the source code to handle CONFIG_LCD_CLASS_DEVICE=y and CONFIG_LCD_CLASS_DEVICE=m differently. (not that I can find) CONFIG_VT does select INPUT and CONFIG_DRM_I915 does --- ~Randy (re)read Documentation/ManagementStyle --
Below a patch (against 2.6.34-rc3 + Jiri's picolcd branch) which should fix above issue keeping the deps optional. With it it's not yet possible to select the deps from HID menu. I did a few oldconfig and compile-tests with PICOLCD=y/m and same for the deps (not switched FB for those tests). Bruno HID_PICOLCD should depend on LCD_CLASS_DEVICE, otherwise the build fails when HID_PICOLCD=y and LCD_CLASS_DEVICE=m: hid-picolcd.c:(.text+0x84523f): undefined reference to `lcd_device_unregister' hid-picolcd.c:(.text+0x8478ab): undefined reference to `lcd_device_register' hid-picolcd.c:(.text+0x84c15f): undefined reference to `lcd_device_unregister' Reported-by: Randy Dunlap <randy.dunlap@oracle.com> Signed-off-by: Bruno Prémont <bonbons@linux-vserver.org> diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig index a2ecd83..39df4f5 100644 --- a/drivers/hid/Kconfig +++ b/drivers/hid/Kconfig @@ -285,6 +285,35 @@ config HID_PICOLCD Features that are not (yet) supported: - IR +config HID_PICOLCD_FB + bool "Framebuffer support" + default !EMBEDDED + depends on HID_PICOLCD + depends on HID_PICOLCD=FB || FB=y + select FB_DEFERRED_IO + select FB_SYS_FILLRECT + select FB_SYS_COPYAREA + select FB_SYS_IMAGEBLIT + select FB_SYS_FOPS + +config HID_PICOLCD_BACKLIGHT + bool "Backlight control" + default !EMBEDDED + depends on HID_PICOLCD + depends on HID_PICOLCD=BACKLIGHT_CLASS_DEVICE || BACKLIGHT_CLASS_DEVICE=y + +config HID_PICOLCD_LCD + bool "Contrast control" + default !EMBEDDED + depends on HID_PICOLCD + depends on HID_PICOLCD=LCD_CLASS_DEVICE || LCD_CLASS_DEVICE=y + +config HID_PICOLCD_LEDS + bool "GPO via leds class" + default !EMBEDDED + depends on HID_PICOLCD + depends on HID_PICOLCD=LEDS_CLASS || LEDS_CLASS=y + config HID_QUANTA tristate "Quanta Optical Touch" depends on USB_HID diff --git a/drivers/hid/hid-picolcd.c b/drivers/hid/hid-picolcd.c index 0eacc6b..0fbc7d3 100644 --- a/drivers/hid/hid-picolcd.c +++ ...
HID_PICOLCD should depend on LCD_CLASS_DEVICE, otherwise the build fails when HID_PICOLCD=y and LCD_CLASS_DEVICE=m: hid-picolcd.c:(.text+0x84523f): undefined reference to `lcd_device_unregister' hid-picolcd.c:(.text+0x8478ab): undefined reference to `lcd_device_register' hid-picolcd.c:(.text+0x84c15f): undefined reference to `lcd_device_unregister' Same applies to FB, BACKLIGHT_CLASS_DEVICE and LEDS_CLASS. Add suboptions for those features to handle the deps on kbuild side and just check HID_PICOLCD_* in the code. Reported-by: Randy Dunlap <randy.dunlap@oracle.com> Signed-off-by: Bruno Prémont <bonbons@linux-vserver.org> --- Here is a better patch, with added documentation and stripped select clauses under HID_PICOLCD as they are handled by HID_PICOLCD_FB. drivers/hid/Kconfig | 53 +++++++++++++++++++++++++++++++++++++------- drivers/hid/hid-picolcd.c | 40 +++++++++++++++++----------------- 2 files changed, 64 insertions(+), 29 deletions(-) diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig index a2ecd83..782a34e 100644 --- a/drivers/hid/Kconfig +++ b/drivers/hid/Kconfig @@ -265,11 +265,6 @@ config HID_PETALYNX config HID_PICOLCD tristate "PicoLCD (graphic version)" depends on USB_HID - select FB_DEFERRED_IO if FB - select FB_SYS_FILLRECT if FB - select FB_SYS_COPYAREA if FB - select FB_SYS_IMAGEBLIT if FB - select FB_SYS_FOPS if FB ---help--- This provides support for Minibox PicoLCD devices, currently only the graphical ones are supported. @@ -277,14 +272,54 @@ config HID_PICOLCD This includes support for the following device features: - Keypad - Switching between Firmware and Flash mode - - Framebuffer for monochrome 256x64 display - - Backlight control (needs CONFIG_BACKLIGHT_CLASS_DEVICE) - - Contrast control (needs CONFIG_LCD_CLASS_DEVICE) - - General purpose outputs (needs CONFIG_LEDS_CLASS) - EEProm / Flash access (via debugfs) + Features to selectively ...
Could we perhaps also make the sub-choices for individual features availabel only if !EMBEDDED as well? It's probably too much to ask for a single device during oldconfig run, for example ... Thanks, -- Jiri Kosina SUSE Labs, Novell Inc. --
HID_PICOLCD should depend on LCD_CLASS_DEVICE, otherwise the build fails when HID_PICOLCD=y and LCD_CLASS_DEVICE=m: hid-picolcd.c:(.text+0x84523f): undefined reference to `lcd_device_unregister' hid-picolcd.c:(.text+0x8478ab): undefined reference to `lcd_device_register' hid-picolcd.c:(.text+0x84c15f): undefined reference to `lcd_device_unregister' Same applies to FB, BACKLIGHT_CLASS_DEVICE and LEDS_CLASS. Add suboptions for those features to handle the deps on kbuild side and just check HID_PICOLCD_* in the code. Reported-by: Randy Dunlap <randy.dunlap@oracle.com> Signed-off-by: Bruno Prémont <bonbons@linux-vserver.org> --- Here is an updated patch to make the sub-choices visible only in EMBEDDED case. Thanks, Bruno drivers/hid/Kconfig | 53 +++++++++++++++++++++++++++++++++++++------- drivers/hid/hid-picolcd.c | 40 +++++++++++++++++----------------- 2 files changed, 64 insertions(+), 29 deletions(-) diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig index a2ecd83..621c52c 100644 --- a/drivers/hid/Kconfig +++ b/drivers/hid/Kconfig @@ -265,11 +265,6 @@ config HID_PETALYNX config HID_PICOLCD tristate "PicoLCD (graphic version)" depends on USB_HID - select FB_DEFERRED_IO if FB - select FB_SYS_FILLRECT if FB - select FB_SYS_COPYAREA if FB - select FB_SYS_IMAGEBLIT if FB - select FB_SYS_FOPS if FB ---help--- This provides support for Minibox PicoLCD devices, currently only the graphical ones are supported. @@ -277,14 +272,54 @@ config HID_PICOLCD This includes support for the following device features: - Keypad - Switching between Firmware and Flash mode - - Framebuffer for monochrome 256x64 display - - Backlight control (needs CONFIG_BACKLIGHT_CLASS_DEVICE) - - Contrast control (needs CONFIG_LCD_CLASS_DEVICE) - - General purpose outputs (needs CONFIG_LEDS_CLASS) - EEProm / Flash access (via debugfs) + Features selectively enabled: + - Framebuffer for monochrome ...
Applied, thanks Bruno. -- Jiri Kosina SUSE Labs, Novell Inc. --
A newer attempt still produces the same result:
drivers/input/Kconfig:9:error: found recursive dependency: INPUT ->
HID_SUPPORT -> HID_PICOLCD_FB -> FB -> FB_STI -> VT -> INPUT
(it's only FB which causes the loop, LEDS, LCD and BACKLIGHT are fine)
This is with following patch on top of the improved deps patch I sent
a few minutes ago deeper in this thread.
Is there a way around this?
diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
index 782a34e..711c091 100644
--- a/drivers/hid/Kconfig
+++ b/drivers/hid/Kconfig
@@ -285,7 +285,7 @@ config HID_PICOLCD_FB
bool "Framebuffer support"
default !EMBEDDED
depends on HID_PICOLCD
- depends on HID_PICOLCD=FB || FB=y
+ select FB
select FB_DEFERRED_IO
select FB_SYS_FILLRECT
select FB_SYS_COPYAREA
@@ -299,7 +299,8 @@ config HID_PICOLCD_BACKLIGHT
bool "Backlight control"
default !EMBEDDED
depends on HID_PICOLCD
- depends on HID_PICOLCD=BACKLIGHT_CLASS_DEVICE || BACKLIGHT_CLASS_DEVICE=y
+ select BACKLIGHT_LCD_SUPPORT
+ select BACKLIGHT_CLASS_DEVICE
---help---
Provide access to PicoLCD's backlight control via backlight
class.
@@ -308,7 +309,8 @@ config HID_PICOLCD_LCD
bool "Contrast control"
default !EMBEDDED
depends on HID_PICOLCD
- depends on HID_PICOLCD=LCD_CLASS_DEVICE || LCD_CLASS_DEVICE=y
+ select BACKLIGHT_LCD_SUPPORT
+ select LCD_CLASS_DEVICE
---help---
Provide access to PicoLCD's LCD contrast via lcd class.
@@ -316,7 +318,8 @@ config HID_PICOLCD_LEDS
bool "GPO via leds class"
default !EMBEDDED
depends on HID_PICOLCD
- depends on HID_PICOLCD=LEDS_CLASS || LEDS_CLASS=y
+ select NEW_LEDS
+ select LEDS_CLASS
---help---
Provide access to PicoLCD's GPO pins via leds class.
--
Well, lesson #1 is that select is evil^W^W should only be used to enable library-like code, or as Documentation/kbuild/kconfig-language.txt says: Note: select should be used with care. select will force a symbol to a value without visiting the dependencies. By abusing select you are able to select a symbol FOO even if FOO depends on BAR that is not set. In general use select only for non-visible symbols (no prompts anywhere) and for symbols with no dependencies. That will limit the usefulness but on the other hand avoid the illegal configurations all over. kconfig should one day warn about such things. If you'll go back to the unpatched (by this patch) version here, --- ~Randy --
I've read that paragraph and I would definitely prefer not to have to use select for what this patch should achieve! It should really be a helper for the user going through menuconfig like "please check all what is needed to satisfy this item's dependencies" A "try-select-deep" or whatever it could be called. A well-behaved "GUI" implementation of this would show what would get newly checked and give the user the opportunity to not proceed or fine-tune So best is probably to just forget about all of this non-leaf select usage for now. (and not have one half doing one way and the other half doing it the other way) Thanks, Bruno --
Seen in dmesg, 2.6.34-rc2-mmotm0323 didn't do this. Tossing it at all the
likely suspects, hopefully somebody will recognize it and save me the
bisecting. ;)
[ 11.488535] ctnetlink v0.93: registering with nfnetlink.
[ 11.488579]
[ 11.488579] ===================================================
[ 11.489529] [ INFO: suspicious rcu_dereference_check() usage. ]
[ 11.489988] ---------------------------------------------------
[ 11.490494] net/netfilter/nf_conntrack_ecache.c:88 invoked rcu_dereference_check() without protection!
[ 11.491024]
[ 11.491024] other info that might help us debug this:
[ 11.491025]
[ 11.492834]
[ 11.492835] rcu_scheduler_active = 1, debug_locks = 0
[ 11.494124] 1 lock held by swapper/1:
[ 11.494776] #0: (nf_ct_ecache_mutex){+.+...}, at: [<ffffffff8148c606>] nf_conntrack_register_notifier+0x1a/0x76
[ 11.495505]
[ 11.495506] stack backtrace:
[ 11.496927] Pid: 1, comm: swapper Not tainted 2.6.34-rc3-mmotm0405 #1
[ 11.497668] Call Trace:
[ 11.498419] [<ffffffff810638c5>] lockdep_rcu_dereference+0xaa/0xb2
[ 11.499185] [<ffffffff8148c629>] nf_conntrack_register_notifier+0x3d/0x76
[ 11.499967] [<ffffffff81b5dd7b>] ctnetlink_init+0x71/0xd5
[ 11.500775] [<ffffffff81b5dd0a>] ? ctnetlink_init+0x0/0xd5
[ 11.501579] [<ffffffff810001ef>] do_one_initcall+0x59/0x14e
[ 11.502396] [<ffffffff81b3268a>] kernel_init+0x144/0x1ce
[ 11.503218] [<ffffffff81003514>] kernel_thread_helper+0x4/0x10
[ 11.504047] [<ffffffff8159cc00>] ? restore_args+0x0/0x30
[ 11.504882] [<ffffffff81b32546>] ? kernel_init+0x0/0x1ce
[ 11.505715] [<ffffffff81003510>] ? kernel_thread_helper+0x0/0x10
[ 11.506636] ip_tables: (C) 2000-2006 Netfilter Core Team
[ 11.507372] TCP bic registered
There are some unnecessary rcu_dereference() calls in the conntrack notifier registration and unregistration functions. Does this fix it?
Well, it *changed* it. Does the rcu_defererence_check() only fire on the
first time it hits something, so we've fixed the first one and now we get to
see the second one?
(For what it's worth, if this is going to be one-at-a-time whack-a-mole, I'm
OK on that, just want to know up front.)
[ 9.299425] ip_tables: (C) 2000-2006 Netfilter Core Team
[ 9.299486]
[ 9.299486] ===================================================
[ 9.300499] [ INFO: suspicious rcu_dereference_check() usage. ]
[ 9.301001] ---------------------------------------------------
[ 9.301523] net/netfilter/nf_log.c:55 invoked rcu_dereference_check() without protection!
[ 9.302066]
[ 9.302066] other info that might help us debug this:
[ 9.302067]
[ 9.303748]
[ 9.303748] rcu_scheduler_active = 1, debug_locks = 0
[ 9.304990] 1 lock held by swapper/1:
[ 9.305645] #0: (nf_log_mutex){+.+...}, at: [<ffffffff8148427a>] nf_log_register+0x57/0x111
[ 9.306342]
[ 9.306343] stack backtrace:
[ 9.307729] Pid: 1, comm: swapper Not tainted 2.6.34-rc3-mmotm0405 #2
[ 9.308447] Call Trace:
[ 9.309170] [<ffffffff810638c5>] lockdep_rcu_dereference+0xaa/0xb2
[ 9.309935] [<ffffffff81484301>] nf_log_register+0xde/0x111
[ 9.310688] [<ffffffff81b6064b>] ? log_tg_init+0x0/0x29
[ 9.311465] [<ffffffff81b60670>] log_tg_init+0x25/0x29
[ 9.312233] [<ffffffff810001ef>] do_one_initcall+0x59/0x14e
[ 9.313030] [<ffffffff81b3268a>] kernel_init+0x144/0x1ce
[ 9.313819] [<ffffffff81003514>] kernel_thread_helper+0x4/0x10
[ 9.314625] [<ffffffff8159cb80>] ? restore_args+0x0/0x30
[ 9.315434] [<ffffffff81b32546>] ? kernel_init+0x0/0x1ce
[ 9.316224] [<ffffffff81003510>] ? kernel_thread_helper+0x0/0x10
[ 9.317037] TCP bic registered
It appears that way, otherwise you should have seen a second warning in I went through the other files and I believe this should be it. We already removed most of these incorrect rcu_dereference()
Confirming - the second version of the patch fixes all the network-related RCU complaints I've been able to trigger...
Thanks. I've added the attached commit to the nf-next tree. I'll push it to Dave shortly so this can get included in the next tree.
Hit another one. I seem to be on a roll...
Seen in dmesg, happened near end of the initrd..
[ 26.756864]
[ 26.756866] ===================================================
[ 26.756869] [ INFO: suspicious rcu_dereference_check() usage. ]
[ 26.756871] ---------------------------------------------------
[ 26.756874] fs/proc/array.c:241 invoked rcu_dereference_check() without protection!
[ 26.756876]
[ 26.756877] other info that might help us debug this:
[ 26.756877]
[ 26.756879]
[ 26.756880] rcu_scheduler_active = 1, debug_locks = 0
[ 26.756883] 2 locks held by pidof/2197:
[ 26.756884] #0: (&p->lock){+.+.+.}, at: [<ffffffff810f4c2a>] seq_read+0x3a/0x42d
[ 26.756894] #1: (&(&sighand->siglock)->rlock){......}, at: [<ffffffff81047eff>] lock_task_sighand+0x79/0xcf
[ 26.756903]
[ 26.756903] stack backtrace:
[ 26.756906] Pid: 2197, comm: pidof Not tainted 2.6.34-rc3-mmotm0405 #3
[ 26.756909] Call Trace:
[ 26.756914] [<ffffffff810638c5>] lockdep_rcu_dereference+0xaa/0xb2
[ 26.756919] [<ffffffff8112ed88>] collect_sigign_sigcatch+0x37/0xa0
[ 26.756923] [<ffffffff8112f066>] do_task_stat+0x275/0x837
[ 26.756927] [<ffffffff81063f83>] ? mark_held_locks+0x52/0x70
[ 26.756931] [<ffffffff810af6be>] ? get_page_from_freelist+0x390/0x5ab
[ 26.756935] [<ffffffff81062e7b>] ? register_lock_class+0x1e/0x325
[ 26.756938] [<ffffffff8106421a>] ? trace_hardirqs_on+0xd/0xf
[ 26.756942] [<ffffffff810af742>] ? get_page_from_freelist+0x414/0x5ab
[ 26.756946] [<ffffffff81063d32>] ? mark_lock+0x2d/0x22c
[ 26.756949] [<ffffffff81063d32>] ? mark_lock+0x2d/0x22c
[ 26.756953] [<ffffffff81064fe7>] ? __lock_acquire+0x391/0xcfc
[ 26.756956] [<ffffffff810b006f>] ? __alloc_pages_nodemask+0x796/0x823
[ 26.756961] [<ffffffff810d4e60>] ? ____cache_alloc+0x10a/0x5cb
[ 26.756964] [<ffffffff810d4e60>] ? ____cache_alloc+0x10a/0x5cb
[ 26.756969] [<ffffffff8159f84c>] ? sub_preempt_count+0x35/0x49
[ 26.756973] [<ffffffff8112f637>] ...Color me confused. I cloned James Toy's git repository at
git://zen-kernel.org/kernel/mmotm.git, and gitk claims that I am on tag
2010-04-05-16-09, which matches the string above. But when I look at
fs/proc/array.c near line 241, I see:
238 static void collect_sigign_sigcatch(struct task_struct *p, sigset_t *ign,
239 sigset_t *catch)
240 {
241 struct k_sigaction *k;
242 int i;
243
244 k = p->sighand->action;
245 for (i = 1; i <= _NSIG; ++i, ++k) {
246 if (k->sa.sa_handler == SIG_IGN)
247 sigaddset(ign, i);
248 else if (k->sa.sa_handler != SIG_DFL)
249 sigaddset(catch, i);
250 }
251 }
It seems unlikely that line 241 generated the above error.
Am I on the wrong version? Or do the git tags not mean what I think
that they mean?
--
Andrew's -mm tree has 3 patches from Oleg Nesterov that hit that file, so the
code is different from what you show. Color *me* confused why your clone of
mmotm.git doesn't seem to contain them - I'm not sure how James Toy builds
that git tree. Perhaps the tag is applied before those patches are - the
'mm.patch' that updates the Makefile with the version is usually in the
*middle* of the 'series' file. What does HEAD of that tree look like?
My tree has:
/* needs ->siglock or rcu_read_lock() */
static void collect_sigign_sigcatch(struct task_struct *p, sigset_t *ign,
sigset_t *catch)
{
struct sighand_struct *sighand = rcu_dereference(p->sighand);
And that rcu_dereference() does it.
Oleg, looks like proc-make-collect_sigign_sigcatch-rcu-safe.patch is the
offender here, it added the line that causes the whinge.
Good point... The last commit is branch "master" and tagged 2010-04-05-16-09, but the commit line is "Linux 2.6.34-rc3", which seems If collect_sigign_sigcatch() is OK to call by updaters as well as readers, we need something like: struct sighand_struct *sighand; sighand = rcu_dereference_check(p->sighand, rcu_read_lock_held() || lockdep_is_held(&???)); Where the "???" is replaced with whichever of the two locks is protecting updates. My guess would be the sighand lock, but I would not rely on my guesses in this case. ;-) Thanx, Paul --
Yes, it should be p->sighand->siglock. Actually, I was going to change another caller, do_task_stat(), to call collect_sigign_sigcatch() without ->siglock too, but now I am not sure when/if this will happen. OK, thanks, I'll send the patch to make rcu_dereference_check() happy. While we are here... __exit_signal() does sighand = rcu_dereference_check(tsk->sighand, rcu_read_lock_held() || lockdep_tasklist_lock_is_held()); What is the point? We know that the single caller must hold tasklist, otherwise everything is broken. Perhaps it would be better to use rcu_dereference_raw() ? In fact, I don't really understand why __exit_signal() needs rcu_dereference() at all. Oleg. --