On Tue, 20 Jul 2010 13:32:19 +0800, Chen Gong wrote:
> 于 7/19/2010 7:43 PM, Jean Delvare 写道:
> > 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.
>
> I don't think the original version is more meaningful than now.
The original has struct members sorted logically: all fields which are
related to temperature measurement are at the end of the structure.
This is also consistent with what almost all other hwmon drivers do.
> >> @@ -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.
>
> In fact, I hate hard-coded. You are right, driver name != device name.
> How about a new macro definition for it ?
I don't think there is any need for this. Hard-coding is a problem when
the value is used multiple times. Here it is used only once, so a macro
will only make the code harder to read.
> >> 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?
>
> Please see the 2nd version. This version is out-dated. But, I still
> insist that if it is probable in theory, the fix should be reasonable.
I'll look into it.
--
Jean Delvare
_______________________________________________
lm-sensors mailing list
lm-sensors@xxxxxxxxxxxxxx
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
