<div dir="ltr">I can't comment on the driver implementation choices.  They have been reviewed and accepted by upstream.  I'm not sure what version of the driver you are looking at.</div><div class="gmail_extra"><br><div class="gmail_quote">On Fri, May 5, 2017 at 10:43 AM, Patrick Williams <span dir="ltr"><<a href="mailto:patrick@stwcx.xyz" target="_blank">patrick@stwcx.xyz</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Rick,<br>
<span class=""><br>
On Fri, May 05, 2017 at 09:48:01AM -0700, Rick Altherr wrote:<br>
> I've chatted with Patrick V. separately about the driver.  AST2400/2500 fan<br>
> tach hardware measures only one fan at a time.  I think we can adjust the<br>
> driver settings to reduce the measurement time but it will scale with # of<br>
> tachs being read.<br>
<br>
</span>I never looked at this driver before but it looks like it is doing an<br>
'msleep' in the hwmon read path after resetting a counter register and<br>
then counting rotations?  Two comments:<br>
<br>
1. As it stands, it doesn't appear that this driver is actually<br>
multi-reader safe (either thread or process).  There is no locking or<br>
queueing to prevent one reader from resetting the result register while<br>
another is performing the msleep loop.  Multi-threading might "go<br>
faster" but it will give entirely wrong results by my reading.<br>
<br>
2. It seems bad to do a long-running msleep in the hwmon read path to<br>
begin with.  Should this driver be restructured to have a kthread read<br>
the channels in the background on a polling interval instead of<br>
initiating by userspace action?<br>
<span class="HOEnZb"><font color="#888888"><br>
--<br>
Patrick Williams<br>
</font></span></blockquote></div><br></div>