linux-kernel - Re: [PATCH v4] drm/i915/hwmon: expose fan speed

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: <ZrYB-GI9L2RSc2bt@smile.fi.intel.com>
Date: Fri, 9 Aug 2024 14:48:08 +0300
From: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
To: Raag Jadav <raag.jadav@...el.com>
Cc: jani.nikula@...ux.intel.com, joonas.lahtinen@...ux.intel.com,
	rodrigo.vivi@...el.com, tursulin@...ulin.net, airlied@...il.com,
	daniel@...ll.ch, linux@...ck-us.net, andi.shyti@...ux.intel.com,
	intel-gfx@...ts.freedesktop.org, dri-devel@...ts.freedesktop.org,
	linux-hwmon@...r.kernel.org, linux-kernel@...r.kernel.org,
	anshuman.gupta@...el.com, badal.nilawar@...el.com,
	riana.tauro@...el.com, ashutosh.dixit@...el.com,
	karthik.poosa@...el.com
Subject: Re: [PATCH v4] drm/i915/hwmon: expose fan speed

On Fri, Aug 09, 2024 at 11:45:25AM +0530, Raag Jadav wrote:
> Add hwmon support for fan1_input attribute, which will expose fan speed
> in RPM. With this in place we can monitor fan speed using lm-sensors tool.
> 
> $ sensors
> i915-pci-0300
> Adapter: PCI adapter
> in0:         653.00 mV
> fan1:        3833 RPM
> power1:           N/A  (max =  43.00 W)
> energy1:      32.02 kJ

> v2:
> - Add mutex protection
> - Handle overflow
> - Add ABI documentation
> - Aesthetic adjustments (Riana)
> 
> v3:
> - Declare rotations as "long" and drop redundant casting
> - Change date and version in ABI documentation
> - Add commenter name in changelog (Riana)
> 
> v4:
> - Fix wakeref leak
> - Drop switch case and simplify hwm_fan_xx() (Andi)

I do not understand why we pollute Git history with changelogs, but it's
probably the ugly atavism in DRM workflow.

...

> +hwm_fan_is_visible(const struct hwm_drvdata *ddat, u32 attr)
> +{
> +	struct i915_hwmon *hwmon = ddat->hwmon;
> +
> +	return attr == hwmon_fan_input &&
> +	       i915_mmio_reg_valid(hwmon->rg.fan_speed) ? 0444 : 0;

Not sure why ternary here, it's not well readable in my opinion.

	if (attr == hwmon_fan_input && i915_mmio_reg_valid(hwmon->rg.fan_speed))
		return 0444;

	return 0;

looks better, no?

> +}

...

> +	/*
> +	 * HW register value is accumulated count of pulses from
> +	 * PWM fan with the scale of 2 pulses per rotation.
> +	 */
> +	rotations = pulses >> 1;

In accordance with the comment the

	rotations = pulses / 2;

looks better.

...

(1)

> +	time = time_now - fi->time_prev;
> +

I think location of this blank line is better at (1) above.

> +	if (unlikely(!time)) {
> +		ret = -EAGAIN;
> +		goto exit;
> +	}

...

> +	/* Convert to minutes for calculating RPM */
> +	*val = DIV_ROUND_UP(rotations * (60 * MSEC_PER_SEC), time);

Have you considered to keep jiffies in the fi and use something from jiffies.h
here? To me it feels like we multiply and divide when it can be avoided.
Please, think about it (I haven't checked myself, just an idea to share).

Also comment probably needs to be expanded to explain the formulas behind all
this.

-- 
With Best Regards,
Andy Shevchenko