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] [thread-next>] [day] [month] [year] [list]

Message-Id: <1398C020-EF5A-42F2-8A1A-7F6782DC3E4A@jmuir.com>
Date:   Mon, 28 Nov 2016 15:10:38 -0800
From:   John Muir <john@...ir.com>
To:     Guenter Roeck <linux@...ck-us.net>
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 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.

The update_ready_time function was only intended to be called AFTER the device has moved to a new mode. For the probe case I simply ignored the fact that it was already in continuous mode at startup and lazily re-used the update_ready_time function causing the code to run through a few extra instructions. I was attempting to re-use the logic in multiple cases.

I’ll update the code in my next patch series and you can re-review.

> Side note: Per datasheet, M0 is irrelevant if M1=1. The above check does not
> match M1=1, M0=1, but that condition would still reflect continuous mode.
> 

OK. Noted.

Thanks,

John.