[RFC PATCH linux v6 3/9] hwmon: occ: Add sysfs interface
Andrew Jeffery
andrew at aj.id.au
Tue Dec 13 12:17:50 AEDT 2016
Hi Eddie,
On Tue, 2016-11-29 at 17:03 -0600, eajames.ibm at gmail.com wrote:
> > From: "Edward A. James" <eajames at us.ibm.com>
>
> Add a generic mechanism to expose the sensors provided by the OCC in
> sysfs.
>
> > Signed-off-by: Edward A. James <eajames at us.ibm.com>
> ---
> drivers/hwmon/occ/Makefile | 2 +-
> drivers/hwmon/occ/occ_sysfs.c | 497 ++++++++++++++++++++++++++++++++++++++++++
> drivers/hwmon/occ/occ_sysfs.h | 51 +++++
> 3 files changed, 549 insertions(+), 1 deletion(-)
> create mode 100644 drivers/hwmon/occ/occ_sysfs.c
> create mode 100644 drivers/hwmon/occ/occ_sysfs.h
>
> diff --git a/drivers/hwmon/occ/Makefile b/drivers/hwmon/occ/Makefile
> index 93cb52f..a6881f9 100644
> --- a/drivers/hwmon/occ/Makefile
> +++ b/drivers/hwmon/occ/Makefile
> @@ -1 +1 @@
> -obj-$(CONFIG_SENSORS_PPC_OCC) += occ.o
> +obj-$(CONFIG_SENSORS_PPC_OCC) += occ.o occ_sysfs.o
> diff --git a/drivers/hwmon/occ/occ_sysfs.c b/drivers/hwmon/occ/occ_sysfs.c
> new file mode 100644
> index 0000000..aab4fbe
> --- /dev/null
> +++ b/drivers/hwmon/occ/occ_sysfs.c
> @@ -0,0 +1,497 @@
> +/*
> + * occ_sysfs.c - OCC sysfs interface
> + *
> + * This file contains the methods and data structures for the OCC hwmon driver.
> + *
> + * Copyright 2016 IBM Corp.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + */
> +
> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/slab.h>
> +#include <linux/jiffies.h>
> +#include <linux/hwmon.h>
> +#include <linux/hwmon-sysfs.h>
> +#include <linux/err.h>
> +#include <linux/mutex.h>
> +#include <linux/delay.h>
> +#include <linux/kernel.h>
> +#include <linux/device.h>
> +
> +#include "occ_sysfs.h"
> +
> +#define OCC_DATA_MAX 4096
> +
> +/* To generate attn to OCC */
> +#define ATTN_DATA 0x0006B035
> +/* For BMC to read/write SRAM */
> +#define OCB_ADDRESS 0x0006B070
> +#define OCB_DATA 0x0006B075
> +#define OCB_STATUS_CONTROL_AND 0x0006B072
> +#define OCB_STATUS_CONTROL_OR 0x0006B073
> +
> +#define RESP_DATA_LENGTH 3
> +#define RESP_HEADER_OFFSET 5
> +#define SENSOR_STR_OFFSET 37
> +#define SENSOR_BLOCK_NUM_OFFSET 43
> +#define SENSOR_BLOCK_OFFSET 45
> +
> +#define MAX_SENSOR_ATTR_LEN 32
A number of these #defines are unused. Please audit them and remove any
that are unnecessary.
> +
> +struct sensor_attr_data {
> + enum sensor_type type;
> + u32 hwmon_index;
> + u32 attr_id;
> + char name[MAX_SENSOR_ATTR_LEN];
> + struct device_attribute dev_attr;
> +};
> +
> +static ssize_t show_input(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + int val;
> + struct sensor_attr_data *sdata = container_of(attr,
> + struct sensor_attr_data,
> + dev_attr);
> + struct occ_sysfs *driver = dev_get_drvdata(dev);
> +
> + val = occ_get_sensor_value(driver->occ, sdata->type, sdata->hwmon_index - 1);
> + if (sdata->type == TEMP)
> + val *= 1000; /* in millidegree Celsius */
> +
> + return snprintf(buf, PAGE_SIZE - 1, "%d\n", val);
> +}
> +
> +static ssize_t show_label(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + int val;
> + struct sensor_attr_data *sdata = container_of(attr,
> + struct sensor_attr_data,
> + dev_attr);
> + struct occ_sysfs *driver = dev_get_drvdata(dev);
> +
> + val = occ_get_sensor_id(driver->occ, sdata->type,
> + sdata->hwmon_index - 1);
> +
> + return snprintf(buf, PAGE_SIZE - 1, "%d\n", val);
Can we improve this at all? What can I do with this number? For
instance, can I use it to look up the sensor in the OCC documentation?
The kernel documentation suggests this should be a text string, which I
guess it is once you render it down with snprintf, but that's a bit
obscure :)
At least, can you please add some comments about what we can expect
this value to mean?
> +}
> +
> +static ssize_t show_caps(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + int val;
> + struct caps_sensor *sensor;
> + struct sensor_attr_data *sdata = container_of(attr,
> + struct sensor_attr_data,
> + dev_attr);
> + struct occ_sysfs *driver = dev_get_drvdata(dev);
> +
> + sensor = occ_get_sensor(driver->occ, CAPS);
> + if (!sensor) {
> + val = -1;
> + return snprintf(buf, PAGE_SIZE - 1, "%d\n", val);
> + }
> +
> + val = occ_get_caps_value(driver->occ, sensor, sdata->hwmon_index - 1,
> + sdata->attr_id);
> +
> + return snprintf(buf, PAGE_SIZE - 1, "%d\n", val);
> +}
> +
> +static ssize_t show_update_interval(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct occ_sysfs *driver = dev_get_drvdata(dev);
> +
> + return snprintf(buf, PAGE_SIZE - 1, "%lu\n", driver->update_interval);
> +}
> +
> +static ssize_t store_update_interval(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf, size_t count)
> +{
> + struct occ_sysfs *driver = dev_get_drvdata(dev);
> + unsigned long val;
> + int rc;
> +
> + rc = kstrtoul(buf, 10, &val);
> + if (rc)
> + return rc;
> +
> + driver->update_interval = val;
> + occ_set_update_interval(driver->occ, val);
> +
> + return count;
> +}
> +
> +static DEVICE_ATTR(update_interval, S_IWUSR | S_IRUGO, show_update_interval,
> + store_update_interval);
> +
> +static ssize_t show_name(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + return snprintf(buf, PAGE_SIZE - 1, "occ\n");
> +}
> +
> +static DEVICE_ATTR(name, S_IRUGO, show_name, NULL);
> +
> +static ssize_t show_user_powercap(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct occ_sysfs *driver = dev_get_drvdata(dev);
> +
> + return snprintf(buf, PAGE_SIZE - 1, "%u\n", driver->user_powercap);
> +}
> +
> +static ssize_t store_user_powercap(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf, size_t count)
> +{
> + struct occ_sysfs *driver = dev_get_drvdata(dev);
> + u16 val;
> + int rc;
> +
> + rc = kstrtou16(buf, 10, &val);
> + if (rc)
> + return rc;
> +
> + dev_dbg(dev, "set user powercap to: %u\n", val);
> + rc = occ_set_user_powercap(driver->occ, val);
> + if (rc != 0) {
> + dev_err(dev,
> + "ERROR: Set User Powercap: wrong return status: %x\n",
Should probably be '0x%x'.
> + rc);
> + if (rc == 0x13)
> + dev_info(dev,
> + "ERROR: set invalid powercap value: %x\n",
As above.
> + val);
> + return -EINVAL;
> + }
> +
> + driver->user_powercap = val;
> +
> + return count;
> +}
> +
> +static DEVICE_ATTR(user_powercap, S_IWUSR | S_IRUGO, show_user_powercap,
> + store_user_powercap);
> +
> +static void deinit_sensor_groups(struct device *dev,
> + struct sensor_group *sensor_groups)
> +{
> + int cnt;
Nit: I'd prefer 'i' here
> +
> + for (cnt = 0; cnt < MAX_OCC_SENSOR_TYPE; cnt++) {
> + if (sensor_groups[cnt].group.attrs)
> + devm_kfree(dev, sensor_groups[cnt].group.attrs);
> + if (sensor_groups[cnt].sattr)
> + devm_kfree(dev, sensor_groups[cnt].sattr);
> + sensor_groups[cnt].group.attrs = NULL;
> + sensor_groups[cnt].sattr = NULL;
> + }
> +}
> +
> +static void sensor_attr_init(struct sensor_attr_data *sdata,
> + char *sensor_group_name,
> + char *attr_name,
> + ssize_t (*show)(struct device *dev,
> + struct device_attribute *attr,
> + char *buf))
> +{
> + sysfs_attr_init(&sdata->dev_attr.attr);
> +
> + snprintf(sdata->name, MAX_SENSOR_ATTR_LEN, "%s%d_%s",
> + sensor_group_name, sdata->hwmon_index, attr_name);
> + sdata->dev_attr.attr.name = sdata->name;
> + sdata->dev_attr.attr.mode = S_IRUGO;
> + sdata->dev_attr.show = show;
> +}
> +
> +static int create_sensor_group(struct occ_sysfs *driver,
> + enum sensor_type type, int sensor_num)
> +{
> + struct device *dev = driver->dev;
> + struct sensor_group *sensor_groups = driver->sensor_groups;
> + struct sensor_attr_data *sdata;
> + int rc, cnt;
> +
> + /* each sensor has 'label' and 'input' attributes */
> + sensor_groups[type].group.attrs =
> + devm_kzalloc(dev, sizeof(struct attribute *) *
> + sensor_num * 2 + 1, GFP_KERNEL);
> + if (!sensor_groups[type].group.attrs) {
> + rc = -ENOMEM;
> + goto err;
> + }
> +
> + sensor_groups[type].sattr =
> + devm_kzalloc(dev, sizeof(struct sensor_attr_data) *
> + sensor_num * 2, GFP_KERNEL);
> + if (!sensor_groups[type].sattr) {
> + rc = -ENOMEM;
> + goto err;
> + }
> +
> + for (cnt = 0; cnt < sensor_num; cnt++) {
Same nit as above - 'i' is pretty standard for this
> + sdata = &sensor_groups[type].sattr[cnt];
> + /* hwmon attributes index starts from 1 */
> + sdata->hwmon_index = cnt + 1;
> + sdata->type = type;
> + sensor_attr_init(sdata, sensor_groups[type].name, "input",
> + show_input);
> + sensor_groups[type].group.attrs[cnt] = &sdata->dev_attr.attr;
> +
> + sdata = &sensor_groups[type].sattr[cnt + sensor_num];
> + sdata->hwmon_index = cnt + 1;
> + sdata->type = type;
> + sensor_attr_init(sdata, sensor_groups[type].name, "label",
> + show_label);
> + sensor_groups[type].group.attrs[cnt + sensor_num] =
> + &sdata->dev_attr.attr;
> + }
> +
> + rc = sysfs_create_group(&dev->kobj, &sensor_groups[type].group);
> + if (rc)
> + goto err;
> +
> + return 0;
> +err:
> + deinit_sensor_groups(dev, sensor_groups);
> + return rc;
> +}
> +
> +static void caps_sensor_attr_init(struct sensor_attr_data *sdata,
> + char *attr_name, uint32_t hwmon_index,
> + uint32_t attr_id)
> +{
> + sdata->type = CAPS;
> + sdata->hwmon_index = hwmon_index;
> + sdata->attr_id = attr_id;
> +
> + snprintf(sdata->name, MAX_SENSOR_ATTR_LEN, "%s%d_%s",
> + "caps", sdata->hwmon_index, attr_name);
> +
> + sysfs_attr_init(&sdata->dev_attr.attr);
> + sdata->dev_attr.attr.name = sdata->name;
> + sdata->dev_attr.attr.mode = S_IRUGO;
> + sdata->dev_attr.show = show_caps;
> +}
> +
> +static int create_caps_sensor_group(struct occ_sysfs *driver,
> + int sensor_num)
> +{
> + struct device *dev = driver->dev;
> + struct sensor_group *sensor_groups = driver->sensor_groups;
> + int field_num = driver->num_caps_fields;
> + struct sensor_attr_data *sdata;
> + int i, j, rc;
> +
> + sensor_groups[CAPS].group.attrs =
> + devm_kzalloc(dev, sizeof(struct attribute *) * sensor_num *
> + field_num + 1, GFP_KERNEL);
> + if (!sensor_groups[CAPS].group.attrs) {
> + rc = -ENOMEM;
> + goto err;
> + }
> +
> + sensor_groups[CAPS].sattr =
> + devm_kzalloc(dev, sizeof(struct sensor_attr_data) *
> + sensor_num * field_num, GFP_KERNEL);
> + if (!sensor_groups[CAPS].sattr) {
> + rc = -ENOMEM;
> + goto err;
> + }
> +
> + for (j = 0; j < sensor_num; ++j) {
> + for (i = 0; i < field_num; ++i) {
> + sdata = &sensor_groups[CAPS].sattr[j * field_num + i];
> + caps_sensor_attr_init(sdata,
> + driver->caps_names[i],
> + j + 1, i);
> + sensor_groups[CAPS].group.attrs[j * field_num + i] =
> + &sdata->dev_attr.attr;
> + }
> + }
> +
> + rc = sysfs_create_group(&dev->kobj, &sensor_groups[CAPS].group);
> + if (rc)
> + goto err;
> +
> + return rc;
> +err:
> + deinit_sensor_groups(dev, sensor_groups);
> + return rc;
> +}
> +
> +static void occ_remove_hwmon_attrs(struct occ_sysfs *driver)
> +{
> + struct device *dev = driver->dev;
> +
> + device_remove_file(dev, &dev_attr_user_powercap);
> + device_remove_file(dev, &dev_attr_update_interval);
> + device_remove_file(dev, &dev_attr_name);
We may not have registered this and so unconditionally removing it felt
a bit questionable, but looking at the API's implementation the errors
are just swallowed anyway.
> +}
> +
> +static int occ_create_hwmon_attrs(struct occ_sysfs *driver)
> +{
> + int i, rc, id, sensor_num;
> + struct device *dev = driver->dev;
> + struct sensor_group *sensor_groups = driver->sensor_groups;
> + struct occ_blocks _resp, *resp = &_resp;
> + occ_get_response_blocks(driver->occ, resp);
> +
> + for (i = 0; i < MAX_OCC_SENSOR_TYPE; ++i)
> + resp->sensor_block_id[i] = -1;
> +
> + /* read sensor data from occ. */
> + rc = occ_update_device(driver->occ);
> + if (rc != 0) {
> + dev_err(dev, "ERROR: cannot get occ sensor data: %d\n", rc);
Please drop the "ERROR:" prefix and audit the rest of the patch for the
issue.
> + return rc;
> + }
> + if (!resp->blocks)
> + return -1;
Please pick a sensible standard error code to use here.
> +
> + rc = device_create_file(dev, &dev_attr_name);
> + if (rc)
> + goto error;
> +
> + rc = device_create_file(dev, &dev_attr_update_interval);
> + if (rc)
> + goto error;
> +
> + if (resp->sensor_block_id[CAPS] >= 0) {
> + /* user powercap: only for master OCC */
> + rc = device_create_file(dev, &dev_attr_user_powercap);
> + if (rc)
> + goto error;
> + }
> +
> + sensor_groups[FREQ].name = "freq";
> + sensor_groups[TEMP].name = "temp";
> + sensor_groups[POWER].name = "power";
> + sensor_groups[CAPS].name = "caps";
> +
> + for (i = 0; i < MAX_OCC_SENSOR_TYPE; i++) {
> + id = resp->sensor_block_id[i];
> + if (id < 0)
> + continue;
> +
> + sensor_num = resp->blocks[id].sensor_num;
> + if (i == CAPS)
> + rc = create_caps_sensor_group(driver, sensor_num);
> + else
> + rc = create_sensor_group(driver, i, sensor_num);
> + if (rc)
> + goto error;
> + }
> +
> + return 0;
> +
> +error:
> + dev_err(dev, "ERROR: cannot create hwmon attributes: %d\n", rc);
> + occ_remove_hwmon_attrs(driver);
> + return rc;
> +}
> +
> +static ssize_t show_occ_online(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct occ_sysfs *driver = dev_get_drvdata(dev);
> +
> + return snprintf(buf, PAGE_SIZE - 1, "%u\n", driver->occ_online);
> +}
> +
> +static ssize_t store_occ_online(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf, size_t count)
> +{
> + struct occ_sysfs *driver = dev_get_drvdata(dev);
> + unsigned long val;
> + int rc;
> +
> + rc = kstrtoul(buf, 10, &val);
> + if (rc)
> + return rc;
> +
> + if (val == 1) {
> + if (driver->occ_online)
> + return count;
> +
> + driver->dev = hwmon_device_register(dev);
> + if (IS_ERR(driver->dev))
> + return PTR_ERR(driver->dev);
> +
> + dev_set_drvdata(driver->dev, driver);
> +
> + rc = occ_create_hwmon_attrs(driver);
> + if (rc) {
> + hwmon_device_unregister(driver->dev);
> + driver->dev = NULL;
> + return rc;
> + }
> + } else if (val == 0) {
> + if (!driver->occ_online)
> + return count;
> +
> + occ_remove_hwmon_attrs(driver);
> + hwmon_device_unregister(driver->dev);
> + driver->dev = NULL;
> + } else
> + return -EINVAL;
> +
> + driver->occ_online = val;
> + return count;
> +}
> +
> +static DEVICE_ATTR(online, S_IWUSR | S_IRUGO, show_occ_online,
> + store_occ_online);
> +
> +struct occ_sysfs *occ_sysfs_start(struct device *dev, struct occ *occ,
> + struct occ_sysfs_config *config)
> +{
> + struct occ_sysfs *hwmon = devm_kzalloc(dev, sizeof(*hwmon), GFP_KERNEL);
> + int rc;
> +
> + if (!hwmon)
> + return ERR_PTR(-ENOMEM);
> +
> + hwmon->occ = occ;
> + hwmon->num_caps_fields = config->num_caps_fields;
> + hwmon->caps_names = config->caps_names;
> +
> + dev_set_drvdata(dev, hwmon);
> +
> + rc = device_create_file(dev, &dev_attr_online);
> + if (rc)
> + return ERR_PTR(rc);
> +
> + return hwmon;
> +}
> +EXPORT_SYMBOL(occ_sysfs_start);
> +
> +int occ_sysfs_stop(struct occ_sysfs *driver)
> +{
> + occ_remove_hwmon_attrs(driver);
> + hwmon_device_unregister(driver->dev);
> +
> + device_remove_file(driver->dev, &dev_attr_online);
> +
> + return 0;
> +}
> +EXPORT_SYMBOL(occ_sysfs_stop);
> +
> > +MODULE_AUTHOR("Eddie James <eajames at us.ibm.com>");
> +MODULE_DESCRIPTION("OCC sysfs driver");
> +MODULE_LICENSE("GPL");
> diff --git a/drivers/hwmon/occ/occ_sysfs.h b/drivers/hwmon/occ/occ_sysfs.h
> new file mode 100644
> index 0000000..e0219b6
> --- /dev/null
> +++ b/drivers/hwmon/occ/occ_sysfs.h
> @@ -0,0 +1,51 @@
> +#ifndef OCC_SYSFS_H
> +#define OCC_SYSFS_H
> +/*
> + * occ_sysfs.c - OCC sysfs interface
> + *
> + * This file contains the methods and data structures for the OCC hwmon driver.
> + *
> + * Copyright 2016 IBM Corp.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + */
> +
> +#include "occ.h"
> +
> +struct sensor_group {
> + char *name;
> + struct sensor_attr_data *sattr;
> + struct attribute_group group;
> +};
> +
> +struct occ_sysfs_config {
> + unsigned int num_caps_fields;
> + char **caps_names;
> +};
> +
> +struct occ_sysfs {
> + struct device *dev;
> + struct occ *occ;
> +
> + u16 user_powercap;
> + bool occ_online;
> + struct sensor_group sensor_groups[MAX_OCC_SENSOR_TYPE];
> + unsigned long update_interval;
> + unsigned int num_caps_fields;
> + char **caps_names;
> +};
> +
> +struct occ_sysfs *occ_sysfs_start(struct device *dev, struct occ *occ,
> + struct occ_sysfs_config *config);
> +
> +int occ_sysfs_stop(struct occ_sysfs *driver);
> +
> +#endif
Andrew
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: This is a digitally signed message part
URL: <http://lists.ozlabs.org/pipermail/openbmc/attachments/20161213/7afe06fd/attachment-0001.sig>
More information about the openbmc
mailing list