Re: [PATCH 4/6] hwmon: sparx5: add LAN969x PWM support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] hwmon: redundant custom lock violates subsystem locking guidelines
--

commit 28a1c23dcff1b1ead30305d4b8a45894536ca4ec
Author: Robert Marko <robert.marko@sartura.hr>

    hwmon: sparx5: add LAN969x PWM support

This commit adds PWM support for the LAN969x fan controller, exposing pwm1
and pwm1_freq attributes. It implements a custom mutex to serialize fan
controller register access and parses standard PWM specifier properties from
the device tree to configure the hardware.

> diff --git a/drivers/hwmon/sparx5-temp.c b/drivers/hwmon/sparx5-temp.c
> index 922c3c33a049a..5130140e1b579 100644
> --- a/drivers/hwmon/sparx5-temp.c
> +++ b/drivers/hwmon/sparx5-temp.c
> @@ -41,8 +48,11 @@ struct s5_hwmon {
>  	void __iomem *base;
>  	void __iomem *fan;
>  	struct clk *clk;
> +	struct mutex lock; /* protects fan controller state and registers */

[Severity: Low]
Is this custom mutex necessary?

The hwmon subsystem core already serializes sysfs operations for attributes
registered with the info parameter of devm_hwmon_device_register_with_info()
via its own internal locking in hwmon_attr_show() and hwmon_attr_store().

If follow-up patches in the series introduce a thermal cooling device that
requires serialization against sysfs, the hwmon architectural guidelines
explicitly mandate using the hwmon_lock() and hwmon_unlock() API instead of
a private mutex.

Could this private lock be replaced with the hwmon core locking to avoid
violating the subsystem's API contract?

[ ... ]

> +static int s5_write_pwm_freq(struct s5_hwmon *hwmon, long val)
> +{
> +	unsigned long rate, divider;
> +	u32 data;
> +
> +	if (val <= 0)
> +		return -EINVAL;
> +
> +	rate = DIV_ROUND_CLOSEST(hwmon->clk_rate, FAN_PWM_FREQ_SCALE);
> +	if (val < DIV_ROUND_CLOSEST(rate, FIELD_MAX(FAN_PWM_FREQ_DIV)) ||
> +	    val > rate)
> +		return -ERANGE;
> +
> +	divider = DIV_ROUND_CLOSEST(rate, val);
> +	divider = min_t(unsigned long, divider, FIELD_MAX(FAN_PWM_FREQ_DIV));
> +
> +	mutex_lock(&hwmon->lock);

[Severity: Low]
Since s5_write_pwm_freq() is invoked via sysfs for an info-registered
attribute, the execution is already serialized by the hwmon core. There is
also no concurrency during probe() for this driver.

Does taking hwmon->lock here add redundant locking complexity that can be
safely removed?

> +	data = readl_relaxed(hwmon->fan + FAN_PWM_FREQ);
> +	data &= ~FAN_PWM_FREQ_DIV;
> +	data |= FIELD_PREP(FAN_PWM_FREQ_DIV, divider);
> +	writel_relaxed(data, hwmon->fan + FAN_PWM_FREQ);
> +	mutex_unlock(&hwmon->lock);
> +
> +	return 0;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261013125735.1542349-1-robert.marko@sartura.hr?part=4