linux-kernel - RE: [PATCH] Add support for the Philips SA56004 temperature sensor.

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [thread-next>] [day] [month] [year] [list]

Message-ID: <6E4D2678AC543844917CA081C9D6B33F049B1DE3@XMB-AMS-103.cisco.com>
Date:	Sat, 4 Jun 2011 18:32:01 +0200
From:	"Stijn Devriendt (sdevrien)" <sdevrien@...co.com>
To:	"anish singh" <anish198519851985@...il.com>
Cc:	<khali@...ux-fr.org>, <lm-sensors@...sensors.org>,
	<linux-kernel@...r.kernel.org>, <guenter.roeck@...csson.com>
Subject: RE: [PATCH] Add support for the Philips SA56004 temperature sensor.

> From: anish singh [mailto:anish198519851985@...il.com] 
>
> I am no expert on HWMON but just want to 
> add some points.
> @@ -454,7 +477,7 @@ static struct lm90_data *lm90_update_device(struct device *dev)
>
>               if (data->flags & LM90_HAVE_LOCAL_EXT) {
>                       lm90_read16(client, LM90_REG_R_LOCAL_TEMP,
> -                                   MAX6657_REG_R_LOCAL_TEMPL,
> +                                   data->reg_local_ext,
>                                   &data->temp11[4]);
> I don't think this variable reg_local_ext should exist as 
> register address should be "# defined" and should not be
> part of lm90_data but i do see a case here where we are
> assuming MAX6657 is only having this LM90_HAVE_LOCAL_EXT
> flag set.So i think we should have some more branching here
> to detect the device and pass the corresponding register but as
> i said i am no expert.
> 

Only MAX6657 and SA56004 have the local temperature extension
register and unfortunately they reside at different offsets.
Therefore the probing will detect the right chip and, if supported,
use the correct register.

>               } else {
>                       if (lm90_read_reg(client, LM90_REG_R_LOCAL_TEMP,
> @@ -1372,6 +1400,11 @@ static int lm90_probe(struct i2c_client *new_client,
>       /* Set maximum conversion rate */
>       data->max_convrate = lm90_params[data->kind].max_convrate;
>
> +       if (data->flags & LM90_HAVE_LOCAL_EXT) {
> +               data->reg_local_ext = lm90_params[data->kind].reg_local_ext;
> +               BUG_ON(data->reg_local_ext == 0);
> +       }
> +
> I think this BUG_ON is too harsh in probe.We generally use pr_err
> to print if something which is supposed to be set is not set.As BUG_ON
> will call kernel panic,right?

The reason for adding the BUG_ON rather than the error was that it is
in fact a coding error when the flag is set without specifying the offset.
Such a condition should never make it into a running system and should be
caught during coding or review.
BUG_ON only does an oops, panic is optional depending on panic_on_oops being
set.

Regards,
Stijn