Hi Chen,
On Thu, 15 Jul 2010 10:46:43 +0800, Chen Gong wrote:
> cleanup some redundant codes in coretemp.c.
>
> Signed-off-by: Chen Gong <gong.chen@xxxxxxxxxxxxxxx>
> ---
> drivers/hwmon/coretemp.c | 18 ++++--------------
> 1 files changed, 4 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/hwmon/coretemp.c b/drivers/hwmon/coretemp.c
> index 7b7c5b8..728e9c3 100644
> --- a/drivers/hwmon/coretemp.c
> +++ b/drivers/hwmon/coretemp.c
> @@ -54,13 +54,13 @@ struct coretemp_data {
> const char *name;
> u32 id;
> u16 core_id;
> + u8 alarm;
> char valid; /* zero until following fields are valid */
> unsigned long last_updated; /* in jiffies */
> int temp;
> int pkg_temp;
> int tjmax;
> int ttarget;
> - u8 alarm;
> };
>
> /*
Gratuitous change, undesirable. I understand you make the structure a
few bytes smaller, but we really don't care about this. Having fields
ordered in a way that makes sense if much more important.
> @@ -320,14 +320,14 @@ static int __devinit coretemp_probe(struct platform_device *pdev)
> #ifdef CONFIG_SMP
> data->core_id = c->cpu_core_id;
> #endif
> - data->name = "coretemp";
> + data->name = DRVNAME;
This is conceptually wrong. data->name holds a _device_ name, not a
_driver_ name. It happens to be the same string, but it doesn't have to.
> mutex_init(&data->update_lock);
>
> /* test if we can access the THERM_STATUS MSR */
> err = rdmsr_safe_on_cpu(data->id, MSR_IA32_THERM_STATUS, &eax, &edx);
> if (err) {
> dev_err(&pdev->dev,
> - "Unable to access THERM_STATUS MSR, giving up\n");
> + "Unable to access THERM_STATUS MSR, giving up\n");
> goto exit_free;
> }
>
Again, a gratuitous change nobody asked for, the original alignment was
better.
> @@ -555,8 +555,7 @@ static int __init coretemp_init(void)
> if (c->cpuid_level >= 6 && (cpuid_eax(0x06) & 0x01)) {
> err = coretemp_device_add(i);
> if (err)
> - goto exit_devices_unreg;
> -
> + goto exit;
> } else {
> printk(KERN_INFO DRVNAME ": CPU (model=0x%x)"
> " has no thermal sensor.\n", c->x86_model);
> @@ -567,15 +566,6 @@ static int __init coretemp_init(void)
> register_hotcpu_notifier(&coretemp_cpu_notifier);
> #endif
> return 0;
> -
> -exit_devices_unreg:
> - mutex_lock(&pdev_list_mutex);
> - list_for_each_entry_safe(p, n, &pdev_list, list) {
> - platform_device_unregister(p->pdev);
> - list_del(&p->list);
> - kfree(p);
> - }
> - mutex_unlock(&pdev_list_mutex);
> exit:
> return err;
> }
This isn't correct, sorry. I understand you may want the driver to keep
running even if one of the cores couldn't be registered with the
driver, contrary to what the driver does right now. But if you change
the driver to do this, change it consistently. With the above code, on
error the module will still fail to load (as it returns a non-zero
value) but it leave devices and drivers registered. As all the static
memory allocated for the module will be freed when the module is
unloaded, this will crash your kernel.
Before you go on, please tell us if you are solving a real-world
problem. I don't really expect coretemp_device_add() to fail for one
core and succeed for the others, so it seems like a theoretical only
discussion. What is your actual motivation for these changes?
--
Jean Delvare
_______________________________________________
lm-sensors mailing list
lm-sensors@xxxxxxxxxxxxxx
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
