* Re: [PATCH v3 -rcu] workqueue: Convert for_each_wq to use built-in list check
From: Paul E. McKenney @ 2019-08-16 16:45 UTC (permalink / raw)
To: Joel Fernandes (Google)
Cc: linux-kernel, Greg Kroah-Hartman, Jonathan Corbet, Josh Triplett,
Lai Jiangshan, linux-doc, Mathieu Desnoyers, Rafael J. Wysocki,
rcu, Steven Rostedt, Tejun Heo
In-Reply-To: <20190815141842.GB20599@google.com>
On Thu, Aug 15, 2019 at 10:18:42AM -0400, Joel Fernandes (Google) wrote:
> list_for_each_entry_rcu now has support to check for RCU reader sections
> as well as lock. Just use the support in it, instead of explicitly
> checking in the caller.
>
> Acked-by: Tejun Heo <tj@kernel.org>
> Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
Pulled into -rcu for testing and further review, thank you!
Thanx, Paul
> ---
> v1->v3: Changed lock_is_held() to lockdep_is_held()
>
> kernel/workqueue.c | 10 ++--------
> 1 file changed, 2 insertions(+), 8 deletions(-)
>
> diff --git a/kernel/workqueue.c b/kernel/workqueue.c
> index 601d61150b65..e882477ebf6e 100644
> --- a/kernel/workqueue.c
> +++ b/kernel/workqueue.c
> @@ -364,11 +364,6 @@ static void workqueue_sysfs_unregister(struct workqueue_struct *wq);
> !lockdep_is_held(&wq_pool_mutex), \
> "RCU or wq_pool_mutex should be held")
>
> -#define assert_rcu_or_wq_mutex(wq) \
> - RCU_LOCKDEP_WARN(!rcu_read_lock_held() && \
> - !lockdep_is_held(&wq->mutex), \
> - "RCU or wq->mutex should be held")
> -
> #define assert_rcu_or_wq_mutex_or_pool_mutex(wq) \
> RCU_LOCKDEP_WARN(!rcu_read_lock_held() && \
> !lockdep_is_held(&wq->mutex) && \
> @@ -425,9 +420,8 @@ static void workqueue_sysfs_unregister(struct workqueue_struct *wq);
> * ignored.
> */
> #define for_each_pwq(pwq, wq) \
> - list_for_each_entry_rcu((pwq), &(wq)->pwqs, pwqs_node) \
> - if (({ assert_rcu_or_wq_mutex(wq); false; })) { } \
> - else
> + list_for_each_entry_rcu((pwq), &(wq)->pwqs, pwqs_node, \
> + lockdep_is_held(&(wq->mutex)))
>
> #ifdef CONFIG_DEBUG_OBJECTS_WORK
>
> --
> 2.23.0.rc1.153.gdeed80330f-goog
>
^ permalink raw reply
* Re: [PATCH v5 2/2] hwmon: pmbus: Add Inspur Power System power supply driver
From: Guenter Roeck @ 2019-08-16 16:12 UTC (permalink / raw)
To: Vijay Khemka
Cc: John Wang, jdelvare@suse.com, corbet@lwn.net,
linux-hwmon@vger.kernel.org, linux-doc@vger.kernel.org,
linux-kernel@vger.kernel.org, openbmc@lists.ozlabs.org,
duanzhijia01@inspur.com, mine260309@gmail.com, joel@jms.id.au
In-Reply-To: <45938741-A766-405A-86E8-5C946B395F27@fb.com>
On Fri, Aug 16, 2019 at 04:01:05PM +0000, Vijay Khemka wrote:
>
>
> On 8/16/19, 3:20 AM, "openbmc on behalf of John Wang" <openbmc-bounces+vijaykhemka=fb.com@lists.ozlabs.org on behalf of wangzqbj@inspur.com> wrote:
>
> Add the driver to monitor Inspur Power System power supplies
> with hwmon over pmbus.
>
> This driver adds sysfs attributes for additional power supply data,
> including vendor, model, part_number, serial number,
> firmware revision, hardware revision, and psu mode(active/standby).
>
> Signed-off-by: John Wang <wangzqbj@inspur.com>
> ---
> v5:
> - Align sysfs attrs description in inspur-ipsps1.rst
> (Use tab instead of space to sperate names and values)
> v4:
> - Remove the additional tabs in the Makefile
> - Rebased on 5.3-rc4, not 5.2
> v3:
> - Sort kconfig/makefile entries alphabetically
> - Remove unnecessary initialization
> - Use ATTRIBUTE_GROUPS instead of expanding directly
> - Use memscan to avoid reimplementation
> v2:
> - Fix typos in commit message
> - Invert Christmas tree
> - Configure device with sysfs attrs, not debugfs entries
> - Fix errno in fw_version_read, ENODATA to EPROTO
> - Change the print format of fw-version
> - Use sysfs_streq instead of strcmp("xxx" "\n", "xxx")
> - Document sysfs attributes
> ---
> Documentation/hwmon/inspur-ipsps1.rst | 79 +++++++++
> drivers/hwmon/pmbus/Kconfig | 9 +
> drivers/hwmon/pmbus/Makefile | 1 +
> drivers/hwmon/pmbus/inspur-ipsps.c | 226 ++++++++++++++++++++++++++
> 4 files changed, 315 insertions(+)
> create mode 100644 Documentation/hwmon/inspur-ipsps1.rst
> create mode 100644 drivers/hwmon/pmbus/inspur-ipsps.c
>
> diff --git a/Documentation/hwmon/inspur-ipsps1.rst b/Documentation/hwmon/inspur-ipsps1.rst
> new file mode 100644
> index 000000000000..2b871ae3448f
> --- /dev/null
> +++ b/Documentation/hwmon/inspur-ipsps1.rst
> @@ -0,0 +1,79 @@
> +Kernel driver inspur-ipsps1
> +=======================
> +
> +Supported chips:
> +
> + * Inspur Power System power supply unit
> +
> +Author: John Wang <wangzqbj@inspur.com>
> +
> +Description
> +-----------
> +
> +This driver supports Inspur Power System power supplies. This driver
> +is a client to the core PMBus driver.
> +
> +Usage Notes
> +-----------
> +
> +This driver does not auto-detect devices. You will have to instantiate the
> +devices explicitly. Please see Documentation/i2c/instantiating-devices for
> +details.
> +
> +Sysfs entries
> +-------------
> +
> +The following attributes are supported:
> +
> +======================= ======================================================
> +curr1_input Measured input current
> +curr1_label "iin"
> +curr1_max Maximum current
> +curr1_max_alarm Current high alarm
> +curr2_input Measured output current in mA.
> +curr2_label "iout1"
> +curr2_crit Critical maximum current
> +curr2_crit_alarm Current critical high alarm
> +curr2_max Maximum current
> +curr2_max_alarm Current high alarm
> +
> +fan1_alarm Fan 1 warning.
> +fan1_fault Fan 1 fault.
> +fan1_input Fan 1 speed in RPM.
> +
> +in1_alarm Input voltage under-voltage alarm.
> +in1_input Measured input voltage in mV.
> +in1_label "vin"
> +in2_input Measured output voltage in mV.
> +in2_label "vout1"
> +in2_lcrit Critical minimum output voltage
> +in2_lcrit_alarm Output voltage critical low alarm
> +in2_max Maximum output voltage
> +in2_max_alarm Output voltage high alarm
> +in2_min Minimum output voltage
> +in2_min_alarm Output voltage low alarm
> +
> +power1_alarm Input fault or alarm.
> +power1_input Measured input power in uW.
> +power1_label "pin"
> +power1_max Input power limit
> +power2_max_alarm Output power high alarm
> +power2_max Output power limit
> +power2_input Measured output power in uW.
> +power2_label "pout"
> +
> +temp[1-3]_input Measured temperature
> +temp[1-2]_max Maximum temperature
> +temp[1-3]_max_alarm Temperature high alarm
> +
> +vendor Manufacturer name
> +model Product model
> +part_number Product part number
> +serial_number Product serial number
> +fw_version Firmware version
> +hw_version Hardware version
> +mode Work mode. Can be set to active or
> + standby, when set to standby, PSU will
> + automatically switch between standby
> + and redundancy mode.
> I don't think it is aligned yet. Please use space only instead of tabs.
>
First, it is aligned. It just appears unaligned in the patch because
'+' at the beginning of the lins shifts the output. We want the result
to be aligned, not the patch file.
Second, let's use tabs like every other hwmon documentation file. I don't
see the point of using spaces just to make the patch file appear aligned.
Unless there is serious feedback, I am going to apply this patch to
hwmon-next.
Guenter
> +======================= ======================================================
> diff --git a/drivers/hwmon/pmbus/Kconfig b/drivers/hwmon/pmbus/Kconfig
> index b6588483fae1..d62d69bb7e49 100644
> --- a/drivers/hwmon/pmbus/Kconfig
> +++ b/drivers/hwmon/pmbus/Kconfig
> @@ -46,6 +46,15 @@ config SENSORS_IBM_CFFPS
> This driver can also be built as a module. If so, the module will
> be called ibm-cffps.
>
> +config SENSORS_INSPUR_IPSPS
> + tristate "INSPUR Power System Power Supply"
> + help
> + If you say yes here you get hardware monitoring support for the INSPUR
> + Power System power supply.
> +
> + This driver can also be built as a module. If so, the module will
> + be called inspur-ipsps.
> +
> config SENSORS_IR35221
> tristate "Infineon IR35221"
> help
> diff --git a/drivers/hwmon/pmbus/Makefile b/drivers/hwmon/pmbus/Makefile
> index c950ea9a5d00..03bacfcfd660 100644
> --- a/drivers/hwmon/pmbus/Makefile
> +++ b/drivers/hwmon/pmbus/Makefile
> @@ -7,6 +7,7 @@ obj-$(CONFIG_PMBUS) += pmbus_core.o
> obj-$(CONFIG_SENSORS_PMBUS) += pmbus.o
> obj-$(CONFIG_SENSORS_ADM1275) += adm1275.o
> obj-$(CONFIG_SENSORS_IBM_CFFPS) += ibm-cffps.o
> +obj-$(CONFIG_SENSORS_INSPUR_IPSPS) += inspur-ipsps.o
> obj-$(CONFIG_SENSORS_IR35221) += ir35221.o
> obj-$(CONFIG_SENSORS_IR38064) += ir38064.o
> obj-$(CONFIG_SENSORS_IRPS5401) += irps5401.o
> diff --git a/drivers/hwmon/pmbus/inspur-ipsps.c b/drivers/hwmon/pmbus/inspur-ipsps.c
> new file mode 100644
> index 000000000000..fa981b881a60
> --- /dev/null
> +++ b/drivers/hwmon/pmbus/inspur-ipsps.c
> @@ -0,0 +1,226 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright 2019 Inspur Corp.
> + */
> +
> +#include <linux/debugfs.h>
> +#include <linux/device.h>
> +#include <linux/fs.h>
> +#include <linux/i2c.h>
> +#include <linux/module.h>
> +#include <linux/pmbus.h>
> +#include <linux/hwmon-sysfs.h>
> +
> +#include "pmbus.h"
> +
> +#define IPSPS_REG_VENDOR_ID 0x99
> +#define IPSPS_REG_MODEL 0x9A
> +#define IPSPS_REG_FW_VERSION 0x9B
> +#define IPSPS_REG_PN 0x9C
> +#define IPSPS_REG_SN 0x9E
> +#define IPSPS_REG_HW_VERSION 0xB0
> +#define IPSPS_REG_MODE 0xFC
> +
> +#define MODE_ACTIVE 0x55
> +#define MODE_STANDBY 0x0E
> +#define MODE_REDUNDANCY 0x00
> +
> +#define MODE_ACTIVE_STRING "active"
> +#define MODE_STANDBY_STRING "standby"
> +#define MODE_REDUNDANCY_STRING "redundancy"
> +
> +enum ipsps_index {
> + vendor,
> + model,
> + fw_version,
> + part_number,
> + serial_number,
> + hw_version,
> + mode,
> + num_regs,
> +};
> +
> +static const u8 ipsps_regs[num_regs] = {
> + [vendor] = IPSPS_REG_VENDOR_ID,
> + [model] = IPSPS_REG_MODEL,
> + [fw_version] = IPSPS_REG_FW_VERSION,
> + [part_number] = IPSPS_REG_PN,
> + [serial_number] = IPSPS_REG_SN,
> + [hw_version] = IPSPS_REG_HW_VERSION,
> + [mode] = IPSPS_REG_MODE,
> +};
> +
> +static ssize_t ipsps_string_show(struct device *dev,
> + struct device_attribute *devattr,
> + char *buf)
> +{
> + u8 reg;
> + int rc;
> + char *p;
> + char data[I2C_SMBUS_BLOCK_MAX + 1];
> + struct i2c_client *client = to_i2c_client(dev->parent);
> + struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> +
> + reg = ipsps_regs[attr->index];
> + rc = i2c_smbus_read_block_data(client, reg, data);
> + if (rc < 0)
> + return rc;
> +
> + /* filled with printable characters, ending with # */
> + p = memscan(data, '#', rc);
> + *p = '\0';
> +
> + return snprintf(buf, PAGE_SIZE, "%s\n", data);
> +}
> +
> +static ssize_t ipsps_fw_version_show(struct device *dev,
> + struct device_attribute *devattr,
> + char *buf)
> +{
> + u8 reg;
> + int rc;
> + u8 data[I2C_SMBUS_BLOCK_MAX] = { 0 };
> + struct i2c_client *client = to_i2c_client(dev->parent);
> + struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> +
> + reg = ipsps_regs[attr->index];
> + rc = i2c_smbus_read_block_data(client, reg, data);
> + if (rc < 0)
> + return rc;
> +
> + if (rc != 6)
> + return -EPROTO;
> +
> + return snprintf(buf, PAGE_SIZE, "%u.%02u%u-%u.%02u\n",
> + data[1], data[2]/* < 100 */, data[3]/*< 10*/,
> + data[4], data[5]/* < 100 */);
> +}
> +
> +static ssize_t ipsps_mode_show(struct device *dev,
> + struct device_attribute *devattr, char *buf)
> +{
> + u8 reg;
> + int rc;
> + struct i2c_client *client = to_i2c_client(dev->parent);
> + struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> +
> + reg = ipsps_regs[attr->index];
> + rc = i2c_smbus_read_byte_data(client, reg);
> + if (rc < 0)
> + return rc;
> +
> + switch (rc) {
> + case MODE_ACTIVE:
> + return snprintf(buf, PAGE_SIZE, "[%s] %s %s\n",
> + MODE_ACTIVE_STRING,
> + MODE_STANDBY_STRING, MODE_REDUNDANCY_STRING);
> + case MODE_STANDBY:
> + return snprintf(buf, PAGE_SIZE, "%s [%s] %s\n",
> + MODE_ACTIVE_STRING,
> + MODE_STANDBY_STRING, MODE_REDUNDANCY_STRING);
> + case MODE_REDUNDANCY:
> + return snprintf(buf, PAGE_SIZE, "%s %s [%s]\n",
> + MODE_ACTIVE_STRING,
> + MODE_STANDBY_STRING, MODE_REDUNDANCY_STRING);
> + default:
> + return snprintf(buf, PAGE_SIZE, "unspecified\n");
> + }
> +}
> +
> +static ssize_t ipsps_mode_store(struct device *dev,
> + struct device_attribute *devattr,
> + const char *buf, size_t count)
> +{
> + u8 reg;
> + int rc;
> + struct i2c_client *client = to_i2c_client(dev->parent);
> + struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> +
> + reg = ipsps_regs[attr->index];
> + if (sysfs_streq(MODE_STANDBY_STRING, buf)) {
> + rc = i2c_smbus_write_byte_data(client, reg,
> + MODE_STANDBY);
> + if (rc < 0)
> + return rc;
> + return count;
> + } else if (sysfs_streq(MODE_ACTIVE_STRING, buf)) {
> + rc = i2c_smbus_write_byte_data(client, reg,
> + MODE_ACTIVE);
> + if (rc < 0)
> + return rc;
> + return count;
> + }
> +
> + return -EINVAL;
> +}
> +
> +static SENSOR_DEVICE_ATTR_RO(vendor, ipsps_string, vendor);
> +static SENSOR_DEVICE_ATTR_RO(model, ipsps_string, model);
> +static SENSOR_DEVICE_ATTR_RO(part_number, ipsps_string, part_number);
> +static SENSOR_DEVICE_ATTR_RO(serial_number, ipsps_string, serial_number);
> +static SENSOR_DEVICE_ATTR_RO(hw_version, ipsps_string, hw_version);
> +static SENSOR_DEVICE_ATTR_RO(fw_version, ipsps_fw_version, fw_version);
> +static SENSOR_DEVICE_ATTR_RW(mode, ipsps_mode, mode);
> +
> +static struct attribute *ipsps_attrs[] = {
> + &sensor_dev_attr_vendor.dev_attr.attr,
> + &sensor_dev_attr_model.dev_attr.attr,
> + &sensor_dev_attr_part_number.dev_attr.attr,
> + &sensor_dev_attr_serial_number.dev_attr.attr,
> + &sensor_dev_attr_hw_version.dev_attr.attr,
> + &sensor_dev_attr_fw_version.dev_attr.attr,
> + &sensor_dev_attr_mode.dev_attr.attr,
> + NULL,
> +};
> +
> +ATTRIBUTE_GROUPS(ipsps);
> +
> +static struct pmbus_driver_info ipsps_info = {
> + .pages = 1,
> + .func[0] = PMBUS_HAVE_VIN | PMBUS_HAVE_VOUT | PMBUS_HAVE_IOUT |
> + PMBUS_HAVE_IIN | PMBUS_HAVE_POUT | PMBUS_HAVE_PIN |
> + PMBUS_HAVE_FAN12 | PMBUS_HAVE_TEMP | PMBUS_HAVE_TEMP2 |
> + PMBUS_HAVE_TEMP3 | PMBUS_HAVE_STATUS_VOUT |
> + PMBUS_HAVE_STATUS_IOUT | PMBUS_HAVE_STATUS_INPUT |
> + PMBUS_HAVE_STATUS_TEMP | PMBUS_HAVE_STATUS_FAN12,
> + .groups = ipsps_groups,
> +};
> +
> +static struct pmbus_platform_data ipsps_pdata = {
> + .flags = PMBUS_SKIP_STATUS_CHECK,
> +};
> +
> +static int ipsps_probe(struct i2c_client *client,
> + const struct i2c_device_id *id)
> +{
> + client->dev.platform_data = &ipsps_pdata;
> + return pmbus_do_probe(client, id, &ipsps_info);
> +}
> +
> +static const struct i2c_device_id ipsps_id[] = {
> + { "inspur_ipsps1", 0 },
> + {}
> +};
> +MODULE_DEVICE_TABLE(i2c, ipsps_id);
> +
> +static const struct of_device_id ipsps_of_match[] = {
> + { .compatible = "inspur,ipsps1" },
> + {}
> +};
> +MODULE_DEVICE_TABLE(of, ipsps_of_match);
> +
> +static struct i2c_driver ipsps_driver = {
> + .driver = {
> + .name = "inspur-ipsps",
> + .of_match_table = ipsps_of_match,
> + },
> + .probe = ipsps_probe,
> + .remove = pmbus_do_remove,
> + .id_table = ipsps_id,
> +};
> +
> +module_i2c_driver(ipsps_driver);
> +
> +MODULE_AUTHOR("John Wang");
> +MODULE_DESCRIPTION("PMBus driver for Inspur Power System power supplies");
> +MODULE_LICENSE("GPL");
> --
> 2.17.1
>
>
>
^ permalink raw reply
* Re: [PATCH v5 2/2] hwmon: pmbus: Add Inspur Power System power supply driver
From: Vijay Khemka @ 2019-08-16 16:01 UTC (permalink / raw)
To: John Wang, jdelvare@suse.com, linux@roeck-us.net, corbet@lwn.net,
linux-hwmon@vger.kernel.org, linux-doc@vger.kernel.org,
linux-kernel@vger.kernel.org, openbmc@lists.ozlabs.org,
duanzhijia01@inspur.com, mine260309@gmail.com, joel@jms.id.au
In-Reply-To: <20190816101944.3586-1-wangzqbj@inspur.com>
On 8/16/19, 3:20 AM, "openbmc on behalf of John Wang" <openbmc-bounces+vijaykhemka=fb.com@lists.ozlabs.org on behalf of wangzqbj@inspur.com> wrote:
Add the driver to monitor Inspur Power System power supplies
with hwmon over pmbus.
This driver adds sysfs attributes for additional power supply data,
including vendor, model, part_number, serial number,
firmware revision, hardware revision, and psu mode(active/standby).
Signed-off-by: John Wang <wangzqbj@inspur.com>
---
v5:
- Align sysfs attrs description in inspur-ipsps1.rst
(Use tab instead of space to sperate names and values)
v4:
- Remove the additional tabs in the Makefile
- Rebased on 5.3-rc4, not 5.2
v3:
- Sort kconfig/makefile entries alphabetically
- Remove unnecessary initialization
- Use ATTRIBUTE_GROUPS instead of expanding directly
- Use memscan to avoid reimplementation
v2:
- Fix typos in commit message
- Invert Christmas tree
- Configure device with sysfs attrs, not debugfs entries
- Fix errno in fw_version_read, ENODATA to EPROTO
- Change the print format of fw-version
- Use sysfs_streq instead of strcmp("xxx" "\n", "xxx")
- Document sysfs attributes
---
Documentation/hwmon/inspur-ipsps1.rst | 79 +++++++++
drivers/hwmon/pmbus/Kconfig | 9 +
drivers/hwmon/pmbus/Makefile | 1 +
drivers/hwmon/pmbus/inspur-ipsps.c | 226 ++++++++++++++++++++++++++
4 files changed, 315 insertions(+)
create mode 100644 Documentation/hwmon/inspur-ipsps1.rst
create mode 100644 drivers/hwmon/pmbus/inspur-ipsps.c
diff --git a/Documentation/hwmon/inspur-ipsps1.rst b/Documentation/hwmon/inspur-ipsps1.rst
new file mode 100644
index 000000000000..2b871ae3448f
--- /dev/null
+++ b/Documentation/hwmon/inspur-ipsps1.rst
@@ -0,0 +1,79 @@
+Kernel driver inspur-ipsps1
+=======================
+
+Supported chips:
+
+ * Inspur Power System power supply unit
+
+Author: John Wang <wangzqbj@inspur.com>
+
+Description
+-----------
+
+This driver supports Inspur Power System power supplies. This driver
+is a client to the core PMBus driver.
+
+Usage Notes
+-----------
+
+This driver does not auto-detect devices. You will have to instantiate the
+devices explicitly. Please see Documentation/i2c/instantiating-devices for
+details.
+
+Sysfs entries
+-------------
+
+The following attributes are supported:
+
+======================= ======================================================
+curr1_input Measured input current
+curr1_label "iin"
+curr1_max Maximum current
+curr1_max_alarm Current high alarm
+curr2_input Measured output current in mA.
+curr2_label "iout1"
+curr2_crit Critical maximum current
+curr2_crit_alarm Current critical high alarm
+curr2_max Maximum current
+curr2_max_alarm Current high alarm
+
+fan1_alarm Fan 1 warning.
+fan1_fault Fan 1 fault.
+fan1_input Fan 1 speed in RPM.
+
+in1_alarm Input voltage under-voltage alarm.
+in1_input Measured input voltage in mV.
+in1_label "vin"
+in2_input Measured output voltage in mV.
+in2_label "vout1"
+in2_lcrit Critical minimum output voltage
+in2_lcrit_alarm Output voltage critical low alarm
+in2_max Maximum output voltage
+in2_max_alarm Output voltage high alarm
+in2_min Minimum output voltage
+in2_min_alarm Output voltage low alarm
+
+power1_alarm Input fault or alarm.
+power1_input Measured input power in uW.
+power1_label "pin"
+power1_max Input power limit
+power2_max_alarm Output power high alarm
+power2_max Output power limit
+power2_input Measured output power in uW.
+power2_label "pout"
+
+temp[1-3]_input Measured temperature
+temp[1-2]_max Maximum temperature
+temp[1-3]_max_alarm Temperature high alarm
+
+vendor Manufacturer name
+model Product model
+part_number Product part number
+serial_number Product serial number
+fw_version Firmware version
+hw_version Hardware version
+mode Work mode. Can be set to active or
+ standby, when set to standby, PSU will
+ automatically switch between standby
+ and redundancy mode.
I don't think it is aligned yet. Please use space only instead of tabs.
+======================= ======================================================
diff --git a/drivers/hwmon/pmbus/Kconfig b/drivers/hwmon/pmbus/Kconfig
index b6588483fae1..d62d69bb7e49 100644
--- a/drivers/hwmon/pmbus/Kconfig
+++ b/drivers/hwmon/pmbus/Kconfig
@@ -46,6 +46,15 @@ config SENSORS_IBM_CFFPS
This driver can also be built as a module. If so, the module will
be called ibm-cffps.
+config SENSORS_INSPUR_IPSPS
+ tristate "INSPUR Power System Power Supply"
+ help
+ If you say yes here you get hardware monitoring support for the INSPUR
+ Power System power supply.
+
+ This driver can also be built as a module. If so, the module will
+ be called inspur-ipsps.
+
config SENSORS_IR35221
tristate "Infineon IR35221"
help
diff --git a/drivers/hwmon/pmbus/Makefile b/drivers/hwmon/pmbus/Makefile
index c950ea9a5d00..03bacfcfd660 100644
--- a/drivers/hwmon/pmbus/Makefile
+++ b/drivers/hwmon/pmbus/Makefile
@@ -7,6 +7,7 @@ obj-$(CONFIG_PMBUS) += pmbus_core.o
obj-$(CONFIG_SENSORS_PMBUS) += pmbus.o
obj-$(CONFIG_SENSORS_ADM1275) += adm1275.o
obj-$(CONFIG_SENSORS_IBM_CFFPS) += ibm-cffps.o
+obj-$(CONFIG_SENSORS_INSPUR_IPSPS) += inspur-ipsps.o
obj-$(CONFIG_SENSORS_IR35221) += ir35221.o
obj-$(CONFIG_SENSORS_IR38064) += ir38064.o
obj-$(CONFIG_SENSORS_IRPS5401) += irps5401.o
diff --git a/drivers/hwmon/pmbus/inspur-ipsps.c b/drivers/hwmon/pmbus/inspur-ipsps.c
new file mode 100644
index 000000000000..fa981b881a60
--- /dev/null
+++ b/drivers/hwmon/pmbus/inspur-ipsps.c
@@ -0,0 +1,226 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright 2019 Inspur Corp.
+ */
+
+#include <linux/debugfs.h>
+#include <linux/device.h>
+#include <linux/fs.h>
+#include <linux/i2c.h>
+#include <linux/module.h>
+#include <linux/pmbus.h>
+#include <linux/hwmon-sysfs.h>
+
+#include "pmbus.h"
+
+#define IPSPS_REG_VENDOR_ID 0x99
+#define IPSPS_REG_MODEL 0x9A
+#define IPSPS_REG_FW_VERSION 0x9B
+#define IPSPS_REG_PN 0x9C
+#define IPSPS_REG_SN 0x9E
+#define IPSPS_REG_HW_VERSION 0xB0
+#define IPSPS_REG_MODE 0xFC
+
+#define MODE_ACTIVE 0x55
+#define MODE_STANDBY 0x0E
+#define MODE_REDUNDANCY 0x00
+
+#define MODE_ACTIVE_STRING "active"
+#define MODE_STANDBY_STRING "standby"
+#define MODE_REDUNDANCY_STRING "redundancy"
+
+enum ipsps_index {
+ vendor,
+ model,
+ fw_version,
+ part_number,
+ serial_number,
+ hw_version,
+ mode,
+ num_regs,
+};
+
+static const u8 ipsps_regs[num_regs] = {
+ [vendor] = IPSPS_REG_VENDOR_ID,
+ [model] = IPSPS_REG_MODEL,
+ [fw_version] = IPSPS_REG_FW_VERSION,
+ [part_number] = IPSPS_REG_PN,
+ [serial_number] = IPSPS_REG_SN,
+ [hw_version] = IPSPS_REG_HW_VERSION,
+ [mode] = IPSPS_REG_MODE,
+};
+
+static ssize_t ipsps_string_show(struct device *dev,
+ struct device_attribute *devattr,
+ char *buf)
+{
+ u8 reg;
+ int rc;
+ char *p;
+ char data[I2C_SMBUS_BLOCK_MAX + 1];
+ struct i2c_client *client = to_i2c_client(dev->parent);
+ struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
+
+ reg = ipsps_regs[attr->index];
+ rc = i2c_smbus_read_block_data(client, reg, data);
+ if (rc < 0)
+ return rc;
+
+ /* filled with printable characters, ending with # */
+ p = memscan(data, '#', rc);
+ *p = '\0';
+
+ return snprintf(buf, PAGE_SIZE, "%s\n", data);
+}
+
+static ssize_t ipsps_fw_version_show(struct device *dev,
+ struct device_attribute *devattr,
+ char *buf)
+{
+ u8 reg;
+ int rc;
+ u8 data[I2C_SMBUS_BLOCK_MAX] = { 0 };
+ struct i2c_client *client = to_i2c_client(dev->parent);
+ struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
+
+ reg = ipsps_regs[attr->index];
+ rc = i2c_smbus_read_block_data(client, reg, data);
+ if (rc < 0)
+ return rc;
+
+ if (rc != 6)
+ return -EPROTO;
+
+ return snprintf(buf, PAGE_SIZE, "%u.%02u%u-%u.%02u\n",
+ data[1], data[2]/* < 100 */, data[3]/*< 10*/,
+ data[4], data[5]/* < 100 */);
+}
+
+static ssize_t ipsps_mode_show(struct device *dev,
+ struct device_attribute *devattr, char *buf)
+{
+ u8 reg;
+ int rc;
+ struct i2c_client *client = to_i2c_client(dev->parent);
+ struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
+
+ reg = ipsps_regs[attr->index];
+ rc = i2c_smbus_read_byte_data(client, reg);
+ if (rc < 0)
+ return rc;
+
+ switch (rc) {
+ case MODE_ACTIVE:
+ return snprintf(buf, PAGE_SIZE, "[%s] %s %s\n",
+ MODE_ACTIVE_STRING,
+ MODE_STANDBY_STRING, MODE_REDUNDANCY_STRING);
+ case MODE_STANDBY:
+ return snprintf(buf, PAGE_SIZE, "%s [%s] %s\n",
+ MODE_ACTIVE_STRING,
+ MODE_STANDBY_STRING, MODE_REDUNDANCY_STRING);
+ case MODE_REDUNDANCY:
+ return snprintf(buf, PAGE_SIZE, "%s %s [%s]\n",
+ MODE_ACTIVE_STRING,
+ MODE_STANDBY_STRING, MODE_REDUNDANCY_STRING);
+ default:
+ return snprintf(buf, PAGE_SIZE, "unspecified\n");
+ }
+}
+
+static ssize_t ipsps_mode_store(struct device *dev,
+ struct device_attribute *devattr,
+ const char *buf, size_t count)
+{
+ u8 reg;
+ int rc;
+ struct i2c_client *client = to_i2c_client(dev->parent);
+ struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
+
+ reg = ipsps_regs[attr->index];
+ if (sysfs_streq(MODE_STANDBY_STRING, buf)) {
+ rc = i2c_smbus_write_byte_data(client, reg,
+ MODE_STANDBY);
+ if (rc < 0)
+ return rc;
+ return count;
+ } else if (sysfs_streq(MODE_ACTIVE_STRING, buf)) {
+ rc = i2c_smbus_write_byte_data(client, reg,
+ MODE_ACTIVE);
+ if (rc < 0)
+ return rc;
+ return count;
+ }
+
+ return -EINVAL;
+}
+
+static SENSOR_DEVICE_ATTR_RO(vendor, ipsps_string, vendor);
+static SENSOR_DEVICE_ATTR_RO(model, ipsps_string, model);
+static SENSOR_DEVICE_ATTR_RO(part_number, ipsps_string, part_number);
+static SENSOR_DEVICE_ATTR_RO(serial_number, ipsps_string, serial_number);
+static SENSOR_DEVICE_ATTR_RO(hw_version, ipsps_string, hw_version);
+static SENSOR_DEVICE_ATTR_RO(fw_version, ipsps_fw_version, fw_version);
+static SENSOR_DEVICE_ATTR_RW(mode, ipsps_mode, mode);
+
+static struct attribute *ipsps_attrs[] = {
+ &sensor_dev_attr_vendor.dev_attr.attr,
+ &sensor_dev_attr_model.dev_attr.attr,
+ &sensor_dev_attr_part_number.dev_attr.attr,
+ &sensor_dev_attr_serial_number.dev_attr.attr,
+ &sensor_dev_attr_hw_version.dev_attr.attr,
+ &sensor_dev_attr_fw_version.dev_attr.attr,
+ &sensor_dev_attr_mode.dev_attr.attr,
+ NULL,
+};
+
+ATTRIBUTE_GROUPS(ipsps);
+
+static struct pmbus_driver_info ipsps_info = {
+ .pages = 1,
+ .func[0] = PMBUS_HAVE_VIN | PMBUS_HAVE_VOUT | PMBUS_HAVE_IOUT |
+ PMBUS_HAVE_IIN | PMBUS_HAVE_POUT | PMBUS_HAVE_PIN |
+ PMBUS_HAVE_FAN12 | PMBUS_HAVE_TEMP | PMBUS_HAVE_TEMP2 |
+ PMBUS_HAVE_TEMP3 | PMBUS_HAVE_STATUS_VOUT |
+ PMBUS_HAVE_STATUS_IOUT | PMBUS_HAVE_STATUS_INPUT |
+ PMBUS_HAVE_STATUS_TEMP | PMBUS_HAVE_STATUS_FAN12,
+ .groups = ipsps_groups,
+};
+
+static struct pmbus_platform_data ipsps_pdata = {
+ .flags = PMBUS_SKIP_STATUS_CHECK,
+};
+
+static int ipsps_probe(struct i2c_client *client,
+ const struct i2c_device_id *id)
+{
+ client->dev.platform_data = &ipsps_pdata;
+ return pmbus_do_probe(client, id, &ipsps_info);
+}
+
+static const struct i2c_device_id ipsps_id[] = {
+ { "inspur_ipsps1", 0 },
+ {}
+};
+MODULE_DEVICE_TABLE(i2c, ipsps_id);
+
+static const struct of_device_id ipsps_of_match[] = {
+ { .compatible = "inspur,ipsps1" },
+ {}
+};
+MODULE_DEVICE_TABLE(of, ipsps_of_match);
+
+static struct i2c_driver ipsps_driver = {
+ .driver = {
+ .name = "inspur-ipsps",
+ .of_match_table = ipsps_of_match,
+ },
+ .probe = ipsps_probe,
+ .remove = pmbus_do_remove,
+ .id_table = ipsps_id,
+};
+
+module_i2c_driver(ipsps_driver);
+
+MODULE_AUTHOR("John Wang");
+MODULE_DESCRIPTION("PMBus driver for Inspur Power System power supplies");
+MODULE_LICENSE("GPL");
--
2.17.1
^ permalink raw reply
* Re: [PATCH] x86/CPU/AMD: Clear RDRAND CPUID bit on AMD family 15h/16h
From: Andy Lutomirski @ 2019-08-16 15:19 UTC (permalink / raw)
To: Lendacky, Thomas, linux-kernel@vger.kernel.org,
linux-doc@vger.kernel.org, linux-pm@vger.kernel.org,
x86@kernel.org
Cc: Thomas Gleixner, Ingo Molnar, Borislav Petkov, Rafael J . Wysocki,
Pavel Machek, Chen Yu, Jonathan Corbet, Andrew Cooper
In-Reply-To: <776cb5c2d33e7fd0d2893904724c0e52b394f24a.1565817448.git.thomas.lendacky@amd.com>
On 8/14/19 2:17 PM, Lendacky, Thomas wrote:
> From: Tom Lendacky <thomas.lendacky@amd.com>
>
> There have been reports of RDRAND issues after resuming from suspend on
> some AMD family 15h and family 16h systems. This issue stems from BIOS
> not performing the proper steps during resume to ensure RDRAND continues
> to function properly.
Can you or someone from AMD document *precisely* what goes wrong here?
The APM is crystal clear:
Hardware modifies the CF flag to indicate whether the value returned in
the destination register is valid. If CF = 1, the value is valid. If CF
= 0, the value is invalid.
If BIOS screws up and somehow RDRAND starts failing and returning CF =
0, then I think it's legitimate to call it a BIOS bug. Some degree of
documentation would be nice, as would a way for BIOS to indicate to the
OS that it does not have this bug.
But, from the reports, it sounds like RDRAND starts failing, setting CF
= 1, and returning 0xFFFF.... in the destination register. If true,
then this is, in my book, a severe CPU bug. Software is supposed to be
able to trust that, if RDRAND sets CF = 1, the result is a
cryptographically secure random number, even if everything else in the
system is actively malicious. On a SEV-ES system, this should be
considered a security hole -- even if the hypervisor and BIOS collude,
RDRAND in the guest should work as defined by the manual.
So, can you clarify what is actually going on? And, if there is an
issue where the CPU does not behave as documented in the APM, and AMD
issue an erratum? And ideally also fix it in microcode or in a stepping
and give an indication that the issue is fixed?
^ permalink raw reply
* Re: Non-random RDRAND Re: [PATCH] x86/CPU/AMD: Clear RDRAND CPUID bit on AMD family 15h/16h
From: Neil Horman @ 2019-08-16 14:42 UTC (permalink / raw)
To: Theodore Y. Ts'o, Pavel Machek, Lendacky, Thomas,
linux-kernel@vger.kernel.org, linux-doc@vger.kernel.org,
linux-pm@vger.kernel.org, x86@kernel.org, Thomas Gleixner,
Ingo Molnar, Borislav Petkov, Rafael J . Wysocki, Chen Yu,
Jonathan Corbet
In-Reply-To: <20190815151224.GB18727@mit.edu>
On Thu, Aug 15, 2019 at 11:12:24AM -0400, Theodore Y. Ts'o wrote:
> On Thu, Aug 15, 2019 at 01:24:35AM +0200, Pavel Machek wrote:
> > Burn it with fire!
> >
> > I mean... people were afraid RDRAND would be backdoored, and you now
> > confirm ... it indeed _is_ backdoored? /., here's news for you!
>
> To be fair to AMD, I wouldn't call it a backdoor. Hanlon's razor is
> applicable here:
>
> "Never attribute to malice that which can be adequately
> explained by neglect."
>
> (Sometimes other words are used instead of neglect, but i'm trying to
> be nice.)
>
Is it worth setting up a quirk for the Excavator era cpus, that triggers
a call to rdseed on resume? Working under the assumption that calling
rdseed would kick the rdrand instruction back into gear.
Neil
>
> - Ted
>
> P.S. Also applicable:
>
> https://www.youtube.com/watch?v=XZxzJGgox_E
>
^ permalink raw reply
* Re: [PATCH v8 22/27] binfmt_elf: Extract .note.gnu.property from an ELF file
From: Dave Martin @ 2019-08-16 13:55 UTC (permalink / raw)
To: Yu-cheng Yu
Cc: x86, H. Peter Anvin, Thomas Gleixner, Ingo Molnar, linux-kernel,
linux-doc, linux-mm, linux-arch, linux-api, Arnd Bergmann,
Andy Lutomirski, Balbir Singh, Borislav Petkov, Cyrill Gorcunov,
Dave Hansen, Eugene Syromiatnikov, Florian Weimer, H.J. Lu,
Jann Horn, Jonathan Corbet, Kees Cook, Mike Kravetz, Nadav Amit,
Oleg Nesterov, Pavel Machek, Peter Zijlstra, Randy Dunlap,
Ravi V. Shankar, Vedvyas Shanbhogue
In-Reply-To: <20190813205225.12032-23-yu-cheng.yu@intel.com>
On Tue, Aug 13, 2019 at 01:52:20PM -0700, Yu-cheng Yu wrote:
> An ELF file's .note.gnu.property indicates features the executable file
> can support. For example, the property GNU_PROPERTY_X86_FEATURE_1_AND
> indicates the file supports GNU_PROPERTY_X86_FEATURE_1_IBT and/or
> GNU_PROPERTY_X86_FEATURE_1_SHSTK.
>
> With this patch, if an arch needs to setup features from ELF properties,
> it needs CONFIG_ARCH_USE_GNU_PROPERTY to be set, and specific
> arch_parse_property() and arch_setup_property().
>
> For example, for X86_64:
>
> int arch_setup_property(void *ehdr, void *phdr, struct file *f, bool inter)
> {
> int r;
> uint32_t property;
>
> r = get_gnu_property(ehdr, phdr, f, GNU_PROPERTY_X86_FEATURE_1_AND,
> &property);
> ...
> }
>
> This patch is derived from code provided by H.J. Lu <hjl.tools@gmail.com>.
This is a nice simplification over the previous version, but I'm still
wondering whether it would be better to follow others folks' suggestions
and simply iterate over all the properties found, calling an arch
function for each note that the core doesn't care about.
Something like the following pseudocode:
include/x86/elf.h:
int arch_elf_property(p)
{
if (p->pr_type == GNU_PROPERTY_X86_FEATURE_1_AND)
return elf_property_x86_feature_1_and(p);
else
return 0;
}
binfmt_elf.c:
while (p = find next property)
arch_elf_property(p);
This would also be more efficient when more than one property needs to
be extracted, since it ensures the file is only read once.
Anyway, comments below...
>
> Signed-off-by: Yu-cheng Yu <yu-cheng.yu@intel.com>
> ---
> fs/Kconfig.binfmt | 3 +
> fs/Makefile | 1 +
> fs/binfmt_elf.c | 20 +++++
> fs/gnu_property.c | 178 +++++++++++++++++++++++++++++++++++++++
> include/linux/elf.h | 11 +++
> include/uapi/linux/elf.h | 14 +++
> 6 files changed, 227 insertions(+)
> create mode 100644 fs/gnu_property.c
>
> diff --git a/fs/Kconfig.binfmt b/fs/Kconfig.binfmt
> index 62dc4f577ba1..d2cfe0729a73 100644
> --- a/fs/Kconfig.binfmt
> +++ b/fs/Kconfig.binfmt
> @@ -36,6 +36,9 @@ config COMPAT_BINFMT_ELF
> config ARCH_BINFMT_ELF_STATE
> bool
>
> +config ARCH_USE_GNU_PROPERTY
> + bool
> +
> config BINFMT_ELF_FDPIC
> bool "Kernel support for FDPIC ELF binaries"
> default y if !BINFMT_ELF
> diff --git a/fs/Makefile b/fs/Makefile
> index d60089fd689b..939b1eb7e8cc 100644
> --- a/fs/Makefile
> +++ b/fs/Makefile
> @@ -44,6 +44,7 @@ obj-$(CONFIG_BINFMT_ELF) += binfmt_elf.o
> obj-$(CONFIG_COMPAT_BINFMT_ELF) += compat_binfmt_elf.o
> obj-$(CONFIG_BINFMT_ELF_FDPIC) += binfmt_elf_fdpic.o
> obj-$(CONFIG_BINFMT_FLAT) += binfmt_flat.o
> +obj-$(CONFIG_ARCH_USE_GNU_PROPERTY) += gnu_property.o
>
> obj-$(CONFIG_FS_MBCACHE) += mbcache.o
> obj-$(CONFIG_FS_POSIX_ACL) += posix_acl.o
> diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
> index d4e11b2e04f6..a4e87fcb10a8 100644
> --- a/fs/binfmt_elf.c
> +++ b/fs/binfmt_elf.c
> @@ -852,6 +852,21 @@ static int load_elf_binary(struct linux_binprm *bprm)
> }
> }
>
> + if (interpreter) {
> + retval = arch_parse_property(&loc->interp_elf_ex,
> + interp_elf_phdata,
> + interpreter, true,
> + &arch_state);
> + } else {
> + retval = arch_parse_property(&loc->elf_ex,
> + elf_phdata,
> + bprm->file, false,
> + &arch_state);
> + }
> +
> + if (retval)
> + goto out_free_dentry;
> +
> /*
> * Allow arch code to reject the ELF at this point, whilst it's
> * still possible to return an error to the code that invoked
> @@ -1080,6 +1095,11 @@ static int load_elf_binary(struct linux_binprm *bprm)
> goto out_free_dentry;
> }
>
> + retval = arch_setup_property(&arch_state);
> +
> + if (retval < 0)
> + goto out_free_dentry;
> +
> if (interpreter) {
> unsigned long interp_map_addr = 0;
>
> diff --git a/fs/gnu_property.c b/fs/gnu_property.c
> new file mode 100644
> index 000000000000..b22b43f4d6a0
> --- /dev/null
> +++ b/fs/gnu_property.c
> @@ -0,0 +1,178 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Extract an ELF file's .note.gnu.property.
> + *
> + * The path from the ELF header to .note.gnu.property is:
> + * elfhdr->elf_phdr->elf_note.
> + *
> + * .note.gnu.property layout:
> + *
> + * struct elf_note {
> + * u32 n_namesz; --> sizeof(n_name[]); always (4)
> + * u32 n_ndescsz;--> sizeof(property[])
> + * u32 n_type; --> always NT_GNU_PROPERTY_TYPE_0 (5)
> + * };
> + * char n_name[4]; --> always 'GNU\0'
> + *
> + * struct {
> + * struct gnu_property {
> + * u32 pr_type;
> + * u32 pr_datasz;
> + * };
> + * u8 pr_data[pr_datasz];
> + * }[];
> + */
Do we need all this comment? We already have Elf{32,64}_Nhdr and
struct gnu_property in <uapi/elf.h>.
> +
> +#include <linux/elf.h>
> +#include <linux/slab.h>
> +#include <linux/fs.h>
> +#include <linux/string.h>
> +#include <linux/compat.h>
> +
> +/*
> + * Search a note's payload for 'pr_type'.
> + */
> +static int check_note_payload(void *buf, unsigned long len, u32 pr_type,
> + u32 *property)
> +{
> + u32 pr_type_max = 0;
> +
> + *property = 0;
> +
> + while (len > 0) {
> + struct gnu_property *pr = buf;
> + unsigned long pr_len;
> +
> + if (sizeof(*pr) > len)
checkpatch? (space required between sizeof and "(")
> + return 0;
Shouldn't this be an error?
I'd have thought we should return 0 only if the property is found.
> +
> + pr_len = sizeof(*pr) + pr->pr_datasz;
Overflow?
> +
> + if (pr_len > len)
> + return -ENOEXEC;
These seem to be the same class of error, i.e., trailing garbage in the
note, so why don't we return the same thing in both cases?
Maybe
if (sizeof (*pr) > len ||
pr->pr_datasz > len - sizeof (*pr))
return -ENOEXEC;
pr_len = sizeof (*pr) + pr->pr_datasz;
> + /* property types are in ascending order */
> + if ((pr_type_max != 0) && (pr->pr_type > pr_type_max))
> + return 0;
Redundant (). The first part of the condition may be redundant too.
We also don't check for pr->pr_type == pr_type_max (which is presume
is also not supposed to happen).
Do we really need to check this anyway? I presume this rule is only in
the spec to facilitate binary search (which the spec then defeats by
having a variable property size).
If we consider the ELF file invalid when this check fails, shouldn't
this be -ENOEXEC?
> + if (pr->pr_type > pr_type)
> + return 0;
> +
> + if ((pr->pr_type == pr_type) &&
> + (pr->pr_datasz >= sizeof(u32))) {
> + *property = *(u32 *)(buf + sizeof(*pr));
> + return 0;
> + }
Shouldn't pr->pr_datasz be exactly == sizeof (u32)?
> +
> + if (pr->pr_type > pr_type_max)
> + pr_type_max = pr->pr_type;
All these checks have my head spinning... if we ignore the ordering
requirement, can't we reduce it all to
if (pr->pr_type == pr_type) {
if (pr->pr_datasz != sizeof (u32))
return -ENOEXEC;
*property = *(u32 *)(buf + sizeof (*pr));
return 0;
}
> +
Do we need to up to the appropriate alignment after each property?
> + buf += pr_len;
> + len -= pr_len;
> + }
> +
> + return 0;
-ENOENT?
> +}
> +
> +/*
> + * Look at an ELF file's NT_GNU_PROPERTY for the property of pr_type.
> + *
> + * Input:
> + * buf: the buffer containing the whole note.
> + * len: size of buf.
> + * align: alignment of the note's payload.
> + * pr_type: the property type.
> + *
> + * Output:
> + * The property found.
> + *
> + * Return:
> + * Zero or error.
> + */
> +static int check_note(void *buf, unsigned long len, int align,
> + u32 pr_type, u32 *property)
check_note_payload() and check_note() are somewhat misleadingly named,
since they don't just check.
Maybe call them gnu_property_type_0_extract_property(),
note_extract_property()?
Admittedly the first of those names would be super-verbose :/
> +{
> + struct elf_note *n = buf;
> + char *note_name = buf + sizeof(*n);
> + unsigned long payload_offset;
> + unsigned long payload_len;
> +
> + if (len < sizeof(*n) + 4)
> + return -ENOEXEC;
> +
> + if ((n->n_namesz != 4) || strncmp("GNU", note_name, 3))
> + return -ENOEXEC;
Should that be , n->n_namesz? (or , sizeof ("GNU"))?
Also, no check on n->n_type?
Alternatively, we could just not bother to check the note header:
this was found via PT_GNU_PROPERTY, so if it's not a properly
formatted NT_GNU_PROPERTY_TYPE_0 then the file is garbage anyway
and it doesn't matter exactly what we do.
Personally I would check it though.
> +
> + payload_offset = round_up(sizeof(*n) + n->n_namesz, align);
> + payload_len = n->n_descsz;
> +
> + if (payload_offset + payload_len > len)
May this overflow on 32-bit?
What about:
if (payload_offset > len ||
payload_len > len - payload_offset)
> + return -ENOEXEC;
> +
> + buf += payload_offset;
> + len -= payload_offset;
> +
> + return check_note_payload(buf, len, pr_type, property);
> +}
> +
> +#define find_note(phdr, nr_phdrs, align, pos, len) { \
> + int cnt; \
> + \
> + for (cnt = 0; cnt < nr_phdrs; cnt++) { \
Just to avoid future surprises:
(nr_phdrs)
> + if ((phdr)[cnt].p_align != align) \
Similarly:
(align)
> + continue; \
> + if ((phdr)[cnt].p_type == PT_GNU_PROPERTY) { \
> + pos = (phdr)[cnt].p_offset; \
> + len = (phdr)[cnt].p_filesz; \
> + } \
> + } \
> +}
> +
> +int get_gnu_property(void *ehdr, void *phdr, struct file *file,
> + u32 pr_type, u32 *property)
> +{
> + Elf64_Ehdr *ehdr64 = ehdr;
> + Elf32_Ehdr *ehdr32 = ehdr;
> + void *buf;
> + int align;
> + loff_t pos = 0;
> + unsigned long len = 0;
> + int err = 0;
> +
> + /*
> + * Find PT_GNU_PROPERTY from ELF program headers.
> + */
> + if (ehdr64->e_ident[EI_CLASS] == ELFCLASS64) {
Can we trust e_ident[EI_CLASS] to tell us how big the header is?
We don't check that anywhere AFAICT. For the ELF interpreter in
particular, we kmalloc() the appropriate header size determined by
e_machine, so a malicious binary could have e_machine = EM_I386 with
e_ident[ELFCLASS] == ELFCLASSS64, causing a buffer overrun here.
For the main elf header, we get away with it because the bprm->buf[] is
statically allocated as BINPRM_BUF_SIZE and zero-padded in the case of a
short read.
We could pass in the header size explicitly here, or otherwise
validate that e_ident[ELFCLASS] is sane before calling in.
> + align = 8;
> + find_note((Elf64_Phdr *)phdr, ehdr64->e_phnum, align, pos, len);
> + } else if (ehdr32->e_ident[EI_CLASS] == ELFCLASS32) {
> + align = 4;
> + find_note((Elf32_Phdr *)phdr, ehdr32->e_phnum, align, pos, len);
> + }
Maybe make the name of find_note upper case, or pass pos and len by
reference. Otherwise, this looks a bit like a function call -- in
which case pos and len couldn't be modified.
> +
> + /*
> + * Read in the whole note. PT_GNU_PROPERTY
> + * is not expected to be larger than a page.
> + */
> + if (len == 0)
> + return 0;
> +
> + if (len > PAGE_SIZE)
> + return -ENOEXEC;
Add a comment explaining the rationale?
> +
> + buf = kmalloc(len, GFP_KERNEL);
> + if (!buf)
> + return -ENOMEM;
> +
> + err = kernel_read(file, buf, len, &pos);
> + if (err < len) {
> + if (err >= 0)
> + err = -EIO;
> + goto out;
> + }
> +
> + err = check_note(buf, len, align, pr_type, property);
> +out:
> + kfree(buf);
> + return err;
> +}
[...]
Cheers
---Dave
^ permalink raw reply
* [PATCH] Documentation PCI: Fix pciebus-howto.rst filename typo
From: Bjorn Helgaas @ 2019-08-16 13:53 UTC (permalink / raw)
To: linux-pci
Cc: Changbin Du, Mauro Carvalho Chehab, Jonathan Corbet, linux-doc,
linux-kernel, Bjorn Helgaas
From: Bjorn Helgaas <bhelgaas@google.com>
2e6422444894 ("Documentation: PCI: convert PCIEBUS-HOWTO.txt to reST")
incorrectly renamed PCIEBUS-HOWTO.txt to picebus-howto.rst.
Rename it to pciebus-howto.rst.
Fixes: 2e6422444894 ("Documentation: PCI: convert PCIEBUS-HOWTO.txt to reST")
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
Documentation/PCI/index.rst | 2 +-
Documentation/PCI/{picebus-howto.rst => pciebus-howto.rst} | 0
2 files changed, 1 insertion(+), 1 deletion(-)
rename Documentation/PCI/{picebus-howto.rst => pciebus-howto.rst} (100%)
diff --git a/Documentation/PCI/index.rst b/Documentation/PCI/index.rst
index f4c6121868c3..6768305e4c26 100644
--- a/Documentation/PCI/index.rst
+++ b/Documentation/PCI/index.rst
@@ -9,7 +9,7 @@ Linux PCI Bus Subsystem
:numbered:
pci
- picebus-howto
+ pciebus-howto
pci-iov-howto
msi-howto
acpi-info
diff --git a/Documentation/PCI/picebus-howto.rst b/Documentation/PCI/pciebus-howto.rst
similarity index 100%
rename from Documentation/PCI/picebus-howto.rst
rename to Documentation/PCI/pciebus-howto.rst
--
2.23.0.rc1.153.gdeed80330f-goog
^ permalink raw reply related
* [PATCH] docs: process: fix broken link
From: Jacob Huisman @ 2019-08-16 12:22 UTC (permalink / raw)
To: Jonathan Corbet, Federico Vaga, Harry Wei, Alex Shi
Cc: linux-doc, linux-kernel
http://linux.yyz.us/patch-format.html seems to be down since
approximately September 2018. There is a working archive copy on
arhive.org. Replaced the links in documenation + translations.
Signed-off-by: Jacob Huisman <jacobhuisman@kernelthusiast.com>
---
Documentation/process/howto.rst | 2 +-
Documentation/process/submitting-patches.rst | 2 +-
Documentation/translations/it_IT/process/howto.rst | 2 +-
Documentation/translations/it_IT/process/submitting-patches.rst | 2 +-
Documentation/translations/ja_JP/SubmittingPatches | 2 +-
Documentation/translations/ja_JP/howto.rst | 2 +-
Documentation/translations/ko_KR/howto.rst | 2 +-
Documentation/translations/zh_CN/process/howto.rst | 2 +-
Documentation/translations/zh_CN/process/submitting-patches.rst | 2 +-
9 files changed, 9 insertions(+), 9 deletions(-)
diff --git a/Documentation/process/howto.rst b/Documentation/process/howto.rst
index 6ab75c11d2c3..b6f5a379ad6c 100644
--- a/Documentation/process/howto.rst
+++ b/Documentation/process/howto.rst
@@ -123,7 +123,7 @@ required reading:
https://www.ozlabs.org/~akpm/stuff/tpp.txt
"Linux kernel patch submission format"
- http://linux.yyz.us/patch-format.html
+ https://web.archive.org/web/20180829112450/http://linux.yyz.us/patch-format.html
:ref:`Documentation/process/stable-api-nonsense.rst <stable_api_nonsense>`
This file describes the rationale behind the conscious decision to
diff --git a/Documentation/process/submitting-patches.rst b/Documentation/process/submitting-patches.rst
index 9c4299293c72..fb56297f70dc 100644
--- a/Documentation/process/submitting-patches.rst
+++ b/Documentation/process/submitting-patches.rst
@@ -844,7 +844,7 @@ Andrew Morton, "The perfect patch" (tpp).
<http://www.ozlabs.org/~akpm/stuff/tpp.txt>
Jeff Garzik, "Linux kernel patch submission format".
- <http://linux.yyz.us/patch-format.html>
+ <https://web.archive.org/web/20180829112450/http://linux.yyz.us/patch-format.html>
Greg Kroah-Hartman, "How to piss off a kernel subsystem maintainer".
<http://www.kroah.com/log/linux/maintainer.html>
diff --git a/Documentation/translations/it_IT/process/howto.rst b/Documentation/translations/it_IT/process/howto.rst
index 44e6077730e8..1db5a1082389 100644
--- a/Documentation/translations/it_IT/process/howto.rst
+++ b/Documentation/translations/it_IT/process/howto.rst
@@ -129,7 +129,7 @@ Di seguito una lista di file che sono presenti nei sorgente del kernel e che
https://www.ozlabs.org/~akpm/stuff/tpp.txt
"Linux kernel patch submission format"
- http://linux.yyz.us/patch-format.html
+ https://web.archive.org/web/20180829112450/http://linux.yyz.us/patch-format.html
:ref:`Documentation/translations/it_IT/process/stable-api-nonsense.rst <it_stable_api_nonsense>`
diff --git a/Documentation/translations/it_IT/process/submitting-patches.rst b/Documentation/translations/it_IT/process/submitting-patches.rst
index 7d7ea92c5c5a..cba1f8cb61ed 100644
--- a/Documentation/translations/it_IT/process/submitting-patches.rst
+++ b/Documentation/translations/it_IT/process/submitting-patches.rst
@@ -868,7 +868,7 @@ Andrew Morton, "La patch perfetta" (tpp).
<http://www.ozlabs.org/~akpm/stuff/tpp.txt>
Jeff Garzik, "Formato per la sottomissione di patch per il kernel Linux"
- <http://linux.yyz.us/patch-format.html>
+ <https://web.archive.org/web/20180829112450/http://linux.yyz.us/patch-format.html>
Greg Kroah-Hartman, "Come scocciare un manutentore di un sottosistema"
<http://www.kroah.com/log/linux/maintainer.html>
diff --git a/Documentation/translations/ja_JP/SubmittingPatches b/Documentation/translations/ja_JP/SubmittingPatches
index ad979c3c06a6..dd0c3280ba5a 100644
--- a/Documentation/translations/ja_JP/SubmittingPatches
+++ b/Documentation/translations/ja_JP/SubmittingPatches
@@ -693,7 +693,7 @@ Andrew Morton, "The perfect patch" (tpp).
<http://www.ozlabs.org/~akpm/stuff/tpp.txt>
Jeff Garzik, "Linux kernel patch submission format".
- <http://linux.yyz.us/patch-format.html>
+ <https://web.archive.org/web/20180829112450/http://linux.yyz.us/patch-format.html>
Greg Kroah-Hartman, "How to piss off a kernel subsystem maintainer".
<http://www.kroah.com/log/2005/03/31/>
diff --git a/Documentation/translations/ja_JP/howto.rst b/Documentation/translations/ja_JP/howto.rst
index 2621b770a745..73ebdab4ced7 100644
--- a/Documentation/translations/ja_JP/howto.rst
+++ b/Documentation/translations/ja_JP/howto.rst
@@ -139,7 +139,7 @@ linux-api@vger.kernel.org に送ることを勧めます。
"The Perfect Patch"
http://www.ozlabs.org/~akpm/stuff/tpp.txt
"Linux kernel patch submission format"
- http://linux.yyz.us/patch-format.html
+ https://web.archive.org/web/20180829112450/http://linux.yyz.us/patch-format.html
:ref:`Documentation/process/stable-api-nonsense.rst <stable_api_nonsense>`
このファイルはカーネルの中に不変の API を持たないことにした意識的
diff --git a/Documentation/translations/ko_KR/howto.rst b/Documentation/translations/ko_KR/howto.rst
index bcd63731b80a..b3f51b19de7c 100644
--- a/Documentation/translations/ko_KR/howto.rst
+++ b/Documentation/translations/ko_KR/howto.rst
@@ -135,7 +135,7 @@ mtk.manpages@gmail.com의 메인테이너에게 보낼 것을 권장한다.
https://www.ozlabs.org/~akpm/stuff/tpp.txt
"Linux kernel patch submission format"
- http://linux.yyz.us/patch-format.html
+ https://web.archive.org/web/20180829112450/http://linux.yyz.us/patch-format.html
:ref:`Documentation/process/stable-api-nonsense.rst <stable_api_nonsense>`
이 문서는 의도적으로 커널이 불변하는 API를 갖지 않도록 결정한
diff --git a/Documentation/translations/zh_CN/process/howto.rst b/Documentation/translations/zh_CN/process/howto.rst
index b244a7190eb6..a8e6ab818983 100644
--- a/Documentation/translations/zh_CN/process/howto.rst
+++ b/Documentation/translations/zh_CN/process/howto.rst
@@ -113,7 +113,7 @@ Linux内核代码中包含有大量的文档。这些文档对于学习如何与
"Linux kernel patch submission format"
- http://linux.yyz.us/patch-format.html
+ https://web.archive.org/web/20180829112450/http://linux.yyz.us/patch-format.html
:ref:`Documentation/translations/zh_CN/process/stable-api-nonsense.rst <cn_stable_api_nonsense>`
论证内核为什么特意不包括稳定的内核内部API，也就是说不包括像这样的特
diff --git a/Documentation/translations/zh_CN/process/submitting-patches.rst b/Documentation/translations/zh_CN/process/submitting-patches.rst
index 437c23b367bb..1bb4271ab420 100644
--- a/Documentation/translations/zh_CN/process/submitting-patches.rst
+++ b/Documentation/translations/zh_CN/process/submitting-patches.rst
@@ -652,7 +652,7 @@ Andrew Morton, "The perfect patch" (tpp).
<http://www.ozlabs.org/~akpm/stuff/tpp.txt>
Jeff Garzik, "Linux kernel patch submission format".
- <http://linux.yyz.us/patch-format.html>
+ <https://web.archive.org/web/20180829112450/http://linux.yyz.us/patch-format.html>
Greg Kroah-Hartman, "How to piss off a kernel subsystem maintainer".
<http://www.kroah.com/log/linux/maintainer.html>
--
2.17.1
^ permalink raw reply related
* [PATCH v5 2/2] hwmon: pmbus: Add Inspur Power System power supply driver
From: John Wang @ 2019-08-16 10:19 UTC (permalink / raw)
To: jdelvare, linux, corbet, linux-hwmon, linux-doc, linux-kernel,
openbmc, duanzhijia01, mine260309, joel
Add the driver to monitor Inspur Power System power supplies
with hwmon over pmbus.
This driver adds sysfs attributes for additional power supply data,
including vendor, model, part_number, serial number,
firmware revision, hardware revision, and psu mode(active/standby).
Signed-off-by: John Wang <wangzqbj@inspur.com>
---
v5:
- Align sysfs attrs description in inspur-ipsps1.rst
(Use tab instead of space to sperate names and values)
v4:
- Remove the additional tabs in the Makefile
- Rebased on 5.3-rc4, not 5.2
v3:
- Sort kconfig/makefile entries alphabetically
- Remove unnecessary initialization
- Use ATTRIBUTE_GROUPS instead of expanding directly
- Use memscan to avoid reimplementation
v2:
- Fix typos in commit message
- Invert Christmas tree
- Configure device with sysfs attrs, not debugfs entries
- Fix errno in fw_version_read, ENODATA to EPROTO
- Change the print format of fw-version
- Use sysfs_streq instead of strcmp("xxx" "\n", "xxx")
- Document sysfs attributes
---
Documentation/hwmon/inspur-ipsps1.rst | 79 +++++++++
drivers/hwmon/pmbus/Kconfig | 9 +
drivers/hwmon/pmbus/Makefile | 1 +
drivers/hwmon/pmbus/inspur-ipsps.c | 226 ++++++++++++++++++++++++++
4 files changed, 315 insertions(+)
create mode 100644 Documentation/hwmon/inspur-ipsps1.rst
create mode 100644 drivers/hwmon/pmbus/inspur-ipsps.c
diff --git a/Documentation/hwmon/inspur-ipsps1.rst b/Documentation/hwmon/inspur-ipsps1.rst
new file mode 100644
index 000000000000..2b871ae3448f
--- /dev/null
+++ b/Documentation/hwmon/inspur-ipsps1.rst
@@ -0,0 +1,79 @@
+Kernel driver inspur-ipsps1
+=======================
+
+Supported chips:
+
+ * Inspur Power System power supply unit
+
+Author: John Wang <wangzqbj@inspur.com>
+
+Description
+-----------
+
+This driver supports Inspur Power System power supplies. This driver
+is a client to the core PMBus driver.
+
+Usage Notes
+-----------
+
+This driver does not auto-detect devices. You will have to instantiate the
+devices explicitly. Please see Documentation/i2c/instantiating-devices for
+details.
+
+Sysfs entries
+-------------
+
+The following attributes are supported:
+
+======================= ======================================================
+curr1_input Measured input current
+curr1_label "iin"
+curr1_max Maximum current
+curr1_max_alarm Current high alarm
+curr2_input Measured output current in mA.
+curr2_label "iout1"
+curr2_crit Critical maximum current
+curr2_crit_alarm Current critical high alarm
+curr2_max Maximum current
+curr2_max_alarm Current high alarm
+
+fan1_alarm Fan 1 warning.
+fan1_fault Fan 1 fault.
+fan1_input Fan 1 speed in RPM.
+
+in1_alarm Input voltage under-voltage alarm.
+in1_input Measured input voltage in mV.
+in1_label "vin"
+in2_input Measured output voltage in mV.
+in2_label "vout1"
+in2_lcrit Critical minimum output voltage
+in2_lcrit_alarm Output voltage critical low alarm
+in2_max Maximum output voltage
+in2_max_alarm Output voltage high alarm
+in2_min Minimum output voltage
+in2_min_alarm Output voltage low alarm
+
+power1_alarm Input fault or alarm.
+power1_input Measured input power in uW.
+power1_label "pin"
+power1_max Input power limit
+power2_max_alarm Output power high alarm
+power2_max Output power limit
+power2_input Measured output power in uW.
+power2_label "pout"
+
+temp[1-3]_input Measured temperature
+temp[1-2]_max Maximum temperature
+temp[1-3]_max_alarm Temperature high alarm
+
+vendor Manufacturer name
+model Product model
+part_number Product part number
+serial_number Product serial number
+fw_version Firmware version
+hw_version Hardware version
+mode Work mode. Can be set to active or
+ standby, when set to standby, PSU will
+ automatically switch between standby
+ and redundancy mode.
+======================= ======================================================
diff --git a/drivers/hwmon/pmbus/Kconfig b/drivers/hwmon/pmbus/Kconfig
index b6588483fae1..d62d69bb7e49 100644
--- a/drivers/hwmon/pmbus/Kconfig
+++ b/drivers/hwmon/pmbus/Kconfig
@@ -46,6 +46,15 @@ config SENSORS_IBM_CFFPS
This driver can also be built as a module. If so, the module will
be called ibm-cffps.
+config SENSORS_INSPUR_IPSPS
+ tristate "INSPUR Power System Power Supply"
+ help
+ If you say yes here you get hardware monitoring support for the INSPUR
+ Power System power supply.
+
+ This driver can also be built as a module. If so, the module will
+ be called inspur-ipsps.
+
config SENSORS_IR35221
tristate "Infineon IR35221"
help
diff --git a/drivers/hwmon/pmbus/Makefile b/drivers/hwmon/pmbus/Makefile
index c950ea9a5d00..03bacfcfd660 100644
--- a/drivers/hwmon/pmbus/Makefile
+++ b/drivers/hwmon/pmbus/Makefile
@@ -7,6 +7,7 @@ obj-$(CONFIG_PMBUS) += pmbus_core.o
obj-$(CONFIG_SENSORS_PMBUS) += pmbus.o
obj-$(CONFIG_SENSORS_ADM1275) += adm1275.o
obj-$(CONFIG_SENSORS_IBM_CFFPS) += ibm-cffps.o
+obj-$(CONFIG_SENSORS_INSPUR_IPSPS) += inspur-ipsps.o
obj-$(CONFIG_SENSORS_IR35221) += ir35221.o
obj-$(CONFIG_SENSORS_IR38064) += ir38064.o
obj-$(CONFIG_SENSORS_IRPS5401) += irps5401.o
diff --git a/drivers/hwmon/pmbus/inspur-ipsps.c b/drivers/hwmon/pmbus/inspur-ipsps.c
new file mode 100644
index 000000000000..fa981b881a60
--- /dev/null
+++ b/drivers/hwmon/pmbus/inspur-ipsps.c
@@ -0,0 +1,226 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright 2019 Inspur Corp.
+ */
+
+#include <linux/debugfs.h>
+#include <linux/device.h>
+#include <linux/fs.h>
+#include <linux/i2c.h>
+#include <linux/module.h>
+#include <linux/pmbus.h>
+#include <linux/hwmon-sysfs.h>
+
+#include "pmbus.h"
+
+#define IPSPS_REG_VENDOR_ID 0x99
+#define IPSPS_REG_MODEL 0x9A
+#define IPSPS_REG_FW_VERSION 0x9B
+#define IPSPS_REG_PN 0x9C
+#define IPSPS_REG_SN 0x9E
+#define IPSPS_REG_HW_VERSION 0xB0
+#define IPSPS_REG_MODE 0xFC
+
+#define MODE_ACTIVE 0x55
+#define MODE_STANDBY 0x0E
+#define MODE_REDUNDANCY 0x00
+
+#define MODE_ACTIVE_STRING "active"
+#define MODE_STANDBY_STRING "standby"
+#define MODE_REDUNDANCY_STRING "redundancy"
+
+enum ipsps_index {
+ vendor,
+ model,
+ fw_version,
+ part_number,
+ serial_number,
+ hw_version,
+ mode,
+ num_regs,
+};
+
+static const u8 ipsps_regs[num_regs] = {
+ [vendor] = IPSPS_REG_VENDOR_ID,
+ [model] = IPSPS_REG_MODEL,
+ [fw_version] = IPSPS_REG_FW_VERSION,
+ [part_number] = IPSPS_REG_PN,
+ [serial_number] = IPSPS_REG_SN,
+ [hw_version] = IPSPS_REG_HW_VERSION,
+ [mode] = IPSPS_REG_MODE,
+};
+
+static ssize_t ipsps_string_show(struct device *dev,
+ struct device_attribute *devattr,
+ char *buf)
+{
+ u8 reg;
+ int rc;
+ char *p;
+ char data[I2C_SMBUS_BLOCK_MAX + 1];
+ struct i2c_client *client = to_i2c_client(dev->parent);
+ struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
+
+ reg = ipsps_regs[attr->index];
+ rc = i2c_smbus_read_block_data(client, reg, data);
+ if (rc < 0)
+ return rc;
+
+ /* filled with printable characters, ending with # */
+ p = memscan(data, '#', rc);
+ *p = '\0';
+
+ return snprintf(buf, PAGE_SIZE, "%s\n", data);
+}
+
+static ssize_t ipsps_fw_version_show(struct device *dev,
+ struct device_attribute *devattr,
+ char *buf)
+{
+ u8 reg;
+ int rc;
+ u8 data[I2C_SMBUS_BLOCK_MAX] = { 0 };
+ struct i2c_client *client = to_i2c_client(dev->parent);
+ struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
+
+ reg = ipsps_regs[attr->index];
+ rc = i2c_smbus_read_block_data(client, reg, data);
+ if (rc < 0)
+ return rc;
+
+ if (rc != 6)
+ return -EPROTO;
+
+ return snprintf(buf, PAGE_SIZE, "%u.%02u%u-%u.%02u\n",
+ data[1], data[2]/* < 100 */, data[3]/*< 10*/,
+ data[4], data[5]/* < 100 */);
+}
+
+static ssize_t ipsps_mode_show(struct device *dev,
+ struct device_attribute *devattr, char *buf)
+{
+ u8 reg;
+ int rc;
+ struct i2c_client *client = to_i2c_client(dev->parent);
+ struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
+
+ reg = ipsps_regs[attr->index];
+ rc = i2c_smbus_read_byte_data(client, reg);
+ if (rc < 0)
+ return rc;
+
+ switch (rc) {
+ case MODE_ACTIVE:
+ return snprintf(buf, PAGE_SIZE, "[%s] %s %s\n",
+ MODE_ACTIVE_STRING,
+ MODE_STANDBY_STRING, MODE_REDUNDANCY_STRING);
+ case MODE_STANDBY:
+ return snprintf(buf, PAGE_SIZE, "%s [%s] %s\n",
+ MODE_ACTIVE_STRING,
+ MODE_STANDBY_STRING, MODE_REDUNDANCY_STRING);
+ case MODE_REDUNDANCY:
+ return snprintf(buf, PAGE_SIZE, "%s %s [%s]\n",
+ MODE_ACTIVE_STRING,
+ MODE_STANDBY_STRING, MODE_REDUNDANCY_STRING);
+ default:
+ return snprintf(buf, PAGE_SIZE, "unspecified\n");
+ }
+}
+
+static ssize_t ipsps_mode_store(struct device *dev,
+ struct device_attribute *devattr,
+ const char *buf, size_t count)
+{
+ u8 reg;
+ int rc;
+ struct i2c_client *client = to_i2c_client(dev->parent);
+ struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
+
+ reg = ipsps_regs[attr->index];
+ if (sysfs_streq(MODE_STANDBY_STRING, buf)) {
+ rc = i2c_smbus_write_byte_data(client, reg,
+ MODE_STANDBY);
+ if (rc < 0)
+ return rc;
+ return count;
+ } else if (sysfs_streq(MODE_ACTIVE_STRING, buf)) {
+ rc = i2c_smbus_write_byte_data(client, reg,
+ MODE_ACTIVE);
+ if (rc < 0)
+ return rc;
+ return count;
+ }
+
+ return -EINVAL;
+}
+
+static SENSOR_DEVICE_ATTR_RO(vendor, ipsps_string, vendor);
+static SENSOR_DEVICE_ATTR_RO(model, ipsps_string, model);
+static SENSOR_DEVICE_ATTR_RO(part_number, ipsps_string, part_number);
+static SENSOR_DEVICE_ATTR_RO(serial_number, ipsps_string, serial_number);
+static SENSOR_DEVICE_ATTR_RO(hw_version, ipsps_string, hw_version);
+static SENSOR_DEVICE_ATTR_RO(fw_version, ipsps_fw_version, fw_version);
+static SENSOR_DEVICE_ATTR_RW(mode, ipsps_mode, mode);
+
+static struct attribute *ipsps_attrs[] = {
+ &sensor_dev_attr_vendor.dev_attr.attr,
+ &sensor_dev_attr_model.dev_attr.attr,
+ &sensor_dev_attr_part_number.dev_attr.attr,
+ &sensor_dev_attr_serial_number.dev_attr.attr,
+ &sensor_dev_attr_hw_version.dev_attr.attr,
+ &sensor_dev_attr_fw_version.dev_attr.attr,
+ &sensor_dev_attr_mode.dev_attr.attr,
+ NULL,
+};
+
+ATTRIBUTE_GROUPS(ipsps);
+
+static struct pmbus_driver_info ipsps_info = {
+ .pages = 1,
+ .func[0] = PMBUS_HAVE_VIN | PMBUS_HAVE_VOUT | PMBUS_HAVE_IOUT |
+ PMBUS_HAVE_IIN | PMBUS_HAVE_POUT | PMBUS_HAVE_PIN |
+ PMBUS_HAVE_FAN12 | PMBUS_HAVE_TEMP | PMBUS_HAVE_TEMP2 |
+ PMBUS_HAVE_TEMP3 | PMBUS_HAVE_STATUS_VOUT |
+ PMBUS_HAVE_STATUS_IOUT | PMBUS_HAVE_STATUS_INPUT |
+ PMBUS_HAVE_STATUS_TEMP | PMBUS_HAVE_STATUS_FAN12,
+ .groups = ipsps_groups,
+};
+
+static struct pmbus_platform_data ipsps_pdata = {
+ .flags = PMBUS_SKIP_STATUS_CHECK,
+};
+
+static int ipsps_probe(struct i2c_client *client,
+ const struct i2c_device_id *id)
+{
+ client->dev.platform_data = &ipsps_pdata;
+ return pmbus_do_probe(client, id, &ipsps_info);
+}
+
+static const struct i2c_device_id ipsps_id[] = {
+ { "inspur_ipsps1", 0 },
+ {}
+};
+MODULE_DEVICE_TABLE(i2c, ipsps_id);
+
+static const struct of_device_id ipsps_of_match[] = {
+ { .compatible = "inspur,ipsps1" },
+ {}
+};
+MODULE_DEVICE_TABLE(of, ipsps_of_match);
+
+static struct i2c_driver ipsps_driver = {
+ .driver = {
+ .name = "inspur-ipsps",
+ .of_match_table = ipsps_of_match,
+ },
+ .probe = ipsps_probe,
+ .remove = pmbus_do_remove,
+ .id_table = ipsps_id,
+};
+
+module_i2c_driver(ipsps_driver);
+
+MODULE_AUTHOR("John Wang");
+MODULE_DESCRIPTION("PMBus driver for Inspur Power System power supplies");
+MODULE_LICENSE("GPL");
--
2.17.1
^ permalink raw reply related
* Re: [UNVERIFIED SENDER] Re: [PATCH 0/9] arm64: Stolen time support
From: Steven Price @ 2019-08-16 10:23 UTC (permalink / raw)
To: Alexander Graf, Marc Zyngier
Cc: kvm, Catalin Marinas, linux-doc, Russell King, linux-kernel,
Paolo Bonzini, Will Deacon, kvmarm, linux-arm-kernel
In-Reply-To: <bda4e0f7-e5f4-32af-e998-00b6240b5260@amazon.com>
On 14/08/2019 15:52, Alexander Graf wrote:
>
>
> On 14.08.19 16:19, Marc Zyngier wrote:
>> On Wed, 14 Aug 2019 14:02:25 +0100,
>> Alexander Graf <graf@amazon.com> wrote:
>>>
>>>
>>>
>>> On 05.08.19 15:06, Steven Price wrote:
>>>> On 03/08/2019 19:05, Marc Zyngier wrote:
>>>>> On Fri, 2 Aug 2019 15:50:08 +0100
>>>>> Steven Price <steven.price@arm.com> wrote:
>>>>>
>>>>> Hi Steven,
>>>>>
>>>>>> This series add support for paravirtualized time for arm64 guests and
>>>>>> KVM hosts following the specification in Arm's document DEN 0057A:
>>>>>>
>>>>>> https://developer.arm.com/docs/den0057/a
>>>>>>
>>>>>> It implements support for stolen time, allowing the guest to
>>>>>> identify time when it is forcibly not executing.
>>>>>>
>>>>>> It doesn't implement support for Live Physical Time (LPT) as there
>>>>>> are
>>>>>> some concerns about the overheads and approach in the above
>>>>>> specification, and I expect an updated version of the
>>>>>> specification to
>>>>>> be released soon with just the stolen time parts.
>>>>>
>>>>> Thanks for posting this.
>>>>>
>>>>> My current concern with this series is around the fact that we
>>>>> allocate
>>>>> memory from the kernel on behalf of the guest. It is the first example
>>>>> of such thing in the ARM port, and I can't really say I'm fond of it.
>>>>>
>>>>> x86 seems to get away with it by having the memory allocated from
>>>>> userspace, why I tend to like more. Yes, put_user is more
>>>>> expensive than a straight store, but this isn't done too often either.
>>>>>
>>>>> What is the rational for your current approach?
>>>>
>>>> As I see it there are 3 approaches that can be taken here:
>>>>
>>>> 1. Hypervisor allocates memory and adds it to the virtual machine. This
>>>> means that everything to do with the 'device' is encapsulated behind
>>>> the
>>>> KVM_CREATE_DEVICE / KVM_[GS]ET_DEVICE_ATTR ioctls. But since we want
>>>> the
>>>> stolen time structure to be fast it cannot be a trapping region and has
>>>> to be backed by real memory - in this case allocated by the host
>>>> kernel.
>>>>
>>>> 2. Host user space allocates memory. Similar to above, but this time
>>>> user space needs to manage the memory region as well as the usual
>>>> KVM_CREATE_DEVICE dance. I've no objection to this, but it means
>>>> kvmtool/QEMU needs to be much more aware of what is going on (e.g. how
>>>> to size the memory region).
>>>
>>> You ideally want to get the host overhead for a VM to as little as you
>>> can. I'm not terribly fond of the idea of reserving a full page just
>>> because we're too afraid of having the guest donate memory.
>>
>> Well, reduce the amount of memory you give to the guest by one page,
>> and allocate that page to the stolen time device. Problem solved!
>>
>> Seriously, if you're worried about the allocation of a single page,
>> you should first look at how many holes we have in the vcpu structure,
>> for example (even better, with the 8.4 NV patches applied). Just
>> fixing that would give you that page back *per vcpu*.
>
> I'm worried about additional memory slots, about fragmenting the
> cachable guest memory regions, about avoidable HV taxes.
>
> I think we need to distinguish here between the KVM implementation and
> the hypervisor/guest interface. Just because in KVM we can save overhead
> today doesn't mean that the HV interface should be built around the
> assumption that "memory is free".
The HV interface just requires that the host provides some memory for
the structures to live in. The memory can be adjacent (or even within)
the normal memory of the guest. The only requirement is that the guest
isn't told to use this memory for normal allocations (i.e. it should
either be explicitly reserved or just not contained within the normal
memory block).
>>
>>>> 3. Guest kernel "donates" the memory to the hypervisor for the
>>>> structure. As far as I'm aware this is what x86 does. The problems I
>>>> see
>>>> this approach are:
>>>>
>>>> a) kexec becomes much more tricky - there needs to be a disabling
>>>> mechanism for the guest to stop the hypervisor scribbling on memory
>>>> before starting the new kernel.
>>>
>>> I wouldn't call "quiesce a device" much more tricky. We have to do
>>> that for other devices as well today.
>>
>> And since there is no standard way of doing it, we keep inventing
>> weird and wonderful ways of doing so -- cue the terrible GICv3 LPI
>> situation, and all the various hacks to keep existing IOMMU mappings
>> around across firmware/kernel handovers as well as kexec.
>
> Well, the good news here is that we don't have to keep it around ;).
>
>>
>>>
>>>> b) If there is more than one entity that is interested in the
>>>> information (e.g. firmware and kernel) then this requires some form of
>>>> arbitration in the guest because the hypervisor doesn't want to have to
>>>> track an arbitrary number of regions to update.
>>>
>>> Why would FW care?
>>
>> Exactly. It doesn't care. Not caring means it doesn't know about the
>> page the guest has allocated for stolen time, and starts using it for
>> its own purposes. Hello, memory corruption. Same thing goes if you
>> reboot into a non stolen time aware kernel.
>
> If you reboot, you go via the vcpu reset path which clears the map, no?
> Same goes for FW entry. If you enter firmware that does not set up the
> map, you never see it.
Doing this per-vcpu implies you are probably going to have to allocate
an entire page per vcpu. Because it's entirely possible for a guest to
reset an individual vcpu. Or at the least there's some messy reference
counting going on here.
Having a region of memory provided by the host means the structures can
be packed and there's nothing to be done in the reset path.
>>
>>>
>>>> c) Performance can suffer if the host kernel doesn't have a suitably
>>>> aligned/sized area to use. As you say - put_user() is more expensive.
>>>
>>> Just define the interface to always require natural alignment when
>>> donating a memory location?
>>>
>>>> The structure is updated on every return to the VM.
>>>
>>> If you really do suffer from put_user(), there are alternatives. You
>>> could just map the page on the registration hcall and then leave it
>>> pinned until the vcpu gets destroyed again.
>>
>> put_user() should be cheap enough. It is one of the things we tend to
>> optimise anyway. And yes, worse case, we pin the page.
>>
>>>
>>>> Of course x86 does prove the third approach can work, but I'm not sure
>>>> which is actually better. Avoid the kexec cancellation requirements was
>>>> the main driver of the current approach. Although many of the
>>>
>>> I really don't understand the problem with kexec cancellation. Worst
>>> case, let guest FW set it up for you and propagate only the address
>>> down via ACPI/DT. That way you can mark the respective memory as
>>> reserved too.
>>
>> We already went down that road with the LPI hack. I'm not going there
>> again if we can avoid it. And it turn out that we can. Just allocate
>> the stolen time page as a separate memblock, give it to KVM for that
>> purpose.
>>
>> Your suggestion of letting the guest firmware set something up only
>> works if whatever you're booting after that understands it. If it
>> doesn't, you're screwed.
>
> Why? For UEFI, mark the region as reserved in the memory map. For DT,
> just mark it straight on reserved.
>
> That said, I'm not advocating for doing it in the FW. I think this can
> be solved really easily with a simple guest driver to enable and a vcpu
> reset hook to disable the map.
>
>>
>>> But even with a Linux only mechanism, just take a look at
>>> arch/x86/kernel/kvmclock.c. All they do to remove the map is to hook
>>> into machine_crash_shutdown() and machine_shutdown().
>>
>> I'm not going to take something that is Linux specific. It has to work
>> for all guests, at all times, whether they know about the hypervisor
>> service or not.
>
> If they don't know about the HV service, they don't register the writer,
> so they don't see corruption.
>
> If they know about the HV service and they don't support kexec, they
> don't have to worry because a vcpu reset should also clear the map.
>
> If they do support kexec, they already have a mechanism to quiesce devices.
>
> So I don't understand how this is Linux specific? The question was Linux
> specific, so I answered with precedence to show that disabling on kexec
> is not all that hard :).
My concern is more around a something like Jailhouse as a
guest-hypervisor. There Linux gives up CPUs to run another OS. This
hand-off of a CPU is much easier if there's just a structure in memory
somewhere which doesn't move.
The kexec case like you say can be handled as a device to quiesce.
I don't think either scheme is unworkable, but I do think getting the
host to provide the memory is easier for both guest and host. Marc had a
good point that getting user space to allocate the memory is probably
preferable to getting the host kernel to do so, so I'm reworking the
code to do that.
Steve
^ permalink raw reply
* Re: [PATCH] docs: mtd: Update spi nor reference driver
From: John Garry @ 2019-08-16 10:20 UTC (permalink / raw)
To: Schrempf Frieder, corbet@lwn.net, mchehab+samsung@kernel.org,
linux-mtd@lists.infradead.org
Cc: linux-kernel@vger.kernel.org, linux-doc@vger.kernel.org,
marek.vasut@gmail.com, tudor.ambarus@microchip.com,
broonie@kernel.org, miquel.raynal@bootlin.com, richard@nod.at,
vigneshr@ti.com, wanghuiqiang
In-Reply-To: <9b074db7-b95d-a081-2fba-7b2b82997332@kontron.de>
On 06/08/2019 17:40, Schrempf Frieder wrote:
> Cc: +MTD/SPI-NOR/SPI maintainers
>
> Hi John,
>
> On 06.08.19 18:35, John Garry wrote:
>> On 06/08/2019 17:06, John Garry wrote:
>>> The reference driver no longer exists since commit 50f1242c6742 ("mtd:
>>> fsl-quadspi: Remove the driver as it was replaced by spi-fsl-qspi.c").
>>>
>>> Update reference to spi-fsl-qspi.c driver.
>>>
>>> Signed-off-by: John Garry <john.garry@huawei.com>
>>>
>>> diff --git a/Documentation/driver-api/mtd/spi-nor.rst
>>> b/Documentation/driver-api/mtd/spi-nor.rst
>>> index f5333e3bf486..1f0437676762 100644
>>> --- a/Documentation/driver-api/mtd/spi-nor.rst
>>> +++ b/Documentation/driver-api/mtd/spi-nor.rst
>>
>> In fact this document has many references to Freescale QuadSPI - could
>> someone kindly review this complete document for up-to-date accuracy?
>
> The new driver spi-fsl-qspi.c is not a SPI NOR controller driver
> anymore. It is now a SPI controller driver that uses the SPI MEM API, so
> referencing it here is obsolete.
>
> Actually it seems like the whole file is obsolete and needs to be
> removed or replaced by proper documentation of the SPI MEM API.
Hi,
Could someone kindly advise on the following:
I am looking at ACPI support only for an mtd spi nor driver we're
targeting for mainline support.
So for the host, I could use a proprietary HID in the DSDT for matching
in the kernel driver.
About the child spi flash devices, is the recommendation to just use
PRP0001 HID and "jedec,spi-nor" compatible?
thanks,
John
>
> @Maintainers:
> Maybe the docs under Documentation/driver-api/mtd should be officially
> maintained by the MTD subsystem (and added to MAINTAINERS). And if there
> will be some driver API docs for SPI MEM it should probably live in
> Documentation/driver-api/spi instead of Documentation/driver-api/mtd, as
> spi-mem.c itself is in drivers/spi.
>
> Regards,
> Frieder
>
>>
>> Thanks,
>> John
>>
>>> @@ -59,7 +59,7 @@ Part III - How can drivers use the framework?
>>>
>>> The main API is spi_nor_scan(). Before you call the hook, a driver
>>> should
>>> initialize the necessary fields for spi_nor{}. Please see
>>> -drivers/mtd/spi-nor/spi-nor.c for detail. Please also refer to
>>> fsl-quadspi.c
>>> +drivers/mtd/spi-nor/spi-nor.c for detail. Please also refer to
>>> spi-fsl-qspi.c
>>> when you want to write a new driver for a SPI NOR controller.
>>> Another API is spi_nor_restore(), this is used to restore the status
>>> of SPI
>>> flash chip such as addressing mode. Call it whenever detach the
>>> driver from
>>>
>>
>>
>>
>> ______________________________________________________
>> Linux MTD discussion mailing list
>> http://lists.infradead.org/mailman/listinfo/linux-mtd/
^ permalink raw reply
* Re: Non-random RDRAND Re: [PATCH] x86/CPU/AMD: Clear RDRAND CPUID bit on AMD family 15h/16h
From: Pavel Machek @ 2019-08-16 9:07 UTC (permalink / raw)
To: Theodore Y. Ts'o, Lendacky, Thomas, nhorman,
linux-kernel@vger.kernel.org, linux-doc@vger.kernel.org,
linux-pm@vger.kernel.org, x86@kernel.org, Thomas Gleixner,
Ingo Molnar, Borislav Petkov, Rafael J . Wysocki, Chen Yu,
Jonathan Corbet
In-Reply-To: <20190815151224.GB18727@mit.edu>
[-- Attachment #1: Type: text/plain, Size: 1031 bytes --]
On Thu 2019-08-15 11:12:24, Theodore Y. Ts'o wrote:
> On Thu, Aug 15, 2019 at 01:24:35AM +0200, Pavel Machek wrote:
> > Burn it with fire!
> >
> > I mean... people were afraid RDRAND would be backdoored, and you now
> > confirm ... it indeed _is_ backdoored? /., here's news for you!
>
> To be fair to AMD, I wouldn't call it a backdoor. Hanlon's razor is
> applicable here:
>
> "Never attribute to malice that which can be adequately
> explained by neglect."
> (Sometimes other words are used instead of neglect, but i'm trying to
> be nice.)
You are right, I thought it was returning values with low entropy, and
it returns ~0 (so -- really really low entropy :-) and can't be
clasified as a backdoor.
Anyway, AMD is _not_ doing good job right now.
I'd expect:
a) CVE reference
b) real fix; if BIOS can init the rng, so can kernel
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]
^ permalink raw reply
* Re: [PATCH v4 2/2] hwmon: pmbus: Add Inspur Power System power supply driver
From: Guenter Roeck @ 2019-08-16 3:13 UTC (permalink / raw)
To: John Wang
Cc: Vijay Khemka, linux-hwmon@vger.kernel.org, jdelvare@suse.com,
linux-doc@vger.kernel.org, openbmc@lists.ozlabs.org,
corbet@lwn.net, linux-kernel@vger.kernel.org,
duanzhijia01@inspur.com
In-Reply-To: <CAHkHK08ZkZDoVQ0qfLPcO=_=-OAmx+N5BGHRgZkoxLUNQzTp5g@mail.gmail.com>
On 8/15/19 7:44 PM, John Wang wrote:
> On Fri, Aug 16, 2019 at 3:41 AM Guenter Roeck <linux@roeck-us.net> wrote:
>>
>> On Thu, Aug 15, 2019 at 06:43:52PM +0000, Vijay Khemka wrote:
>>>
>>>
>>> On 8/13/19, 1:36 AM, "openbmc on behalf of John Wang" <openbmc-bounces+vijaykhemka=fb.com@lists.ozlabs.org on behalf of wangzqbj@inspur.com> wrote:
>>>
>>> Add the driver to monitor Inspur Power System power supplies
>>> with hwmon over pmbus.
>>>
>>> This driver adds sysfs attributes for additional power supply data,
>>> including vendor, model, part_number, serial number,
>>> firmware revision, hardware revision, and psu mode(active/standby).
>>>
>>> Signed-off-by: John Wang <wangzqbj@inspur.com>
>>> ---
>>> v4:
>>> - Remove the additional tabs in the Makefile
>>> - Rebased on 5.3-rc4, not 5.2
>>> v3:
>>> - Sort kconfig/makefile entries alphabetically
>>> - Remove unnecessary initialization
>>> - Use ATTRIBUTE_GROUPS instead of expanding directly
>>> - Use memscan to avoid reimplementation
>>> v2:
>>> - Fix typos in commit message
>>> - Invert Christmas tree
>>> - Configure device with sysfs attrs, not debugfs entries
>>> - Fix errno in fw_version_read, ENODATA to EPROTO
>>> - Change the print format of fw-version
>>> - Use sysfs_streq instead of strcmp("xxx" "\n", "xxx")
>>> - Document sysfs attributes
>>> ---
>>> Documentation/hwmon/inspur-ipsps1.rst | 79 +++++++++
>>> drivers/hwmon/pmbus/Kconfig | 9 +
>>> drivers/hwmon/pmbus/Makefile | 1 +
>>> drivers/hwmon/pmbus/inspur-ipsps.c | 226 ++++++++++++++++++++++++++
>>> 4 files changed, 315 insertions(+)
>>> create mode 100644 Documentation/hwmon/inspur-ipsps1.rst
>>> create mode 100644 drivers/hwmon/pmbus/inspur-ipsps.c
>>>
>>> diff --git a/Documentation/hwmon/inspur-ipsps1.rst b/Documentation/hwmon/inspur-ipsps1.rst
>>> new file mode 100644
>>> index 000000000000..aa19f0ccc8b0
>>> --- /dev/null
>>> +++ b/Documentation/hwmon/inspur-ipsps1.rst
>>> @@ -0,0 +1,79 @@
>>> +Kernel driver inspur-ipsps1
>>> +=======================
>>> +
>>> +Supported chips:
>>> +
>>> + * Inspur Power System power supply unit
>>> +
>>> +Author: John Wang <wangzqbj@inspur.com>
>>> +
>>> +Description
>>> +-----------
>>> +
>>> +This driver supports Inspur Power System power supplies. This driver
>>> +is a client to the core PMBus driver.
>>> +
>>> +Usage Notes
>>> +-----------
>>> +
>>> +This driver does not auto-detect devices. You will have to instantiate the
>>> +devices explicitly. Please see Documentation/i2c/instantiating-devices for
>>> +details.
>>> +
>>> +Sysfs entries
>>> +-------------
>>> +
>>> +The following attributes are supported:
>>> +
>>> +======================= ======================================================
>>> +curr1_input Measured input current
>>> +curr1_label "iin"
>>> +curr1_max Maximum current
>>> +curr1_max_alarm Current high alarm
>>> +curr2_input Measured output current in mA.
>>> +curr2_label "iout1"
>>> +curr2_crit Critical maximum current
>>> +curr2_crit_alarm Current critical high alarm
>>> +curr2_max Maximum current
>>> +curr2_max_alarm Current high alarm
>>> +
>>> Please align above details.
>
> Sorry for the mix of table and space
>
>>> +fan1_alarm Fan 1 warning.
>>> +fan1_fault Fan 1 fault.
>>> +fan1_input Fan 1 speed in RPM.
>>> +
>>> +in1_alarm Input voltage under-voltage alarm.
>>> +in1_input Measured input voltage in mV.
>>> +in1_label "vin"
>>> +in2_input Measured output voltage in mV.
>>> +in2_label "vout1"
>>> +in2_lcrit Critical minimum output voltage
>>> +in2_lcrit_alarm Output voltage critical low alarm
>>> +in2_max Maximum output voltage
>>> +in2_max_alarm Output voltage high alarm
>>> +in2_min Minimum output voltage
>>> +in2_min_alarm Output voltage low alarm
>>> +
>>> +power1_alarm Input fault or alarm.
>>> +power1_input Measured input power in uW.
>>> +power1_label "pin"
>>> +power1_max Input power limit
>>> +power2_max_alarm Output power high alarm
>>> +power2_max Output power limit
>>> +power2_input Measured output power in uW.
>>> +power2_label "pout"
>>> +
>>> Same alignment issue in description.
>
> will fix.
>
>>> +temp[1-3]_input Measured temperature
>>> +temp[1-2]_max Maximum temperature
>>> +temp[1-3]_max_alarm Temperature high alarm
>>> +
>>> +vendor Manufacturer name
>>> +model Product model
>>> +part_number Product part number
>>> +serial_number Product serial number
>>> +fw_version Firmware version
>>> +hw_version Hardware version
>>> +mode Work mode. Can be set to active or
>>> + standby, when set to standby, PSU will
>>> + automatically switch between standby
>>> + and redundancy mode.
>>> +======================= ======================================================
>>> diff --git a/drivers/hwmon/pmbus/Kconfig b/drivers/hwmon/pmbus/Kconfig
>>> index b6588483fae1..d62d69bb7e49 100644
>>> --- a/drivers/hwmon/pmbus/Kconfig
>>> +++ b/drivers/hwmon/pmbus/Kconfig
>>> @@ -46,6 +46,15 @@ config SENSORS_IBM_CFFPS
>>> This driver can also be built as a module. If so, the module will
>>> be called ibm-cffps.
>>>
>>> +config SENSORS_INSPUR_IPSPS
>>> + tristate "INSPUR Power System Power Supply"
>>> + help
>>> + If you say yes here you get hardware monitoring support for the INSPUR
>>> + Power System power supply.
>>> +
>>> + This driver can also be built as a module. If so, the module will
>>> + be called inspur-ipsps.
>>> +
>>> config SENSORS_IR35221
>>> tristate "Infineon IR35221"
>>> help
>>> diff --git a/drivers/hwmon/pmbus/Makefile b/drivers/hwmon/pmbus/Makefile
>>> index c950ea9a5d00..03bacfcfd660 100644
>>> --- a/drivers/hwmon/pmbus/Makefile
>>> +++ b/drivers/hwmon/pmbus/Makefile
>>> @@ -7,6 +7,7 @@ obj-$(CONFIG_PMBUS) += pmbus_core.o
>>> obj-$(CONFIG_SENSORS_PMBUS) += pmbus.o
>>> obj-$(CONFIG_SENSORS_ADM1275) += adm1275.o
>>> obj-$(CONFIG_SENSORS_IBM_CFFPS) += ibm-cffps.o
>>> +obj-$(CONFIG_SENSORS_INSPUR_IPSPS) += inspur-ipsps.o
>>> obj-$(CONFIG_SENSORS_IR35221) += ir35221.o
>>> obj-$(CONFIG_SENSORS_IR38064) += ir38064.o
>>> obj-$(CONFIG_SENSORS_IRPS5401) += irps5401.o
>>> diff --git a/drivers/hwmon/pmbus/inspur-ipsps.c b/drivers/hwmon/pmbus/inspur-ipsps.c
>>> new file mode 100644
>>> index 000000000000..fa981b881a60
>>> --- /dev/null
>>> +++ b/drivers/hwmon/pmbus/inspur-ipsps.c
>>> @@ -0,0 +1,226 @@
>>> +// SPDX-License-Identifier: GPL-2.0-or-later
>>> +/*
>>> + * Copyright 2019 Inspur Corp.
>>> + */
>>> +
>>> +#include <linux/debugfs.h>
>>> +#include <linux/device.h>
>>> +#include <linux/fs.h>
>>> +#include <linux/i2c.h>
>>> +#include <linux/module.h>
>>> +#include <linux/pmbus.h>
>>> +#include <linux/hwmon-sysfs.h>
>>> +
>>> +#include "pmbus.h"
>>> +
>>> +#define IPSPS_REG_VENDOR_ID 0x99
>>> +#define IPSPS_REG_MODEL 0x9A
>>> +#define IPSPS_REG_FW_VERSION 0x9B
>>> +#define IPSPS_REG_PN 0x9C
>>> +#define IPSPS_REG_SN 0x9E
>>> +#define IPSPS_REG_HW_VERSION 0xB0
>>> +#define IPSPS_REG_MODE 0xFC
>>> +
>>> +#define MODE_ACTIVE 0x55
>>> +#define MODE_STANDBY 0x0E
>>> +#define MODE_REDUNDANCY 0x00
>>> +
>>> +#define MODE_ACTIVE_STRING "active"
>>> +#define MODE_STANDBY_STRING "standby"
>>> +#define MODE_REDUNDANCY_STRING "redundancy"
>>> +
>>> +enum ipsps_index {
>>> + vendor,
>>> + model,
>>> + fw_version,
>>> + part_number,
>>> + serial_number,
>>> + hw_version,
>>> + mode,
>>> + num_regs,
>>> +};
>>> +
>>> +static const u8 ipsps_regs[num_regs] = {
>>> + [vendor] = IPSPS_REG_VENDOR_ID,
>>> + [model] = IPSPS_REG_MODEL,
>>> + [fw_version] = IPSPS_REG_FW_VERSION,
>>> + [part_number] = IPSPS_REG_PN,
>>> + [serial_number] = IPSPS_REG_SN,
>>> + [hw_version] = IPSPS_REG_HW_VERSION,
>>> + [mode] = IPSPS_REG_MODE,
>>> +};
>>> +
>>> +static ssize_t ipsps_string_show(struct device *dev,
>>> + struct device_attribute *devattr,
>>> + char *buf)
>>> +{
>>> + u8 reg;
>>> + int rc;
>>> + char *p;
>>> + char data[I2C_SMBUS_BLOCK_MAX + 1];
>>> + struct i2c_client *client = to_i2c_client(dev->parent);
>>> + struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
>>> +
>>> + reg = ipsps_regs[attr->index];
>>> + rc = i2c_smbus_read_block_data(client, reg, data);
>>> + if (rc < 0)
>>> + return rc;
>>> +
>>> + /* filled with printable characters, ending with # */
>>> + p = memscan(data, '#', rc);
>>> + *p = '\0';
>>> +
>>> + return snprintf(buf, PAGE_SIZE, "%s\n", data);
>>> +}
>>> +
>>> +static ssize_t ipsps_fw_version_show(struct device *dev,
>>> + struct device_attribute *devattr,
>>> + char *buf)
>>> +{
>>> + u8 reg;
>>> + int rc;
>>> + u8 data[I2C_SMBUS_BLOCK_MAX] = { 0 };
>>> + struct i2c_client *client = to_i2c_client(dev->parent);
>>> + struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
>>> +
>>> + reg = ipsps_regs[attr->index];
>>> + rc = i2c_smbus_read_block_data(client, reg, data);
>>> + if (rc < 0)
>>> + return rc;
>>> +
>>> + if (rc != 6)
>>> + return -EPROTO;
>>> +
>>> + return snprintf(buf, PAGE_SIZE, "%u.%02u%u-%u.%02u\n",
>>> + data[1], data[2]/* < 100 */, data[3]/*< 10*/,
>>> + data[4], data[5]/* < 100 */);
>>> +}
>>> +
>>> +static ssize_t ipsps_mode_show(struct device *dev,
>>> + struct device_attribute *devattr, char *buf)
>>> +{
>>> + u8 reg;
>>> + int rc;
>>> + struct i2c_client *client = to_i2c_client(dev->parent);
>>> + struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
>>> +
>>> + reg = ipsps_regs[attr->index];
>>> + rc = i2c_smbus_read_byte_data(client, reg);
>>> + if (rc < 0)
>>> + return rc;
>>> +
>>> + switch (rc) {
>>> + case MODE_ACTIVE:
>>> + return snprintf(buf, PAGE_SIZE, "[%s] %s %s\n",
>>> + MODE_ACTIVE_STRING,
>>> + MODE_STANDBY_STRING, MODE_REDUNDANCY_STRING);
>>> + case MODE_STANDBY:
>>> + return snprintf(buf, PAGE_SIZE, "%s [%s] %s\n",
>>> + MODE_ACTIVE_STRING,
>>> + MODE_STANDBY_STRING, MODE_REDUNDANCY_STRING);
>>> + case MODE_REDUNDANCY:
>>> + return snprintf(buf, PAGE_SIZE, "%s %s [%s]\n",
>>> + MODE_ACTIVE_STRING,
>>> + MODE_STANDBY_STRING, MODE_REDUNDANCY_STRING);
>>> + default:
>>> + return snprintf(buf, PAGE_SIZE, "unspecified\n");
>>> + }
>>> +}
>>> +
>>> +static ssize_t ipsps_mode_store(struct device *dev,
>>> + struct device_attribute *devattr,
>>> + const char *buf, size_t count)
>>> +{
>>> + u8 reg;
>>> + int rc;
>>> + struct i2c_client *client = to_i2c_client(dev->parent);
>>> + struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
>>> +
>>> + reg = ipsps_regs[attr->index];
>>> + if (sysfs_streq(MODE_STANDBY_STRING, buf)) {
>>> + rc = i2c_smbus_write_byte_data(client, reg,
>>> + MODE_STANDBY);
>>> + if (rc < 0)
>>> + return rc;
>>> + return count;
>>> + } else if (sysfs_streq(MODE_ACTIVE_STRING, buf)) {
>>> + rc = i2c_smbus_write_byte_data(client, reg,
>>> + MODE_ACTIVE);
>>> + if (rc < 0)
>>> + return rc;
>>> + return count;
>>> + }
>>> +
>>> + return -EINVAL;
>>> +}
>>> +
>>> +static SENSOR_DEVICE_ATTR_RO(vendor, ipsps_string, vendor);
>>> +static SENSOR_DEVICE_ATTR_RO(model, ipsps_string, model);
>>> +static SENSOR_DEVICE_ATTR_RO(part_number, ipsps_string, part_number);
>>> +static SENSOR_DEVICE_ATTR_RO(serial_number, ipsps_string, serial_number);
>>> +static SENSOR_DEVICE_ATTR_RO(hw_version, ipsps_string, hw_version);
>>> +static SENSOR_DEVICE_ATTR_RO(fw_version, ipsps_fw_version, fw_version);
>>> +static SENSOR_DEVICE_ATTR_RW(mode, ipsps_mode, mode);
>>> +
>>> +static struct attribute *ipsps_attrs[] = {
>>> + &sensor_dev_attr_vendor.dev_attr.attr,
>>> + &sensor_dev_attr_model.dev_attr.attr,
>>> + &sensor_dev_attr_part_number.dev_attr.attr,
>>> + &sensor_dev_attr_serial_number.dev_attr.attr,
>>> + &sensor_dev_attr_hw_version.dev_attr.attr,
>>> + &sensor_dev_attr_fw_version.dev_attr.attr,
>>> + &sensor_dev_attr_mode.dev_attr.attr,
>>> + NULL,
>>> +};
>>> +
>>> +ATTRIBUTE_GROUPS(ipsps);
>>> +
>>> +static struct pmbus_driver_info ipsps_info = {
>>> + .pages = 1,
>>> + .func[0] = PMBUS_HAVE_VIN | PMBUS_HAVE_VOUT | PMBUS_HAVE_IOUT |
>>> + PMBUS_HAVE_IIN | PMBUS_HAVE_POUT | PMBUS_HAVE_PIN |
>>> + PMBUS_HAVE_FAN12 | PMBUS_HAVE_TEMP | PMBUS_HAVE_TEMP2 |
>>> + PMBUS_HAVE_TEMP3 | PMBUS_HAVE_STATUS_VOUT |
>>> + PMBUS_HAVE_STATUS_IOUT | PMBUS_HAVE_STATUS_INPUT |
>>> + PMBUS_HAVE_STATUS_TEMP | PMBUS_HAVE_STATUS_FAN12,
>>> This can be dynamic read by chip identify function
>>
>> PMBUS_SKIP_STATUS_CHECK weakens auto-detetcion to some degree,
>> and auto-detection takes time since it needs to poll all registers
>> to determine if they exist. I don't mind if you insist, but I don't
>> immediately see the benefits.
>
> ipsps does not specify that the vendor must support the status_cml
> register( some vendor do not support),
> so PMBUS_SKIP_STATUS_CHECK is used here.
>
I did not question that. I did question the request to use auto-detection,
which is quite unusual since you have a dedicated driver for the chip anyway.
Asking more directly: What is the problem with specifying capabilities
directly, and what is the advantage of using auto-detection ?
Guenter
>>
>>> + .groups = ipsps_groups,
>>> +};
>>> +
>>> +static struct pmbus_platform_data ipsps_pdata = {
>>> + .flags = PMBUS_SKIP_STATUS_CHECK,
>>> +};
>>> +
>>> +static int ipsps_probe(struct i2c_client *client,
>>> + const struct i2c_device_id *id)
>>> +{
>>> + client->dev.platform_data = &ipsps_pdata;
>>> Allocate memory for this platform data inside tis function rather than having global variable.
>>
>> Does that have any value other than consuming more memory
>> if there are multiple instances of the driver ?
>>
>>> + return pmbus_do_probe(client, id, &ipsps_info);
>>> +}
>>> +
>>> +static const struct i2c_device_id ipsps_id[] = {
>>> + { "inspur_ipsps1", 0 },
>>> + {}
>>> +};
>>> +MODULE_DEVICE_TABLE(i2c, ipsps_id);
>>> +
>>> +static const struct of_device_id ipsps_of_match[] = {
>>> + { .compatible = "inspur,ipsps1" },
>>> + {}
>>> +};
>>> +MODULE_DEVICE_TABLE(of, ipsps_of_match);
>>> +
>>> +static struct i2c_driver ipsps_driver = {
>>> + .driver = {
>>> + .name = "inspur-ipsps",
>>> + .of_match_table = ipsps_of_match,
>>> + },
>>> + .probe = ipsps_probe,
>>> + .remove = pmbus_do_remove,
>>> + .id_table = ipsps_id,
>>> +};
>>> +
>>> +module_i2c_driver(ipsps_driver);
>>> +
>>> +MODULE_AUTHOR("John Wang");
>>> +MODULE_DESCRIPTION("PMBus driver for Inspur Power System power supplies");
>>> +MODULE_LICENSE("GPL");
>>> --
>>> 2.17.1
>>>
>>>
>>>
>
^ permalink raw reply
* Re: [PATCH v4 2/2] hwmon: pmbus: Add Inspur Power System power supply driver
From: John Wang @ 2019-08-16 2:44 UTC (permalink / raw)
To: Guenter Roeck
Cc: Vijay Khemka, linux-hwmon@vger.kernel.org, jdelvare@suse.com,
linux-doc@vger.kernel.org, openbmc@lists.ozlabs.org,
corbet@lwn.net, linux-kernel@vger.kernel.org,
duanzhijia01@inspur.com
In-Reply-To: <20190815194102.GA11916@roeck-us.net>
On Fri, Aug 16, 2019 at 3:41 AM Guenter Roeck <linux@roeck-us.net> wrote:
>
> On Thu, Aug 15, 2019 at 06:43:52PM +0000, Vijay Khemka wrote:
> >
> >
> > On 8/13/19, 1:36 AM, "openbmc on behalf of John Wang" <openbmc-bounces+vijaykhemka=fb.com@lists.ozlabs.org on behalf of wangzqbj@inspur.com> wrote:
> >
> > Add the driver to monitor Inspur Power System power supplies
> > with hwmon over pmbus.
> >
> > This driver adds sysfs attributes for additional power supply data,
> > including vendor, model, part_number, serial number,
> > firmware revision, hardware revision, and psu mode(active/standby).
> >
> > Signed-off-by: John Wang <wangzqbj@inspur.com>
> > ---
> > v4:
> > - Remove the additional tabs in the Makefile
> > - Rebased on 5.3-rc4, not 5.2
> > v3:
> > - Sort kconfig/makefile entries alphabetically
> > - Remove unnecessary initialization
> > - Use ATTRIBUTE_GROUPS instead of expanding directly
> > - Use memscan to avoid reimplementation
> > v2:
> > - Fix typos in commit message
> > - Invert Christmas tree
> > - Configure device with sysfs attrs, not debugfs entries
> > - Fix errno in fw_version_read, ENODATA to EPROTO
> > - Change the print format of fw-version
> > - Use sysfs_streq instead of strcmp("xxx" "\n", "xxx")
> > - Document sysfs attributes
> > ---
> > Documentation/hwmon/inspur-ipsps1.rst | 79 +++++++++
> > drivers/hwmon/pmbus/Kconfig | 9 +
> > drivers/hwmon/pmbus/Makefile | 1 +
> > drivers/hwmon/pmbus/inspur-ipsps.c | 226 ++++++++++++++++++++++++++
> > 4 files changed, 315 insertions(+)
> > create mode 100644 Documentation/hwmon/inspur-ipsps1.rst
> > create mode 100644 drivers/hwmon/pmbus/inspur-ipsps.c
> >
> > diff --git a/Documentation/hwmon/inspur-ipsps1.rst b/Documentation/hwmon/inspur-ipsps1.rst
> > new file mode 100644
> > index 000000000000..aa19f0ccc8b0
> > --- /dev/null
> > +++ b/Documentation/hwmon/inspur-ipsps1.rst
> > @@ -0,0 +1,79 @@
> > +Kernel driver inspur-ipsps1
> > +=======================
> > +
> > +Supported chips:
> > +
> > + * Inspur Power System power supply unit
> > +
> > +Author: John Wang <wangzqbj@inspur.com>
> > +
> > +Description
> > +-----------
> > +
> > +This driver supports Inspur Power System power supplies. This driver
> > +is a client to the core PMBus driver.
> > +
> > +Usage Notes
> > +-----------
> > +
> > +This driver does not auto-detect devices. You will have to instantiate the
> > +devices explicitly. Please see Documentation/i2c/instantiating-devices for
> > +details.
> > +
> > +Sysfs entries
> > +-------------
> > +
> > +The following attributes are supported:
> > +
> > +======================= ======================================================
> > +curr1_input Measured input current
> > +curr1_label "iin"
> > +curr1_max Maximum current
> > +curr1_max_alarm Current high alarm
> > +curr2_input Measured output current in mA.
> > +curr2_label "iout1"
> > +curr2_crit Critical maximum current
> > +curr2_crit_alarm Current critical high alarm
> > +curr2_max Maximum current
> > +curr2_max_alarm Current high alarm
> > +
> > Please align above details.
Sorry for the mix of table and space
> > +fan1_alarm Fan 1 warning.
> > +fan1_fault Fan 1 fault.
> > +fan1_input Fan 1 speed in RPM.
> > +
> > +in1_alarm Input voltage under-voltage alarm.
> > +in1_input Measured input voltage in mV.
> > +in1_label "vin"
> > +in2_input Measured output voltage in mV.
> > +in2_label "vout1"
> > +in2_lcrit Critical minimum output voltage
> > +in2_lcrit_alarm Output voltage critical low alarm
> > +in2_max Maximum output voltage
> > +in2_max_alarm Output voltage high alarm
> > +in2_min Minimum output voltage
> > +in2_min_alarm Output voltage low alarm
> > +
> > +power1_alarm Input fault or alarm.
> > +power1_input Measured input power in uW.
> > +power1_label "pin"
> > +power1_max Input power limit
> > +power2_max_alarm Output power high alarm
> > +power2_max Output power limit
> > +power2_input Measured output power in uW.
> > +power2_label "pout"
> > +
> > Same alignment issue in description.
will fix.
> > +temp[1-3]_input Measured temperature
> > +temp[1-2]_max Maximum temperature
> > +temp[1-3]_max_alarm Temperature high alarm
> > +
> > +vendor Manufacturer name
> > +model Product model
> > +part_number Product part number
> > +serial_number Product serial number
> > +fw_version Firmware version
> > +hw_version Hardware version
> > +mode Work mode. Can be set to active or
> > + standby, when set to standby, PSU will
> > + automatically switch between standby
> > + and redundancy mode.
> > +======================= ======================================================
> > diff --git a/drivers/hwmon/pmbus/Kconfig b/drivers/hwmon/pmbus/Kconfig
> > index b6588483fae1..d62d69bb7e49 100644
> > --- a/drivers/hwmon/pmbus/Kconfig
> > +++ b/drivers/hwmon/pmbus/Kconfig
> > @@ -46,6 +46,15 @@ config SENSORS_IBM_CFFPS
> > This driver can also be built as a module. If so, the module will
> > be called ibm-cffps.
> >
> > +config SENSORS_INSPUR_IPSPS
> > + tristate "INSPUR Power System Power Supply"
> > + help
> > + If you say yes here you get hardware monitoring support for the INSPUR
> > + Power System power supply.
> > +
> > + This driver can also be built as a module. If so, the module will
> > + be called inspur-ipsps.
> > +
> > config SENSORS_IR35221
> > tristate "Infineon IR35221"
> > help
> > diff --git a/drivers/hwmon/pmbus/Makefile b/drivers/hwmon/pmbus/Makefile
> > index c950ea9a5d00..03bacfcfd660 100644
> > --- a/drivers/hwmon/pmbus/Makefile
> > +++ b/drivers/hwmon/pmbus/Makefile
> > @@ -7,6 +7,7 @@ obj-$(CONFIG_PMBUS) += pmbus_core.o
> > obj-$(CONFIG_SENSORS_PMBUS) += pmbus.o
> > obj-$(CONFIG_SENSORS_ADM1275) += adm1275.o
> > obj-$(CONFIG_SENSORS_IBM_CFFPS) += ibm-cffps.o
> > +obj-$(CONFIG_SENSORS_INSPUR_IPSPS) += inspur-ipsps.o
> > obj-$(CONFIG_SENSORS_IR35221) += ir35221.o
> > obj-$(CONFIG_SENSORS_IR38064) += ir38064.o
> > obj-$(CONFIG_SENSORS_IRPS5401) += irps5401.o
> > diff --git a/drivers/hwmon/pmbus/inspur-ipsps.c b/drivers/hwmon/pmbus/inspur-ipsps.c
> > new file mode 100644
> > index 000000000000..fa981b881a60
> > --- /dev/null
> > +++ b/drivers/hwmon/pmbus/inspur-ipsps.c
> > @@ -0,0 +1,226 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +/*
> > + * Copyright 2019 Inspur Corp.
> > + */
> > +
> > +#include <linux/debugfs.h>
> > +#include <linux/device.h>
> > +#include <linux/fs.h>
> > +#include <linux/i2c.h>
> > +#include <linux/module.h>
> > +#include <linux/pmbus.h>
> > +#include <linux/hwmon-sysfs.h>
> > +
> > +#include "pmbus.h"
> > +
> > +#define IPSPS_REG_VENDOR_ID 0x99
> > +#define IPSPS_REG_MODEL 0x9A
> > +#define IPSPS_REG_FW_VERSION 0x9B
> > +#define IPSPS_REG_PN 0x9C
> > +#define IPSPS_REG_SN 0x9E
> > +#define IPSPS_REG_HW_VERSION 0xB0
> > +#define IPSPS_REG_MODE 0xFC
> > +
> > +#define MODE_ACTIVE 0x55
> > +#define MODE_STANDBY 0x0E
> > +#define MODE_REDUNDANCY 0x00
> > +
> > +#define MODE_ACTIVE_STRING "active"
> > +#define MODE_STANDBY_STRING "standby"
> > +#define MODE_REDUNDANCY_STRING "redundancy"
> > +
> > +enum ipsps_index {
> > + vendor,
> > + model,
> > + fw_version,
> > + part_number,
> > + serial_number,
> > + hw_version,
> > + mode,
> > + num_regs,
> > +};
> > +
> > +static const u8 ipsps_regs[num_regs] = {
> > + [vendor] = IPSPS_REG_VENDOR_ID,
> > + [model] = IPSPS_REG_MODEL,
> > + [fw_version] = IPSPS_REG_FW_VERSION,
> > + [part_number] = IPSPS_REG_PN,
> > + [serial_number] = IPSPS_REG_SN,
> > + [hw_version] = IPSPS_REG_HW_VERSION,
> > + [mode] = IPSPS_REG_MODE,
> > +};
> > +
> > +static ssize_t ipsps_string_show(struct device *dev,
> > + struct device_attribute *devattr,
> > + char *buf)
> > +{
> > + u8 reg;
> > + int rc;
> > + char *p;
> > + char data[I2C_SMBUS_BLOCK_MAX + 1];
> > + struct i2c_client *client = to_i2c_client(dev->parent);
> > + struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> > +
> > + reg = ipsps_regs[attr->index];
> > + rc = i2c_smbus_read_block_data(client, reg, data);
> > + if (rc < 0)
> > + return rc;
> > +
> > + /* filled with printable characters, ending with # */
> > + p = memscan(data, '#', rc);
> > + *p = '\0';
> > +
> > + return snprintf(buf, PAGE_SIZE, "%s\n", data);
> > +}
> > +
> > +static ssize_t ipsps_fw_version_show(struct device *dev,
> > + struct device_attribute *devattr,
> > + char *buf)
> > +{
> > + u8 reg;
> > + int rc;
> > + u8 data[I2C_SMBUS_BLOCK_MAX] = { 0 };
> > + struct i2c_client *client = to_i2c_client(dev->parent);
> > + struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> > +
> > + reg = ipsps_regs[attr->index];
> > + rc = i2c_smbus_read_block_data(client, reg, data);
> > + if (rc < 0)
> > + return rc;
> > +
> > + if (rc != 6)
> > + return -EPROTO;
> > +
> > + return snprintf(buf, PAGE_SIZE, "%u.%02u%u-%u.%02u\n",
> > + data[1], data[2]/* < 100 */, data[3]/*< 10*/,
> > + data[4], data[5]/* < 100 */);
> > +}
> > +
> > +static ssize_t ipsps_mode_show(struct device *dev,
> > + struct device_attribute *devattr, char *buf)
> > +{
> > + u8 reg;
> > + int rc;
> > + struct i2c_client *client = to_i2c_client(dev->parent);
> > + struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> > +
> > + reg = ipsps_regs[attr->index];
> > + rc = i2c_smbus_read_byte_data(client, reg);
> > + if (rc < 0)
> > + return rc;
> > +
> > + switch (rc) {
> > + case MODE_ACTIVE:
> > + return snprintf(buf, PAGE_SIZE, "[%s] %s %s\n",
> > + MODE_ACTIVE_STRING,
> > + MODE_STANDBY_STRING, MODE_REDUNDANCY_STRING);
> > + case MODE_STANDBY:
> > + return snprintf(buf, PAGE_SIZE, "%s [%s] %s\n",
> > + MODE_ACTIVE_STRING,
> > + MODE_STANDBY_STRING, MODE_REDUNDANCY_STRING);
> > + case MODE_REDUNDANCY:
> > + return snprintf(buf, PAGE_SIZE, "%s %s [%s]\n",
> > + MODE_ACTIVE_STRING,
> > + MODE_STANDBY_STRING, MODE_REDUNDANCY_STRING);
> > + default:
> > + return snprintf(buf, PAGE_SIZE, "unspecified\n");
> > + }
> > +}
> > +
> > +static ssize_t ipsps_mode_store(struct device *dev,
> > + struct device_attribute *devattr,
> > + const char *buf, size_t count)
> > +{
> > + u8 reg;
> > + int rc;
> > + struct i2c_client *client = to_i2c_client(dev->parent);
> > + struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> > +
> > + reg = ipsps_regs[attr->index];
> > + if (sysfs_streq(MODE_STANDBY_STRING, buf)) {
> > + rc = i2c_smbus_write_byte_data(client, reg,
> > + MODE_STANDBY);
> > + if (rc < 0)
> > + return rc;
> > + return count;
> > + } else if (sysfs_streq(MODE_ACTIVE_STRING, buf)) {
> > + rc = i2c_smbus_write_byte_data(client, reg,
> > + MODE_ACTIVE);
> > + if (rc < 0)
> > + return rc;
> > + return count;
> > + }
> > +
> > + return -EINVAL;
> > +}
> > +
> > +static SENSOR_DEVICE_ATTR_RO(vendor, ipsps_string, vendor);
> > +static SENSOR_DEVICE_ATTR_RO(model, ipsps_string, model);
> > +static SENSOR_DEVICE_ATTR_RO(part_number, ipsps_string, part_number);
> > +static SENSOR_DEVICE_ATTR_RO(serial_number, ipsps_string, serial_number);
> > +static SENSOR_DEVICE_ATTR_RO(hw_version, ipsps_string, hw_version);
> > +static SENSOR_DEVICE_ATTR_RO(fw_version, ipsps_fw_version, fw_version);
> > +static SENSOR_DEVICE_ATTR_RW(mode, ipsps_mode, mode);
> > +
> > +static struct attribute *ipsps_attrs[] = {
> > + &sensor_dev_attr_vendor.dev_attr.attr,
> > + &sensor_dev_attr_model.dev_attr.attr,
> > + &sensor_dev_attr_part_number.dev_attr.attr,
> > + &sensor_dev_attr_serial_number.dev_attr.attr,
> > + &sensor_dev_attr_hw_version.dev_attr.attr,
> > + &sensor_dev_attr_fw_version.dev_attr.attr,
> > + &sensor_dev_attr_mode.dev_attr.attr,
> > + NULL,
> > +};
> > +
> > +ATTRIBUTE_GROUPS(ipsps);
> > +
> > +static struct pmbus_driver_info ipsps_info = {
> > + .pages = 1,
> > + .func[0] = PMBUS_HAVE_VIN | PMBUS_HAVE_VOUT | PMBUS_HAVE_IOUT |
> > + PMBUS_HAVE_IIN | PMBUS_HAVE_POUT | PMBUS_HAVE_PIN |
> > + PMBUS_HAVE_FAN12 | PMBUS_HAVE_TEMP | PMBUS_HAVE_TEMP2 |
> > + PMBUS_HAVE_TEMP3 | PMBUS_HAVE_STATUS_VOUT |
> > + PMBUS_HAVE_STATUS_IOUT | PMBUS_HAVE_STATUS_INPUT |
> > + PMBUS_HAVE_STATUS_TEMP | PMBUS_HAVE_STATUS_FAN12,
> > This can be dynamic read by chip identify function
>
> PMBUS_SKIP_STATUS_CHECK weakens auto-detetcion to some degree,
> and auto-detection takes time since it needs to poll all registers
> to determine if they exist. I don't mind if you insist, but I don't
> immediately see the benefits.
ipsps does not specify that the vendor must support the status_cml
register( some vendor do not support),
so PMBUS_SKIP_STATUS_CHECK is used here.
>
> > + .groups = ipsps_groups,
> > +};
> > +
> > +static struct pmbus_platform_data ipsps_pdata = {
> > + .flags = PMBUS_SKIP_STATUS_CHECK,
> > +};
> > +
> > +static int ipsps_probe(struct i2c_client *client,
> > + const struct i2c_device_id *id)
> > +{
> > + client->dev.platform_data = &ipsps_pdata;
> > Allocate memory for this platform data inside tis function rather than having global variable.
>
> Does that have any value other than consuming more memory
> if there are multiple instances of the driver ?
>
> > + return pmbus_do_probe(client, id, &ipsps_info);
> > +}
> > +
> > +static const struct i2c_device_id ipsps_id[] = {
> > + { "inspur_ipsps1", 0 },
> > + {}
> > +};
> > +MODULE_DEVICE_TABLE(i2c, ipsps_id);
> > +
> > +static const struct of_device_id ipsps_of_match[] = {
> > + { .compatible = "inspur,ipsps1" },
> > + {}
> > +};
> > +MODULE_DEVICE_TABLE(of, ipsps_of_match);
> > +
> > +static struct i2c_driver ipsps_driver = {
> > + .driver = {
> > + .name = "inspur-ipsps",
> > + .of_match_table = ipsps_of_match,
> > + },
> > + .probe = ipsps_probe,
> > + .remove = pmbus_do_remove,
> > + .id_table = ipsps_id,
> > +};
> > +
> > +module_i2c_driver(ipsps_driver);
> > +
> > +MODULE_AUTHOR("John Wang");
> > +MODULE_DESCRIPTION("PMBus driver for Inspur Power System power supplies");
> > +MODULE_LICENSE("GPL");
> > --
> > 2.17.1
> >
> >
> >
^ permalink raw reply
* Re: [PATCH v7 3/7] of/platform: Add functional dependency link from DT bindings
From: Saravana Kannan @ 2019-08-16 1:50 UTC (permalink / raw)
To: Frank Rowand
Cc: Rob Herring, Mark Rutland, Greg Kroah-Hartman, Rafael J. Wysocki,
Jonathan Corbet,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, LKML,
David Collins, Android Kernel Team, Linux Doc Mailing List
In-Reply-To: <141d2e16-26cc-1f05-1ac0-6784bab5ae88@gmail.com>
On Wed, Aug 7, 2019 at 7:06 PM Frank Rowand <frowand.list@gmail.com> wrote:
>
> On 7/23/19 5:10 PM, Saravana Kannan wrote:
> > Add device-links after the devices are created (but before they are
> > probed) by looking at common DT bindings like clocks and
> > interconnects.
> >
> > Automatically adding device-links for functional dependencies at the
> > framework level provides the following benefits:
> >
> > - Optimizes device probe order and avoids the useless work of
> > attempting probes of devices that will not probe successfully
> > (because their suppliers aren't present or haven't probed yet).
> >
> > For example, in a commonly available mobile SoC, registering just
> > one consumer device's driver at an initcall level earlier than the
> > supplier device's driver causes 11 failed probe attempts before the
> > consumer device probes successfully. This was with a kernel with all
> > the drivers statically compiled in. This problem gets a lot worse if
> > all the drivers are loaded as modules without direct symbol
> > dependencies.
> >
> > - Supplier devices like clock providers, interconnect providers, etc
> > need to keep the resources they provide active and at a particular
> > state(s) during boot up even if their current set of consumers don't
> > request the resource to be active. This is because the rest of the
> > consumers might not have probed yet and turning off the resource
> > before all the consumers have probed could lead to a hang or
> > undesired user experience.
> >
> > Some frameworks (Eg: regulator) handle this today by turning off
> > "unused" resources at late_initcall_sync and hoping all the devices
> > have probed by then. This is not a valid assumption for systems with
> > loadable modules. Other frameworks (Eg: clock) just don't handle
> > this due to the lack of a clear signal for when they can turn off
> > resources. This leads to downstream hacks to handle cases like this
> > that can easily be solved in the upstream kernel.
> >
> > By linking devices before they are probed, we give suppliers a clear
> > count of the number of dependent consumers. Once all of the
> > consumers are active, the suppliers can turn off the unused
> > resources without making assumptions about the number of consumers.
> >
> > By default we just add device-links to track "driver presence" (probe
> > succeeded) of the supplier device. If any other functionality provided
> > by device-links are needed, it is left to the consumer/supplier
> > devices to change the link when they probe.
> >
> > Signed-off-by: Saravana Kannan <saravanak@google.com>
> > ---
> > .../admin-guide/kernel-parameters.txt | 5 +
> > drivers/of/platform.c | 165 ++++++++++++++++++
> > 2 files changed, 170 insertions(+)
> >
>
> Documentation/admin-guide/kernel-paramers.rst:
>
> After line 129, add:
>
> OF Devicetree is enabled
Will do.
> > diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> > index 46b826fcb5ad..12937349d79d 100644
> > --- a/Documentation/admin-guide/kernel-parameters.txt
> > +++ b/Documentation/admin-guide/kernel-parameters.txt
> > @@ -3170,6 +3170,11 @@
> > This can be set from sysctl after boot.
> > See Documentation/admin-guide/sysctl/vm.rst for details.
> >
> > + of_devlink [KNL] Make device links from common DT bindings. Useful
> > + for optimizing probe order and making sure resources
> > + aren't turned off before the consumer devices have
> > + probed.
>
> of_supplier_depend instead of of_devlink ????
I'm open to other names, but of_supplier_depend is just odd.
of_devlink stands for of_device_links. If someone wants to know what
device links do, they can read up on the device links documentation?
>
> of_supplier_depend
> [OF, KNL] Make device links from consumer devicetree
> nodes to supplier devicetree nodes.
We are creating device links between devices. Not device tree nodes.
So this would be wrong. I'll replace it with "devicetree nodes" with
"devices"?
> The
> consumer / supplier relationships are inferred from
> scanning the devicetree.
I like this part. How about clarifying it with "from scanning the
common bindings in the devicetree"? Because we aren't trying to scan
the device specific properties.
> The driver for a consumer
> device will not be probed until the drivers for all of
> its supplier devices have been successfully probed.
A driver is never probed. It's a device that's probed. The dependency
is between devices and not drivers. So, how about I replace this with:
"The consumer device will not be probed until all of its supplier
devices are successfully probed"?
Also, any reason you removed the "resources aren't turned off ..."
part? That's one of the biggest improvements of this patch series. Do
you want to rewrite that part too? Or I'll leave my current wording of
that part as is?
>
>
> > +
> > ohci1394_dma=early [HW] enable debugging via the ohci1394 driver.
> > See Documentation/debugging-via-ohci1394.txt for more
> > info.
> > diff --git a/drivers/of/platform.c b/drivers/of/platform.c
> > index 7801e25e6895..4344419a26fc 100644
> > --- a/drivers/of/platform.c
> > +++ b/drivers/of/platform.c
> > @@ -508,6 +508,170 @@ int of_platform_default_populate(struct device_node *root,
> > }
> > EXPORT_SYMBOL_GPL(of_platform_default_populate);
> >
>
> > +bool of_link_is_valid(struct device_node *con, struct device_node *sup)
>
> Change to less vague:
>
> bool of_ancestor_of(struct device_node *test_np, struct device_node *np)
I thought about that when I wrote the code. But the consumer being the
ancestor of the supplier is just one of the things that makes a link
invalid. There can be other tests we might add as we go. That's why I
kept the function name as of_link_is_valid(). Even if I add an
"ancestor_of" helper function, I'll still have of_link_is_valid() as a
wrapper around it.
> > +{
> > + of_node_get(sup);
> > + /*
> > + * Don't allow linking a device node as a consumer of one of its
> > + * descendant nodes. By definition, a child node can't be a functional
> > + * dependency for the parent node.
> > + */
> > + while (sup) {
> > + if (sup == con) {
> > + of_node_put(sup);
> > + return false;
> > + }
> > + sup = of_get_next_parent(sup);
> > + }
> > + return true;
>
> Change to more generic:
>
> of_node_get(test_np);
> while (test_np) {
> if (test_np == np) {
> of_node_put(test_np);
> return true;
> }
> test_np = of_get_next_parent(test_np);
> }
> return false;
If you do insist on this change, I think we need better names than
test_np and np. It's not clear which node needs to be the ancestor for
this function to return true. With consumer/supplier, it's kinda
obvious that a consumer can't be an ancestor of a supplier.
> > +}
> > +
>
>
> /**
> * of_link_to_phandle - Add device link to supplier
> * @dev: consumer device
> * @sup_np: pointer to the supplier device tree node
Could you suggest something that makes it a bit more clear that this
phandle/node doesn't need to be an actual device (as in, one with
compatible property)? phandle kinda make it clear at least to me. I'd
prefer just saying "phandle to supplier". Thoughts?
Also, what's the guideline on which functions needs doc headers? I
always leaned towards adding them only for non-static functions.
That's why I skipped this one. What's the reasoning for needing one
for this? I'm happy to do this, just want to see that there's some
consistent guideline that's being followed and something I can use for
future patches.
> *
> * TODO: ...
> *
> * Return:
> * * 0 if link successfully created for supplier or of_devlink is false
The "or of_devlink is false" isn't true for this function?
> * * an error if unable to create link
> */
>
> Should have dev_debug() or pr_warn() or something on errors in this
> function -- the caller does not report any issue
I think that'll be too spammy during bootup. This function is expected
to fail often and it's not necessary a catastrophic failure. The
caller can print something if they care to. The current set of callers
don't.
> > +static int of_link_to_phandle(struct device *dev, struct device_node *sup_np)
> > +{
> > + struct platform_device *sup_dev;
> > + u32 dl_flags = DL_FLAG_AUTOPROBE_CONSUMER;
> > + int ret = 0;
> > +
> > + /*
>
> > + * Since we are trying to create device links, we need to find
> > + * the actual device node that owns this supplier phandle.
> > + * Often times it's the same node, but sometimes it can be one
> > + * of the parents. So walk up the parent till you find a
> > + * device.
>
> Change comment to:
>
> * Find the device node that contains the supplier phandle. It may
> * be @sup_np or it may be an ancestor of @sup_np.
Aren't the existing comments giving a better explanation with more context?
But I'll do this.
>
> > + */
>
> See comment in caller of of_link_to_phandle() - do not hide the final
> of_node_put() of sup_np inside of_link_to_phandle(), so need to do
> an of_node_get() here.
>
> of_node_get(sup_np);
Will do. Good point.
>
> > + while (sup_np && !of_find_property(sup_np, "compatible", NULL))
> > + sup_np = of_get_next_parent(sup_np);
> > + if (!sup_np)
>
> > + return 0;
>
> This case should never occur(?), it is an error.
>
> return -ENODEV;
I'm not too sure about all the possible DT combinations to say this?
In that case, this isn't an error. As in, the consumer doesn't need to
wait for this non-existent device to get populated/added. I'd lean
towards leaving it as is and address it later if this is actually a
problem. I want of_link_to_phandle to fail only when a link can be
created, but isn't due to current system state (device isn't added,
creating a cyclic link, etc).
>
> > +
> > + if (!of_link_is_valid(dev->of_node, sup_np)) {
> > + of_node_put(sup_np);
> > + return 0;
>
> Do not use a name that obscures what the function is doing, also
> return an actual issue.
>
> if (of_ancestor_of(sup_np, dev->of_node)) {
> of_node_put(sup_np);
> return -EINVAL;
See my comment about not erroring on cases where a link can't ever be
created? So in your case, you'd want the caller to check the error
value to device which ones to ignore and which ones not to? That seems
a bit more fragile when this function is potentially changed in the
furture.
>
> > + }
> > + sup_dev = of_find_device_by_node(sup_np);
> > + of_node_put(sup_np);
> > + if (!sup_dev)
> > + return -ENODEV;
> > + if (!device_link_add(dev, &sup_dev->dev, dl_flags))
> > + ret = -ENODEV;
For example, in the earlier comment you suggested -ENODEV if there's
no device with "compatible" property that encapsulates the supplier
phandle. But -ENODEV makes more sense for this case where there's
actually no device because it hasn't been added yet. And the caller
needs to be able to distinguish between these two. Are we just going
to arbitrarily pick error values just to make sure they don't overlap?
I don't have a strong opinion one way or another, but I'm trying to
understand what's better in the long run where this function can
evolve to add more checks or handle more cases.
> > + put_device(&sup_dev->dev);
> > + return ret;
> > +}
> > +
>
> /**
> * parse_prop_cells - Property parsing functions for suppliers
> *
> * @np: pointer to a device tree node containing a list
> * @prop_name: Name of property holding a phandle value
> * @phandle_index: For properties holding a table of phandles, this is the
> * index into the table
> * @list_name: property name that contains a list
> * @cells_name: property name that specifies phandles' arguments count
> *
> * This function is useful to parse lists of phandles and their arguments.
> *
> * Return:
> * * Node pointer with refcount incremented, use of_node_put() on it when done.
> * * NULL if not found.
> */
>
> > +static struct device_node *parse_prop_cells(struct device_node *np,
> > + const char *prop, int index,
> > + const char *binding,
> > + const char *cell)
>
> Make names consistent with of_parse_phandle_with_args():
> Change prop to prop_name
> Change index to phandle_index
You call this index even in of_parse_phandle_with_args()
> Change binding to list_name
> Change cell to cells_name
This going to cause a lot more line wraps for barely better names. But
I'll reluctantly do this.
>
> > +{
> > + struct of_phandle_args sup_args;
> > +
>
> > + /* Don't need to check property name for every index. */
> > + if (!index && strcmp(prop, binding))
> > + return NULL;
>
> I read the discussion on whether to check property name only once
> in version 6.
>
> This check is fragile, depending upon the calling code to be properly
> structured. Do the check for all values of index. The reduction of
> overhead from not checking does not justify the fragileness and the
> extra complexity for the code reader to understand why the check can
> be bypassed when
> index is not zero.
This is used only in this file. I understand needing the balance
between code complexity/fragility and efficiency. But I think you push
the line too far away from efficiency. This code is literally never
going to fail because it's a static function called only inside this
file. And the check isn't that hard to understand with that tiny
comment.
> > +
> > + if (of_parse_phandle_with_args(np, binding, cell, index, &sup_args))
> > + return NULL;
> > +
> > + return sup_args.np;
> > +}
> > +
> > +static struct device_node *parse_clocks(struct device_node *np,
> > + const char *prop, int index)
>
> Change prop to prop_name
> Change index to phandle_index
>
> > +{
> > + return parse_prop_cells(np, prop, index, "clocks", "#clock-cells");
> > +}
> > +
> > +static struct device_node *parse_interconnects(struct device_node *np,
> > + const char *prop, int index)
>
> Change prop to prop_name
> Change index to phandle_index
>
> > +{
> > + return parse_prop_cells(np, prop, index, "interconnects",
> > + "#interconnect-cells");
> > +}
> > +
> > +static int strcmp_suffix(const char *str, const char *suffix)
> > +{
> > + unsigned int len, suffix_len;
> > +
> > + len = strlen(str);
> > + suffix_len = strlen(suffix);
> > + if (len <= suffix_len)
> > + return -1;
> > + return strcmp(str + len - suffix_len, suffix);
> > +}
> > +
> > +static struct device_node *parse_regulators(struct device_node *np,
> > + const char *prop, int index)
>
> Change prop to prop_name
> Change index to phandle_index
>
Will do all the renames you list above.
> > +{
> > + if (index || strcmp_suffix(prop, "-supply"))
> > + return NULL;
> > +
> > + return of_parse_phandle(np, prop, 0);
> > +}
> > +
> > +/**
>
> > + * struct supplier_bindings - Information for parsing supplier DT binding
> > + *
> > + * @parse_prop: If the function cannot parse the property, return NULL.
> > + * Otherwise, return the phandle listed in the property
> > + * that corresponds to the index.
>
> There is no documentation of dynamic function parameters in the docbook
> description of a struct. Use this format for now and I will clean up when
> I clean up all of the devicetree docbook info.
>
> Change above comment to:
>
> * struct supplier_bindings - Property parsing functions for suppliers
> *
> * @parse_prop: function name
> * parse_prop() finds the node corresponding to a supplier phandle
> * @parse_prop.np: Pointer to device node holding supplier phandle property
> * @parse_prop.prop_name: Name of property holding a phandle value
> * @parse_prop.index: For properties holding a table of phandles, this is the
> * index into the table
> *
> * Return:
> * * parse_prop() return values are
> * * Node pointer with refcount incremented, use of_node_put() on it when done.
> * * NULL if not found.
Will do. Thanks for writing it.
> > + */
> > +struct supplier_bindings {
> > + struct device_node *(*parse_prop)(struct device_node *np,
> > + const char *name, int index);
>
> Change name to prop_name
> Change index to phandle_index
>
> > +};
> > +
> > +static const struct supplier_bindings bindings[] = {
> > + { .parse_prop = parse_clocks, },
> > + { .parse_prop = parse_interconnects, },
> > + { .parse_prop = parse_regulators, },
>
> > + { },
>
> {},
>
> > +};
> > +
>
> /**
> * of_link_property - TODO:
> * dev:
> * con_np:
> * prop:
> *
> * TODO...
> *
> * Any failed attempt to create a link will NOT result in an immediate return.
> * of_link_property() must create all possible links even when one of more
> * attempts to create a link fail.
>
> Why? isn't one failure enough to prevent probing this device?
> Continuing to scan just results in extra work... which will be
> repeated every time device_link_check_waiting_consumers() is called
Context:
As I said in the cover letter, avoiding unnecessary probes is just one
of the reasons for this patch. The other (arguably more important)
reason for this patch is to make sure suppliers know that they have
consumers that are yet to be probed. That way, suppliers can leave
their resource on AND in the right state if they were left on by the
bootloader. For example, if a clock was left on and at 200 MHz, the
clock provider needs to keep that clock ON and at 200 MHz till all the
consumers are probed.
Answer: Let's say a consumer device Z has suppliers A, B and C. If the
linking fails at A and you return immediately, then B and C could
probe and then figure that they have no more consumers (they don't see
a link to Z) and turn off their resources. And Z could fail
catastrophically.
> *
> * Return:
> * * 0 if TODO:
> * * -ENODEV on error
> */
>
>
> I left some "TODO:" sections to be filled out above.
Will do.
>
>
> > +static bool of_link_property(struct device *dev, struct device_node *con_np,
> > + const char *prop)
>
> Returns 0 or -ENODEV, so bool is incorrect
>
> (Also fixed on 8/8 in patch: "[PATCH 1/2] of/platform: Fix fn definitons for
> of_link_is_valid() and of_link_property()")
Right.
>
> > +{
> > + struct device_node *phandle;
> > + struct supplier_bindings *s = bindings;
> > + unsigned int i = 0;
>
> > + bool done = true, matched = false;
>
> Change to:
>
> bool matched = false;
> int ret = 0;
>
> /* do not stop at first failed link, link all available suppliers */
>
> > +
> > + while (!matched && s->parse_prop) {
> > + while ((phandle = s->parse_prop(con_np, prop, i))) {
> > + matched = true;
> > + i++;
> > + if (of_link_to_phandle(dev, phandle))
>
>
> Remove comment:
>
> > + /*
> > + * Don't stop at the first failure. See
> > + * Documentation for bus_type.add_links for
> > + * more details.
> > + */
Ok
>
> > + done = false;
>
> ret = -ENODEV;
This is nicer. Thanks.
>
> Do not hide of_node_put() inside of_link_to_phandle(), do it here:
>
> of_node_put(phandle);
Ok
>
> > + }
> > + s++;
> > + }
>
> > + return done ? 0 : -ENODEV;
>
> return ret;
>
> > +}
> > +
> > +static bool of_devlink;
> > +core_param(of_devlink, of_devlink, bool, 0);
> > +
>
> /**
> * of_link_to_suppliers - Add device links to suppliers
> * @dev: consumer device
> *
> * Create device links to all available suppliers of @dev.
> * Must NOT stop at the first failed link.
> * If some suppliers are not yet available, this function will be
> * called again when additional suppliers become available.
> *
> * Return:
> * * 0 if links successfully created for all suppliers
> * * an error if one or more suppliers not yet available
> */
Ok
> > +static int of_link_to_suppliers(struct device *dev)
> > +{
> > + struct property *p;
>
> > + bool done = true;
>
> remove done
>
> int ret = 0;
>
> > +
> > + if (!of_devlink)
> > + return 0;
>
> > + if (unlikely(!dev->of_node))
> > + return 0;
>
> Check not needed, for_each_property_of_node() will detect !dev->of_node.
>
> > +
> > + for_each_property_of_node(dev->of_node, p)
> > + if (of_link_property(dev, dev->of_node, p->name))
>
> > + done = false;
>
> ret = -EAGAIN;
>
> > +
> > + return done ? 0 : -ENODEV;
>
> return ret;
Thanks. I think I was too caught up on the rest of the logic
complexity that I missed this obviously ugly code. Will fix.
Thanks,
Saravana
^ permalink raw reply
* Re: [PATCH] Documentation/admin-guide: Embargoed hardware security issues
From: Randy Dunlap @ 2019-08-15 23:10 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Jonathan Corbet, linux-kernel, security, linux-doc,
Thomas Gleixner, Jiri Kosina, Mauro Carvalho Chehab
In-Reply-To: <20190815223147.GA28275@kroah.com>
On 8/15/19 3:31 PM, Greg Kroah-Hartman wrote:
> On Thu, Aug 15, 2019 at 03:12:34PM -0700, Randy Dunlap wrote:
>> On 8/15/19 2:20 PM, Greg Kroah-Hartman wrote:
>>>>> +The hardware security team will provide a per incident specific encrypted
>>>> s/per incident specific/incident-specific/
>>> Fixed. And changed /a/ to /an/
>>
>> eh? still should be /a per incident/
>
> The sentence was changed to:
> The hardware security team will provide an incident-specific
> encrypted...
>
> is not "an" correct here?
That's good then. I didn't see the complete sentence.
--
~Randy
^ permalink raw reply
* Re: [PATCH v2] Documentation/admin-guide: Embargoed hardware security issues
From: Greg Kroah-Hartman @ 2019-08-15 22:31 UTC (permalink / raw)
To: Randy Dunlap
Cc: linux-kernel, Jonathan Corbet, security, linux-doc,
Thomas Gleixner, Jiri Kosina, Mauro Carvalho Chehab,
Josh Poimboeuf, Laura Abbott, Ben Hutchings, Tyler Hicks,
Konrad Rzeszutek Wilk, Jiri Kosina
In-Reply-To: <635f5f3d-dc1e-90a0-8d85-d26a786bb910@infradead.org>
On Thu, Aug 15, 2019 at 03:15:11PM -0700, Randy Dunlap wrote:
> On 8/15/19 2:25 PM, Greg Kroah-Hartman wrote:
> > v2: updated list of people with document from Jiri as I had the old one
> > grammer tweaks based on Jon's review
> > moved document to Documentation/process/
>
> If you get to do a v3, you can change the $Subject also.
Doh!
^ permalink raw reply
* Re: [PATCH] Documentation/admin-guide: Embargoed hardware security issues
From: Greg Kroah-Hartman @ 2019-08-15 22:31 UTC (permalink / raw)
To: Randy Dunlap
Cc: Jonathan Corbet, linux-kernel, security, linux-doc,
Thomas Gleixner, Jiri Kosina, Mauro Carvalho Chehab
In-Reply-To: <e3ae0d66-b9eb-97ba-647a-57f3e2eb4af2@infradead.org>
On Thu, Aug 15, 2019 at 03:12:34PM -0700, Randy Dunlap wrote:
> On 8/15/19 2:20 PM, Greg Kroah-Hartman wrote:
> >>> +The hardware security team will provide a per incident specific encrypted
> >> s/per incident specific/incident-specific/
> > Fixed. And changed /a/ to /an/
>
> eh? still should be /a per incident/
The sentence was changed to:
The hardware security team will provide an incident-specific
encrypted...
is not "an" correct here?
thanks,
greg k-h
^ permalink raw reply
* Re: [PATCH v2] Documentation/admin-guide: Embargoed hardware security issues
From: Randy Dunlap @ 2019-08-15 22:15 UTC (permalink / raw)
To: Greg Kroah-Hartman, linux-kernel, Jonathan Corbet
Cc: security, linux-doc, Thomas Gleixner, Jiri Kosina,
Mauro Carvalho Chehab, Josh Poimboeuf, Laura Abbott,
Ben Hutchings, Tyler Hicks, Konrad Rzeszutek Wilk, Jiri Kosina
In-Reply-To: <20190815212505.GC12041@kroah.com>
On 8/15/19 2:25 PM, Greg Kroah-Hartman wrote:
> v2: updated list of people with document from Jiri as I had the old one
> grammer tweaks based on Jon's review
> moved document to Documentation/process/
If you get to do a v3, you can change the $Subject also.
--
~Randy
^ permalink raw reply
* Re: [PATCH] Documentation/admin-guide: Embargoed hardware security issues
From: Randy Dunlap @ 2019-08-15 22:12 UTC (permalink / raw)
To: Greg Kroah-Hartman, Jonathan Corbet
Cc: linux-kernel, security, linux-doc, Thomas Gleixner, Jiri Kosina,
Mauro Carvalho Chehab
In-Reply-To: <20190815212019.GB12041@kroah.com>
On 8/15/19 2:20 PM, Greg Kroah-Hartman wrote:
>>> +The hardware security team will provide a per incident specific encrypted
>> s/per incident specific/incident-specific/
> Fixed. And changed /a/ to /an/
eh? still should be /a per incident/
--
~Randy
^ permalink raw reply
* Re: [PATCH] x86/CPU/AMD: Clear RDRAND CPUID bit on AMD family 15h/16h
From: Andrew Cooper @ 2019-08-15 21:25 UTC (permalink / raw)
To: Borislav Petkov
Cc: Lendacky, Thomas, linux-kernel@vger.kernel.org,
linux-doc@vger.kernel.org, linux-pm@vger.kernel.org,
x86@kernel.org, Thomas Gleixner, Ingo Molnar, Rafael J . Wysocki,
Pavel Machek, Chen Yu, Jonathan Corbet
In-Reply-To: <20190815210547.GL15313@zn.tnic>
On 15/08/2019 22:05, Borislav Petkov wrote:
> On Thu, Aug 15, 2019 at 09:59:03PM +0100, Andrew Cooper wrote:
>> If you're virtualised, the write to MSR_AMD64_CPUID_FN_1 almost
>> certainly won't take effect, which means userspace will still be able to
>> see the bit.
> msr_clear_bit() has a return value. We should check it before
> doing anything further. I hope the HV actually signals the write
> success/failure properly so that we get a correct return value.
I'm afraid that a number of hypervisors do write-discard, given the
propensity of OSes (certainly traditionally) to go poking at bits like
this without wrmsr_safe().
You either need to read the MSR back and observe that the bit has really
changed, or in this case as Thomas suggests, look at CPUID again (which
will likely be the faster option for the non-virtualised case).
~Andrew
^ permalink raw reply
* [PATCH v2] Documentation/admin-guide: Embargoed hardware security issues
From: Greg Kroah-Hartman @ 2019-08-15 21:25 UTC (permalink / raw)
To: linux-kernel, Jonathan Corbet
Cc: security, linux-doc, Thomas Gleixner, Jiri Kosina,
Mauro Carvalho Chehab, Josh Poimboeuf, Laura Abbott,
Ben Hutchings, Tyler Hicks, Konrad Rzeszutek Wilk, Jiri Kosina
In-Reply-To: <20190725130113.GA12932@kroah.com>
From: Thomas Gleixner <tglx@linutronix.de>
To address the requirements of embargoed hardware issues, like Meltdown,
Spectre, L1TF etc. it is necessary to define and document a process for
handling embargoed hardware security issues.
Following the discussion at the maintainer summit 2018 in Edinburgh
(https://lwn.net/Articles/769417/) the volunteered people have worked
out a process and a Memorandum of Understanding. The latter addresses
the fact that the Linux kernel community cannot sign NDAs for various
reasons.
The initial contact point for hardware security issues is different from
the regular kernel security contact to provide a known and neutral
interface for hardware vendors and researchers. The initial primary
contact team is proposed to be staffed by Linux Foundation Fellows, who
are not associated to a vendor or a distribution and are well connected
in the industry as a whole.
The process is designed with the experience of the past incidents in
mind and tries to address the remaining gaps, so future (hopefully rare)
incidents can be handled more efficiently. It won't remove the fact,
that most of this has to be done behind closed doors, but it is set up
to avoid big bureaucratic hurdles for individual developers.
The process is solely for handling hardware security issues and cannot
be used for regular kernel (software only) security bugs.
This memo can help with hardware companies who, and I quote, "[my
manager] doesn't want to bet his job on the list keeping things secret."
This despite numerous leaks directly from that company over the years,
and none ever so far from the kernel security team. Cognitive
dissidence seems to be a requirement to be a good manager.
To accelerate the adoption of this process, we introduce the concept of
ambassadors in participating companies. The ambassadors are there to
guide people to comply with the process, but are not automatically
involved in the disclosure of a particular incident.
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Reviewed-by: Josh Poimboeuf <jpoimboe@redhat.com>
Acked-by: Laura Abbott <labbott@redhat.com>
Acked-by: Ben Hutchings <ben@decadent.org.uk>
Reviewed-by: Tyler Hicks <tyhicks@canonical.com>
Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Reviewed-by: Jiri Kosina <jkosina@suse.cz>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
v2: updated list of people with document from Jiri as I had the old one
grammer tweaks based on Jon's review
moved document to Documentation/process/
Documentation/process/embargoed-hardware-issues.rst | 279 ++++++++++++++++++++
Documentation/process/index.rst | 1
2 files changed, 280 insertions(+)
--- /dev/null
+++ b/Documentation/process/embargoed-hardware-issues.rst
@@ -0,0 +1,279 @@
+Embargoed hardware issues
+=========================
+
+Scope
+-----
+
+Hardware issues which result in security problems are a different category
+of security bugs than pure software bugs which only affect the Linux
+kernel.
+
+Hardware issues like Meltdown, Spectre, L1TF etc. must be treated
+differently because they usually affect all Operating Systems ("OS") and
+therefore need coordination across different OS vendors, distributions,
+hardware vendors and other parties. For some of the issues, software
+mitigations can depend on microcode or firmware updates, which need further
+coordination.
+
+.. _Contact:
+
+Contact
+-------
+
+The Linux kernel hardware security team is separate from the regular Linux
+kernel security team.
+
+The team only handles the coordination of embargoed hardware security
+issues. Reports of pure software security bugs in the Linux kernel are not
+handled by this team and the reporter will be guided to contact the regular
+Linux kernel security team (:ref:`Documentation/admin-guide/
+<securitybugs>`) instead.
+
+The team can be contacted by email at <hardware-security@kernel.org>. This
+is a private list of security officers who will help you to coordinate an
+issue according to our documented process.
+
+The list is encrypted and email to the list can be sent by either PGP or
+S/MIME encrypted and must be signed with the reporter's PGP key or S/MIME
+certificate. The list's PGP key and S/MIME certificate are available from
+https://www.kernel.org/....
+
+While hardware security issues are often handled by the affected hardware
+vendor, we welcome contact from researchers or individuals who have
+identified a potential hardware flaw.
+
+Hardware security officers
+^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+The current team of hardware security officers:
+
+ - Linus Torvalds (Linux Foundation Fellow)
+ - Greg Kroah-Hartman (Linux Foundation Fellow)
+ - Thomas Gleixner (Linux Foundation Fellow)
+
+Operation of mailing-lists
+^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+The encrypted mailing-lists which are used in our process are hosted on
+Linux Foundation's IT infrastructure. By providing this service Linux
+Foundation's director of IT Infrastructure security technically has the
+ability to access the embargoed information, but is obliged to
+confidentiality by his employment contract. Linux Foundation's director of
+IT Infrastructure security is also responsible for the kernel.org
+infrastructure.
+
+The Linux Foundation's current director of IT Infrastructure security is
+Konstantin Ryabitsev.
+
+
+Non-disclosure agreements
+-------------------------
+
+The Linux kernel hardware security team is not a formal body and therefore
+unable to enter into any non-disclosure agreements. The kernel community
+is aware of the sensitive nature of such issues and offers a Memorandum of
+Understanding instead.
+
+
+Memorandum of Understanding
+---------------------------
+
+The Linux kernel community has a deep understanding of the requirement to
+keep hardware security issues under embargo for coordination between
+different OS vendors, distributors, hardware vendors and other parties.
+
+The Linux kernel community has successfully handled hardware security
+issues in the past and has the necessary mechanisms in place to allow
+community compliant development under embargo restrictions.
+
+The Linux kernel community has a dedicated hardware security team for
+initial contact, which oversees the process of handling such issues under
+embargo rules.
+
+The hardware security team identifies the developers (domain experts) who
+will form the initial response team for a particular issue. The initial
+response team can bring in further developers (domain experts) to address
+the issue in the best technical way.
+
+All involved developers pledge to adhere to the embargo rules and to keep
+the received information confidential. Violation of the pledge will lead to
+immediate exclusion from the current issue and removal from all related
+mailing-lists. In addition, the hardware security team will also exclude
+the offender from future issues. The impact of this consequence is a highly
+effective deterrent in our community. In case a violation happens the
+hardware security team will inform the involved parties immediately. If you
+or anyone becomes aware of a potential violation, please report it
+immediately to the Hardware security officers.
+
+
+Process
+^^^^^^^
+
+Due to the globally distributed nature of Linux kernel development,
+face-to-face meetings are almost impossible to address hardware security
+issues. Phone conferences are hard to coordinate due to time zones and
+other factors and should be only used when absolutely necessary. Encrypted
+email has been proven to be the most effective and secure communication
+method for these types of issues.
+
+Start of Disclosure
+"""""""""""""""""""
+
+Disclosure starts by contacting the Linux kernel hardware security team by
+email. This initial contact should contain a description of the problem and
+a list of any known affected hardware. If your organization builds or
+distributes the affected hardware, we encourage you to also consider what
+other hardware could be affected.
+
+The hardware security team will provide an incident-specific encrypted
+mailing-list which will be used for initial discussion with the reporter,
+further disclosure and coordination.
+
+The hardware security team will provide the disclosing party a list of
+developers (domain experts) who should be informed initially about the
+issue after confirming with the developers that they will adhere to this
+Memorandum of Understanding and the documented process. These developers
+form the initial response team and will be responsible for handling the
+issue after initial contact. The hardware security team is supporting the
+response team, but is not necessarily involved in the mitigation
+development process.
+
+While individual developers might be covered by a non-disclosure agreement
+via their employer, they cannot enter individual non-disclosure agreements
+in their role as Linux kernel developers. They will, however, agree to
+adhere to this documented process and the Memorandum of Understanding.
+
+
+Disclosure
+""""""""""
+
+The disclosing party provides detailed information to the initial response
+team via the specific encrypted mailing-list.
+
+From our experience the technical documentation of these issues is usually
+a sufficient starting point and further technical clarification is best
+done via email.
+
+Mitigation development
+""""""""""""""""""""""
+
+The initial response team sets up an encrypted mailing-list or repurposes
+an existing one if appropriate. The disclosing party should provide a list
+of contacts for all other parties who have already been, or should be
+informed about the issue. The response team contacts these parties so they
+can name experts who should be subscribed to the mailing-list.
+
+Using a mailing-list is close to the normal Linux development process and
+has been successfully used in developing mitigations for various hardware
+security issues in the past.
+
+The mailing-list operates in the same way as normal Linux development.
+Patches are posted, discussed and reviewed and if agreed on applied to a
+non-public git repository which is only accessible to the participating
+developers via a secure connection. The repository contains the main
+development branch against the mainline kernel and backport branches for
+stable kernel versions as necessary.
+
+The initial response team will identify further experts from the Linux
+kernel developer community as needed and inform the disclosing party about
+their participation. Bringing in experts can happen at any time of the
+development process and often needs to be handled in a timely manner.
+
+Coordinated release
+"""""""""""""""""""
+
+The involved parties will negotiate the date and time where the embargo
+ends. At that point the prepared mitigations are integrated into the
+relevant kernel trees and published.
+
+While we understand that hardware security issues need coordinated embargo
+time, the embargo time should be constrained to the minimum time which is
+required for all involved parties to develop, test and prepare the
+mitigations. Extending embargo time artificially to meet conference talk
+dates or other non-technical reasons is creating more work and burden for
+the involved developers and response teams as the patches need to be kept
+up to date in order to follow the ongoing upstream kernel development,
+which might create conflicting changes.
+
+CVE assignment
+""""""""""""""
+
+Neither the hardware security team nor the initial response team assign
+CVEs, nor are CVEs required for the development process. If CVEs are
+provided by the disclosing party they can be used for documentation
+purposes.
+
+Process ambassadors
+-------------------
+
+For assistance with this process we have established ambassadors in various
+organizations, who can answer questions about or provide guidance on the
+reporting process and further handling. Ambassadors are not involved in the
+disclosure of a particular issue, unless requested by a response team or by
+an involved disclosed party. The current ambassadors list:
+
+ ============= ========================================================
+ ARM
+ AMD
+ IBM
+ Intel
+ Qualcomm
+
+ Microsoft
+ VMware
+ XEN
+
+ Canonical Tyler Hicks <tyhicks@canonical.com>
+ Debian Ben Hutchings <ben@decadent.org.uk>
+ Oracle Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
+ Red Hat Josh Poimboeuf <jpoimboe@redhat.com>
+ SUSE Jiri Kosina <jkosina@suse.cz>
+
+ Amazon
+ Google
+ ============== ========================================================
+
+If you want your organization to be added to the ambassadors list, please
+contact the hardware security team. The nominated ambassador has to
+understand and support our process fully and is ideally well connected in
+the Linux kernel community.
+
+Encrypted mailing-lists
+-----------------------
+
+We use encrypted mailing-lists for communication. The operating principle
+of these lists is that email sent to the list is encrypted either with the
+list's PGP key or with the list's S/MIME certificate. The mailing-list
+software decrypts the email and re-encrypts it individually for each
+subscriber with the subscriber's PGP key or S/MIME certificate. Details
+about the mailing-list software and the setup which is used to ensure the
+security of the lists and protection of the data can be found here:
+https://www.kernel.org/....
+
+List keys
+^^^^^^^^^
+
+For initial contact see :ref:`Contact`. For incident specific mailing-lists
+the key and S/MIME certificate are conveyed to the subscribers by email
+sent from the specific list.
+
+Subscription to incident specific lists
+^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+Subscription is handled by the response teams. Disclosed parties who want
+to participate in the communication send a list of potential subscribers to
+the response team so the response team can validate subscription requests.
+
+Each subscriber needs to send a subscription request to the response team
+by email. The email must be signed with the subscriber's PGP key or S/MIME
+certificate. If a PGP key is used, it must be available from a public key
+server and is ideally connected to the Linux kernel's PGP web of trust. See
+also: https://www.kernel.org/signature.html.
+
+The response team verifies that the subscriber request is valid and adds
+the subscriber to the list. After subscription the subscriber will receive
+email from the mailing-list which is signed either with the list's PGP key
+or the list's S/MIME certificate. The subscriber's email client can extract
+the PGP key or the S/MIME certificate from the signature so the subscriber
+can send encrypted email to the list.
+
--- a/Documentation/process/index.rst
+++ b/Documentation/process/index.rst
@@ -45,6 +45,7 @@ Other guides to the community that are o
submit-checklist
kernel-docs
deprecated
+ embargoed-hardware-issues
These are some overall technical guides that have been put here for now for
lack of a better place.
^ permalink raw reply
* Re: [PATCH] Documentation/admin-guide: Embargoed hardware security issues
From: Greg Kroah-Hartman @ 2019-08-15 21:20 UTC (permalink / raw)
To: Jonathan Corbet
Cc: linux-kernel, security, linux-doc, Thomas Gleixner, Jiri Kosina,
Mauro Carvalho Chehab
In-Reply-To: <20190725151302.16a3e0e3@lwn.net>
On Thu, Jul 25, 2019 at 03:13:02PM -0600, Jonathan Corbet wrote:
> On Thu, 25 Jul 2019 15:01:13 +0200
> Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:
>
> > From: Thomas Gleixner <tglx@linutronix.de>
> >
> > To address the requirements of embargoed hardware issues, like Meltdown,
> > Spectre, L1TF, etc. it is necessary to define and document a process for
> > handling embargoed hardware security issues.
> >
> > Following the discussion at the maintainer summit 2018 in Edinburgh
> > (https://lwn.net/Articles/769417/) the volunteered people have worked
> > out a process and a Memorandum of Understanding. The latter addresses
> > the fact that the Linux kernel community cannot sign NDAs for various
> > reasons.
> >
>
> [...]
>
> > Documentation/admin-guide/embargoed-hardware-issues.rst | 281 ++++++++++++++++
> > Documentation/admin-guide/index.rst | 1
> > 2 files changed, 282 insertions(+)
>
> So I would argue that the admin guide (which is aimed at sysadmins) is the
> wrong place for this document. It's process information and is best placed
> in the process manual (Documentation/process) IMO. (Yes, I know
> security-bugs.rst is in the admin guide; I remember there was a discussion
> at the time and it ended up there, but I'm not really sure that's right
> either).
Ok, will move it to the process directory.
> > Note, this document has gone through numerous reviews by a number of
> > kernel developers, developers at some of the Linux distros, as well as
> > all of the lawyers from almost all open source-related companies. It's
> > been sitting on my local drive with no comments for a few months now,
> > and it's about time to get this out and merged properly.
> >
> > If anyone has any final comments, please let me know.
>
> I do think it could benefit from a pass for basic language issues; I can do
> that if such an effort would be welcome.
It would be!
> > If anyone from any company listed below wishes to add their name to the
> > document, please send a follow-on patch and I will be glad to add it to
> > the series. I had a number of "I'll sign up" type comments from
> > different people, but I want something with a "s-o-b" to keep people on
> > the hook for this, so I did not add their name to the file without that.
> >
> > thanks,
> >
> > greg k-h
> >
> >
> >
> > --- /dev/null
> > +++ b/Documentation/admin-guide/embargoed-hardware-issues.rst
> > @@ -0,0 +1,281 @@
> > +.. _embargoedhardwareissues:
>
> This label isn't used anywhere.
Odd, ok, dropped.
>
> > +Embargoed hardware issues
> > +=========================
> > +
> > +Scope
> > +-----
> > +
> > +Hardware issues which result in security problems are a different category
> > +of security bugs than pure software bugs which only affect the Linux
> > +kernel.
> > +
> > +Hardware issues like Meltdown, Spectre, L1TF etc. must be treated
> > +differently because they usually affect all Operating Systems (“OS“) and
>
> Somebody may well complain about the "smart quotes" here; non-ascii stuff
> has led to unhappiness in the past.
Now fixed, thanks.
> > +therefore need coordination across different OS vendors, distributions,
> > +hardware vendors and other parties. For some of the issues, software
> > +mitigations can depend on microcode or firmware updates, which need further
> > +coordination.
> > +
> > +.. _Contact:
> > +
> > +Contact
> > +-------
> > +
> > +The Linux kernel hardware security team is separate from the regular Linux
> > +kernel security team.
> > +
> > +The team is only handling the coordination of embargoed hardware security
>
> s/is only handling/only handles/
Fixed.
> > +issues. Reports of pure software security bugs in the Linux kernel are not
> > +handled by this team and the reporter will be guided to contact the regular
> > +Linux kernel security team (:ref:`Documentation/admin-guide/
> > +<securitybugs>`) instead.
> > +
> > +The team can be contacted by email at <hardware-security@kernel.org>. This
> > +is a private list of security officers who will help you to coordinate an
> > +issue according to our documented process.
> > +
> > +The list is encrypted and email to the list can be sent by either PGP or
> > +S/MIME encrypted and must be signed with the reporter's PGP key or S/MIME
> > +certificate. The list's PGP key and S/MIME certificate are available from
> > +https://www.kernel.org/....
>
> Somebody needs to fill in some dots there...:)
Will work on that...
> > +While hardware security issues are often handled by the affected hardware
> > +vendor, we welcome contact from researchers or individuals who identified a
>
> who *have* identified
fixed.
> > +potential hardware flaw.
> > +
> > +Hardware security officers
> > +^^^^^^^^^^^^^^^^^^^^^^^^^^
> > +
> > +The current team of hardware security officers:
> > +
> > + - Linus Torvalds (Linux Foundation Fellow)
> > + - Greg Kroah-Hartman (Linux Foundation Fellow)
> > + - Thomas Gleixner (Linux Foundation Fellow)
> > +
> > +Operation of mailing-lists
> > +^^^^^^^^^^^^^^^^^^^^^^^^^^
>
> I would de-hyphenate "mailing list" throughout. But that's me.
I'll let the original author have it his way :)
> > +The encrypted mailing-lists which are used in our process are hosted on
> > +Linux Foundation's IT infrastructure. By providing this service Linux
> > +Foundation's director of IT Infrastructure security technically has the
> > +ability to access the embargoed information, but is obliged to
> > +confidentiality by his employment contract. Linux Foundation's director of
> > +IT Infrastructure security is also responsible for the kernel.org
> > +infrastructure.
> > +
> > +The Linux Foundation's current director of IT Infrastructure security is
> > +Konstantin Ryabitsev.
> > +
> > +
> > +Non-disclosure agreements
> > +-------------------------
> > +
> > +The Linux kernel hardware security team is not a formal body and therefore
> > +unable to enter into any non-disclosure agreements. The kernel community
> > +is aware of the sensitive nature of such issues and offers a Memorandum of
> > +Understanding instead.
> > +
> > +
> > +Memorandum of Understanding
> > +---------------------------
> > +
> > +The Linux kernel community has a deep understanding of the requirement to
> > +keep hardware security issues under embargo for coordination between
> > +different OS vendors, distributors, hardware vendors and other parties.
> > +
> > +The Linux kernel community has successfully handled hardware security
> > +issues in the past and has the necessary mechanisms in place to allow
> > +community compliant development under embargo restrictions.
> > +
> > +The Linux kernel community has a dedicated hardware security team for
> > +initial contact, which oversees the process of handling such issues under
> > +embargo rules.
> > +
> > +The hardware security team identifies the developers (domain experts) which
> > +form the initial response team for a particular issue. The initial response
>
> s/which form/who will form/
fixed.
> > +team can bring in further developers (domain experts) to address the issue
> > +in the best technical way.
>
> Does the reporter get any say in who can be in this group? That should
> probably be made explicit either way.
That is discussed later in the document, and the reporter can offer up
people, but that's about it. It's up to us running the list to be the
ones in charge of this, and that's one of the main reasons this document
exists.
I'll leave the wording as-is for now, given all of the lawyers who have
already agreed with it. If we find people objecting to how it all is
written now in the future, we can revisit it.
> > +All involved developers pledge to adhere to the embargo rules and to keep
> > +the received information confidential. Violation of the pledge will lead to
> > +immediate exclusion from the current issue and removal from all related
> > +mailing-lists. In addition, the hardware security team will also exclude
> > +the offender from future issues. The impact of this consequence is a highly
> > +effective deterrent in our community. In case a violation happens the
> > +hardware security team will inform the involved parties immediately. If you
> > +or anyone becomes aware of a potential violation, please report it
> > +immediately to the Hardware security officers.
> > +
> > +
> > +Process
> > +^^^^^^^
> > +
> > +Due to the globally distributed nature of Linux kernel development, face to
> > +face meetings are almost impossible to address hardware security issues.
>
> face-to-face
fixed.
> > +Phone conferences are hard to coordinate due to time zones and other
> > +factors and should be only used when absolutely necessary. Encrypted email
> > +has been proven to be the most effective and secure communication method
> > +for these types of issues.
> > +
> > +Start of Disclosure
> > +"""""""""""""""""""
> > +
> > +Disclosure starts by contacting the Linux kernel hardware security team by
> > +email. This initial contact should contain a description of the problem and
> > +a list of any known affected hardware. If your organization builds or
> > +distributes the affected hardware, we encourage you to also consider what
> > +other hardware could be affected.
> > +
> > +The hardware security team will provide a per incident specific encrypted
>
> s/per incident specific/incident-specific/
Fixed. And changed /a/ to /an/
> > +mailing-list which will be used for initial discussion with the reporter,
> > +further disclosure and coordination.
> > +
> > +The hardware security team will provide the disclosing party a list of
> > +developers (domain experts) who should be informed initially about the
> > +issue after confirming with the developers that they will adhere to this
> > +Memorandum of Understanding and the documented process. These developers
> > +form the initial response team and will be responsible for handling the
> > +issue after initial contact. The hardware security team is supporting the
> > +response team, but is not necessarily involved in the mitigation
> > +development process.
>
> Again, "should be informed" is conditional, suggesting that the reporter
> might have some sort of veto power. But the actual policy is not clear.
Embrace the grey, it is much better here :)
> > +While individual developers might be covered by a non-disclosure agreement
> > +via their employer, they cannot enter individual non-disclosure agreements
> > +in their role as Linux kernel developers. They will, however, adhere to
> > +this documented process and the Memorandum of Understanding.
>
> They will *agree to* adhere ... We expect that actual adherence will be
> the case but there is no way (even if an NDA were involved) to guarantee
> that.
True, fixed up now.
> > +Disclosure
> > +""""""""""
> > +
> > +The disclosing party provides detailed information to the initial response
> > +team via the specific encrypted mailing-list.
> > +
> > +From our experience the technical documentation of these issues is usually
> > +a sufficient starting point and further technical clarification is best
> > +done via email.
> > +
> > +Mitigation development
> > +""""""""""""""""""""""
> > +
> > +The initial response team sets up an encrypted mailing-list or repurposes
> > +an existing one if appropriate. The disclosing party should provide a list
> > +of contacts for all other parties who have already been, or should be
> > +informed about the issue. The response team contacts these parties so they
> > +can name experts who should be subscribed to the mailing-list.
> > +
> > +Using a mailing-list is close to the normal Linux development process and
> > +has been successfully used in developing mitigations for various hardware
> > +security issues in the past.
> > +
> > +The mailing-list operates in the same way as normal Linux development.
> > +Patches are posted, discussed and reviewed and if agreed on applied to a
> > +non-public git repository which is only accessible to the participating
> > +developers via a secure connection. The repository contains the main
> > +development branch against the mainline kernel and backport branches for
> > +stable kernel versions as necessary.
>
> Do we want to envision a KPTI-like situation where the mitigation can be
> developed publicly? Or perhaps just handle any such case if and when it
> ever arises?
We can handle such cases if and when it arises. And who's to say it
already hasn't? :)
> > +The initial response team will identify further experts from the Linux
> > +kernel developer community as needed and inform the disclosing party about
> > +their participation. Bringing in experts can happen at any time of the
> > +development process and often needs to be handled in a timely manner.
> > +
> > +Coordinated release
> > +"""""""""""""""""""
> > +
> > +The involved parties will negotiate the date and time where the embargo
> > +ends. At that point the prepared mitigations are integrated into the
> > +relevant kernel trees and published.
> > +
> > +While we understand that hardware security issues need coordinated embargo
> > +time, the embargo time should be constrained to the minimum time which is
> > +required for all involved parties to develop, test and prepare the
> > +mitigations. Extending embargo time artificially to meet conference talk
> > +dates or other non-technical reasons is creating more work and burden for
> > +the involved developers and response teams as the patches need to be kept
> > +up to date in order to follow the ongoing upstream kernel development,
> > +which might create conflicting changes.
> > +
> > +CVE assignment
> > +""""""""""""""
> > +
> > +Neither the hardware security team nor the initial response team assign
> > +CVEs, nor are CVEs required for the development process. If CVEs are
> > +provided by the disclosing party they can be used for documentation
> > +purposes.
> > +
> > +Process ambassadors
> > +-------------------
> > +
> > +For assistance with this process we have established ambassadors in various
> > +organizations, who can answer questions about or provide guidance on the
> > +reporting process and further handling. Ambassadors are not involved in the
> > +disclosure of a particular issue, unless requested by a response team or by
> > +an involved disclosed party. The current ambassadors list:
> > +
> > + ============== ========================================================
> > + ARM
> > + AMD
> > + IBM
> > + Intel
> > + Qualcomm
> > +
> > + Microsoft
> > + VMware
> > + XEN
> > +
> > + Canonical
> > + Debian
> > + Oracle
> > + Redhat
> > + Suse Jiri Kosina <jkosina@suse.com>
> > +
> > + Amazon
> > + Google
> > + ============== ========================================================
>
> Having companies without names seems a little weird. Unless perhaps you
> have people teed up to add their names in follow-on patches?
Jiri posted an update with more names.
The blank lines are there for where we want names. I have a bunch of
people who have agreed, but I want them sending add-on patches for the
document, with their s-o-b on it, so that everyone "knows" they really
agree with this, and that someone didn't just add their name to the
document without them realizing.
> > +If you want your organization to be added to the ambassadors list, please
> > +contact the hardware security team. The nominated ambassador has to
> > +understand and support our process fully and is ideally well connected in
> > +the Linux kernel community.
> > +
> > +Encrypted mailing-lists
> > +-----------------------
> > +
> > +We use encrypted mailing-lists for communication. The operating principle
> > +of these lists is that email sent to the list is encrypted either with the
> > +list's PGP key or with the list's S/MIME certificate. The mailing-list
> > +software decrypts the email and re-encrypts it individually for each
> > +subscriber with the subscriber's PGP key or S/MIME certificate. Details
> > +about the mailing-list software and the setup which is used to ensure the
> > +security of the lists and protection of the data can be found here:
> > +https://www.kernel.org/....
>
> That URL is also in need of completion.
Yeah, we will work on that next...
>
> The topic of encrypted mailing lists is visited several times in this
> document; I wonder if that could be coalesced somehow?
Edits are welcome :)
> > +List keys
> > +^^^^^^^^^
> > +
> > +For initial contact see :ref:`Contact`. For incident specific mailing-lists
> > +the key and S/MIME certificate are conveyed to the subscribers by email
> > +sent from the specific list.
> > +
> > +Subscription to incident specific lists
> > +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > +
> > +Subscription is handled by the response teams. Disclosed parties who want
> > +to participate in the communication send a list of potential subscribers to
> > +the response team so the response team can validate subscription requests.
> > +
> > +Each subscriber needs to send a subscription request to the response team
> > +by email. The email must be signed with the subscriber's PGP key or S/MIME
> > +certificate. If a PGP key is used, it must be available from a public key
> > +server and is ideally connected to the Linux kernel's PGP web of trust. See
> > +also: https://www.kernel.org/signature.html.
>
> The "public key server" thing isn't working quite as well as it was; does
> this requirement need to be revisited?
We have a private web right now, and it's the best we have at the
moment. So until we get something else, let's stick with it.
Many thanks for the edits, I'll send out a new version soon.
greg k-h
^ permalink raw reply
* Re: [PATCH] x86/CPU/AMD: Clear RDRAND CPUID bit on AMD family 15h/16h
From: Borislav Petkov @ 2019-08-15 21:05 UTC (permalink / raw)
To: Andrew Cooper
Cc: Lendacky, Thomas, linux-kernel@vger.kernel.org,
linux-doc@vger.kernel.org, linux-pm@vger.kernel.org,
x86@kernel.org, Thomas Gleixner, Ingo Molnar, Rafael J . Wysocki,
Pavel Machek, Chen Yu, Jonathan Corbet
In-Reply-To: <a24a2c7d-cfab-a049-37e8-7260a9063a7c@citrix.com>
On Thu, Aug 15, 2019 at 09:59:03PM +0100, Andrew Cooper wrote:
> If you're virtualised, the write to MSR_AMD64_CPUID_FN_1 almost
> certainly won't take effect, which means userspace will still be able to
> see the bit.
msr_clear_bit() has a return value. We should check it before
doing anything further. I hope the HV actually signals the write
success/failure properly so that we get a correct return value.
--
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.
^ permalink raw reply
* Re: [PATCH] Documentation/admin-guide: Embargoed hardware security issues
From: Greg Kroah-Hartman @ 2019-08-15 21:04 UTC (permalink / raw)
To: Jiri Kosina
Cc: linux-kernel, Jonathan Corbet, security, linux-doc,
Thomas Gleixner, Mauro Carvalho Chehab
In-Reply-To: <20190805151244.GA28296@kroah.com>
On Mon, Aug 05, 2019 at 05:12:44PM +0200, Greg Kroah-Hartman wrote:
> On Sun, Aug 04, 2019 at 02:17:00AM +0200, Jiri Kosina wrote:
> > On Thu, 25 Jul 2019, Greg Kroah-Hartman wrote:
> >
> > > To address the requirements of embargoed hardware issues, like Meltdown,
> > > Spectre, L1TF, etc. it is necessary to define and document a process for
> > > handling embargoed hardware security issues.
> >
> > I don't know what exactly went wrong, but there is a much more up-to-date
> > version of that document (especially when it comes to vendor contacts),
> > which I sent around on Thu, 2 May 2019 20:23:48 +0200 (CEST) already.
> > Please find it below.
>
> Ah, sorry, don't know what happened here, we had too many different
> versions floating around.
>
> I'll take your version, make the edits recommended and send out a new
> one in a few days, thanks!
Looks like your version only had the difference being the list of
ambassadors and a bunch of people who reviewed the document. No
text changes in the document itself, which was good to see we all agreed
on the correct wording :)
thanks,
greg k-h
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox