linux-kernel - Re: [PATCH 1/3] hwmon: Add Texas Instruments TMP108 temperature sensor driver.

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <20161128233323.GA15459@roeck-us.net>
Date:   Mon, 28 Nov 2016 15:33:23 -0800
From:   Guenter Roeck <linux@...ck-us.net>
To:     John Muir <john@...ir.com>
Cc:     Jean Delvare <jdelvare@...e.com>, Jonathan Corbet <corbet@....net>,
        Mark Rutland <mark.rutland@....com>,
        Rob Herring <robh+dt@...nel.org>,
        Linux List <linux-kernel@...r.kernel.org>,
        linux-hwmon@...r.kernel.org, linux-doc@...r.kernel.org
Subject: Re: [PATCH 1/3] hwmon: Add Texas Instruments TMP108 temperature
 sensor driver.

On Mon, Nov 28, 2016 at 03:10:38PM -0800, John Muir wrote:
> On Nov 28, 2016, at 2:19 PM, Guenter Roeck <linux@...ck-us.net> wrote:
> > 
> > The tmp102 driver adds the timeout if the device was in shutdown mode (SD=1).
> > 
> > 	if (tmp102->config_orig & TMP102_CONF_SD) {
> > 		...
> > 		tmp102->ready_time += msecs_to_jiffies(CONVERSION_TIME_MS);
> > 	}
> > 
> > Your code adds the timeout if the device was in continuous operation mode (M1=1).
> > 
> > 	if ((tmp108->config & TMP108_CONF_MODE_MASK)
> > 	    == TMP108_MODE_CONTINUOUS) {
> > 		tmp108->ready_time +=
> > 		msecs_to_jiffies(TMP108_CONVERSION_TIME_MS);
> > 	}
> > 
> > Unless I am missing something, that is exactly the opposite.
> 
> Note that the TMP102 code is looking at ‘config_orig’ which was the initial device state, whereas in my proposed driver the code looks at the current configuration.
> 
Ah, yes. Your code also uses tmp108->config in tmp108_restore_config(),
which I guess confused me.

Not sure I understand why you would want to add the wait time even if M1
was set before (and the tmp102 driver doesn't do that). Really, if that
is what you want, it would be much simpler if you just unconditionally
set ready_time to 30 ms after the current time after setting the M1 bit,
no matter if it was set before or not. I personally don't think that would
be such a good idea, but it would at least be less obfuscating than the
current code. Also, I would suggest to either drop tmp108_restore_config(),
or to have it restore the real original state.

Thanks,
Guenter