[PATCH] Longhaul: Disable driver by default

[PATCH] Longhaul: Disable driver by default

[Rafal Bilski]

This is only solution I can think of. User decides if he wants this
driver on his machine. I don't have enough knowledge and time to find
the reason why same code works on some machines and doesn't on others
which use same, or very similar,  chipset and processor.

Signed-off-by: Rafal Bilski <rafalbilski@interia.pl>

---
 drivers/cpufreq/longhaul.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/cpufreq/longhaul.c b/drivers/cpufreq/longhaul.c
index 53ddbc7..0bf5bd1 100644
--- a/drivers/cpufreq/longhaul.c
+++ b/drivers/cpufreq/longhaul.c
@@ -77,7 +77,7 @@ static unsigned int longhaul_index;
 static int scale_voltage;
 static int disable_acpi_c3;
 static int revid_errata;
-
+static int enable;
 
 /* Clock ratios multiplied by 10 */
 static int mults[32];
@@ -965,6 +965,10 @@ static int __init longhaul_init(void)
 	if (!x86_match_cpu(longhaul_id))
 		return -ENODEV;
 
+	if (!enable) {
+		printk(KERN_ERR PFX "Option \"enable\" not set. Aborting.\n");
+		return -ENODEV;
+	}
 #ifdef CONFIG_SMP
 	if (num_online_cpus() > 1) {
 		printk(KERN_ERR PFX "More than 1 CPU detected, "
@@ -1021,6 +1025,10 @@ MODULE_PARM_DESC(scale_voltage, "Scale voltage of processor");
  * such. */
 module_param(revid_errata, int, 0644);
 MODULE_PARM_DESC(revid_errata, "Ignore CPU Revision ID");
+/* By default driver is disabled to prevent incompatible
+ * system freeze. */
+module_param(enable, int, 0644);
+MODULE_PARM_DESC(enable, "Enable driver");
 
 MODULE_AUTHOR("Dave Jones <davej@redhat.com>");
 MODULE_DESCRIPTION("Longhaul driver for VIA Cyrix processors.");
-- 
1.8.0

Re: [PATCH] Longhaul: Disable driver by default

[Rafael J. Wysocki]

On Tuesday, November 27, 2012 10:13:55 PM Rafal Bilski wrote:
> This is only solution I can think of. User decides if he wants this
> driver on his machine. I don't have enough knowledge and time to find
> the reason why same code works on some machines and doesn't on others
> which use same, or very similar,  chipset and processor.

I always have problems with patches like this one, because they are pretty much
guaranteed to make someone complain.

Is there any way to blacklist the affected machine you have?

Rafael


> Signed-off-by: Rafal Bilski <rafalbilski@interia.pl>
> 
> ---
>  drivers/cpufreq/longhaul.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/cpufreq/longhaul.c b/drivers/cpufreq/longhaul.c
> index 53ddbc7..0bf5bd1 100644
> --- a/drivers/cpufreq/longhaul.c
> +++ b/drivers/cpufreq/longhaul.c
> @@ -77,7 +77,7 @@ static unsigned int longhaul_index;
>  static int scale_voltage;
>  static int disable_acpi_c3;
>  static int revid_errata;
> -
> +static int enable;
>  
>  /* Clock ratios multiplied by 10 */
>  static int mults[32];
> @@ -965,6 +965,10 @@ static int __init longhaul_init(void)
>  	if (!x86_match_cpu(longhaul_id))
>  		return -ENODEV;
>  
> +	if (!enable) {
> +		printk(KERN_ERR PFX "Option \"enable\" not set. Aborting.\n");
> +		return -ENODEV;
> +	}
>  #ifdef CONFIG_SMP
>  	if (num_online_cpus() > 1) {
>  		printk(KERN_ERR PFX "More than 1 CPU detected, "
> @@ -1021,6 +1025,10 @@ MODULE_PARM_DESC(scale_voltage, "Scale voltage of processor");
>   * such. */
>  module_param(revid_errata, int, 0644);
>  MODULE_PARM_DESC(revid_errata, "Ignore CPU Revision ID");
> +/* By default driver is disabled to prevent incompatible
> + * system freeze. */
> +module_param(enable, int, 0644);
> +MODULE_PARM_DESC(enable, "Enable driver");
>  
>  MODULE_AUTHOR("Dave Jones <davej@redhat.com>");
>  MODULE_DESCRIPTION("Longhaul driver for VIA Cyrix processors.");
> 
-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH] Longhaul: Disable driver by default

[Dave Jones]

On Tue, Nov 27, 2012 at 11:33:10PM +0100, Rafael J. Wysocki wrote:
 > On Tuesday, November 27, 2012 10:13:55 PM Rafal Bilski wrote:
 > > This is only solution I can think of. User decides if he wants this
 > > driver on his machine. I don't have enough knowledge and time to find
 > > the reason why same code works on some machines and doesn't on others
 > > which use same, or very similar,  chipset and processor.
 > 
 > I always have problems with patches like this one, because they are pretty much
 > guaranteed to make someone complain.
 > 
 > Is there any way to blacklist the affected machine you have?

There are a lot of marginal VIA systems out there. Mostly due to really
poor quality motherboards (I had several myself that ended up with leaking
capacitors). They work fine until you put them under load and then start
tweaking the voltage. Rafal spent a long time trying to get them stable
(see the git history for longhaul.c).  

Given those CPUs are pretty underpowered today, and there are many better
alternatives if you care about power saving that much, I'd vote for
not worrying about it too much.  We even stopped building it in Fedora
due to a) the limited userbase and b) when we got bug reports there was
nothing we could really do, so we opted for stability over power saving.

	Dave

Re: [PATCH] Longhaul: Disable driver by default

[Rafał Bilski]

> On Tuesday, November 27, 2012 10:13:55 PM Rafal Bilski wrote:
>> This is only solution I can think of. User decides if he wants this
>> driver on his machine. I don't have enough knowledge and time to find
>> the reason why same code works on some machines and doesn't on others
>> which use same, or very similar,  chipset and processor.
> I always have problems with patches like this one, because they are pretty much
> guaranteed to make someone complain.
>
> Is there any way to blacklist the affected machine you have?
>
> Rafael
No. Also problem seems to be larger than one machine. Also weirder.
One user claims his processor can't run below some frequency even
if it should be perfectly capable of doing so. System in question
freezes straight away. In past on good system I could change
frequency at least a couple of times without any protection. Just by
a chance. I tried to investigate "weird CPU 0 not listed by the BIOS"
message, but I have nothing. Also I seem to have far less time for
anything than in the past.

Sorry
Rafal

>
>
>> Signed-off-by: Rafal Bilski <rafalbilski@interia.pl>
>>
>> ---
>>   drivers/cpufreq/longhaul.c | 10 +++++++++-
>>   1 file changed, 9 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/cpufreq/longhaul.c b/drivers/cpufreq/longhaul.c
>> index 53ddbc7..0bf5bd1 100644
>> --- a/drivers/cpufreq/longhaul.c
>> +++ b/drivers/cpufreq/longhaul.c
>> @@ -77,7 +77,7 @@ static unsigned int longhaul_index;
>>   static int scale_voltage;
>>   static int disable_acpi_c3;
>>   static int revid_errata;
>> -
>> +static int enable;
>>   
>>   /* Clock ratios multiplied by 10 */
>>   static int mults[32];
>> @@ -965,6 +965,10 @@ static int __init longhaul_init(void)
>>   	if (!x86_match_cpu(longhaul_id))
>>   		return -ENODEV;
>>   
>> +	if (!enable) {
>> +		printk(KERN_ERR PFX "Option \"enable\" not set. Aborting.\n");
>> +		return -ENODEV;
>> +	}
>>   #ifdef CONFIG_SMP
>>   	if (num_online_cpus() > 1) {
>>   		printk(KERN_ERR PFX "More than 1 CPU detected, "
>> @@ -1021,6 +1025,10 @@ MODULE_PARM_DESC(scale_voltage, "Scale voltage of processor");
>>    * such. */
>>   module_param(revid_errata, int, 0644);
>>   MODULE_PARM_DESC(revid_errata, "Ignore CPU Revision ID");
>> +/* By default driver is disabled to prevent incompatible
>> + * system freeze. */
>> +module_param(enable, int, 0644);
>> +MODULE_PARM_DESC(enable, "Enable driver");
>>   
>>   MODULE_AUTHOR("Dave Jones <davej@redhat.com>");
>>   MODULE_DESCRIPTION("Longhaul driver for VIA Cyrix processors.");
>>

Re: [PATCH] Longhaul: Disable driver by default

[Rafael J. Wysocki]

On Tuesday, November 27, 2012 11:38:01 PM Rafał Bilski wrote:
> 
> > On Tuesday, November 27, 2012 10:13:55 PM Rafal Bilski wrote:
> >> This is only solution I can think of. User decides if he wants this
> >> driver on his machine. I don't have enough knowledge and time to find
> >> the reason why same code works on some machines and doesn't on others
> >> which use same, or very similar,  chipset and processor.
> > I always have problems with patches like this one, because they are pretty much
> > guaranteed to make someone complain.
> >
> > Is there any way to blacklist the affected machine you have?
> >
> > Rafael
> No. Also problem seems to be larger than one machine. Also weirder.
> One user claims his processor can't run below some frequency even
> if it should be perfectly capable of doing so. System in question
> freezes straight away. In past on good system I could change
> frequency at least a couple of times without any protection. Just by
> a chance. I tried to investigate "weird CPU 0 not listed by the BIOS"
> message, but I have nothing. Also I seem to have far less time for
> anything than in the past.

OK, I'll take the patch, then, but I won't include it in my first v3.8 pull
request.

Thanks,
Rafael


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH] Longhaul: Disable driver by default

[Rafael J. Wysocki]

On Wednesday, November 28, 2012 12:44:54 AM Rafael J. Wysocki wrote:
> On Tuesday, November 27, 2012 11:38:01 PM Rafał Bilski wrote:
> > 
> > > On Tuesday, November 27, 2012 10:13:55 PM Rafal Bilski wrote:
> > >> This is only solution I can think of. User decides if he wants this
> > >> driver on his machine. I don't have enough knowledge and time to find
> > >> the reason why same code works on some machines and doesn't on others
> > >> which use same, or very similar,  chipset and processor.
> > > I always have problems with patches like this one, because they are pretty much
> > > guaranteed to make someone complain.
> > >
> > > Is there any way to blacklist the affected machine you have?
> > >
> > > Rafael
> > No. Also problem seems to be larger than one machine. Also weirder.
> > One user claims his processor can't run below some frequency even
> > if it should be perfectly capable of doing so. System in question
> > freezes straight away. In past on good system I could change
> > frequency at least a couple of times without any protection. Just by
> > a chance. I tried to investigate "weird CPU 0 not listed by the BIOS"
> > message, but I have nothing. Also I seem to have far less time for
> > anything than in the past.
> 
> OK, I'll take the patch, then, but I won't include it in my first v3.8 pull
> request.

I'm queuing it up for submission as v3.8 material later in the cycle.

Thanks,
Rafael


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.