RE: [PATCH][v6] x86, suspend: Save/restore extra MSR registers for suspend

[Chen, Yu C]

Hi, Ingo, thanks for your comments,
(Sorry for my late response because I missed the email you sent to me.)

> -----Original Message-----
> From: Chen, Yu C
> Sent: Tuesday, November 17, 2015 11:06 PM
> To: tglx@xxxxxxxxxxxxx; mingo@xxxxxxxxxx; hpa@xxxxxxxxx
> Cc: rjw@xxxxxxxxxxxxx; pavel@xxxxxx; bp@xxxxxxx; luto@xxxxxxxxxx;
> linux@xxxxxxxxxxx; Zhang, Rui; Brown, Len; x86@xxxxxxxxxx; linux-
> pm@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; Chen, Yu C
> Subject: [PATCH][v6] x86, suspend: Save/restore extra MSR registers for
> suspend
> 
> A bug is reported that, after resumed from S3, CPU is running at a low speed.
> After investigation, BIOS has modified the value of THERM_CONTROL
> register during S3, and changes it from 0 to 0x10, thus enable the clock
> modulation(bit4), but with undefined CPU Duty Cycle(bit1:3), which causes
> the problem.
> 
> and the quirk mechanism ensures that, once resumed from suspended, the
> MSRs indicated by these IDs will be restored to their original values before
> suspended.
> 
> Since both 64/32-bit kernels are affected, this patch covers 64/32-bit
> common code path. And because the MSRs specified by the user might not
> be available or readable in any situation, we use rdmsrl_safe to safely save
> these MSRs.
> 
> Tested-by: Marcin Kaszewski <marcin.kaszewski@xxxxxxxxx>
> Acked-by: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>
> Acked-by: Pavel Machek <pavel@xxxxxx>
> Signed-off-by: Chen Yu <yu.c.chen@xxxxxxxxx>

> [Ingo] 
> Btw., you say a bug was reported - but there's no Reported-by line. Do you know 
> who reported it, can the reporter be credited?
[Yu] OK, I will modify it to Reported-and-tested-by:  Marcin Kaszewski <marcin.kaszewski@xxxxxxxxx>

> [Ingo]
> s/msr_save_data/saved_msr 
> Please don't use short in the kernel, the compiler will align it anyway. 
> use unsigned int instead.
> s/msr_context/saved_msrs
> s/msr_to_save/saved_msrs
> No need to initialize to NULL AFAICS.
> Using 'msr' consistently
> s/"x86/pm: Quirk already applied, ...
> Please put an empty line before the return.
> s/BIOSen
[Yu] OK, all are modified.

> [Ingo]
>> +	u32 bdw_msr_id[] = {MSR_IA32_THERM_CONTROL};
> I'm quite sure checkpatch warns about the whitespaces in this initialization 
sequence.
[Yu] hum, checkpatch does not warn about the whitespaces, 
I changed it to { MSR_IA32_THERM_CONTROL };

> [Ingo]
>> +	 .matches = {
>> +		DMI_MATCH(DMI_PRODUCT_NAME, "GRANTLEY"),
>> +		DMI_MATCH(DMI_PRODUCT_VERSION, "E63448-400"),
> are you sure it's only this specific product version that is affected? 
[Yu] We found  this problem only on this platform, not sure if other platforms also have this problem.
> [Ingo]
> Shouldn't  we filter for all 'GRANTLEY' products? We can do no harm by saving/restoring on 
> platforms that don't need it, right?
[Yu] I'm not sure if it is safe for other platforms to save/restore, since this MSR is a little special.
According to Doug Smythies, on some platforms,  the BIOSen change this MSR intentionally, 
 for example, once woken up from S3(before return to Linux), if the battery is  too low, 
the BIOS might enable  this MSR that, limits the CPU to run at a low speed for less energy consuming. 
Currently, the Linux can only control this MSR via thermal processor throttling,  and if BIOS changes
this MSR during S3, the Linux might hardly get a chance to adjust this value (if the temperature  remains stable), 
and it might bring problems. In above situation, we might need some codes to monitor the usage of battery,
 then adjust the speed of CPU?
For E63448-400 platform, we confirmed that we don't want the BIOS to set the MSR for us, so we worked around
It.
 > [Ingo]
> Please double check that the suspend/resume test facility 
> (CONFIG_PM_TEST_SUSPEND)  is run _after_ this callback is
> called. Is there any reason why this is a late  initcall?

[Yu] I use late_initcall because I found the dmi_init used 
subsys_initcall, so I tried to put it as late as possible.. It seems to break
CONFIG_PM_TEST_SUSPEND,  I changed it to __initcall, which is 
a little earlier than late_initcall.


thanks,
Yu 
--
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/