[PATCH] x86/hwmon: conditionalize coretemp's dependency on PCI

[PATCH] x86/hwmon: conditionalize coretemp's dependency on PCI

[Jan Beulich]

PCI specific code is needed only when Atom CPUs are potentially
supported by the kernel.

Signed-off-by: Jan Beulich <jbeulich@novell.com>
Cc: Rudolf Marek <r.marek@assembler.cz>

---
 drivers/hwmon/Kconfig |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

--- linux-2.6.36-rc4/drivers/hwmon/Kconfig	2010-09-13 08:45:02.000000000 +0200
+++ 2.6.36-rc4-x86-coretemp-maybe-pci/drivers/hwmon/Kconfig	2010-09-10 16:24:16.000000000 +0200
@@ -401,7 +401,8 @@ config SENSORS_GL520SM
 
 config SENSORS_CORETEMP
 	tristate "Intel Core/Core2/Atom temperature sensor"
-	depends on X86 && PCI && EXPERIMENTAL
+	depends on X86 && EXPERIMENTAL
+	depends on PCI || (!MATOM && !GENERIC_CPU && !X86_GENERIC)
 	help
 	  If you say yes here you get support for the temperature
 	  sensor inside your CPU. Most of the family 6 CPUs

Re: x86/hwmon: conditionalize coretemp's dependency on PCI

[Guenter Roeck]

On Mon, Sep 13, 2010 at 10:26:33AM -0000, Jan Beulich wrote:
> PCI specific code is needed only when Atom CPUs are potentially
> supported by the kernel.
> 
> Signed-off-by: Jan Beulich <jbeulich@novell.com>
> Cc: Rudolf Marek <r.marek@assembler.cz>
> 
> ---
> drivers/hwmon/Kconfig |    3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> 
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 
> --- linux-2.6.36-rc4/drivers/hwmon/Kconfig	2010-09-13 08:45:02.000000000 +0200
> +++ 2.6.36-rc4-x86-coretemp-maybe-pci/drivers/hwmon/Kconfig	2010-09-10 16:24:16.000000000 +0200
> @@ -401,7 +401,8 @@ config SENSORS_GL520SM
>  
>  config SENSORS_CORETEMP
>  	tristate "Intel Core/Core2/Atom temperature sensor"
> -	depends on X86 && PCI && EXPERIMENTAL
> +	depends on X86 && EXPERIMENTAL
> +	depends on PCI || (!MATOM && !GENERIC_CPU && !X86_GENERIC)
>  	help
>  	  If you say yes here you get support for the temperature
>  	  sensor inside your CPU. Most of the family 6 CPUs

Resending my reply to this one as well. Again, apologies if there is duplication.

The coretemp code unconditionally calls pci functions, even if PCI is not defined.
I am concerned that this might cause problems. It might be better to stick with
the more generic dependency instead of trying to optimize too much.

Guenter

Re: x86/hwmon: conditionalize coretemp's dependency on PCI

[Jan Beulich]

>>> On 24.09.10 at 20:55, Guenter Roeck <guenter.roeck@ericsson.com> wrote:
> On Mon, Sep 13, 2010 at 10:26:33AM -0000, Jan Beulich wrote:
>> PCI specific code is needed only when Atom CPUs are potentially
>> supported by the kernel.
>> 
>> Signed-off-by: Jan Beulich <jbeulich@novell.com>
>> Cc: Rudolf Marek <r.marek@assembler.cz>
>> 
>> ---
>> drivers/hwmon/Kconfig |    3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>> 
>> 
>> 
>> 
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>> the body of a message to majordomo@vger.kernel.org 
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html 
>> Please read the FAQ at  http://www.tux.org/lkml/ 
>> 
>> --- linux-2.6.36-rc4/drivers/hwmon/Kconfig	2010-09-13 08:45:02.000000000 +0200
>> +++ 2.6.36-rc4-x86-coretemp-maybe-pci/drivers/hwmon/Kconfig	2010-09-10 
> 16:24:16.000000000 +0200
>> @@ -401,7 +401,8 @@ config SENSORS_GL520SM
>>  
>>  config SENSORS_CORETEMP
>>  	tristate "Intel Core/Core2/Atom temperature sensor"
>> -	depends on X86 && PCI && EXPERIMENTAL
>> +	depends on X86 && EXPERIMENTAL
>> +	depends on PCI || (!MATOM && !GENERIC_CPU && !X86_GENERIC)
>>  	help
>>  	  If you say yes here you get support for the temperature
>>  	  sensor inside your CPU. Most of the family 6 CPUs
> 
> Resending my reply to this one as well. Again, apologies if there is 
> duplication.
> 
> The coretemp code unconditionally calls pci functions, even if PCI is not 
> defined.
> I am concerned that this might cause problems. It might be better to stick 
> with
> the more generic dependency instead of trying to optimize too much.

pci.h takes care to define stub inline functions for the !CONFIG_PCI
case. It seemed largely odd for a driver like this to depend on PCI
at all, and hence I think it is more transparent to make the needs
explicit.

Jan

Re: x86/hwmon: conditionalize coretemp's dependency on PCI

[Guenter Roeck]

On Mon, Sep 27, 2010 at 03:08:50AM -0400, Jan Beulich wrote:
[...]

> >>  
> >>  config SENSORS_CORETEMP
> >>  	tristate "Intel Core/Core2/Atom temperature sensor"
> >> -	depends on X86 && PCI && EXPERIMENTAL
> >> +	depends on X86 && EXPERIMENTAL
> >> +	depends on PCI || (!MATOM && !GENERIC_CPU && !X86_GENERIC)
> >>  	help
> >>  	  If you say yes here you get support for the temperature
> >>  	  sensor inside your CPU. Most of the family 6 CPUs
> > 
> > Resending my reply to this one as well. Again, apologies if there is 
> > duplication.
> > 
> > The coretemp code unconditionally calls pci functions, even if PCI is not 
> > defined.
> > I am concerned that this might cause problems. It might be better to stick 
> > with
> > the more generic dependency instead of trying to optimize too much.
> 
> pci.h takes care to define stub inline functions for the !CONFIG_PCI
> case. It seemed largely odd for a driver like this to depend on PCI
> at all, and hence I think it is more transparent to make the needs
> explicit.
> 
Seems to me the dependency should not exist in the first place, then.
Otherwise, the driver would still be disabled for GENERIC_CPU, which isn't
good either.

Are there examples of other drivers which are not defining the PCI dependency
but are conditionally calling pci functions ?

Guenter

Re: x86/hwmon: conditionalize coretemp's dependency on PCI

[Jan Beulich]

>>> On 27.09.10 at 14:16, Guenter Roeck <guenter.roeck@ericsson.com> wrote:
> On Mon, Sep 27, 2010 at 03:08:50AM -0400, Jan Beulich wrote:
> [...]
> 
>> >>  
>> >>  config SENSORS_CORETEMP
>> >>  	tristate "Intel Core/Core2/Atom temperature sensor"
>> >> -	depends on X86 && PCI && EXPERIMENTAL
>> >> +	depends on X86 && EXPERIMENTAL
>> >> +	depends on PCI || (!MATOM && !GENERIC_CPU && !X86_GENERIC)
>> >>  	help
>> >>  	  If you say yes here you get support for the temperature
>> >>  	  sensor inside your CPU. Most of the family 6 CPUs
>> > 
>> > Resending my reply to this one as well. Again, apologies if there is 
>> > duplication.
>> > 
>> > The coretemp code unconditionally calls pci functions, even if PCI is not 
>> > defined.
>> > I am concerned that this might cause problems. It might be better to stick 
>> > with
>> > the more generic dependency instead of trying to optimize too much.
>> 
>> pci.h takes care to define stub inline functions for the !CONFIG_PCI
>> case. It seemed largely odd for a driver like this to depend on PCI
>> at all, and hence I think it is more transparent to make the needs
>> explicit.
>> 
> Seems to me the dependency should not exist in the first place, then.
> Otherwise, the driver would still be disabled for GENERIC_CPU, which isn't
> good either.

Oh, not having a dependency on PCI at all would be even better.
I didn't dare to suggest that.

> Are there examples of other drivers which are not defining the PCI 
> dependency
> but are conditionally calling pci functions ?

I'm not aware of any, but also didn't explicitly look for such.

Jan

Re: x86/hwmon: conditionalize coretemp's dependency on PCI

[Guenter Roeck]

On Mon, Sep 27, 2010 at 08:46:54AM -0400, Jan Beulich wrote:
> >>> On 27.09.10 at 14:16, Guenter Roeck <guenter.roeck@ericsson.com> wrote:
> > On Mon, Sep 27, 2010 at 03:08:50AM -0400, Jan Beulich wrote:
> > [...]
> > 
> >> >>  
> >> >>  config SENSORS_CORETEMP
> >> >>  	tristate "Intel Core/Core2/Atom temperature sensor"
> >> >> -	depends on X86 && PCI && EXPERIMENTAL
> >> >> +	depends on X86 && EXPERIMENTAL
> >> >> +	depends on PCI || (!MATOM && !GENERIC_CPU && !X86_GENERIC)
> >> >>  	help
> >> >>  	  If you say yes here you get support for the temperature
> >> >>  	  sensor inside your CPU. Most of the family 6 CPUs
> >> > 
> >> > Resending my reply to this one as well. Again, apologies if there is 
> >> > duplication.
> >> > 
> >> > The coretemp code unconditionally calls pci functions, even if PCI is not 
> >> > defined.
> >> > I am concerned that this might cause problems. It might be better to stick 
> >> > with
> >> > the more generic dependency instead of trying to optimize too much.
> >> 
> >> pci.h takes care to define stub inline functions for the !CONFIG_PCI
> >> case. It seemed largely odd for a driver like this to depend on PCI
> >> at all, and hence I think it is more transparent to make the needs
> >> explicit.
> >> 
> > Seems to me the dependency should not exist in the first place, then.
> > Otherwise, the driver would still be disabled for GENERIC_CPU, which isn't
> > good either.
> 
> Oh, not having a dependency on PCI at all would be even better.
> I didn't dare to suggest that.
> 
> > Are there examples of other drivers which are not defining the PCI 
> > dependency
> > but are conditionally calling pci functions ?
> 
> I'm not aware of any, but also didn't explicitly look for such.
> 
It still compiles if I remove the PCI dependency and disable PCI. So that is one plus.

That whole dependency is just to get the host bridge ID which is then used
to determine tjmax. Personally I would prefer to have a wrong tjmax over not having 
coretemp at all, and the means to determine tjmax through the bridge ID is kind
of bogus anyway. So at least in this case it would make sense for me to remove
the dependency completely. Actually, we won't even change functionality,
since tjmax is only set to a higher value for specific bridge IDs.

So, if you don't mind, please re-submit with the PCI dependency removed entirely.
Unless someone starts screaming, I'll apply that to my -next tree.

Thanks,
Guenter

Re: x86/hwmon: conditionalize coretemp's dependency on PCI

[Jan Beulich]

>>> On 27.09.10 at 15:02, Guenter Roeck <guenter.roeck@ericsson.com> wrote:
> On Mon, Sep 27, 2010 at 08:46:54AM -0400, Jan Beulich wrote:
>> >>> On 27.09.10 at 14:16, Guenter Roeck <guenter.roeck@ericsson.com> wrote:
>> > On Mon, Sep 27, 2010 at 03:08:50AM -0400, Jan Beulich wrote:
>> > [...]
>> > 
>> >> >>  
>> >> >>  config SENSORS_CORETEMP
>> >> >>  	tristate "Intel Core/Core2/Atom temperature sensor"
>> >> >> -	depends on X86 && PCI && EXPERIMENTAL
>> >> >> +	depends on X86 && EXPERIMENTAL
>> >> >> +	depends on PCI || (!MATOM && !GENERIC_CPU && !X86_GENERIC)
>> >> >>  	help
>> >> >>  	  If you say yes here you get support for the temperature
>> >> >>  	  sensor inside your CPU. Most of the family 6 CPUs
>> >> > 
>> >> > Resending my reply to this one as well. Again, apologies if there is 
>> >> > duplication.
>> >> > 
>> >> > The coretemp code unconditionally calls pci functions, even if PCI is not 
>> >> > defined.
>> >> > I am concerned that this might cause problems. It might be better to stick 
> 
>> >> > with
>> >> > the more generic dependency instead of trying to optimize too much.
>> >> 
>> >> pci.h takes care to define stub inline functions for the !CONFIG_PCI
>> >> case. It seemed largely odd for a driver like this to depend on PCI
>> >> at all, and hence I think it is more transparent to make the needs
>> >> explicit.
>> >> 
>> > Seems to me the dependency should not exist in the first place, then.
>> > Otherwise, the driver would still be disabled for GENERIC_CPU, which isn't
>> > good either.
>> 
>> Oh, not having a dependency on PCI at all would be even better.
>> I didn't dare to suggest that.
>> 
>> > Are there examples of other drivers which are not defining the PCI 
>> > dependency
>> > but are conditionally calling pci functions ?
>> 
>> I'm not aware of any, but also didn't explicitly look for such.
>> 
> It still compiles if I remove the PCI dependency and disable PCI. So that is 
> one plus.
> 
> That whole dependency is just to get the host bridge ID which is then used
> to determine tjmax. Personally I would prefer to have a wrong tjmax over not 
> having 
> coretemp at all, and the means to determine tjmax through the bridge ID is 
> kind
> of bogus anyway. So at least in this case it would make sense for me to 
> remove
> the dependency completely. Actually, we won't even change functionality,
> since tjmax is only set to a higher value for specific bridge IDs.
> 
> So, if you don't mind, please re-submit with the PCI dependency removed 
> entirely.

Hmm, no, I'd rather not - I just can't tell what implications it has
if you run on !PCI && MATOM, i.e. I wouldn't be able to really justify
that change other than by second hand knowledge (from what you
wrote above). If you're convinced the dropped dependency doesn't
make things worse, you're much better positioned to craft a respective
patch.

> Unless someone starts screaming, I'll apply that to my -next tree.

Jan

Re: x86/hwmon: conditionalize coretemp's dependency on PCI

[Jean Delvare]

Hi Guenter,

On Mon, 27 Sep 2010 06:02:20 -0700, Guenter Roeck wrote:
> On Mon, Sep 27, 2010 at 08:46:54AM -0400, Jan Beulich wrote:
> > Guenter Roeck <guenter.roeck@ericsson.com> wrote:
> > > Seems to me the dependency should not exist in the first place, then.
> > > Otherwise, the driver would still be disabled for GENERIC_CPU, which isn't
> > > good either.
> > 
> > Oh, not having a dependency on PCI at all would be even better.
> > I didn't dare to suggest that.
> > 
> > > Are there examples of other drivers which are not defining the PCI 
> > > dependency
> > > but are conditionally calling pci functions ?
> > 
> > I'm not aware of any, but also didn't explicitly look for such.
>
> It still compiles if I remove the PCI dependency and disable PCI. So that is one plus.
> 
> That whole dependency is just to get the host bridge ID which is then used
> to determine tjmax. Personally I would prefer to have a wrong tjmax over not having 
> coretemp at all,

No! Reporting broken monitoring values is worse than not returning any
value at all. The coretemp (and k8/k10temp) driver(s) already have a
very bad reputation for presenting relative temperature readings as
absolute ones, please let's not add to it.

At this point there are 2 things which can be done:
* If building a kernel for the Atom platform without PCI support is
  something relatively common, which makes sense, then we should look
  for an alternative way to determine the TjMax value for these CPUs,
  which doesn't require PCI support.
* If building a kernel for the Atom platform without PCI support is
  something rare, which makes little sense, then we can simply either
  prevent the driver from building at all in this case, or have it
  print a big fat warning (with suggestion to rebuild the kernel with
  PCI support) when it can't determine the TjMax value.

> and the means to determine tjmax through the bridge ID is kind
> of bogus anyway.

Do you mean it is strange from a technical perspective, or do you have
evidences that it doesn't work properly? This trick come from Intel
themselves, I would guess they know their business.

> So at least in this case it would make sense for me to remove
> the dependency completely. Actually, we won't even change functionality,
> since tjmax is only set to a higher value for specific bridge IDs.

Higher or lower doesn't make a difference. As long as the coretemp
driver doesn't properly report the temperature values as being
relative, users don't expect the value to change depending on the
kernel version or configuration options. We have had dozens of user
reports because of this.

-- 
Jean Delvare

Re: x86/hwmon: conditionalize coretemp's dependency on PCI

[Guenter Roeck]

On Tue, Sep 28, 2010 at 03:17:59AM -0400, Jean Delvare wrote:
> Hi Guenter,
> 
> On Mon, 27 Sep 2010 06:02:20 -0700, Guenter Roeck wrote:
> > On Mon, Sep 27, 2010 at 08:46:54AM -0400, Jan Beulich wrote:
> > > Guenter Roeck <guenter.roeck@ericsson.com> wrote:
> > > > Seems to me the dependency should not exist in the first place, then.
> > > > Otherwise, the driver would still be disabled for GENERIC_CPU, which isn't
> > > > good either.
> > > 
> > > Oh, not having a dependency on PCI at all would be even better.
> > > I didn't dare to suggest that.
> > > 
> > > > Are there examples of other drivers which are not defining the PCI 
> > > > dependency
> > > > but are conditionally calling pci functions ?
> > > 
> > > I'm not aware of any, but also didn't explicitly look for such.
> >
> > It still compiles if I remove the PCI dependency and disable PCI. So that is one plus.
> > 
> > That whole dependency is just to get the host bridge ID which is then used
> > to determine tjmax. Personally I would prefer to have a wrong tjmax over not having 
> > coretemp at all,
> 
> No! Reporting broken monitoring values is worse than not returning any
> value at all. The coretemp (and k8/k10temp) driver(s) already have a
> very bad reputation for presenting relative temperature readings as
> absolute ones, please let's not add to it.
> 
Point taken.

> At this point there are 2 things which can be done:
> * If building a kernel for the Atom platform without PCI support is
>   something relatively common, which makes sense, then we should look
>   for an alternative way to determine the TjMax value for these CPUs,
>   which doesn't require PCI support.
> * If building a kernel for the Atom platform without PCI support is
>   something rare, which makes little sense, then we can simply either
>   prevent the driver from building at all in this case, or have it
>   print a big fat warning (with suggestion to rebuild the kernel with
>   PCI support) when it can't determine the TjMax value.
> 

I would prefer the first of those. Second might be an option too
if that is not possible.

> > and the means to determine tjmax through the bridge ID is kind
> > of bogus anyway.
> 
> Do you mean it is strange from a technical perspective, or do you have
> evidences that it doesn't work properly? This trick come from Intel
> themselves, I would guess they know their business.
> 
>From a technical perspective. Hard to see what a PCI bridge ID has to do with Tjmax.

> > So at least in this case it would make sense for me to remove
> > the dependency completely. Actually, we won't even change functionality,
> > since tjmax is only set to a higher value for specific bridge IDs.
> 
> Higher or lower doesn't make a difference. As long as the coretemp
> driver doesn't properly report the temperature values as being
> relative, users don't expect the value to change depending on the
> kernel version or configuration options. We have had dozens of user
> reports because of this.
> 
You are right, functionality would change if someone runs a kernel with PCI undefined 
on the specific systems which do use the PCI bridge ID to determine Tjmax. So 
if there are no other options, maybe the big fat warning in that case would make sense.
I would definitely prefer that over disabling coretemp entirely just because it _might_
possibly report a wrong Tjmax (which it doees anyway for many CPUs).

Guenter

Re: x86/hwmon: conditionalize coretemp's dependency on PCI

[Jean Delvare]

On Tue, 28 Sep 2010 05:00:00 -0700, Guenter Roeck wrote:
> On Tue, Sep 28, 2010 at 03:17:59AM -0400, Jean Delvare wrote:
> > Do you mean it is strange from a technical perspective, or do you have
> > evidences that it doesn't work properly? This trick come from Intel
> > themselves, I would guess they know their business.
>
> From a technical perspective. Hard to see what a PCI bridge ID has to do with Tjmax.

I agree. If you search the archives, you'll see I emitted exactly the
same complaint back then.

> > (...)
> > Higher or lower doesn't make a difference. As long as the coretemp
> > driver doesn't properly report the temperature values as being
> > relative, users don't expect the value to change depending on the
> > kernel version or configuration options. We have had dozens of user
> > reports because of this.
> > 
> You are right, functionality would change if someone runs a kernel with PCI undefined 
> on the specific systems which do use the PCI bridge ID to determine Tjmax. So 
> if there are no other options, maybe the big fat warning in that case would make sense.
> I would definitely prefer that over disabling coretemp entirely just because it _might_
> possibly report a wrong Tjmax (which it doees anyway for many CPUs).

I fully agree.

-- 
Jean Delvare