diff mbox

Longhaul: Disable driver by default

Message ID 1354054435-2459-1-git-send-email-rafalbilski@interia.pl (mailing list archive)
State Accepted, archived
Headers show

Commit Message

Rafa? Bilski Nov. 27, 2012, 10:13 p.m. UTC
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(-)

Comments

Dave Jones Nov. 27, 2012, 10:41 p.m. UTC | #1
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

--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Rafa? Bilski Nov. 27, 2012, 11:38 p.m. UTC | #2
> 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.");
>>

--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Rafael Wysocki Nov. 27, 2012, 11:44 p.m. UTC | #3
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
Rafael Wysocki Dec. 15, 2012, 12:06 a.m. UTC | #4
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
diff mbox

Patch

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.");