[lm-sensors] [RFC PATCH 3/3] hwmon: (hwmon-vid) Determine vrm during initialization, and add vrm mod

[lm-sensors] [RFC PATCH 3/3] hwmon: (hwmon-vid) Determine vrm during initialization, and add vrm mod

[Guenter Roeck]

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@assembler.cz>
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/hwmon/hwmon-vid.c |   56 +++++++++++++++++++++++++++++++++++++--------
 1 files changed, 46 insertions(+), 10 deletions(-)

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);
+MODULE_PARM_DESC(vrm, "VRM/VRD version, multiplied by 10");
+
 /*
  * 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.
  */
-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))
+			return 0;
+		pr_warn("Requested unsupported VRM version (%u)\n", vrm);
+		vrm = 0;
+	}
+
 	if (c->x86 < 6)		/* Any CPU with family lower than 6 */
 		return 0;	/* doesn't have VID and/or CPUID */
 
@@ -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;
+
+	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)
+{
+}
+
+u8 vid_which_vrm(void)
+{
+	if (!vrm)
+		pr_info("Unknown VRM version of your CPU\n");
+
+	return vrm;
+}
 EXPORT_SYMBOL(vid_which_vrm);
 
+module_init(vid_init);
+module_exit(vid_exit);
+
 MODULE_AUTHOR("Rudolf Marek <r.marek@assembler.cz>");
 
 MODULE_DESCRIPTION("hwmon-vid driver");
-- 
1.7.5.4


_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [RFC PATCH 3/3] hwmon: (hwmon-vid) Determine vrm during initialization, and add vrm

[Jean Delvare]

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@assembler.cz>
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> ---
>  drivers/hwmon/hwmon-vid.c |   56 +++++++++++++++++++++++++++++++++++++--------
>  1 files changed, 46 insertions(+), 10 deletions(-)
> 

I couldn't apply this one either...

> 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.

> +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.

> +
>  /*
>   * 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.

>   */
> -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?

> +			return 0;
> +		pr_warn("Requested unsupported VRM version (%u)\n", vrm);
> +		vrm = 0;
> +	}

Let's fail the module load in that case.

> +
>  	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.

>  
> @@ -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?

> +
> +	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?

> +
> +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.

>  
> +module_init(vid_init);
> +module_exit(vid_exit);
> +
>  MODULE_AUTHOR("Rudolf Marek <r.marek@assembler.cz>");
>  
>  MODULE_DESCRIPTION("hwmon-vid driver");


-- 
Jean Delvare

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [RFC PATCH 3/3] hwmon: (hwmon-vid) Determine vrm during initialization, and add vrm

[Guenter Roeck]

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@assembler.cz>
> > Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> > ---
> >  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@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [RFC PATCH 3/3] hwmon: (hwmon-vid) Determine vrm during initialization, and add vrm

[Guenter Roeck]

Hi Jean,

On Tue, 2012-01-31 at 16:38 -0500, Jean Delvare wrote:

[ ... ]

> >  /*
> >   * 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.
> 
I started looking into this.

Problem is that some callers pass a fixed vrm value to vid_from_reg().
See lm87.c or lm93.c. Can we safely ignore this, or do we have to retain
the parameter and use it if provided (ie if it is != 0) ?

On the plus side, if we retain the parameter and use it to override the
configured/calculated value, we can retain the API, and I can submit a
series of patches to remove vrm usage from the various drivers, instead
of a single large patch which does it all in one go. So this would have
some benefits.

[ ... ]

> > +}
> >  EXPORT_SYMBOL(vid_which_vrm);

> There should be no user left for this function if you don't attempt to
> preserve the API.
> 
Actually, turns out there is at least one user - the value is used in
w83627ehf.c to configure the chip. Not sure how to do that if the driver
can not retrieve the vrm value.

Then there is vid_to_reg(), which is currently an inline function. Guess
I'll have to move that to hwmon-vid.c and export it.

Thanks,
Guenter



_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [RFC PATCH 3/3] hwmon: (hwmon-vid) Determine vrm during initialization, and add vrm

[Jean Delvare]

Hi Guenter,

On Tue, 31 Jan 2012 14:03:02 -0800, Guenter Roeck wrote:
> On Tue, 2012-01-31 at 16:38 -0500, Jean Delvare wrote:
> > On Wed, 25 Jan 2012 04:43:34 -0800, Guenter Roeck wrote:
> > > +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 ?

Yes.

> > > (...)
> > > @@ -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 ?

Forget about this. Being unable to apply the patch, I couldn't read the
function in its entirety. I was commenting on a code flow which cannot
actually happen.

> > > (...)
> > > +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.

OK, good to know.

> (...)
> To retain the ability to compile the code, I'll have to provide a single
> patch for all affected files. Is that ok ?

If you have to do that, it's OK. We've seen much much larger patches
before.

-- 
Jean Delvare

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [RFC PATCH 3/3] hwmon: (hwmon-vid) Determine vrm during initialization, and add vrm

[Jean Delvare]

On Tue, 31 Jan 2012 15:31:17 -0800, Guenter Roeck wrote:
> On Tue, 2012-01-31 at 16:38 -0500, Jean Delvare wrote:
> > 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.
> 
> I started looking into this.
> 
> Problem is that some callers pass a fixed vrm value to vid_from_reg().
> See lm87.c or lm93.c. Can we safely ignore this, or do we have to retain
> the parameter and use it if provided (ie if it is != 0) ?

I don't see that in lm87.c, I presume you meant lm78.c. For lm78 this
is clearly a bug, the driver was written before multiple VRM versions
existed, and was never adapter to support other VRM versions. So you
can ignore it.

For lm93 the hard-coded value is because the chip support relative
Vcore voltage limits (which adjust themselves as the VID code changes)
and that can only work if the chip itself knows how to interpret the
codes. As the LM93 only knows of VRM 10.x, This strongly suggested that
the chip will never be used in combinations with other CPUs, thus the
hard-coded VRM. I don't think it was so smart though, as the LM93 could
still be used with other CPUs, simply the dynamic Vcore limit feature
can't be used. Also note that the driver now supports the LM94 which
_does_ support VRD 10 and VRD 11, so the hard-coded value is really
incorrect.

In general, if a driver wants to only support a specific VRM value, it
should now call vid_witch_vrm() and fail loading or limit its features
if the value isn't what it wanted.

> On the plus side, if we retain the parameter and use it to override the
> configured/calculated value, we can retain the API, and I can submit a
> series of patches to remove vrm usage from the various drivers, instead
> of a single large patch which does it all in one go. So this would have
> some benefits.

I think we'll end up removing the extra parameter. But if you want to
keep it for the time being because you feel more comfortable that way
or you believe it offers a smoother update path, that's your call.

> > > +}
> > >  EXPORT_SYMBOL(vid_which_vrm);
> >
> > There should be no user left for this function if you don't attempt to
> > preserve the API.
>
> Actually, turns out there is at least one user - the value is used in
> w83627ehf.c to configure the chip. Not sure how to do that if the driver
> can not retrieve the vrm value.

Good point, vid_which_vrm must stay.

> Then there is vid_to_reg(), which is currently an inline function. Guess
> I'll have to move that to hwmon-vid.c and export it.

Correct.

-- 
Jean Delvare

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [RFC PATCH 3/3] hwmon: (hwmon-vid) Determine vrm during initialization, and add vrm

[Guenter Roeck]

On Wed, Feb 01, 2012 at 03:01:38AM -0500, Jean Delvare wrote:
> On Tue, 31 Jan 2012 15:31:17 -0800, Guenter Roeck wrote:
> > On Tue, 2012-01-31 at 16:38 -0500, Jean Delvare wrote:
> > > 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.
> > 
> > I started looking into this.
> > 
> > Problem is that some callers pass a fixed vrm value to vid_from_reg().
> > See lm87.c or lm93.c. Can we safely ignore this, or do we have to retain
> > the parameter and use it if provided (ie if it is != 0) ?
> 
> I don't see that in lm87.c, I presume you meant lm78.c. For lm78 this
> is clearly a bug, the driver was written before multiple VRM versions
> existed, and was never adapter to support other VRM versions. So you
> can ignore it.
> 
> For lm93 the hard-coded value is because the chip support relative
> Vcore voltage limits (which adjust themselves as the VID code changes)
> and that can only work if the chip itself knows how to interpret the
> codes. As the LM93 only knows of VRM 10.x, This strongly suggested that
> the chip will never be used in combinations with other CPUs, thus the
> hard-coded VRM. I don't think it was so smart though, as the LM93 could
> still be used with other CPUs, simply the dynamic Vcore limit feature
> can't be used. Also note that the driver now supports the LM94 which
> _does_ support VRD 10 and VRD 11, so the hard-coded value is really
> incorrect.
> 
> In general, if a driver wants to only support a specific VRM value, it
> should now call vid_witch_vrm() and fail loading or limit its features
> if the value isn't what it wanted.
> 
Ok, makes sense.

> > On the plus side, if we retain the parameter and use it to override the
> > configured/calculated value, we can retain the API, and I can submit a
> > series of patches to remove vrm usage from the various drivers, instead
> > of a single large patch which does it all in one go. So this would have
> > some benefits.
> 
> I think we'll end up removing the extra parameter. But if you want to
> keep it for the time being because you feel more comfortable that way
> or you believe it offers a smoother update path, that's your call.
> 
> > > > +}
> > > >  EXPORT_SYMBOL(vid_which_vrm);
> > >
> > > There should be no user left for this function if you don't attempt to
> > > preserve the API.
> >
> > Actually, turns out there is at least one user - the value is used in
> > w83627ehf.c to configure the chip. Not sure how to do that if the driver
> > can not retrieve the vrm value.
> 
> Good point, vid_which_vrm must stay.
> 
> > Then there is vid_to_reg(), which is currently an inline function. Guess
> > I'll have to move that to hwmon-vid.c and export it.
> 
> Correct.
> 
Another key question: Can I remove the vrm attribute entirely from all drivers,
and/or can I even make it read-only, without going through the feature removal 
process ? It is documented in the ABI, and libsensors supports it.

Maybe I should only apply the patch to hwmon-vid.c now, and schedule vrm removal
for something like 2013 ?

Thanks,
Guenter

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [RFC PATCH 3/3] hwmon: (hwmon-vid) Determine vrm during initialization, and add vrm

[Jean Delvare]

On Wed, 1 Feb 2012 06:48:54 -0800, Guenter Roeck wrote:
> Another key question: Can I remove the vrm attribute entirely from all drivers,
> and/or can I even make it read-only, without going through the feature removal 
> process ? It is documented in the ABI, and libsensors supports it.

It is documented in the ABI as an attribute which can exist, not one
which is mandatory. Thus removing it shouldn't break anything.

As far as libsensors is concerned, version from lm-sensors 3.x does
_not_ know of VRM. At all. I already did not like the implementation
back when I converted libsensors to make use of the standard sysfs
interface, so I decided to not include it :)

Version from lm-sensors 2.x does support it, but at least in the latest
version it is printed through a common helper function (print_vid_info)
which will silently skip the VRM value if not readable. So we're safe
here too.

> Maybe I should only apply the patch to hwmon-vid.c now, and schedule vrm removal
> for something like 2013 ?

I'm fine both ways, thought 2013, is probably too far away. I really
have no problem with you killing this whole legacy thing right now if
you feel like it, but if you prefer to announce it now and do it in 6
months, that's up to you really.

-- 
Jean Delvare

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [RFC PATCH 3/3] hwmon: (hwmon-vid) Determine vrm during initialization, and add vrm

[Guenter Roeck]

On Wed, 2012-02-01 at 10:19 -0500, Jean Delvare wrote:
> On Wed, 1 Feb 2012 06:48:54 -0800, Guenter Roeck wrote:
> > Another key question: Can I remove the vrm attribute entirely from all drivers,
> > and/or can I even make it read-only, without going through the feature removal 
> > process ? It is documented in the ABI, and libsensors supports it.
> 
> It is documented in the ABI as an attribute which can exist, not one
> which is mandatory. Thus removing it shouldn't break anything.
> 
> As far as libsensors is concerned, version from lm-sensors 3.x does
> _not_ know of VRM. At all. I already did not like the implementation
> back when I converted libsensors to make use of the standard sysfs
> interface, so I decided to not include it :)
> 
> Version from lm-sensors 2.x does support it, but at least in the latest
> version it is printed through a common helper function (print_vid_info)
> which will silently skip the VRM value if not readable. So we're safe
> here too.
> 
> > Maybe I should only apply the patch to hwmon-vid.c now, and schedule vrm removal
> > for something like 2013 ?
> 
> I'm fine both ways, thought 2013, is probably too far away. I really
> have no problem with you killing this whole legacy thing right now if
> you feel like it, but if you prefer to announce it now and do it in 6
> months, that's up to you really.
> 
I am all with you. So I'll go ahead and just remove the code.

Another question: Several drivers read vid only once. That doesn't seem
to be correct; I thought the whole point of having vid is to have
dynamic voltage to the CPU. Should this be changed as well ? 

Guenter



_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [RFC PATCH 3/3] hwmon: (hwmon-vid) Determine vrm during initialization, and add vrm

[Jean Delvare]

On Wed, 1 Feb 2012 08:06:31 -0800, Guenter Roeck wrote:
> Another question: Several drivers read vid only once. That doesn't seem
> to be correct; I thought the whole point of having vid is to have
> dynamic voltage to the CPU. Should this be changed as well ? 

Congratulations Guenter, you just volunteered to convert all LPC-based
hwmon drivers to the MFD framework! :D

Seriously, there can be two reasons why VID is read only once:
* Old driver, predating frequency-scalable CPUs, and having never been
  updated. Not sure if there are such drivers remaining.
* LPC-based device with VID read from configuration space. Without
  proper locking this is dangerous, so we only do it once at driver
  load time. The only proper fix is to migrate the driver(s) to MFD
  first.

Good luck,
-- 
Jean Delvare

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [RFC PATCH 3/3] hwmon: (hwmon-vid) Determine vrm during initialization, and add vrm

[Guenter Roeck]

On Wed, 2012-02-01 at 11:19 -0500, Jean Delvare wrote:
> On Wed, 1 Feb 2012 08:06:31 -0800, Guenter Roeck wrote:
> > Another question: Several drivers read vid only once. That doesn't seem
> > to be correct; I thought the whole point of having vid is to have
> > dynamic voltage to the CPU. Should this be changed as well ? 
> 
> Congratulations Guenter, you just volunteered to convert all LPC-based
> hwmon drivers to the MFD framework! :D
> 
> Seriously, there can be two reasons why VID is read only once:
> * Old driver, predating frequency-scalable CPUs, and having never been
>   updated. Not sure if there are such drivers remaining.
> * LPC-based device with VID read from configuration space. Without
>   proper locking this is dangerous, so we only do it once at driver
>   load time. The only proper fix is to migrate the driver(s) to MFD
>   first.
> 
I think I'll rather leave that alone for now ;).

Guenter



_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors