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

["lihuisong (C)" <lihuisong@huawei.com>]

Hi

在 2025/1/22 1:20, Russell King (Oracle) 写道:
> 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!
Yeah, it's a very time-consuming method and not easy to change.

Although some attributes of acpi_power_meter, like power1_model_number, 
can not add to the generic hwmon power attributes,
I still don't think the maximum attribute number of one sensor type 
doesn't need to limit to 32.
We can not make sure that the current generic attributes can fully 
satisfy the useage in furture.


I got an idea. it may just need one patch in hwmon core, like the following:

-->

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

  #define HWMON_CHANNEL_INFO(stype, ...)         \
@@ -444,12 +447,22 @@ struct hwmon_channel_info {
                 }                               \
         })

+#define HWMON_CHANNEL_INFO64(stype, ...)               \
+       (&(const struct hwmon_channel_info) {   \
+               .type = hwmon_##stype,          \
+               .config64 = (const u64 []) {    \
+                       __VA_ARGS__, 0          \
+               }                               \
+       })
+
+
  /**
   * struct hwmon_chip_info - Chip configuration
   * @ops:       Pointer to hwmon operations.
   * @info:      Null-terminated list of channel information.
   */
  struct hwmon_chip_info {
+       bool hwmon_attribute_bit64; // use config64 pointer if it is true.
         const struct hwmon_ops *ops;
         const struct hwmon_channel_info * const *info;
  };


For hwmon core code, we can use the 'config' or 'confit64' and compute 
attribute number based on the 'hwmon_attribute_bit64' value.
New driver can use HWMON_CHANNEL_INFO64 macro. Old drivers are not 
supposed to need to have any modification.

/Huisong
>