Hi Jean, On Tue, 2012-01-31 at 16:38 -0500, Jean Delvare wrote: > Hi Guenter, > > On Wed, 25 Jan 2012 04:43:34 -0800, Guenter Roeck wrote: > > The vrm value is system-wide and only needs to be calculated once. Do it when > > the module is loaded. Provide a module parameter to enable overwriting the > > default value in case it is wrong or can not be calculated. > > > > Use the internal value when calculating the VID voltage; ignore the VRM > > parameter passed in vid_from_reg(). This will enable us to make the vrm > > attribute in individual drivers read-only or to drop it entirely. > > > > Cc: Rudolf Marek <r.marek@xxxxxxxxxxxx> > > Signed-off-by: Guenter Roeck <linux@xxxxxxxxxxxx> > > --- > > drivers/hwmon/hwmon-vid.c | 56 +++++++++++++++++++++++++++++++++++++-------- > > 1 files changed, 46 insertions(+), 10 deletions(-) > > > > I couldn't apply this one either... > Same reason, presumably - it applies on top of the other cleanup patches. I might merge the ones from me into a single patch, actually, to reduce the number of patches. > > diff --git a/drivers/hwmon/hwmon-vid.c b/drivers/hwmon/hwmon-vid.c > > index 40b50b4..26dd0d7 100644 > > --- a/drivers/hwmon/hwmon-vid.c > > +++ b/drivers/hwmon/hwmon-vid.c > > @@ -73,13 +73,20 @@ > > * http://www.intel.com/design/processor/applnots/313214.htm > > */ > > > > +static ushort vrm; > > +module_param(vrm, ushort, S_IRUGO); > > It might be convenient to make the value writable. For example this can > allow debugging the hwmon-vid code without breaking monitoring on the > running system. > Ok. > > +MODULE_PARM_DESC(vrm, "VRM/VRD version, multiplied by 10"); > > This is no longer always true, as explained in doc/vid in the > lm-sensors source tree. This document should probably be updated and > moved to the kernel tree. > Makes sense. Documentation/hwmon/hwmon-vid ? > > + > > /* > > * vrm is the VRM/VRD document version multiplied by 10. > > * val is the 4-bit or more VID code. > > * Returned value is in mV to avoid floating point in the kernel. > > * Some VID have some bits in uV scale, this is rounded to mV. > > + * > > + * Note: vrm is passed for legacy reasons to avoid API changes, > > + * but no longer used. > > I don't see the point of preserving the API. We usually don't do that. > The API was wrong, you're fixing it, let's adjust all drivers > accordingly and be done with it. > Sure, no problem. That also solves the return value problem. > > */ > > -int vid_from_reg(int val, u8 vrm) > > +int vid_from_reg(int val, u8 _vrm) > > { > > int vid; > > > > @@ -151,9 +158,6 @@ int vid_from_reg(int val, u8 vrm) > > val &= 0x7f; > > return val > 0x77 ? 0 : (1500000 - (val * 12500) + 500) / 1000; > > default: /* report 0 for unknown */ > > - if (vrm) > > - pr_warn("Requested unsupported VRM version (%u)\n", > > - (unsigned int)vrm); > > return 0; > > } > > } > > @@ -234,7 +238,7 @@ static struct vrm_model vrm_models[] = { > > * + quirk for Eden ULV 500 MHz). > > * Note: something similar might be needed for model A, I'm not sure. > > */ > > -static u8 get_via_model_d_vrm(void) > > +static u8 __init get_via_model_d_vrm(void) > > { > > unsigned int vid, brand, dummy; > > static const char *brands[4] = { > > @@ -259,7 +263,7 @@ static u8 get_via_model_d_vrm(void) > > } > > } > > > > -static u8 find_vrm(u8 family, u8 model, u8 stepping, u8 vendor) > > +static u8 __init find_vrm(u8 family, u8 model, u8 stepping, u8 vendor) > > { > > int i; > > > > @@ -275,11 +279,24 @@ static u8 find_vrm(u8 family, u8 model, u8 stepping, u8 vendor) > > return 0; > > } > > > > -u8 vid_which_vrm(void) > > +static int __init vid_init(void) > > { > > struct cpuinfo_x86 *c = &cpu_data(0); > > u8 vrm_ret; > > > > + if (vrm) { > > + /* > > + * Validate vrm by calling vid_from_reg() with a known valid > > + * vid. If it returns a non-zero voltage, we know that vrm is > > + * valid. If the configured vrm is invalid, try to get it > > + * from CPU version data. > > + */ > > + if (vid_from_reg(2, vrm)) > > This is a little fragile... At the moment 2 is valid for all supported > VRM versions but this might not be the case for some future VRM version. > > What about having vid_from_reg return -EINVAL or some such when it > doesn't know the VRM code passed by the user? > That was just because I tried to preserve the API. If I don't do that, I can return -EINVAL. > > + return 0; > > + pr_warn("Requested unsupported VRM version (%u)\n", vrm); > > + vrm = 0; > > + } > > Let's fail the module load in that case. > Ok, makes sense. After all, it can be fixed easily by providing a correct module parameter. > > + > > if (c->x86 < 6) /* Any CPU with family lower than 6 */ > > return 0; /* doesn't have VID and/or CPUID */ > > In that case (and also in the unknown VRM case below) the driver will > load and hwmon drivers will call vid_from_reg(). At the moment this > will return 0, which drivers will happily expose to user-space. Ideally > all drivers would handle this (or -EINVAL, see above) as an error and > NOT create the cpu[0-*]_vid attributes. This would be a separate patch > I guess. > I'll think about it. > > > > @@ -288,19 +305,38 @@ u8 vid_which_vrm(void) > > vrm_ret = get_via_model_d_vrm(); > > if (vrm_ret == 0) > > pr_info("Unknown VRM version of your x86 CPU\n"); > > - return vrm_ret; > > + > > + vrm = vrm_ret; > > Isn't this overriding the module parameter passed by the user? > Yes. That was on purpose, since the resulting vrm can then be accessed by userland through the respective sysfs entry. I thought that was a good idea. Maybe not ? > > + > > + return 0; > > } > > > > /* and now for something completely different for the non-x86 world */ > > #else > > -u8 vid_which_vrm(void) > > + > > +static int __init vid_init(void) > > { > > - pr_info("Unknown VRM version of your CPU\n"); > > return 0; > > } > > + > > #endif > > + > > +static void __exit vid_exit(void) > > +{ > > +} > > Do you really have to create that stub? > I think so. The driver refuses to unload if it does not exist. > > + > > +u8 vid_which_vrm(void) > > +{ > > + if (!vrm) > > + pr_info("Unknown VRM version of your CPU\n"); > > + > > + return vrm; > > +} > > EXPORT_SYMBOL(vid_which_vrm); > > There should be no user left for this function if you don't attempt to > preserve the API. > Yes, I'll remove it in the next version. To retain the ability to compile the code, I'll have to provide a single patch for all affected files. Is that ok ? Thanks, Guenter _______________________________________________ lm-sensors mailing list lm-sensors@xxxxxxxxxxxxxx http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
