Receive the following warning while inserting module `i5k_amb`. A check of the 5.8.0-rc8 source tree shows this still in effect and 44 other modules with the same usage would return the same warnings. If the fix is similar to bug #192201 (which it seems to be semi-related to), then this should be a trivial fix and has been floating around since 4.4.x? ``` Jul 24 00:17:06 host systemd-modules-load[676]: Inserted module 'i5k_amb' Jul 24 00:17:06 host kernel: i5k_amb i5k_amb.0: hwmon_device_register() is deprecated. Please convert the driver to use hwmon_device_register_with_info(). ``` Cheers, --Rob
Have you still access to the hardware?
Note that the "trivial fix" is unacceptable here. It is a kludge/workaround that was accepted for the thermal subsystem because that subsystem registers with hwmon without providing a parent device and because it generates sysfs attributes on the fly, not while registering with the hwmon subsystem. That is not the case here.
This patch should fix the warning, however it is compile-tested only. Can you apply it with git to hwmon-next and report if everything works? From e0dd025664290700f88cd00b2a798426b1be5b18 Mon Sep 17 00:00:00 2001 From: Armin Wolf <W_Armin@gmx.de> Date: Sat, 2 Oct 2021 21:54:18 +0200 Subject: [PATCH] hwmon: (i5k_amb) Use new hwmon registration api Use devm_hwmon_device_register_with_info() to close bug bugzilla.kernel.org/show_bug.cgi?id=208693, which includes renaming tempX_mid/_max to tempX_max/_crit to comply with the hwmon sysfs standards. Signed-off-by: Armin Wolf <W_Armin@gmx.de> --- drivers/hwmon/i5k_amb.c | 519 +++++++++++++++++++--------------------- 1 file changed, 245 insertions(+), 274 deletions(-) diff --git a/drivers/hwmon/i5k_amb.c b/drivers/hwmon/i5k_amb.c index 783fa936e4d1..e706f533b0e1 100644 --- a/drivers/hwmon/i5k_amb.c +++ b/drivers/hwmon/i5k_amb.c @@ -9,7 +9,6 @@ #include <linux/module.h> #include <linux/hwmon.h> -#include <linux/hwmon-sysfs.h> #include <linux/err.h> #include <linux/mutex.h> #include <linux/log2.h> @@ -77,39 +76,21 @@ static unsigned long amb_reg_temp(unsigned int amb) * might prevent us from seeing the 16th DIMM in the channel. */ #define REAL_MAX_AMBS_PER_CHANNEL 15 -#define KNOBS_PER_AMB 6 -static unsigned long amb_num_from_reg(unsigned int byte_num, unsigned int bit) +static unsigned int amb_from_channel(int channel) { - return byte_num * MAX_AMBS_PER_CHANNEL + bit; -} + unsigned int mem_channel = channel / REAL_MAX_AMBS_PER_CHANNEL; -#define AMB_SYSFS_NAME_LEN 16 -struct i5k_device_attribute { - struct sensor_device_attribute s_attr; - char name[AMB_SYSFS_NAME_LEN]; -}; + return channel + mem_channel; +} struct i5k_amb_data { - struct device *hwmon_dev; - unsigned long amb_base; unsigned long amb_len; u16 amb_present[MAX_MEM_CHANNELS]; void __iomem *amb_mmio; - struct i5k_device_attribute *attrs; - unsigned int num_attrs; }; -static ssize_t name_show(struct device *dev, struct device_attribute *devattr, - char *buf) -{ - return sprintf(buf, "%s\n", DRVNAME); -} - - -static DEVICE_ATTR_RO(name); - static struct platform_device *amb_pdev; static u8 amb_read_byte(struct i5k_amb_data *data, unsigned long offset) @@ -123,276 +104,270 @@ static void amb_write_byte(struct i5k_amb_data *data, unsigned long offset, iowrite8(val, data->amb_mmio + offset); } -static ssize_t show_amb_alarm(struct device *dev, - struct device_attribute *devattr, - char *buf) +static umode_t i5k_is_visible(const void *drvdata, enum hwmon_sensor_types type, u32 attr, + int channel) { - struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr); - struct i5k_amb_data *data = dev_get_drvdata(dev); + unsigned int amb = amb_from_channel(channel); + const struct i5k_amb_data *data = drvdata; - if (!(amb_read_byte(data, amb_reg_temp_status(attr->index)) & 0x20) && - (amb_read_byte(data, amb_reg_temp_status(attr->index)) & 0x8)) - return sprintf(buf, "1\n"); - else - return sprintf(buf, "0\n"); -} - -static ssize_t store_amb_min(struct device *dev, - struct device_attribute *devattr, - const char *buf, - size_t count) -{ - struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr); - struct i5k_amb_data *data = dev_get_drvdata(dev); - unsigned long temp; - int ret = kstrtoul(buf, 10, &temp); - if (ret < 0) - return ret; - - temp = temp / 500; - if (temp > 255) - temp = 255; - - amb_write_byte(data, amb_reg_temp_min(attr->index), temp); - return count; -} - -static ssize_t store_amb_mid(struct device *dev, - struct device_attribute *devattr, - const char *buf, - size_t count) -{ - struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr); - struct i5k_amb_data *data = dev_get_drvdata(dev); - unsigned long temp; - int ret = kstrtoul(buf, 10, &temp); - if (ret < 0) - return ret; + switch (type) { + case hwmon_temp: + if (data->amb_present[amb >> CHANNEL_SHIFT] & BIT(amb & DIMM_MASK)) + break; - temp = temp / 500; - if (temp > 255) - temp = 255; + switch (attr) { + case hwmon_temp_input: + case hwmon_temp_alarm: + case hwmon_temp_label: + return 0444; + case hwmon_temp_min: + case hwmon_temp_max: + case hwmon_temp_crit: + return 0644; + default: + break; + } + break; + default: + break; + } - amb_write_byte(data, amb_reg_temp_mid(attr->index), temp); - return count; + return 0; } -static ssize_t store_amb_max(struct device *dev, - struct device_attribute *devattr, - const char *buf, - size_t count) +static int i5k_read(struct device *dev, enum hwmon_sensor_types type, u32 attr, int channel, + long *val) { - struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr); struct i5k_amb_data *data = dev_get_drvdata(dev); - unsigned long temp; - int ret = kstrtoul(buf, 10, &temp); - if (ret < 0) - return ret; - - temp = temp / 500; - if (temp > 255) - temp = 255; + unsigned int amb = amb_from_channel(channel); + + switch (type) { + case hwmon_temp: + switch (attr) { + case hwmon_temp_input: + *val = 500 * amb_read_byte(data, amb_reg_temp(amb)); + + return 0; + case hwmon_temp_min: + *val = 500 * amb_read_byte(data, amb_reg_temp_min(amb)); + + return 0; + case hwmon_temp_max: + *val = 500 * amb_read_byte(data, amb_reg_temp_mid(amb)); + + return 0; + case hwmon_temp_crit: + *val = 500 * amb_read_byte(data, amb_reg_temp_max(amb)); + + return 0; + case hwmon_temp_alarm: + if (!(amb_read_byte(data, amb_reg_temp_status(amb)) & 0x20) && + amb_read_byte(data, amb_reg_temp_status(amb)) & 0x8) + *val = 1; + else + *val = 0; + + return 0; + default: + break; + } + break; + default: + break; + } - amb_write_byte(data, amb_reg_temp_max(attr->index), temp); - return count; + return -EOPNOTSUPP; } -static ssize_t show_amb_min(struct device *dev, - struct device_attribute *devattr, - char *buf) -{ - struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr); - struct i5k_amb_data *data = dev_get_drvdata(dev); - return sprintf(buf, "%d\n", - 500 * amb_read_byte(data, amb_reg_temp_min(attr->index))); -} +static const char * const channel_name[] = { + "Ch. 0 DIMM 0", + "Ch. 0 DIMM 1", + "Ch. 0 DIMM 2", + "Ch. 0 DIMM 3", + "Ch. 0 DIMM 4", + "Ch. 0 DIMM 5", + "Ch. 0 DIMM 6", + "Ch. 0 DIMM 7", + "Ch. 0 DIMM 8", + "Ch. 0 DIMM 9", + "Ch. 0 DIMM 10", + "Ch. 0 DIMM 11", + "Ch. 0 DIMM 12", + "Ch. 0 DIMM 13", + "Ch. 0 DIMM 14", + "Ch. 1 DIMM 0", + "Ch. 1 DIMM 1", + "Ch. 1 DIMM 2", + "Ch. 1 DIMM 3", + "Ch. 1 DIMM 4", + "Ch. 1 DIMM 5", + "Ch. 1 DIMM 6", + "Ch. 1 DIMM 7", + "Ch. 1 DIMM 8", + "Ch. 1 DIMM 9", + "Ch. 1 DIMM 10", + "Ch. 1 DIMM 11", + "Ch. 1 DIMM 12", + "Ch. 1 DIMM 13", + "Ch. 1 DIMM 14", + "Ch. 2 DIMM 0", + "Ch. 2 DIMM 1", + "Ch. 2 DIMM 2", + "Ch. 2 DIMM 3", + "Ch. 2 DIMM 4", + "Ch. 2 DIMM 5", + "Ch. 2 DIMM 6", + "Ch. 2 DIMM 7", + "Ch. 2 DIMM 8", + "Ch. 2 DIMM 9", + "Ch. 2 DIMM 10", + "Ch. 2 DIMM 11", + "Ch. 2 DIMM 12", + "Ch. 2 DIMM 13", + "Ch. 2 DIMM 14", + "Ch. 3 DIMM 0", + "Ch. 3 DIMM 1", + "Ch. 3 DIMM 2", + "Ch. 3 DIMM 3", + "Ch. 3 DIMM 4", + "Ch. 3 DIMM 5", + "Ch. 3 DIMM 6", + "Ch. 3 DIMM 7", + "Ch. 3 DIMM 8", + "Ch. 3 DIMM 9", + "Ch. 3 DIMM 10", + "Ch. 3 DIMM 11", + "Ch. 3 DIMM 12", + "Ch. 3 DIMM 13", + "Ch. 3 DIMM 14", +}; -static ssize_t show_amb_mid(struct device *dev, - struct device_attribute *devattr, - char *buf) +static int i5k_read_string(struct device *dev, enum hwmon_sensor_types type, u32 attr, int channel, + const char **str) { - struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr); - struct i5k_amb_data *data = dev_get_drvdata(dev); - return sprintf(buf, "%d\n", - 500 * amb_read_byte(data, amb_reg_temp_mid(attr->index))); -} + *str = channel_name[channel]; -static ssize_t show_amb_max(struct device *dev, - struct device_attribute *devattr, - char *buf) -{ - struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr); - struct i5k_amb_data *data = dev_get_drvdata(dev); - return sprintf(buf, "%d\n", - 500 * amb_read_byte(data, amb_reg_temp_max(attr->index))); + return 0; } -static ssize_t show_amb_temp(struct device *dev, - struct device_attribute *devattr, - char *buf) +static int i5k_write(struct device *dev, enum hwmon_sensor_types type, u32 attr, int channel, + long val) { - struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr); struct i5k_amb_data *data = dev_get_drvdata(dev); - return sprintf(buf, "%d\n", - 500 * amb_read_byte(data, amb_reg_temp(attr->index))); -} + unsigned int amb = amb_from_channel(channel); + u8 temp = clamp_val(val / 500, 0, 255); -static ssize_t show_label(struct device *dev, - struct device_attribute *devattr, - char *buf) -{ - struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr); + switch (type) { + case hwmon_temp: + switch (attr) { + case hwmon_temp_min: + amb_write_byte(data, amb_reg_temp_min(amb), temp); - return sprintf(buf, "Ch. %d DIMM %d\n", attr->index >> CHANNEL_SHIFT, - attr->index & DIMM_MASK); -} + return 0; + case hwmon_temp_max: + amb_write_byte(data, amb_reg_temp_mid(amb), temp); -static int i5k_amb_hwmon_init(struct platform_device *pdev) -{ - int i, j, k, d = 0; - u16 c; - int res = 0; - int num_ambs = 0; - struct i5k_amb_data *data = platform_get_drvdata(pdev); + return 0; + case hwmon_temp_crit: + amb_write_byte(data, amb_reg_temp_max(amb), temp); - /* Count the number of AMBs found */ - /* ignore the high-order bit, see "Ugly hack" comment above */ - for (i = 0; i < MAX_MEM_CHANNELS; i++) - num_ambs += hweight16(data->amb_present[i] & 0x7fff); - - /* Set up sysfs stuff */ - data->attrs = kzalloc(array3_size(num_ambs, KNOBS_PER_AMB, - sizeof(*data->attrs)), - GFP_KERNEL); - if (!data->attrs) - return -ENOMEM; - data->num_attrs = 0; - - for (i = 0; i < MAX_MEM_CHANNELS; i++) { - c = data->amb_present[i]; - for (j = 0; j < REAL_MAX_AMBS_PER_CHANNEL; j++, c >>= 1) { - struct i5k_device_attribute *iattr; - - k = amb_num_from_reg(i, j); - if (!(c & 0x1)) - continue; - d++; - - /* sysfs label */ - iattr = data->attrs + data->num_attrs; - snprintf(iattr->name, AMB_SYSFS_NAME_LEN, - "temp%d_label", d); - iattr->s_attr.dev_attr.attr.name = iattr->name; - iattr->s_attr.dev_attr.attr.mode = 0444; - iattr->s_attr.dev_attr.show = show_label; - iattr->s_attr.index = k; - sysfs_attr_init(&iattr->s_attr.dev_attr.attr); - res = device_create_file(&pdev->dev, - &iattr->s_attr.dev_attr); - if (res) - goto exit_remove; - data->num_attrs++; - - /* Temperature sysfs knob */ - iattr = data->attrs + data->num_attrs; - snprintf(iattr->name, AMB_SYSFS_NAME_LEN, - "temp%d_input", d); - iattr->s_attr.dev_attr.attr.name = iattr->name; - iattr->s_attr.dev_attr.attr.mode = 0444; - iattr->s_attr.dev_attr.show = show_amb_temp; - iattr->s_attr.index = k; - sysfs_attr_init(&iattr->s_attr.dev_attr.attr); - res = device_create_file(&pdev->dev, - &iattr->s_attr.dev_attr); - if (res) - goto exit_remove; - data->num_attrs++; - - /* Temperature min sysfs knob */ - iattr = data->attrs + data->num_attrs; - snprintf(iattr->name, AMB_SYSFS_NAME_LEN, - "temp%d_min", d); - iattr->s_attr.dev_attr.attr.name = iattr->name; - iattr->s_attr.dev_attr.attr.mode = 0644; - iattr->s_attr.dev_attr.show = show_amb_min; - iattr->s_attr.dev_attr.store = store_amb_min; - iattr->s_attr.index = k; - sysfs_attr_init(&iattr->s_attr.dev_attr.attr); - res = device_create_file(&pdev->dev, - &iattr->s_attr.dev_attr); - if (res) - goto exit_remove; - data->num_attrs++; - - /* Temperature mid sysfs knob */ - iattr = data->attrs + data->num_attrs; - snprintf(iattr->name, AMB_SYSFS_NAME_LEN, - "temp%d_mid", d); - iattr->s_attr.dev_attr.attr.name = iattr->name; - iattr->s_attr.dev_attr.attr.mode = 0644; - iattr->s_attr.dev_attr.show = show_amb_mid; - iattr->s_attr.dev_attr.store = store_amb_mid; - iattr->s_attr.index = k; - sysfs_attr_init(&iattr->s_attr.dev_attr.attr); - res = device_create_file(&pdev->dev, - &iattr->s_attr.dev_attr); - if (res) - goto exit_remove; - data->num_attrs++; - - /* Temperature max sysfs knob */ - iattr = data->attrs + data->num_attrs; - snprintf(iattr->name, AMB_SYSFS_NAME_LEN, - "temp%d_max", d); - iattr->s_attr.dev_attr.attr.name = iattr->name; - iattr->s_attr.dev_attr.attr.mode = 0644; - iattr->s_attr.dev_attr.show = show_amb_max; - iattr->s_attr.dev_attr.store = store_amb_max; - iattr->s_attr.index = k; - sysfs_attr_init(&iattr->s_attr.dev_attr.attr); - res = device_create_file(&pdev->dev, - &iattr->s_attr.dev_attr); - if (res) - goto exit_remove; - data->num_attrs++; - - /* Temperature alarm sysfs knob */ - iattr = data->attrs + data->num_attrs; - snprintf(iattr->name, AMB_SYSFS_NAME_LEN, - "temp%d_alarm", d); - iattr->s_attr.dev_attr.attr.name = iattr->name; - iattr->s_attr.dev_attr.attr.mode = 0444; - iattr->s_attr.dev_attr.show = show_amb_alarm; - iattr->s_attr.index = k; - sysfs_attr_init(&iattr->s_attr.dev_attr.attr); - res = device_create_file(&pdev->dev, - &iattr->s_attr.dev_attr); - if (res) - goto exit_remove; - data->num_attrs++; + return 0; + default: + break; } + break; + default: + break; } - res = device_create_file(&pdev->dev, &dev_attr_name); - if (res) - goto exit_remove; - - data->hwmon_dev = hwmon_device_register(&pdev->dev); - if (IS_ERR(data->hwmon_dev)) { - res = PTR_ERR(data->hwmon_dev); - goto exit_remove; - } + return -EOPNOTSUPP; +} - return res; +static const struct hwmon_ops i5k_ops = { + .is_visible = i5k_is_visible, + .read = i5k_read, + .read_string = i5k_read_string, + .write = i5k_write, +}; -exit_remove: - device_remove_file(&pdev->dev, &dev_attr_name); - for (i = 0; i < data->num_attrs; i++) - device_remove_file(&pdev->dev, &data->attrs[i].s_attr.dev_attr); - kfree(data->attrs); +#define DIMM_FLAGS (HWMON_T_INPUT | HWMON_T_MIN | HWMON_T_MAX | HWMON_T_CRIT | HWMON_T_ALARM \ + | HWMON_T_LABEL) + +static const struct hwmon_channel_info *i5k_info[] = { + HWMON_CHANNEL_INFO(chip, HWMON_C_REGISTER_TZ), + HWMON_CHANNEL_INFO(temp, + /* Channel 1 */ + DIMM_FLAGS, + DIMM_FLAGS, + DIMM_FLAGS, + DIMM_FLAGS, + DIMM_FLAGS, + DIMM_FLAGS, + DIMM_FLAGS, + DIMM_FLAGS, + DIMM_FLAGS, + DIMM_FLAGS, + DIMM_FLAGS, + DIMM_FLAGS, + DIMM_FLAGS, + DIMM_FLAGS, + DIMM_FLAGS, + /* Channel 2 */ + DIMM_FLAGS, + DIMM_FLAGS, + DIMM_FLAGS, + DIMM_FLAGS, + DIMM_FLAGS, + DIMM_FLAGS, + DIMM_FLAGS, + DIMM_FLAGS, + DIMM_FLAGS, + DIMM_FLAGS, + DIMM_FLAGS, + DIMM_FLAGS, + DIMM_FLAGS, + DIMM_FLAGS, + DIMM_FLAGS, + /* Channel 3 */ + DIMM_FLAGS, + DIMM_FLAGS, + DIMM_FLAGS, + DIMM_FLAGS, + DIMM_FLAGS, + DIMM_FLAGS, + DIMM_FLAGS, + DIMM_FLAGS, + DIMM_FLAGS, + DIMM_FLAGS, + DIMM_FLAGS, + DIMM_FLAGS, + DIMM_FLAGS, + DIMM_FLAGS, + DIMM_FLAGS, + /* Channel 4 */ + DIMM_FLAGS, + DIMM_FLAGS, + DIMM_FLAGS, + DIMM_FLAGS, + DIMM_FLAGS, + DIMM_FLAGS, + DIMM_FLAGS, + DIMM_FLAGS, + DIMM_FLAGS, + DIMM_FLAGS, + DIMM_FLAGS, + DIMM_FLAGS, + DIMM_FLAGS, + DIMM_FLAGS, + DIMM_FLAGS + ), + NULL +}; - return res; -} +static const struct hwmon_chip_info i5k_chip_info = { + .ops = &i5k_ops, + .info = i5k_info, +}; static int i5k_amb_add(void) { @@ -413,8 +388,7 @@ static int i5k_amb_add(void) return res; } -static int i5k_find_amb_registers(struct i5k_amb_data *data, - unsigned long devid) +static int i5k_find_amb_registers(struct i5k_amb_data *data, unsigned long devid) { struct pci_dev *pcidev; u32 val32; @@ -498,6 +472,7 @@ MODULE_DEVICE_TABLE(pci, i5k_amb_ids); static int i5k_amb_probe(struct platform_device *pdev) { struct i5k_amb_data *data; + struct device *hwmon_dev; struct resource *reso; int i, res; @@ -540,7 +515,9 @@ static int i5k_amb_probe(struct platform_device *pdev) platform_set_drvdata(pdev, data); - res = i5k_amb_hwmon_init(pdev); + hwmon_dev = devm_hwmon_device_register_with_info(&pdev->dev, DRVNAME, data, &i5k_chip_info, + NULL); + res = PTR_ERR(hwmon_dev); if (res) goto err_init_failed; @@ -557,14 +534,8 @@ static int i5k_amb_probe(struct platform_device *pdev) static int i5k_amb_remove(struct platform_device *pdev) { - int i; struct i5k_amb_data *data = platform_get_drvdata(pdev); - hwmon_device_unregister(data->hwmon_dev); - device_remove_file(&pdev->dev, &dev_attr_name); - for (i = 0; i < data->num_attrs; i++) - device_remove_file(&pdev->dev, &data->attrs[i].s_attr.dev_attr); - kfree(data->attrs); iounmap(data->amb_mmio); release_mem_region(data->amb_base, data->amb_len); kfree(data); -- 2.20.1
Rob, are you still around and would you be able to test the patch? Armin, I suggest attaching patches instead of pasting them into comments. Attachments can be downloaded and applied much more easily. If Rob is no long around or no longer has the hardware, I suggest you simply post the patch to the linux-hwmon list and see if anyone there can test it, or review it.
@Jean, Indeed I'm still around (honestly I'd sorta forgotten about this bug), and pretty sure that I have that Dell server still kicking around. I'll take look in the next couple of days and see if I can find it. Depending on how things play out in the next week, I should be able to get it running again, and see about testing this under the 6.x kernel (and/or manually patching things and attaching a patch). @Armin, thank you for doing the heavy lifting and coding a patch. My C/C++ skills aren't anywhere good enough to write fresh code. But I can patch, build, and troubleshoot more than well enough. I'll report here any issues with the patch.
Created attachment 307556 [details] Prototype patch Compile-tested only!