[lm-sensors] [PATCH] hwmon: (thmc50) Safer device detection

Jean Delvare khali at linux-fr.org
Wed Jun 11 21:02:00 CEST 2008


Hi Krzysztof,

On Wed, 11 Jun 2008 20:34:29 +0200, Krzysztof Helt wrote:
> On Wed, 11 Jun 2008 17:29:04 +0200
> Jean Delvare <khali at linux-fr.org> wrote:
> 
> > Accepting all values >= 0xc0 as the ADM1022/THMC50 device ID is too
> > permissive. Only allow the ID range we know these devices may use.
> > 
> > Signed-off-by: Jean Delvare <khali at linux-fr.org>
> > Cc: Krzysztof Helt <krzysztof.h1 at wp.pl>
> > ---
> > Krzysztof, can you please review and ack this patch? Thanks.
> 
> The patch is simple but I don't see a point of this patch.

The point is to avoid misdetection of other chips as THMC50-compatible.
Your current check uses 11 bits, which isn't much. With my change we're
up to 12 bits, which is still not much, but a little bit better.

> The thmc50 datasheet states that the first thmc50 version has revision 0xc0
> and that the next version will be 0xd0.
> Today, we know that thmc51/adm1028 have revisions starting from
> 0xd0. The adm1028 datasheet states that the first adm1028 version is 0xd0
> and that the next version would be 0xe0.

Given what we know, the datasheet THMC50 and the ADM1022 are wrong.
0xd0 is used for the THMC51 and ADM1028, respectively, not the next
revision of the THMC50 and ADM1022. Which is pretty obvious when you
think of it: the 4 LSBs are there for the revision (or stepping). So
I'm reasonably certain that the ADM1028 datasheet is equally wrong.

> This open-ended aproach for the original thmc50 worked well for us, as 
> user with the thmc51 was able to use the thmc50 without any modification.
> It doesn't show everything correctly (the thmc51 hea one temperature less)
> but the thmc50 compatible functionality simply works.

That's true, but what would have happened if the THMC51 was not
compatible with the THMC50? Your driver writes to some registers at
initialization time (and then exposes more registers via sysfs), this
could have done some damage.

> I would leave this open-ended construction to support next version chips
> from this family. I admit that I am not aware if such a chip exists (from
> Texas Instrument or from Analog Devices).

Given how old these chips are, I'd guess not, we would probably have
heard of them by now. But OTOH I have to admit that the THMC51 took me
by surprise ;)

> The only reason to accept this patch is if there is a chip from another
> family which has revision value greater than 0xdf in the same register.
> In such a case, the patch gives as protection against wrong detection of chip type.
> Otherwise, it is just additional code either dead (no next chip version exists) 
> or disabling people with next generation of sensors from the thmc50 family
> (their chips won't be supported at all).

We have no proof that such devices do not exist. That's the whole point
of my patch. As I2C/SMBus doesn't offer standard device identification
mechanisms, the rule of thumb is: play it safe. We have module
parameters to force the i2c chip drivers to attach to arbitrary devices
if needed. I prefer having to point users to these parameters, than
having to explain to them why loading a given driver had unexpected side
effects.

Note that I didn't invent the code that is my patch, that's exactly
what sensors-detect does.

-- 
Jean Delvare




More information about the lm-sensors mailing list