| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20171207231805.GA12517@roeck-us.net>
Date: Thu, 7 Dec 2017 15:18:05 -0800
From: Guenter Roeck <linux@...ck-us.net>
To: Eddie James <eajames@...ux.vnet.ibm.com>
Cc: linux-hwmon@...r.kernel.org, linux-kernel@...r.kernel.org,
jdelvare@...e.com, mspinler@...ux.vnet.ibm.com, joel@....id.au,
"Edward A. James" <eajames@...ibm.com>
Subject: Re: [PATCH 2/2] hwmon (pmbus): cffps: Add debugfs entries
On Thu, Dec 07, 2017 at 04:19:45PM -0600, Eddie James wrote:
>
>
> On 12/07/2017 02:53 PM, Guenter Roeck wrote:
> >On Thu, Dec 07, 2017 at 01:50:38PM -0600, Eddie James wrote:
> >>From: "Edward A. James" <eajames@...ibm.com>
> >>
> >>Add debugfs entries for additional power supply data, including part
> >>number, serial number, FRU number, firmware revision, ccin, and the
> >>input history of the power supply. The input history is 10 minutes of
> >>input power data in the form of twenty 30-second packets. Each packet
> >>contains average and maximum power for that 30 second period.
> >>
> >>Signed-off-by: Edward A. James <eajames@...ibm.com>
> >>---
> >> drivers/hwmon/pmbus/ibm-cffps.c | 199 +++++++++++++++++++++++++++++++++++++++-
> >> 1 file changed, 198 insertions(+), 1 deletion(-)
> >>
> >>diff --git a/drivers/hwmon/pmbus/ibm-cffps.c b/drivers/hwmon/pmbus/ibm-cffps.c
> >>index cb56da6..6d19399 100644
> >>--- a/drivers/hwmon/pmbus/ibm-cffps.c
> >>+++ b/drivers/hwmon/pmbus/ibm-cffps.c
> >>@@ -8,12 +8,26 @@
> >> */
> >> #include <linux/bitops.h>
> >>+#include <linux/debugfs.h>
> >> #include <linux/device.h>
> >>+#include <linux/fs.h>
> >> #include <linux/i2c.h>
> >>+#include <linux/jiffies.h>
> >> #include <linux/module.h>
> >>+#include <linux/mutex.h>
> >> #include "pmbus.h"
> >>+#define CFFPS_FRU_CMD 0x9A
> >>+#define CFFPS_PN_CMD 0x9B
> >>+#define CFFPS_SN_CMD 0x9E
> >>+#define CFFPS_CCIN_CMD 0xBD
> >>+#define CFFPS_FW_CMD_START 0xFA
> >>+#define CFFPS_FW_NUM_BYTES 4
> >>+
> >>+#define CFFPS_INPUT_HISTORY_CMD 0xD6
> >>+#define CFFPS_INPUT_HISTORY_SIZE 100
> >>+
> >> /* STATUS_MFR_SPECIFIC bits */
> >> #define CFFPS_MFR_FAN_FAULT BIT(0)
> >> #define CFFPS_MFR_THERMAL_FAULT BIT(1)
> >>@@ -24,6 +38,144 @@
> >> #define CFFPS_MFR_VAUX_FAULT BIT(6)
> >> #define CFFPS_MFR_CURRENT_SHARE_WARNING BIT(7)
> >>+enum {
> >>+ CFFPS_DEBUGFS_INPUT_HISTORY = 0,
> >>+ CFFPS_DEBUGFS_FRU,
> >>+ CFFPS_DEBUGFS_PN,
> >>+ CFFPS_DEBUGFS_SN,
> >>+ CFFPS_DEBUGFS_CCIN,
> >>+ CFFPS_DEBUGFS_FW,
> >>+ CFFPS_DEBUGFS_NUM_ENTRIES
> >>+};
> >>+
> >>+struct ibm_cffps_input_history {
> >>+ struct mutex update_lock;
> >>+ unsigned long last_update;
> >>+
> >>+ u8 byte_count;
> >>+ u8 data[CFFPS_INPUT_HISTORY_SIZE];
> >>+};
> >>+
> >>+struct ibm_cffps {
> >>+ struct i2c_client *client;
> >>+
> >>+ struct ibm_cffps_input_history input_history;
> >>+
> >>+ int debugfs_entries[CFFPS_DEBUGFS_NUM_ENTRIES];
> >>+};
> >>+
> >>+#define to_psu(x, y) container_of((x), struct ibm_cffps, debugfs_entries[(y)])
> >>+
> >>+static ssize_t ibm_cffps_read_input_history(struct ibm_cffps *psu,
> >>+ char __user *buf, size_t count,
> >>+ loff_t *ppos)
> >>+{
> >>+ int rc;
> >>+ u8 msgbuf0[1] = { CFFPS_INPUT_HISTORY_CMD };
> >>+ u8 msgbuf1[CFFPS_INPUT_HISTORY_SIZE + 1] = { 0 };
> >>+ struct i2c_msg msg[2] = {
> >>+ {
> >>+ .addr = psu->client->addr,
> >>+ .flags = psu->client->flags,
> >>+ .len = 1,
> >>+ .buf = msgbuf0,
> >>+ }, {
> >>+ .addr = psu->client->addr,
> >>+ .flags = psu->client->flags | I2C_M_RD,
> >>+ .len = CFFPS_INPUT_HISTORY_SIZE + 1,
> >>+ .buf = msgbuf1,
> >>+ },
> >>+ };
> >>+
> >>+ if (!*ppos) {
> >>+ mutex_lock(&psu->input_history.update_lock);
> >>+ if (time_after(jiffies, psu->input_history.last_update + HZ)) {
> >>+ /*
> >>+ * Use a raw i2c transfer, since we need more bytes
> >>+ * than Linux I2C supports through smbus xfr (only 32).
> >>+ */
> >>+ rc = i2c_transfer(psu->client->adapter, msg, 2);
> >>+ if (rc < 0) {
> >>+ mutex_unlock(&psu->input_history.update_lock);
> >>+ return rc;
> >>+ }
> >>+
> >>+ psu->input_history.byte_count = msgbuf1[0];
> >>+ memcpy(psu->input_history.data, &msgbuf1[1],
> >>+ CFFPS_INPUT_HISTORY_SIZE);
> >>+ psu->input_history.last_update = jiffies;
> >>+ }
> >>+
> >>+ mutex_unlock(&psu->input_history.update_lock);
> >>+ }
> >>+
> >>+ return simple_read_from_buffer(buf, count, ppos,
> >>+ psu->input_history.data,
> >>+ psu->input_history.byte_count);
> >>+}
> >>+
> >>+static ssize_t ibm_cffps_debugfs_op(struct file *file, char __user *buf,
> >>+ size_t count, loff_t *ppos)
> >>+{
> >>+ u8 cmd;
> >>+ int i, rc;
> >>+ int *idxp = file->private_data;
> >>+ int idx = *idxp;
> >>+ struct ibm_cffps *psu = to_psu(idxp, idx);
> >>+ char data[I2C_SMBUS_BLOCK_MAX] = { 0 };
> >>+
> >>+ switch (idx) {
> >>+ case CFFPS_DEBUGFS_INPUT_HISTORY:
> >>+ return ibm_cffps_read_input_history(psu, buf, count, ppos);
> >>+ case CFFPS_DEBUGFS_FRU:
> >>+ cmd = CFFPS_FRU_CMD;
> >>+ break;
> >>+ case CFFPS_DEBUGFS_PN:
> >>+ cmd = CFFPS_PN_CMD;
> >>+ break;
> >>+ case CFFPS_DEBUGFS_SN:
> >>+ cmd = CFFPS_SN_CMD;
> >>+ break;
> >>+ case CFFPS_DEBUGFS_CCIN:
> >>+ rc = i2c_smbus_read_word_data(psu->client, CFFPS_CCIN_CMD);
> >>+ if (rc < 0)
> >>+ return rc;
> >>+
> >>+ rc = snprintf(data, 5, "%04X", rc);
> >>+ goto done;
> >>+ case CFFPS_DEBUGFS_FW:
> >>+ for (i = 0; i < CFFPS_FW_NUM_BYTES; ++i) {
> >>+ rc = i2c_smbus_read_byte_data(psu->client,
> >>+ CFFPS_FW_CMD_START + i);
> >>+ if (rc < 0)
> >>+ return rc;
> >>+
> >>+ snprintf(&data[i * 2], 3, "%02X", rc);
> >>+ }
> >>+
> >>+ rc = i * 2;
> >>+ goto done;
> >>+ default:
> >>+ return -EINVAL;
> >>+ }
> >>+
> >>+ rc = i2c_smbus_read_block_data(psu->client, cmd, data);
> >>+ if (rc < 0)
> >>+ return rc;
> >>+
> >>+done:
> >>+ data[rc] = '\n';
> >>+ rc += 2;
> >>+
> >>+ return simple_read_from_buffer(buf, count, ppos, data, rc);
> >>+}
> >>+
> >>+static const struct file_operations ibm_cffps_fops = {
> >>+ .llseek = noop_llseek,
> >>+ .read = ibm_cffps_debugfs_op,
> >>+ .open = simple_open,
> >>+};
> >>+
> >> static int ibm_cffps_read_byte_data(struct i2c_client *client, int page,
> >> int reg)
> >> {
> >>@@ -119,7 +271,52 @@ static int ibm_cffps_read_word_data(struct i2c_client *client, int page,
> >> static int ibm_cffps_probe(struct i2c_client *client,
> >> const struct i2c_device_id *id)
> >> {
> >>- return pmbus_do_probe(client, id, &ibm_cffps_info);
> >>+ int i, rc;
> >>+ struct dentry *debugfs;
> >>+ struct ibm_cffps *psu;
> >>+
> >>+ rc = pmbus_do_probe(client, id, &ibm_cffps_info);
> >>+ if (rc)
> >>+ return rc;
> >>+
> >>+ /* Don't fail the probe if we can't create debugfs */
> >>+ psu = devm_kzalloc(&client->dev, sizeof(*psu), GFP_KERNEL);
> >>+ if (!psu)
> >>+ return 0;
> >>+
> >>+ psu->client = client;
> >>+ mutex_init(&psu->input_history.update_lock);
> >>+ if (jiffies > HZ)
> >>+ /* time_after wouldn't succeed with last_update = 0 in test */
> >... but you are setting it to 'jiffies' elsewhere, so it can still end up
> >being 0 at some point, especially since 'jiffies' starts with a large number
> >and wraps a few minutes after boot. Not sure I understand the problem,
> >actually. Can you elaborate ?
>
> Yea, not sure I understand it either, but basically, if we don't set
> last_update to jiffies - HZ or similar at start up, the check above for
> time_after(jiffies, last_upate + HZ) doesn't succeed soon after the boot.
> This doesn't make any sense to me, unless time_after is broken, but setting
> last_update to something relative to jiffies during probe fixes it.
>
That is easy; just set it unconditionally to jiffies - HZ.
The 'jiffies > HZ' check is wrong; if jiffies happens to be < HZ,
you'd still want to set the value to jiffies - HZ (and it will be
somewhere between ULONG_MAX-HZ+1 .. ULONG_MAX).
Problem is that jiffies doesn't start with 0 but with something large.
That is why drivers typically have a 'valid' flag in addition to
the jiffies check.
Thanks,
Guenter
Powered by blists - more mailing lists