Message-ID: <20150107015955.GA14323@spacedout.fries.net>
Date:	Tue, 6 Jan 2015 19:59:55 -0600
From:	David Fries <David@...es.net>
To:	Mariusz Gorski <marius.gorski@...il.com>
Cc:	Evgeniy Polyakov <zbr@...emap.net>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 0/2] w1: slaves: w1_therm: Add sysfs entry for current
 temperature

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@...es.net>    PGP pub CB1EE8F0
http://fries.net/~david/
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists