[PATCH v2] hwmon (coretemp): Fix build breakage if SMP is undefined

[PATCH v2] hwmon (coretemp): Fix build breakage if SMP is undefined

[Guenter Roeck]

Commit e40cc4bdfd4b89813f072f72bd9c7055814d3f0f introduced
a build breakage if CONFIG_SMP is undefined. This commit
fixes the problem.

Signed-off-by: Guenter Roeck <guenter.roeck@ericsson.com>
---
v2: Fix compile warning due to unused variable i if SMP is undefined.

 drivers/hwmon/coretemp.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/drivers/hwmon/coretemp.c b/drivers/hwmon/coretemp.c
index baa842a..138fe29 100644
--- a/drivers/hwmon/coretemp.c
+++ b/drivers/hwmon/coretemp.c
@@ -492,7 +492,9 @@ exit:
 static void coretemp_device_remove(unsigned int cpu)
 {
 	struct pdev_entry *p;
+#ifdef CONFIG_SMP
 	unsigned int i;
+#endif
 
 	mutex_lock(&pdev_list_mutex);
 	list_for_each_entry(p, &pdev_list, list) {
@@ -503,9 +505,11 @@ static void coretemp_device_remove(unsigned int cpu)
 		list_del(&p->list);
 		mutex_unlock(&pdev_list_mutex);
 		kfree(p);
+#ifdef CONFIG_SMP
 		for_each_cpu(i, cpu_sibling_mask(cpu))
 			if (i != cpu && !coretemp_device_add(i))
 				break;
+#endif
 		return;
 	}
 	mutex_unlock(&pdev_list_mutex);
-- 
1.7.0.87.g0901d

Re: [PATCH v2] hwmon (coretemp): Fix build breakage if SMP is undefined

[Ingo Molnar]

* Guenter Roeck <guenter.roeck@ericsson.com> wrote:

> +#ifdef CONFIG_SMP
> +#endif
> +#ifdef CONFIG_SMP
> +#endif

Hm, this tickles my uglo-meter. Is there no cleaner way, preferably one 
that doesnt involve preprocessor directives?

Thanks,

	Ingo

Re: [PATCH v2] hwmon (coretemp): Fix build breakage if SMP is undefined

[Pekka Enberg]

On Mon, Sep 27, 2010 at 3:59 PM, Ingo Molnar <mingo@elte.hu> wrote:
>
> * Guenter Roeck <guenter.roeck@ericsson.com> wrote:
>
>> +#ifdef CONFIG_SMP
>> +#endif
>> +#ifdef CONFIG_SMP
>> +#endif
>
> Hm, this tickles my uglo-meter. Is there no cleaner way, preferably one
> that doesnt involve preprocessor directives?

Implement cpu_sibling_mask() on UP so that the loop goes away?

Re: [PATCH v2] hwmon (coretemp): Fix build breakage if SMP is undefined

[Guenter Roeck]

On Mon, Sep 27, 2010 at 09:02:57AM -0400, Pekka Enberg wrote:
> On Mon, Sep 27, 2010 at 3:59 PM, Ingo Molnar <mingo@elte.hu> wrote:
> >
> > * Guenter Roeck <guenter.roeck@ericsson.com> wrote:
> >
> >> +#ifdef CONFIG_SMP
> >> +#endif
> >> +#ifdef CONFIG_SMP
> >> +#endif
> >
> > Hm, this tickles my uglo-meter. Is there no cleaner way, preferably one
> > that doesnt involve preprocessor directives?
> 
> Implement cpu_sibling_mask() on UP so that the loop goes away?

So what is the take ? Looks like Linus won't accept my patch without someone 
else signing off on it. If the uglo-meter prevents it from being accepted,
I'll be happy to submit the SMP cleanup patch instead. As I mentioned
before, I would prefer that to go into -next.

Guenter

Re: [PATCH v2] hwmon (coretemp): Fix build breakage if SMP is undefined

[Linus Torvalds]

[-- Attachment #1: Type: text/plain, Size: 1357 bytes --]

On Mon, Sep 27, 2010 at 4:27 PM, Guenter Roeck
<guenter.roeck@ericsson.com> wrote:
> On Mon, Sep 27, 2010 at 09:02:57AM -0400, Pekka Enberg wrote:
>> On Mon, Sep 27, 2010 at 3:59 PM, Ingo Molnar <mingo@elte.hu> wrote:
>> >
>> > * Guenter Roeck <guenter.roeck@ericsson.com> wrote:
>> >
>> >> +#ifdef CONFIG_SMP
>> >> +#endif
>> >> +#ifdef CONFIG_SMP
>> >> +#endif
>> >
>> > Hm, this tickles my uglo-meter. Is there no cleaner way, preferably one
>> > that doesnt involve preprocessor directives?
>>
>> Implement cpu_sibling_mask() on UP so that the loop goes away?
>
> So what is the take ? Looks like Linus won't accept my patch without someone
> else signing off on it. If the uglo-meter prevents it from being accepted,
> I'll be happy to submit the SMP cleanup patch instead. As I mentioned
> before, I would prefer that to go into -next.

I'd _much_ rather see cpu_sibling_mask() on UP, and just have the loop go away.

But that would be a generic change. Something like the (UNTESTED!)
attached. It returns a NULL, since it would always be a bug to
actually _use_ the (nonexistent) mask. And that's fine for things like
for_each_cpu() that will then happily ignore the mask.

Ingo, does this make those randconfig things work? I think it's
prettier than the horrible "sprinkle #ifdef CONFIG_SMP around in
random places".

                        Linus

[-- Attachment #2: patch.diff --]
[-- Type: text/x-patch, Size: 483 bytes --]

 include/linux/smp.h |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/include/linux/smp.h b/include/linux/smp.h
index cfa2d20..ad48077 100644
--- a/include/linux/smp.h
+++ b/include/linux/smp.h
@@ -149,6 +149,8 @@ smp_call_function_any(const struct cpumask *mask, void (*func)(void *info),
 	return smp_call_function_single(0, func, info, wait);
 }
 
+static inline const struct cpumask *cpu_sibling_mask(int cpu) { return NULL; }
+
 #endif /* !SMP */
 
 /*

Re: [PATCH v2] hwmon (coretemp): Fix build breakage if SMP is undefined

[Guenter Roeck]

On Mon, Sep 27, 2010 at 07:44:01PM -0400, Linus Torvalds wrote:
> On Mon, Sep 27, 2010 at 4:27 PM, Guenter Roeck
> <guenter.roeck@ericsson.com> wrote:
> > On Mon, Sep 27, 2010 at 09:02:57AM -0400, Pekka Enberg wrote:
> >> On Mon, Sep 27, 2010 at 3:59 PM, Ingo Molnar <mingo@elte.hu> wrote:
> >> >
> >> > * Guenter Roeck <guenter.roeck@ericsson.com> wrote:
> >> >
> >> >> +#ifdef CONFIG_SMP
> >> >> +#endif
> >> >> +#ifdef CONFIG_SMP
> >> >> +#endif
> >> >
> >> > Hm, this tickles my uglo-meter. Is there no cleaner way, preferably one
> >> > that doesnt involve preprocessor directives?
> >>
> >> Implement cpu_sibling_mask() on UP so that the loop goes away?
> >
> > So what is the take ? Looks like Linus won't accept my patch without someone
> > else signing off on it. If the uglo-meter prevents it from being accepted,
> > I'll be happy to submit the SMP cleanup patch instead. As I mentioned
> > before, I would prefer that to go into -next.
> 
> I'd _much_ rather see cpu_sibling_mask() on UP, and just have the loop go away.
> 
> But that would be a generic change. Something like the (UNTESTED!)

Which is why I didn't do it...

> attached. It returns a NULL, since it would always be a bug to
> actually _use_ the (nonexistent) mask. And that's fine for things like
> for_each_cpu() that will then happily ignore the mask.
> 
> Ingo, does this make those randconfig things work? I think it's
> prettier than the horrible "sprinkle #ifdef CONFIG_SMP around in
> random places".
> 
>                         Linus

>  include/linux/smp.h |    2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)
> 
> diff --git a/include/linux/smp.h b/include/linux/smp.h
> index cfa2d20..ad48077 100644
> --- a/include/linux/smp.h
> +++ b/include/linux/smp.h
> @@ -149,6 +149,8 @@ smp_call_function_any(const struct cpumask *mask, void (*func)(void *info),
>  	return smp_call_function_single(0, func, info, wait);
>  }
>  
> +static inline const struct cpumask *cpu_sibling_mask(int cpu) { return NULL; }
> +
>  #endif /* !SMP */
>  
That works. Every other use of cpu_sibling_mask() is either in smp code
or surrounded with #ifdef CONFIG_SMP.

Ok, I'll submit another version of the patch with the generic change after 
some more testing.

Guenter

Re: [PATCH v2] hwmon (coretemp): Fix build breakage if SMP is undefined

[Guenter Roeck]

On Mon, Sep 27, 2010 at 07:44:01PM -0400, Linus Torvalds wrote:
[...]

>  include/linux/smp.h |    2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)
> 
> diff --git a/include/linux/smp.h b/include/linux/smp.h
> index cfa2d20..ad48077 100644
> --- a/include/linux/smp.h
> +++ b/include/linux/smp.h
> @@ -149,6 +149,8 @@ smp_call_function_any(const struct cpumask *mask, void (*func)(void *info),
>  	return smp_call_function_single(0, func, info, wait);
>  }
>  
> +static inline const struct cpumask *cpu_sibling_mask(int cpu) { return NULL; }
> +
>  #endif /* !SMP */
>  
Not that simple. cpu_sibling_mask()  is defined in asm/smp.h, which is only
included from linux/smp.h if SMP is defined. But many other files do include
asm/smp.h directly. This causes the following error all over the place
if CONFIG_SMP is not defined.

In file included from /opt/scratch/groeck/linux-staging/arch/x86/include/asm/apic.h:473,
                 from arch/x86/xen/enlighten.c:46:
/opt/scratch/groeck/linux-staging/arch/x86/include/asm/smp.h:29: error: conflicting types for cpu_sibling_mask
include/linux/smp.h:152: note: previous definition of cpu_sibling_mask was here

Guenter

Re: [PATCH v2] hwmon (coretemp): Fix build breakage if SMP is undefined

[Linus Torvalds]

On Mon, Sep 27, 2010 at 5:26 PM, Guenter Roeck
<guenter.roeck@ericsson.com> wrote:
>
> Not that simple. cpu_sibling_mask()  is defined in asm/smp.h, which is only
> included from linux/smp.h if SMP is defined. But many other files do include
> asm/smp.h directly. This causes the following error all over the place
> if CONFIG_SMP is not defined.

Yeah, that's broken.  Seriously broken.

And I guess that if you had happened to include <asm/smp.h> in
coretemp.c you would magically have gotten that cpu_sibling_map()
thing, and it would just work - by mistake.

And maybe that's the correct (and really hacky) fix right now. Rather
than introduce an UP-only cpu_sibling_mask(), get the SMP version, and
get it ignored.

Seriously broken, but there it is.

In the long run, I guess we should either
 - disallow naked '<asm/smp.h>' includes
OR
 - just make '<linux/smp.h>' unconditionally include <asm/smp.h>

to at least not have that insane "some things exist in CONFIG_SMP or
not depending on how you include files".

                           Linus

Re: [PATCH v2] hwmon (coretemp): Fix build breakage if SMP is undefined

[Guenter Roeck]

On Mon, Sep 27, 2010 at 09:00:59PM -0400, Linus Torvalds wrote:
> On Mon, Sep 27, 2010 at 5:26 PM, Guenter Roeck
> <guenter.roeck@ericsson.com> wrote:
> >
> > Not that simple. cpu_sibling_mask()  is defined in asm/smp.h, which is only
> > included from linux/smp.h if SMP is defined. But many other files do include
> > asm/smp.h directly. This causes the following error all over the place
> > if CONFIG_SMP is not defined.
> 
> Yeah, that's broken.  Seriously broken.
> 
> And I guess that if you had happened to include <asm/smp.h> in
> coretemp.c you would magically have gotten that cpu_sibling_map()
> thing, and it would just work - by mistake.
> 
Figured.

> And maybe that's the correct (and really hacky) fix right now. Rather

Yes, that is actually what I am testing right now.

> than introduce an UP-only cpu_sibling_mask(), get the SMP version, and
> get it ignored.
> 
> Seriously broken, but there it is.
> 
> In the long run, I guess we should either
>  - disallow naked '<asm/smp.h>' includes
> OR
>  - just make '<linux/smp.h>' unconditionally include <asm/smp.h>
> 
> to at least not have that insane "some things exist in CONFIG_SMP or
> not depending on how you include files".
>

Guess so.

You should get another patch in a few minutes, after my next set of compiles is done.
I'll wait for some feedback before pushing it into my integration branch.

Guenter

Re: [PATCH v2] hwmon (coretemp): Fix build breakage if SMP is undefined

[Guenter Roeck]

On Mon, Sep 27, 2010 at 08:59:46AM -0400, Ingo Molnar wrote:
> 
> * Guenter Roeck <guenter.roeck@ericsson.com> wrote:
> 
> > +#ifdef CONFIG_SMP
> > +#endif
> > +#ifdef CONFIG_SMP
> > +#endif
> 
> Hm, this tickles my uglo-meter. Is there no cleaner way, preferably one 
> that doesnt involve preprocessor directives?
> 
Mine too, but not at 6am in the morning (actually 5am when I wrote it),
and right now I thought it more important to fix the kernel breakage.

I'll think about it and see if I can find something better. But I would
still prefer to have this one applied and submit a cleaner solution later on
(if I find one). Preferrably, as you suggested, without any CONFIG_SMP
declarations.

Fwiw, there are several similar "#ifdef CONFIG_SMP" in this driver already,
so it would definitely be good to find a cleaner solution.

Thanks,
Guenter

Re: [PATCH v2] hwmon (coretemp): Fix build breakage if SMP is undefined

[Guenter Roeck]

On Mon, Sep 27, 2010 at 08:59:46AM -0400, Ingo Molnar wrote:
> 
> * Guenter Roeck <guenter.roeck@ericsson.com> wrote:
> 
> > +#ifdef CONFIG_SMP
> > +#endif
> > +#ifdef CONFIG_SMP
> > +#endif
> 
> Hm, this tickles my uglo-meter. Is there no cleaner way, preferably one 
> that doesnt involve preprocessor directives?
> 
After looking through the code I thought about a much cleaner fix for
handling the SMP defines in coretemp.c and pkgtemp.c. Essentially
it will move all SMP dependencies into a single #ifdef, and also
optimize the loop in question with cpumask_first() / cpumask_next().
Maybe I can get rid of some of the SMP dependencies entirely. However,
the driver also uses phys_proc_id and cpu_core_id from cpuinfo_x86,
and those are only available if SMP is defined.

That would be way too invasive for .36, though. I'll stick with the
current patch and submit a cleanup for .37. That also fits well with
the other pending cleanups for the two drivers.

Thanks,
Guenter