2003-06-15 17:51:11

by Samuel Thibault

[permalink] [raw]
Subject: [2.5 PATCH] bug if cpufreq driver initialization fails

Le dim 15 jun 2003 11:50:44 GMT, Dominik Brodowski a tapot? sur son clavier :
> > However, my 440BX chipset is still not supported, and I couldn't find
> > anything in Intel's documentation...
>
> Do you know of Bruno Ducrot's work?
>
> http://www.poupinou.org/cpufreq/

I read the list archive yesterday, and tried this work. I got the results
http://youpibouh.thefreecat.org/speedstep/ac
http://youpibouh.thefreecat.org/speedstep/battery
and hence tried my gpo_hilo=11, and it works... Sometimes.

I seems like the hardware si sometimes slow to get to the low speed. My
PIII is a 800Mhz/650Mhz, but initialization sometimes shows 800 and 800,
as if the speed measure was done before the hardware actually achieved
the slow down.

As a result, speedstep_cpu_init() fails, hence cpufreq_add_dev() fails,
which is ignored by sysdev_driver_register(). In the end, the module is
kept loaded (since speedstep_init() successes, since
cpufreq_register_driver successes()), and I have to unload it by hand,
which entails kobject badness warning of course, since cpufreq then
tries to remove entries which were actually never added to /sys!

I hence modified drivers/base/sys.c to have sysdev_driver_register()
fail as well, and then I also had to modify kernel/cpufreq.c, because
this failure did not imply a setting cpufreq_driver to NULL (preventing
me from reinsmoding speedstep-ich: EBUSY)

I'd also suggest that the speedstep drivers printks something if
everything went ok (including the cpufreq_frequency_table_cpuinfo()
call), the low & high speed for instance, just to be sure everything
went ok

Regards,
Samuel Thibault

--- linux-2.5.71-orig/drivers/base/sys.c 2003-06-14 17:12:52.000000000 -0400
+++ linux-2.5.71-perso/drivers/base/sys.c 2003-06-15 13:47:45.000000000 -0400
@@ -116,6 +116,7 @@
int sysdev_driver_register(struct sysdev_class * cls,
struct sysdev_driver * drv)
{
+ int ret = 0;
down_write(&system_subsys.rwsem);
if (cls && kset_get(&cls->kset)) {
list_add_tail(&drv->entry,&cls->drivers);
@@ -123,13 +124,16 @@
/* If devices of this class already exist, tell the driver */
if (drv->add) {
struct sys_device *dev;
- list_for_each_entry(dev, &cls->kset.list, kobj.entry)
- drv->add(dev);
+ list_for_each_entry(dev, &cls->kset.list, kobj.entry) {
+ ret = drv->add(dev);
+ if (ret) goto out;
+ }
}
} else
list_add_tail(&drv->entry,&global_drivers);
+out:
up_write(&system_subsys.rwsem);
- return 0;
+ return ret;
}


--- linux-2.5.71-orig/kernel/cpufreq.c 2003-06-14 17:13:39.000000000 -0400
+++ linux-2.5.71-perso/kernel/cpufreq.c 2003-06-15 13:55:36.000000000 -0400
@@ -811,6 +811,8 @@
*/
int cpufreq_register_driver(struct cpufreq_driver *driver_data)
{
+ int ret;
+
if (!driver_data || !driver_data->verify || !driver_data->init ||
((!driver_data->setpolicy) && (!driver_data->target)))
return -EINVAL;
@@ -825,13 +827,17 @@

cpufreq_driver->policy = kmalloc(NR_CPUS * sizeof(struct cpufreq_policy), GFP_KERNEL);
if (!cpufreq_driver->policy) {
- cpufreq_driver = NULL;
- return -ENOMEM;
+ ret = -ENOMEM;
+ goto out;
}

memset(cpufreq_driver->policy, 0, NR_CPUS * sizeof(struct cpufreq_policy));

- return sysdev_driver_register(&cpu_sysdev_class,&cpufreq_sysdev_driver);
+ ret = sysdev_driver_register(&cpu_sysdev_class,&cpufreq_sysdev_driver);
+out:
+ if (ret)
+ cpufreq_driver = NULL;
+ return ret;
}
EXPORT_SYMBOL_GPL(cpufreq_register_driver);


2003-06-15 18:03:06

by Russell King

[permalink] [raw]
Subject: Re: [2.5 PATCH] bug if cpufreq driver initialization fails

On Sun, Jun 15, 2003 at 02:04:36PM -0400, Samuel Thibault wrote:
> I hence modified drivers/base/sys.c to have sysdev_driver_register()
> fail as well, and then I also had to modify kernel/cpufreq.c, because
> this failure did not imply a setting cpufreq_driver to NULL (preventing
> me from reinsmoding speedstep-ich: EBUSY)

Unfortunately, you created a by by doing so. Eg:

- you have 3 devices on kset.list.
- you successfully register 2 of them with a driver.
- you fail one.
- sysdev_driver_register returns failure.
- module is unloaded while other parts of the kernel have references into
the driver.
- the kernel oopses.

> I'd also suggest that the speedstep drivers printks something if
> everything went ok (including the cpufreq_frequency_table_cpuinfo()
> call), the low & high speed for instance, just to be sure everything
> went ok

IMO its better to printk something on failure.

--
Russell King ([email protected]) The developer of ARM Linux
http://www.arm.linux.org.uk/personal/aboutme.html

2003-06-15 18:11:21

by Samuel Thibault

[permalink] [raw]
Subject: Re: [2.5 PATCH] bug if cpufreq driver initialization fails

Le dim 15 jun 2003 19:16:50 GMT, Russell King a tapot? sur son clavier :
> On Sun, Jun 15, 2003 at 02:04:36PM -0400, Samuel Thibault wrote:
> > I hence modified drivers/base/sys.c to have sysdev_driver_register()
> > fail as well, and then I also had to modify kernel/cpufreq.c, because
> > this failure did not imply a setting cpufreq_driver to NULL (preventing
> > me from reinsmoding speedstep-ich: EBUSY)
>
> Unfortunately, you created a by by doing so. Eg:
>
> - you have 3 devices on kset.list.
> - you successfully register 2 of them with a driver.
> - you fail one.
> - sysdev_driver_register returns failure.
> - module is unloaded while other parts of the kernel have references into
> the driver.
> - the kernel oopses.
Ok, that's what I was wondering, might an exit loop removing the already
added driver be done?

(the second half of the patch might still be applied as soon as now)

Regards,
Samuel Thibault

2003-06-15 18:35:56

by Russell King

[permalink] [raw]
Subject: Re: [2.5 PATCH] bug if cpufreq driver initialization fails

On Sun, Jun 15, 2003 at 02:25:07PM -0400, Samuel Thibault wrote:
> Le dim 15 jun 2003 19:16:50 GMT, Russell King a tapot? sur son clavier :
> > On Sun, Jun 15, 2003 at 02:04:36PM -0400, Samuel Thibault wrote:
> > > I hence modified drivers/base/sys.c to have sysdev_driver_register()
> > > fail as well, and then I also had to modify kernel/cpufreq.c, because
> > > this failure did not imply a setting cpufreq_driver to NULL (preventing
> > > me from reinsmoding speedstep-ich: EBUSY)
> >
> > Unfortunately, you created a by by doing so. Eg:
> >
> > - you have 3 devices on kset.list.
> > - you successfully register 2 of them with a driver.
> > - you fail one.
> > - sysdev_driver_register returns failure.
> > - module is unloaded while other parts of the kernel have references into
> > the driver.
> > - the kernel oopses.
> Ok, that's what I was wondering, might an exit loop removing the already
> added driver be done?
>
> (the second half of the patch might still be applied as soon as now)

Also, I think people need to consider whether "all devices failed"
or "any device failed" counts as overall failure.

--
Russell King ([email protected]) The developer of ARM Linux
http://www.arm.linux.org.uk/personal/aboutme.html