Hello, On Thu, Oct 04, 2018 at 09:48:02AM -0700, Guenter Roeck wrote: > > > I would not object to adding trace support into the hwmon subsystem. > > > However, it should be tied to the new API. I would resist patches > > > adding trace support to individual hwmon drivers unless the new API > > > is used and additional driver specific trace support is warranted. > > > > Yes, my idea is to implement it with the _info API inside the hwmon > > core. What do you think about the mentioned solution? Would you be > > in favor of a polling work queue? > > > > "----------. Similar to tz->poll_queue in thermal_core, hwmon core > > could also have a work queue polling the registered sensor inputs > > (by default disabled; enabled only if users configure poll_delay) > > so that the power data can be generated to Ftrace outputs as well." > > > > I am not really in favor of it. This goes well beyond tracing. Tracing > by its nature should be non-invasive and impact the system as little as > possible. Adding a thread which polls thermal sensors, which are often > connected with a slow i2c interface or even blocking, is quite invasive. > > I don't mind adding tracing support to trace sensor access. Adding code > to poll thermal sensors on a regular basis is a completely different > beast. I am not convinced that this should really be done in the kernel. > The same could be accomplished with a simple loop from userspace. I ain't 100% convinced either. I think at this point we can just insert a trace event to the hwmon_attr_show(), unless there is a substantial polling queue in the hwmon core as thermal_core has, although I am not sure what would be a legit reason to add one. > while true; do > cat /sys/class/hwmon/hwmon1/temp1_input > sleep 1 > done The power/perf folks were asking about something hands-free, as neither thermal nor cpufreq requires extra readings or polling, but I feel this should work for them too, reluctantly though. > ... and you could actually trace those accesses in the kernel. > > Now, if the problem is added overhead due to requiring a sysfs access > for each sensor read, we can discuss introducing a different and more > efficient user-space ABI (such as adding a hwmon->iio bridge). > That would however be a different discussion. Yea, that's beyond the topic yet it sounds more interesting for certain people I guess, considering the fact that there are two ina2xx drivers in both hwmon and iio subsystems. > > > Note that this also applies to hwmon drivers registering through > > > thermal. The thermal subsystem calls the _info API but misuses it > > > to avoid a warning generated when using the old API. Of course, > > > I have no influence over the hwmon code in the thermal subsystem, > > > so whatever is done there is essentially wild-wild-west. > > > > I saw they have some obvious code in the hwmon core. If you want, > > we can keep the polling work queue and trace events away from it, > > which sounds plausible to me considering that thermal subsystem > > has its own polling work queue and trace events for sensor data. > > The code in the hwmon core is different. I am referring to hwmon code > in the thermal core. I see, though I can't foresee a conflict if we just add a trace event in the hwmon_attr_show(). And it seems, at least now, it passes a NULL chip pointer via the _info API. Thank you Nicolin
