Re: [PATCH 5/7] hwmon: Add driver for Marvel AlphaServer environmental sensors

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [High] hwmon: (marvel-env) link failure when
  CONFIG_ALPHA_LEGACY_START_ADDRESS=y
- [Low] hwmon: (marvel-env) redundant driver-private mutex instead of
  hwmon_lock()
--

commit 0d0046c1ab7829344c10cd3a7021d92b64106a5d
Author: Matt Turner <mattst88@gmail.com>

hwmon: Add driver for Marvel AlphaServer environmental sensors

This commit adds a hwmon driver for the temperature, fan, voltage, and
power supply sensors exposed by the SRM console configuration tree on
Marvel (EV7) AlphaServers.

> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index 18c682e1a4756..50a1bf966f30b 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -1255,6 +1255,17 @@ config SENSORS_MACSMC_HWMON
>  	  This driver can also be built as a module. If so, the module will
>  	  be called macsmc-hwmon.
>  
> +config SENSORS_MARVEL_ENV
> +	tristate "Marvel (EV7) AlphaServer environmental sensors"
> +	depends on ALPHA_MARVEL || ALPHA_GENERIC

[Severity: High]
Does this fail to link when CONFIG_ALPHA_GENERIC=y and
CONFIG_ALPHA_LEGACY_START_ADDRESS=y (as in alpha defconfig and
allmodconfig)?

marvel_env_refresh() calls marvel_cfgtree_partial_update() in
core_marvel.o, which is omitted on ALPHA_GENERIC builds when
CONFIG_ALPHA_LEGACY_START_ADDRESS is enabled:

arch/alpha/kernel/Makefile:
    ifdef CONFIG_ALPHA_GENERIC
    ...
    ifndef CONFIG_ALPHA_LEGACY_START_ADDRESS
    obj-y	 += core_marvel.o core_titan.o core_wildfire.o
    ...
    endif

Should this depend on ALPHA_MARVEL || (ALPHA_GENERIC &&
!ALPHA_LEGACY_START_ADDRESS)?

[ ... ]

> diff --git a/drivers/hwmon/marvel-env.c b/drivers/hwmon/marvel-env.c
> new file mode 100644
> index 0000000000000..cdc7e059e17bb
> --- /dev/null
> +++ b/drivers/hwmon/marvel-env.c

[ ... ]

> +struct marvel_env_data {
> +	struct device *dev;
> +	struct mutex lock;		/* serializes refreshes and reads */
> +	unsigned int update_interval;	/* ms */
> +	struct marvel_env_node *nodes;
> +	unsigned int nr_nodes;
> +	struct marvel_env_chan *chans[hwmon_max];
> +	unsigned int nr_chans[hwmon_max];
> +};

[ ... ]

> +static int marvel_env_read(struct device *dev, enum hwmon_sensor_types type,
> +			   u32 attr, int channel, long *val)
> +{
> +	struct marvel_env_data *data = dev_get_drvdata(dev);
> +	struct marvel_env_chan *c;
> +	struct marvel_env_node *n;
> +	unsigned int pkt;
> +	u8 status;
> +	int ret;
> +
> +	if (type == hwmon_chip) {
> +		*val = data->update_interval;
> +		return 0;
> +	}
> +
> +	c = &data->chans[type][channel];
> +	n = c->node;
> +	pkt = c->pkt;
> +
> +	mutex_lock(&data->lock);

[Severity: Low]
This isn't a bug, but is the driver-private data->lock mutex needed here
and in marvel_env_write()?

The hwmon core already serializes .read and .write callbacks under its
internal lock, and non-hwmon_ops paths (such as the event workqueue added
later in this series) can use hwmon_lock() and hwmon_unlock() instead of
maintaining a separate mutex.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261114-marvel-env-v1-0-890328e4b7e5@gmail.com?part=5