[PATCH 0/2] w1: slaves: w1_therm: Add sysfs entry for current temperature

[PATCH 0/2] w1: slaves: w1_therm: Add sysfs entry for current temperature

[Mariusz Gorski]

DS18B20 and it's brothers are pretty popular in the RaspberryPi world
when it comes to temperature measurement. All tutorials on the Internet
use the same way of parsing the output of the w1_slave sysfs file.
These patches add a dedicated sysfs entry called 'temp' whose only job
is to output the current temperature.

What could be improved here is the way how dev->bus_mutex is locked/unlocked,
which is split right now between two functions - maybe it would be better
to move all dev->bus_mutex locking into read_rom() and introduce another
mutex for protecting the slave's structures (e.g. family_data).

Mariusz Gorski (2):
  w1: slaves: w1_therm: Extract read_rom function
  w1: slaves: w1_therm: Add temp attribute

 drivers/w1/slaves/w1_therm.c | 83 ++++++++++++++++++++++++++++++++++----------
 1 file changed, 65 insertions(+), 18 deletions(-)

-- 
2.2.1

[PATCH 1/2] w1: slaves: w1_therm: Extract read_rom function

[Mariusz Gorski]

Extract read_rom function to make it reusable by other attributes.

Signed-off-by: Mariusz Gorski <marius.gorski@gmail.com>
---
 drivers/w1/slaves/w1_therm.c | 51 ++++++++++++++++++++++++++++----------------
 1 file changed, 33 insertions(+), 18 deletions(-)

diff --git a/drivers/w1/slaves/w1_therm.c b/drivers/w1/slaves/w1_therm.c
index 1f11a20..3bad3d6 100644
--- a/drivers/w1/slaves/w1_therm.c
+++ b/drivers/w1/slaves/w1_therm.c
@@ -188,24 +188,20 @@ static inline int w1_convert_temp(u8 rom[9], u8 fid)
 }
 
 
-static ssize_t w1_slave_show(struct device *device,
-	struct device_attribute *attr, char *buf)
+/**
+ * read_rom() - Reads slave's 64-bit ROM + 8-bit checksum
+ * @device:	slave device to read from
+ * @rom:	buffer to write to
+ * Return:	negative value on errors, 0=read or checksum failure, 1=success
+ */
+static int read_rom(struct device *device, u8 rom[9])
 {
 	struct w1_slave *sl = dev_to_w1_slave(device);
 	struct w1_master *dev = sl->master;
-	u8 rom[9], crc, verdict, external_power;
 	int i, max_trying = 10;
-	ssize_t c = PAGE_SIZE;
-
-	i = mutex_lock_interruptible(&dev->bus_mutex);
-	if (i != 0)
-		return i;
-
-	memset(rom, 0, sizeof(rom));
+	u8 crc, external_power;
 
 	while (max_trying--) {
-
-		verdict = 0;
 		crc = 0;
 
 		if (!w1_reset_select_slave(sl)) {
@@ -245,7 +241,6 @@ static ssize_t w1_slave_show(struct device *device,
 			}
 
 			if (!w1_reset_select_slave(sl)) {
-
 				w1_write_8(dev, W1_READ_SCRATCHPAD);
 				if ((count = w1_read_block(dev, rom, 9)) != 9) {
 					dev_warn(device, "w1_read_block() "
@@ -256,18 +251,38 @@ static ssize_t w1_slave_show(struct device *device,
 				crc = w1_calc_crc8(rom, 8);
 
 				if (rom[8] == crc)
-					verdict = 1;
+					return 1;
 			}
 		}
-
-		if (verdict)
-			break;
 	}
 
+	return 0;
+}
+
+static ssize_t w1_slave_show(struct device *device,
+	struct device_attribute *attr, char *buf)
+{
+	struct w1_slave *sl = dev_to_w1_slave(device);
+	struct w1_master *dev = sl->master;
+	u8 rom[9], verdict;
+	int i;
+	ssize_t c = PAGE_SIZE;
+
+	i = mutex_lock_interruptible(&dev->bus_mutex);
+	if (i != 0)
+		return i;
+
+	memset(rom, 0, sizeof(rom));
+
+	verdict = read_rom(device, rom);
+	if (verdict < 0)
+		/* Propagate errors to upper layers */
+		return verdict;
+
 	for (i = 0; i < 9; ++i)
 		c -= snprintf(buf + PAGE_SIZE - c, c, "%02x ", rom[i]);
 	c -= snprintf(buf + PAGE_SIZE - c, c, ": crc=%02x %s\n",
-			   crc, (verdict) ? "YES" : "NO");
+			   w1_calc_crc8(rom, 8), (verdict) ? "YES" : "NO");
 	if (verdict)
 		memcpy(sl->family_data, rom, sizeof(rom));
 	else
-- 
2.2.1

[PATCH 2/2] w1: slaves: w1_therm: Add temp attribute

[Mariusz Gorski]

Add new attribute to simplify reading of current temperature.

Signed-off-by: Mariusz Gorski <marius.gorski@gmail.com>
---
 drivers/w1/slaves/w1_therm.c | 32 ++++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

diff --git a/drivers/w1/slaves/w1_therm.c b/drivers/w1/slaves/w1_therm.c
index 3bad3d6..65af193 100644
--- a/drivers/w1/slaves/w1_therm.c
+++ b/drivers/w1/slaves/w1_therm.c
@@ -76,10 +76,15 @@ static void w1_therm_remove_slave(struct w1_slave *sl)
 static ssize_t w1_slave_show(struct device *device,
 	struct device_attribute *attr, char *buf);
 
+static ssize_t temp_show(struct device *device,
+	struct device_attribute *attr, char *buf);
+
 static DEVICE_ATTR_RO(w1_slave);
+static DEVICE_ATTR_RO(temp);
 
 static struct attribute *w1_therm_attrs[] = {
 	&dev_attr_w1_slave.attr,
+	&dev_attr_temp.attr,
 	NULL,
 };
 ATTRIBUTE_GROUPS(w1_therm);
@@ -299,6 +304,33 @@ static ssize_t w1_slave_show(struct device *device,
 	return PAGE_SIZE - c;
 }
 
+static ssize_t temp_show(struct device *device,
+	struct device_attribute *attr, char *buf)
+{
+	struct w1_slave *sl = dev_to_w1_slave(device);
+	struct w1_master *dev = sl->master;
+	u8 rom[9], verdict;
+	int temp;
+
+	if (mutex_lock_interruptible(&dev->bus_mutex))
+		return -EINTR;
+
+	memset(rom, 0, sizeof(rom));
+
+	verdict = read_rom(device, rom);
+
+	if (verdict < 0)
+		return verdict;
+
+	mutex_unlock(&dev->bus_mutex);
+
+	if (!verdict)
+		return -EIO;
+
+	temp = w1_convert_temp(rom, sl->family->fid);
+	return snprintf(buf, PAGE_SIZE, "%d\n", temp);
+}
+
 static int __init w1_therm_init(void)
 {
 	int err, i;
-- 
2.2.1

Re: [PATCH 0/2] w1: slaves: w1_therm: Add sysfs entry for current temperature

[Richard Weinberger]

On Tue, Jan 6, 2015 at 3:29 PM, Mariusz Gorski <marius.gorski@gmail.com> wrote:
> DS18B20 and it's brothers are pretty popular in the RaspberryPi world
> when it comes to temperature measurement. All tutorials on the Internet
> use the same way of parsing the output of the w1_slave sysfs file.
> These patches add a dedicated sysfs entry called 'temp' whose only job
> is to output the current temperature.

And what is the benefit of this patches?

-- 
Thanks,
//richard

Re: [PATCH 0/2] w1: slaves: w1_therm: Add sysfs entry for current temperature

[Mariusz Gorski]

On Tue, Jan 06, 2015 at 04:01:47PM +0100, Richard Weinberger wrote:
> On Tue, Jan 6, 2015 at 3:29 PM, Mariusz Gorski <marius.gorski@gmail.com> wrote:
> > DS18B20 and it's brothers are pretty popular in the RaspberryPi world
> > when it comes to temperature measurement. All tutorials on the Internet
> > use the same way of parsing the output of the w1_slave sysfs file.
> > These patches add a dedicated sysfs entry called 'temp' whose only job
> > is to output the current temperature.
> 
> And what is the benefit of this patches?

Well, instead of having to parse the output of w1_slave:

$ cat w1_slave 
4d 01 55 00 7f ff 0c 10 fd : crc=fd YES
4d 01 55 00 7f ff 0c 10 fd t=20812

the userspace program gets only the interesting information,
which usually is the current temparture:

$ cat temp 
20812

> 
> -- 
> Thanks,
> //richard

Re: [PATCH 0/2] w1: slaves: w1_therm: Add sysfs entry for current temperature

[Richard Weinberger]

Am 06.01.2015 um 17:12 schrieb Mariusz Gorski:
> On Tue, Jan 06, 2015 at 04:01:47PM +0100, Richard Weinberger wrote:
>> On Tue, Jan 6, 2015 at 3:29 PM, Mariusz Gorski <marius.gorski@gmail.com> wrote:
>>> DS18B20 and it's brothers are pretty popular in the RaspberryPi world
>>> when it comes to temperature measurement. All tutorials on the Internet
>>> use the same way of parsing the output of the w1_slave sysfs file.
>>> These patches add a dedicated sysfs entry called 'temp' whose only job
>>> is to output the current temperature.
>>
>> And what is the benefit of this patches?
> 
> Well, instead of having to parse the output of w1_slave:
> 
> $ cat w1_slave 
> 4d 01 55 00 7f ff 0c 10 fd : crc=fd YES
> 4d 01 55 00 7f ff 0c 10 fd t=20812
> 
> the userspace program gets only the interesting information,
> which usually is the current temparture:

A sane user would also check the CRC.

IMHO it is up to the user to format the output for its needs.
Some may want "20812", some "20.8", some else maybe "20.8°C".
Let it up to the user.
Any trivial awk/cut/etc... oneliner would do it.

Thanks,
//richard

Re: [PATCH 2/2] w1: slaves: w1_therm: Add temp attribute

[Greg Kroah-Hartman]

On Tue, Jan 06, 2015 at 03:29:56PM +0100, Mariusz Gorski wrote:
> Add new attribute to simplify reading of current temperature.
> 
> Signed-off-by: Mariusz Gorski <marius.gorski@gmail.com>
> ---
>  drivers/w1/slaves/w1_therm.c | 32 ++++++++++++++++++++++++++++++++
>  1 file changed, 32 insertions(+)

You add sysfs attributes without adding new Documentation/ABI/ entries,
which means I can't take this patch series, sorry.  Please redo it and
add the needed information.

Also, temperature should be a standard sensor attribute, so you might
want to use that subsystem instead of creating your own ABI here.

thanks,

greg k-h

Re: [PATCH 2/2] w1: slaves: w1_therm: Add temp attribute

[Mariusz Gorski]

On Tue, Jan 06, 2015 at 09:42:21AM -0800, Greg Kroah-Hartman wrote:
> On Tue, Jan 06, 2015 at 03:29:56PM +0100, Mariusz Gorski wrote:
> > Add new attribute to simplify reading of current temperature.
> > 
> > Signed-off-by: Mariusz Gorski <marius.gorski@gmail.com>
> > ---
> >  drivers/w1/slaves/w1_therm.c | 32 ++++++++++++++++++++++++++++++++
> >  1 file changed, 32 insertions(+)
> 
> You add sysfs attributes without adding new Documentation/ABI/ entries,
> which means I can't take this patch series, sorry.  Please redo it and
> add the needed information.
> 
> Also, temperature should be a standard sensor attribute, so you might
> want to use that subsystem instead of creating your own ABI here.

Do you mean stuff described in Documentation/thermal/sysfs-api.txt? I am
not exactly sure which subsystem you mean.

> thanks,
> 
> greg k-h

Thanks,
Mariusz

Re: [PATCH 2/2] w1: slaves: w1_therm: Add temp attribute

[Greg Kroah-Hartman]

On Tue, Jan 06, 2015 at 09:19:07PM +0100, Mariusz Gorski wrote:
> On Tue, Jan 06, 2015 at 09:42:21AM -0800, Greg Kroah-Hartman wrote:
> > On Tue, Jan 06, 2015 at 03:29:56PM +0100, Mariusz Gorski wrote:
> > > Add new attribute to simplify reading of current temperature.
> > > 
> > > Signed-off-by: Mariusz Gorski <marius.gorski@gmail.com>
> > > ---
> > >  drivers/w1/slaves/w1_therm.c | 32 ++++++++++++++++++++++++++++++++
> > >  1 file changed, 32 insertions(+)
> > 
> > You add sysfs attributes without adding new Documentation/ABI/ entries,
> > which means I can't take this patch series, sorry.  Please redo it and
> > add the needed information.
> > 
> > Also, temperature should be a standard sensor attribute, so you might
> > want to use that subsystem instead of creating your own ABI here.
> 
> Do you mean stuff described in Documentation/thermal/sysfs-api.txt?

Yes, that is the one.

Re: [PATCH 0/2] w1: slaves: w1_therm: Add sysfs entry for current temperature

[David Fries]

On Tue, Jan 06, 2015 at 03:29:54PM +0100, Mariusz Gorski wrote:
> DS18B20 and it's brothers are pretty popular in the RaspberryPi world
> when it comes to temperature measurement. All tutorials on the Internet
> use the same way of parsing the output of the w1_slave sysfs file.
> These patches add a dedicated sysfs entry called 'temp' whose only job
> is to output the current temperature.
> 
> What could be improved here is the way how dev->bus_mutex is locked/unlocked,
> which is split right now between two functions - maybe it would be better
> to move all dev->bus_mutex locking into read_rom() and introduce another
> mutex for protecting the slave's structures (e.g. family_data).

As to adding another mutex, there are enough little mutexes around the
one wire system that it already feels like a mess and I tried to get 
all the race conditions that I could.

I think you don't understand what the bus_mutex protects.  It protects
other devices from accessing the one wire bus.  The way you coded it,
temp_show will allow only one temperature sensor to be accessed and
read at a time, considering it takes 750 ms that's holding up the bus
for a long time, and depending on how the devices are connected, isn't
needed.  If the temperature sensor is parasite powered, nothing else
can use the bus while it is converting, if it has dedicated power,
other slaves can be accessed at the same time.

I second using a standard sensor attribute if there is one appropriate
rather than here.  I didn't in my application, but then I wasn't 
adding a new method to get the same data, I was using the existing 
netlink method.  If it does get added to a standard sensor access
method, I do request that it be optional (like w1_therm which I don't
use), and not sit there polling the sensor for the current temperature
every X seconds when no one is using the data (OpenBSD does this, or
at least last I saw, and you have no idea how old the sensor data
reading is or have any control over how often it will poll it).

I have 15 DS18B20 temperature sensors scattered around the house on a 
single one wire network.  I don't use the w1_therm system, I talk
through netlink to the kernel.  This has the advantage of being an
asynchronous socket based interface so I can write a packet to request
each sensor to do a conversion, get the write command status, and then
after a timeout, write another to get a return, then read back the
data.

Using the sysfs means blocking while the read is in progress.  Reading
15 means either taking at least 11.250 seconds as you read them
serially, or having 15 threads blocked on one read each (provided they
don't block the other readers).

Thanks for Cc'ing me on this.

> Mariusz Gorski (2):
>   w1: slaves: w1_therm: Extract read_rom function
>   w1: slaves: w1_therm: Add temp attribute
> 
>  drivers/w1/slaves/w1_therm.c | 83 ++++++++++++++++++++++++++++++++++----------
>  1 file changed, 65 insertions(+), 18 deletions(-)
> 
> -- 
> 2.2.1

-- 
David Fries <david@fries.net>    PGP pub CB1EE8F0
http://fries.net/~david/

Re: [PATCH 2/2] w1: slaves: w1_therm: Add temp attribute

[David Fries]

On Tue, Jan 06, 2015 at 03:29:56PM +0100, Mariusz Gorski wrote:
> Add new attribute to simplify reading of current temperature.

> +static ssize_t temp_show(struct device *device,
> +	struct device_attribute *attr, char *buf)
> +{
> +	struct w1_slave *sl = dev_to_w1_slave(device);
> +	struct w1_master *dev = sl->master;
> +	u8 rom[9], verdict;
> +	int temp;
> +
> +	if (mutex_lock_interruptible(&dev->bus_mutex))
> +		return -EINTR;
> +
> +	memset(rom, 0, sizeof(rom));
> +
> +	verdict = read_rom(device, rom);
> +
> +	if (verdict < 0)
> +		return verdict;

I wanted to point out that this returns without unlocking bus_mutex.

-- 
David Fries <david@fries.net>    PGP pub CB1EE8F0
http://fries.net/~david/

Re: [PATCH 0/2] w1: slaves: w1_therm: Add sysfs entry for current temperature

[David Fries]

On Tue, Jan 06, 2015 at 05:50:32PM +0100, Richard Weinberger wrote:
> Am 06.01.2015 um 17:12 schrieb Mariusz Gorski:
> > On Tue, Jan 06, 2015 at 04:01:47PM +0100, Richard Weinberger wrote:
> >> On Tue, Jan 6, 2015 at 3:29 PM, Mariusz Gorski <marius.gorski@gmail.com> wrote:
> >>> DS18B20 and it's brothers are pretty popular in the RaspberryPi world
> >>> when it comes to temperature measurement. All tutorials on the Internet
> >>> use the same way of parsing the output of the w1_slave sysfs file.
> >>> These patches add a dedicated sysfs entry called 'temp' whose only job
> >>> is to output the current temperature.
> >>
> >> And what is the benefit of this patches?
> > 
> > Well, instead of having to parse the output of w1_slave:
> > 
> > $ cat w1_slave 
> > 4d 01 55 00 7f ff 0c 10 fd : crc=fd YES
> > 4d 01 55 00 7f ff 0c 10 fd t=20812
> > 
> > the userspace program gets only the interesting information,
> > which usually is the current temparture:
> 
> A sane user would also check the CRC.

Yes, and more because all 00's will pass a checksum, but be an invalid
reading, because there will be some bit in there outside of the two
temperature return bytes that won't be 0, hence the temperature field
can't be trusted.  All ff's will fail a checksum, but is useful to
know because that pretty much means the sensor is no longer there.

> IMHO it is up to the user to format the output for its needs.
> Some may want "20812", some "20.8", some else maybe "20.8°C".
> Let it up to the user.
> Any trivial awk/cut/etc... oneliner would do it.

I'm not fond of the format of w1_slave printing two sensor readings
and such, it's confusing.  Back in the day (2008) I at least
standardized the t= converted value to millidegrees C.  That was
because DS18B20 gave degrees C where DS18S20 gave millidegrees C, so
it's been worse.

> Thanks,
> //richard

-- 
David Fries <david@fries.net>    PGP pub CB1EE8F0
http://fries.net/~david/