hi dominik,

thanks alot for your detailed reply and patience with a newbie. i have 
attached 2 patches w.r.t. 2.6.7-rc3 that should adress your suggestions.

i also wonder about one thing, maybe you can help me on that. with the 
old speedstep driver, i would have occational hard freezups of my 
machine. this seems to be gone (just experimental evidence, since i 
didn't have one in a few days now where before they occured about 
hourly). so my guess is that something in this patch must have corrected 
it. since my machine just has one physical cpu, i really don't 
understand what went wrong before.  do you have any idea on that?

Dominik Brodowski wrote:

>Hi Chris,
>
>  
>
<snip>

> Even more, under normal circumstances [i.e. idling works,
>which it does on most systems], I see no _technical_ reason to do
>throttling, or even non-temperature related _dynamic throttling_ like other
>OSes do it. For those new to this list, there's some maths in the archives
>which proves that you need more energy == battery power if you do
>throttling for any given task... So it's something we need to discuss 
>during 2.7.
>  
>
 thanks, i didn't know that

>Now, about your patch 1):
>  
>
<snip>

>So I'd prefer, due to the nastiness of the CPUID in the specific 0x0F29 case,
>to move the strstr check to this specific case -- e.g. have
>
>			if (ebx == 0x0e)
>				return SPEEDSTEP_PROCESSOR_P4M;
>			if (strstr(c->x86_model_id,"Mobile Intel(R) Pentium(R) 4") != NULL)
>				return SPEEDSTEP_PROCESSOR_P4M;
>  
>
 > Could you update your patch accordingly, please?
 
thanks, done it. i was somehow anticipating that since reading the 
version string feels like a cheat.

>Now, about your patch 2) [slightly re-ordered]:
>
>  
>
>>-unsigned int speedstep_get_freqs(unsigned int processor,
>>+unsigned int speedstep_get_freqs(unsigned int cpu,
>>+				  unsigned int processor,
>> 				  unsigned int *low_speed,
>> 				  unsigned int *high_speed,
>>-				  void (*set_state) (unsigned int state,
>>+				  void (*set_state) (unsigned int cpu,
>>+						     unsigned int state,
>> 						     unsigned int notify)
>>    
>>
>
>This will break speedstep-smi, as it uses speedstep_get_freqs too...
>
>  
>
ok, i chopped out the notify and cpu switching part from speedstep_set_state

>>+	/* switch to physical CPU where state is to be changed*/
>>+	cpus_allowed = current->cpus_allowed;
>>+
>>+	/* only run on CPU to be set, or on its sibling */
>>+       affected_cpu_map = cpumask_of_cpu(cpu);
>>+#ifdef CONFIG_X86_HT
>>+	hyperthreading = ((cpu_has_ht) && (smp_num_siblings == 2));
>>+	if (hyperthreading) {
>>+		sibling = cpu_sibling_map[cpu];
>>+                cpu_set(sibling, affected_cpu_map);
>>+	}
>>+#endif
>>+	set_cpus_allowed(current, affected_cpu_map);
>>+        BUG_ON(!cpu_isset(smp_processor_id(), affected_cpu_map));
>>    
>>
>
>Due to changes to cpu_sibling_map [it's a cpumask_t now], this won't
>generate appropriate code [if it even compiles] on 2.6.7-rc3. See in
>p4-clockmod.c how "easy" it becomes now.
>  
>
 thanks, done it

<snip>

>>-	/* capability check */
>>-	if (policy->cpu != 0)
>>-		return -ENODEV;
>>    
>>
>
>For the time being, this should be kept. Else you can load two different
>governors, or set two different frequencies, on both siblings. Obviously,
>it'll break. 
>
as far as i understand, the frequencies are always the same, because the 
siblings are explicitly notified. about the 2 different governors, the 
same problem exists in the current p4-clockmod.

>I'll try to fix it in the 2.6.8 timeframe...
>  
>
you mean that a change of governors gets communicated to the siblings?


thanks again for your help!

chris