Re: -tip: origin tree build failure

[Ingo Molnar]

* Andi Kleen <andi@xxxxxxxxxxxxxx> wrote:

> Ingo Molnar <mingo@xxxxxxx> writes:
> 
> > * Ingo Molnar <mingo@xxxxxxx> wrote:
> >
> >>  config MEMORY_FAILURE
> >>  	depends on MMU
> >>  	depends on ARCH_SUPPORTS_MEMORY_FAILURE
> >> +	select PROC_PAGE_MONITOR
> >>  	bool "Enable recovery from hardware memory errors"
> >
> > It also needs to depend on PROC_FS in that case - as per the updated patch 
> > below.
> 
> Thanks for the report.
> 
> MEMORY_FAILURE itself doesn't depend on the page flags, just the injector 
> which has a separate config option. It already has a select 
> PROC_PAGE_MONITOR, but the proc dependency is indeed missing.
> 
> I think the correct fix is the appended patch.

No, your patch is not the correct fix. As i said, PROC_PAGE_MONITOR provides 
the facility and mm/memory-failure.c uses that facility - and your patch does 
not solve that fundamental dependency issue, it just fudges around the 
dependencies.

So your patch can still produce the same kind of build failure (with other 
Kconfig variations):

 mm/built-in.o: In function `hwpoison_filter':
 (.text+0x430ba): undefined reference to `stable_page_flags'

This should be cleaned up for real - there's no reason why stable_page_flags() 
(which is an MM facility) should live in fs/proc/. That's the root of the 
problem.

Also a few more (very small) nits:

> HWPOISON: Add PROC_FS dependency to hwpoison injector

It would be really useful if hwpoison commit titles were in lower case, like 
most other commits in the kernel do? Upper-case is used in special cases - IMO 
this case does not qualify.

> The injector filter requires stable_page_flags() which is supplied by 
> procfs. So make it dependent on that.
> 
> Reported by Ingo Molnar

FYI, we have a Reported-by tag that can be (and should be) used in such cases.

Thanks,

	Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/