Re: [PATCH v1 00/21] hwmon: Fix the type of 'config' in struct hwmon_channel_info to u64

["Russell King (Oracle)" <linux@armlinux.org.uk>]

On Tue, Jan 21, 2025 at 06:50:00AM -0800, Guenter Roeck wrote:
> On 1/21/25 06:12, Armin Wolf wrote:
> > Am 21.01.25 um 07:44 schrieb Huisong Li:
> > > The hwmon_device_register() is deprecated. When I try to repace it with
> > > hwmon_device_register_with_info() for acpi_power_meter driver, I found that
> > > the power channel attribute in linux/hwmon.h have to extend and is more
> > > than 32 after this replacement.
> > > 
> > > However, the maximum number of hwmon channel attributes is 32 which is
> > > limited by current hwmon codes. This is not good to add new channel
> > > attribute for some hwmon sensor type and support more channel attribute.
> > > 
> > > This series are aimed to do this. And also make sure that acpi_power_meter
> > > driver can successfully replace the deprecated hwmon_device_register()
> > > later.
> 
> This explanation completely misses the point. The series tries to make space
> for additional "standard" attributes. Such a change should be independent
> of a driver conversion and be discussed on its own, not in the context
> of a new driver or a driver conversion.

I think something needs to budge here, because I think what you're
asking is actually impossible!

One can't change the type of struct hwmon_channel_info.config to be a
u64 without also updating every hwmon driver that assigns to that
member.

This is not possible:

 struct hwmon_channel_info {
         enum hwmon_sensor_types type;
-        const u32 *config;
+        const u64 *config;
 };

static u32 marvell_hwmon_chip_config[] = {
...
};

static const struct hwmon_channel_info marvell_hwmon_chip = {
        .type = hwmon_chip,
        .config = marvell_hwmon_chip_config,
};

This assignment to .config will cause a warning/error with the above
change. If instead we do:

-	.config = marvell_hwmon_chip_config,
+	.config = (u64 *)marvell_hwmon_chip_config,

which would have to happen to every driver, then no, this also doesn't
work, because config[1] now points beyond the bounds of
marvell_hwmon_chip_config, which only has two u32 entries.

You can't just change the type of struct hwmon_channel_info.config
without patching every driver that assigns to
struct hwmon_channel_info.config as things currently stand.

The only way out of that would be:

1. convert *all* drivers that defines a config array to be defined by
   their own macro in hwmon.h, and then switch that macro to make the
   definitions be a u64 array at the same time as switching struct
    hwmon_channel_info.config

2. convert *all* drivers to use HWMON_CHANNEL_INFO() unconditionally,
   and switch that along with struct hwmon_channel_info.config.

3. add a new member to struct hwmon_channel_info such as
   "const u64 *config64" and then gradually convert drivers to use it.
   Once everyone is converted over, then remove "const u32 *config",
   optionally rename "config64" back to "config" and then re-patch all
   drivers. That'll be joyful, with multiple patches to drivers that
   need to be merged in sync with hwmon changes - and last over several
   kernel release cycles.

This is not going to be an easy change!

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!