Re: [PATCH v1 00/21] hwmon: Fix the type of 'config' in struct hwmon_channel_info to u64
From: lihuisong (C)
Date: Tue Jan 21 2025 - 23:07:05 EST
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