Re: [PATCH net-next v3 2/6] eth: fbnic: expose all hwmon attributes unconditionally as read-only

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider:

Critical: 0 · High: 0 · Medium: 0 · Low: 1

- [Low] fbnic_hwmon_ops keeps a constant-returning .is_visible callback
  instead of the core-provided static-visibility field.

--

eth: fbnic: expose all hwmon attributes unconditionally as read-only

This changes fbnic_hwmon_is_visible() so that it returns 0444 for every
sensor type and attribute instead of matching on hwmon_temp/hwmon_temp_input
and hwmon_in/hwmon_in_input. The stated motivation is to prepare for
threshold and alarm attributes added later in the series.

> diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_hwmon.c b/drivers/net/ethernet/meta/fbnic/fbnic_hwmon.c
> index 6c8c66ab86c1..38bb26cb8e6c 100644
> --- a/drivers/net/ethernet/meta/fbnic/fbnic_hwmon.c
> +++ b/drivers/net/ethernet/meta/fbnic/fbnic_hwmon.c
> @@ -20,12 +20,7 @@ static umode_t fbnic_hwmon_is_visible(const void *drvdata,
>  				      enum hwmon_sensor_types type,
>  				      u32 attr, int channel)
>  {
> -	if (type == hwmon_temp && attr == hwmon_temp_input)
> -		return 0444;
> -	if (type == hwmon_in && attr == hwmon_in_input)
> -		return 0444;
> -
> -	return 0;
> +	return 0444;
>  }

[Severity: Low]
This isn't a bug, but is the callback still needed at all here?  After this
change fbnic_hwmon_is_visible() ignores all three of its discriminating
parameters (type, attr and channel) and returns a constant, which is what
the static visibility field in struct hwmon_ops already covers:

include/linux/hwmon.h:
    struct hwmon_ops {
        umode_t visible;
        umode_t (*is_visible)(const void *drvdata, enum hwmon_sensor_types type,
                              u32 attr, int channel);

The kernel-doc there describes visible as "Static visibility. If non-zero,
'is_visible' is ignored", and is_visible as "Mandatory unless 'visible' is
non-zero".  The core honours this in hwmon_is_visible():

drivers/hwmon/hwmon.c:hwmon_is_visible() {
    if (ops->visible)
        return ops->visible;

    return ops->is_visible(drvdata, type, attr, channel);
}

and hwmon_device_register_with_info() accepts a chip that supplies only
ops->visible.

Would it be preferable to delete fbnic_hwmon_is_visible() and write:

    static const struct hwmon_ops fbnic_hwmon_ops = {
        .visible = 0444,
        .read = fbnic_hwmon_read,
    };

The callback is still a bare "return 0444;" at the end of the series
(d8d0db421cc5), so nothing later in the series appears to reintroduce
per-attribute discrimination.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260908225737.3034403-1-limzhineng2%40gmail.com