[PATCH v10 03/12] peci: Add support for PECI bus driver core
Greg Kroah-Hartman
gregkh at linuxfoundation.org
Wed Jan 23 00:20:47 AEDT 2019
On Mon, Jan 07, 2019 at 01:41:27PM -0800, Jae Hyun Yoo wrote:
> +config PECI
> + bool "PECI support"
> + select RT_MUTEXES
> + select CRC8
> + help
> + The Platform Environment Control Interface (PECI) is a one-wire bus
> + interface that provides a communication channel from Intel processors
> + and chipset components to external monitoring or control devices.
Why can't this be built as a module?
And why do you rely on rt mutexes?
Anyway, the driver core interaction looks good, I have a few other minor
comments on the ioctl handling:
> +typedef int (*peci_ioctl_fn_type)(struct peci_adapter *, void *);
> +
> +static const peci_ioctl_fn_type peci_ioctl_fn[PECI_CMD_MAX] = {
> + peci_ioctl_xfer,
> + peci_ioctl_ping,
> + peci_ioctl_get_dib,
> + peci_ioctl_get_temp,
> + peci_ioctl_rd_pkg_cfg,
> + peci_ioctl_wr_pkg_cfg,
> + peci_ioctl_rd_ia_msr,
> + NULL, /* Reserved */
> + peci_ioctl_rd_pci_cfg,
> + NULL, /* Reserved */
> + peci_ioctl_rd_pci_cfg_local,
> + peci_ioctl_wr_pci_cfg_local,
> +};
That's "interesting", why not have a list that has the ioctl number, and
the function pointer? That way no need for "reserved" entries, and you
don't have to add a new item to your "is this a valid ioctl" check
below:
> +static long peci_ioctl(struct file *file, unsigned int iocmd, unsigned long arg)
> +{
> + struct peci_adapter *adapter = file->private_data;
> + void __user *argp = (void __user *)arg;
> + unsigned int msg_len;
> + enum peci_cmd cmd;
> + int rc = 0;
No need to set rc here.
> + u8 *msg;
> +
> + if (!capable(CAP_SYS_ADMIN))
> + return -EPERM;
Really? Nice, you have userspace tools running as root, what could go
wrong... :)
> +
> + dev_dbg(&adapter->dev, "ioctl, cmd=0x%x, arg=0x%lx\n", iocmd, arg);
I understand the need for this type of thing when debugging the code
initially, but you can remove them all now, ftrace should provide what
you need now, right?
> +
> + switch (iocmd) {
> + case PECI_IOC_XFER:
> + case PECI_IOC_PING:
> + case PECI_IOC_GET_DIB:
> + case PECI_IOC_GET_TEMP:
> + case PECI_IOC_RD_PKG_CFG:
> + case PECI_IOC_WR_PKG_CFG:
> + case PECI_IOC_RD_IA_MSR:
> + case PECI_IOC_RD_PCI_CFG:
> + case PECI_IOC_RD_PCI_CFG_LOCAL:
> + case PECI_IOC_WR_PCI_CFG_LOCAL:
> + cmd = _IOC_NR(iocmd);
> + msg_len = _IOC_SIZE(iocmd);
> + break;
That check up there can be replaced with an iteration over the list of
ioctl functions, right?
> +
> + default:
> + dev_dbg(&adapter->dev, "Invalid ioctl cmd : 0x%x\n", iocmd);
> + return -ENOTTY;
> + }
> +
> + if (!access_ok(argp, msg_len))
> + return -EFAULT;
> +
> + msg = memdup_user(argp, msg_len);
> + if (IS_ERR(msg))
> + return PTR_ERR(msg);
Why call access_ok() if you are going to copy the memory anyways?
memdup_user should fail with -EFAULT if you can't copy the memory
properly.
> +
> + rc = peci_command(adapter, cmd, msg);
Are you _SURE_ you audited your ioctls to not do foolish things with
userspace values? I sure didn't, so can you please have someone else do
this and sign off on this patch that they did so?
> +static int peci_detect(struct peci_adapter *adapter, u8 addr)
> +{
> + struct peci_ping_msg msg;
> +
> + msg.addr = addr;
What about the un-initialized fields in this structure? Can you
properly handle that, and also, is this ok to be on the stack?
> +static ssize_t peci_sysfs_new_device(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf, size_t count)
> +{
> + struct peci_adapter *adapter = to_peci_adapter(dev);
> + struct peci_board_info info = {};
> + struct peci_client *client;
> + char *blank, end;
> + int rc;
> +
> + /* Parse device type */
> + blank = strchr(buf, ' ');
> + if (!blank) {
> + dev_err(dev, "%s: Missing parameters\n", "new_device");
> + return -EINVAL;
> + }
> + if (blank - buf > PECI_NAME_SIZE - 1) {
> + dev_err(dev, "%s: Invalid device type\n", "new_device");
> + return -EINVAL;
> + }
> + memcpy(info.type, buf, blank - buf);
> +
> + /* Parse remaining parameters, reject extra parameters */
> + rc = sscanf(++blank, "%hi%c", &info.addr, &end);
Please do not tell me you are parsing a sysfs write to do some type of
new configuration. That is not what sysfs is for, that is what configfs
is for, please use that instead, this isn't ok.
> + if (rc < 1) {
> + dev_err(dev, "%s: Can't parse client address\n", "new_device");
> + return -EINVAL;
> + }
> + if (rc > 1 && end != '\n') {
> + dev_err(dev, "%s: Extra parameters\n", "new_device");
> + return -EINVAL;
> + }
> +
> + client = peci_new_device(adapter, &info);
> + if (!client)
> + return -EINVAL;
> +
> + /* Keep track of the added device */
> + mutex_lock(&adapter->userspace_clients_lock);
> + list_add_tail(&client->detected, &adapter->userspace_clients);
> + mutex_unlock(&adapter->userspace_clients_lock);
> + dev_info(dev, "%s: Instantiated device %s at 0x%02hx\n", "new_device",
> + info.type, info.addr);
Don't be noisy for things that are expected to happen.
> +
> + return count;
> +}
> +static DEVICE_ATTR(new_device, 0200, NULL, peci_sysfs_new_device);
Not that you should be doing this, but DEVICE_ATTR_RO() is what you want
here, right?
> +
> +static ssize_t peci_sysfs_delete_device(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf, size_t count)
> +{
> + struct peci_adapter *adapter = to_peci_adapter(dev);
> + struct peci_client *client, *next;
> + struct peci_board_info info = {};
> + struct peci_driver *driver;
> + char *blank, end;
> + int rc;
> +
> + /* Parse device type */
> + blank = strchr(buf, ' ');
> + if (!blank) {
> + dev_err(dev, "%s: Missing parameters\n", "delete_device");
> + return -EINVAL;
> + }
> + if (blank - buf > PECI_NAME_SIZE - 1) {
> + dev_err(dev, "%s: Invalid device type\n", "delete_device");
> + return -EINVAL;
> + }
> + memcpy(info.type, buf, blank - buf);
> +
> + /* Parse remaining parameters, reject extra parameters */
> + rc = sscanf(++blank, "%hi%c", &info.addr, &end);
> + if (rc < 1) {
> + dev_err(dev, "%s: Can't parse client address\n",
> + "delete_device");
> + return -EINVAL;
> + }
> + if (rc > 1 && end != '\n') {
> + dev_err(dev, "%s: Extra parameters\n", "delete_device");
> + return -EINVAL;
> + }
Same here, no parsing of configurations through sysfs, that is not ok.
Again, use configfs.
> +
> + /* Make sure the device was added through sysfs */
> + rc = -ENOENT;
> + mutex_lock(&adapter->userspace_clients_lock);
> + list_for_each_entry_safe(client, next, &adapter->userspace_clients,
> + detected) {
> + driver = to_peci_driver(client->dev.driver);
> +
> + if (client->addr == info.addr &&
> + !strncmp(client->name, info.type, PECI_NAME_SIZE)) {
> + dev_info(dev, "%s: Deleting device %s at 0x%02hx\n",
> + "delete_device", client->name, client->addr);
> + list_del(&client->detected);
> + peci_unregister_device(client);
> + rc = count;
> + break;
> + }
> + }
> + mutex_unlock(&adapter->userspace_clients_lock);
> +
> + if (rc < 0)
> + dev_err(dev, "%s: Can't find device in list\n",
> + "delete_device");
So you can just spam the syslog by writing odd values to the file? Not
good.
> +
> + return rc;
> +}
> +static DEVICE_ATTR_IGNORE_LOCKDEP(delete_device, 0200, NULL,
> + peci_sysfs_delete_device);
sysfs files that remove themselves are reserved for a specific circle in
hell, you really don't want to mess with them :(
> +/**
> + * struct peci_adapter - represent a PECI adapter
> + * @owner: owner module of the PECI adpater
> + * @bus_lock: mutex for exclusion of multiple callers
> + * @dev: device interface to this driver
> + * @cdev: character device object to create character device
> + * @nr: the bus number to map
> + * @name: name of the adapter
> + * @userspace_clients_lock: mutex for exclusion of clients handling
> + * @userspace_clients: list of registered clients
> + * @xfer: low-level transfer function pointer of the adapter
> + * @cmd_mask: mask for supportable PECI commands
> + *
> + * Each PECI adapter can communicate with one or more PECI client children.
> + * These make a small bus, sharing a single wired PECI connection.
> + */
> +struct peci_adapter {
> + struct module *owner;
> + struct rt_mutex bus_lock;
> + struct device dev;
> + struct cdev cdev;
Yeah, two differently reference counted variables in the same structure,
what could go wrong! :(
Don't do this, make your cdev a pointer to be sure you get things right,
otherwise this will never work properly.
And shouldn't the character device be a class device? Don't confuse
devices in the driver model with the interaction of them to userspace in
a specific manner, those should be separate things, right?
> + int nr;
> + char name[PECI_NAME_SIZE];
What's wrong with the name in struct device?
> + struct mutex userspace_clients_lock; /* clients list mutex */
> + struct list_head userspace_clients;
> + int (*xfer)(struct peci_adapter *adapter,
> + struct peci_xfer_msg *msg);
> + uint cmd_mask;
u32?
> --- /dev/null
> +++ b/include/uapi/linux/peci-ioctl.h
> @@ -0,0 +1,403 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/* Copyright (c) 2018-2019 Intel Corporation */
> +
> +#ifndef __PECI_IOCTL_H
> +#define __PECI_IOCTL_H
> +
> +#include <linux/ioctl.h>
> +#include <linux/types.h>
> +
> +/* Base Address of 48d */
> +#define PECI_BASE_ADDR 0x30 /* The PECI client's default address of 0x30 */
> +#define PECI_OFFSET_MAX 8 /* Max numver of CPU clients */
> +
> +/* PCI Access */
> +#define MAX_PCI_READ_LEN 24 /* Number of bytes of the PCI Space read */
PCI? Or PECI? I'm confused now, what does PCI have to do with PECI?
> +
> +#define PCI_BUS0_CPU0 0x00
> +#define PCI_BUS0_CPU1 0x80
> +#define PCI_CPUBUSNO_BUS 0x00
> +#define PCI_CPUBUSNO_DEV 0x08
> +#define PCI_CPUBUSNO_FUNC 0x02
> +#define PCI_CPUBUSNO 0xcc
> +#define PCI_CPUBUSNO_1 0xd0
> +#define PCI_CPUBUSNO_VALID 0xd4
You just abused the PCI core namespace here, are you sure about that?
> +
> +/* Package Identifier Read Parameter Value */
> +#define PKG_ID_CPU_ID 0x0000 /* CPUID Info */
> +#define PKG_ID_PLATFORM_ID 0x0001 /* Platform ID */
> +#define PKG_ID_UNCORE_ID 0x0002 /* Uncore Device ID */
> +#define PKG_ID_MAX_THREAD_ID 0x0003 /* Max Thread ID */
> +#define PKG_ID_MICROCODE_REV 0x0004 /* CPU Microcode Update Revision */
> +#define PKG_ID_MACHINE_CHECK_STATUS 0x0005 /* Machine Check Status */
> +
> +/* RdPkgConfig Index */
> +#define MBX_INDEX_CPU_ID 0 /* Package Identifier Read */
> +#define MBX_INDEX_VR_DEBUG 1 /* VR Debug */
> +#define MBX_INDEX_PKG_TEMP_READ 2 /* Package Temperature Read */
> +#define MBX_INDEX_ENERGY_COUNTER 3 /* Energy counter */
> +#define MBX_INDEX_ENERGY_STATUS 4 /* DDR Energy Status */
> +#define MBX_INDEX_WAKE_MODE_BIT 5 /* "Wake on PECI" Mode bit */
> +#define MBX_INDEX_EPI 6 /* Efficient Performance Indication */
> +#define MBX_INDEX_PKG_RAPL_PERF 8 /* Pkg RAPL Performance Status Read */
> +#define MBX_INDEX_PER_CORE_DTS_TEMP 9 /* Per Core DTS Temperature Read */
> +#define MBX_INDEX_DTS_MARGIN 10 /* DTS thermal margin */
> +#define MBX_INDEX_SKT_PWR_THRTL_DUR 11 /* Socket Power Throttled Duration */
> +#define MBX_INDEX_CFG_TDP_CONTROL 12 /* TDP Config Control */
> +#define MBX_INDEX_CFG_TDP_LEVELS 13 /* TDP Config Levels */
> +#define MBX_INDEX_DDR_DIMM_TEMP 14 /* DDR DIMM Temperature */
> +#define MBX_INDEX_CFG_ICCMAX 15 /* Configurable ICCMAX */
> +#define MBX_INDEX_TEMP_TARGET 16 /* Temperature Target Read */
> +#define MBX_INDEX_CURR_CFG_LIMIT 17 /* Current Config Limit */
> +#define MBX_INDEX_DIMM_TEMP_READ 20 /* Package Thermal Status Read */
> +#define MBX_INDEX_DRAM_IMC_TMP_READ 22 /* DRAM IMC Temperature Read */
> +#define MBX_INDEX_DDR_CH_THERM_STAT 23 /* DDR Channel Thermal Status */
> +#define MBX_INDEX_PKG_POWER_LIMIT1 26 /* Package Power Limit1 */
> +#define MBX_INDEX_PKG_POWER_LIMIT2 27 /* Package Power Limit2 */
> +#define MBX_INDEX_TDP 28 /* Thermal design power minimum */
> +#define MBX_INDEX_TDP_HIGH 29 /* Thermal design power maximum */
> +#define MBX_INDEX_TDP_UNITS 30 /* Units for power/energy registers */
> +#define MBX_INDEX_RUN_TIME 31 /* Accumulated Run Time */
> +#define MBX_INDEX_CONSTRAINED_TIME 32 /* Thermally Constrained Time Read */
> +#define MBX_INDEX_TURBO_RATIO 33 /* Turbo Activation Ratio */
> +#define MBX_INDEX_DDR_RAPL_PL1 34 /* DDR RAPL PL1 */
> +#define MBX_INDEX_DDR_PWR_INFO_HIGH 35 /* DRAM Power Info Read (high) */
> +#define MBX_INDEX_DDR_PWR_INFO_LOW 36 /* DRAM Power Info Read (low) */
> +#define MBX_INDEX_DDR_RAPL_PL2 37 /* DDR RAPL PL2 */
> +#define MBX_INDEX_DDR_RAPL_STATUS 38 /* DDR RAPL Performance Status */
> +#define MBX_INDEX_DDR_HOT_ABSOLUTE 43 /* DDR Hottest Dimm Absolute Temp */
> +#define MBX_INDEX_DDR_HOT_RELATIVE 44 /* DDR Hottest Dimm Relative Temp */
> +#define MBX_INDEX_DDR_THROTTLE_TIME 45 /* DDR Throttle Time */
> +#define MBX_INDEX_DDR_THERM_STATUS 46 /* DDR Thermal Status */
> +#define MBX_INDEX_TIME_AVG_TEMP 47 /* Package time-averaged temperature */
> +#define MBX_INDEX_TURBO_RATIO_LIMIT 49 /* Turbo Ratio Limit Read */
> +#define MBX_INDEX_HWP_AUTO_OOB 53 /* HWP Autonomous Out-of-band */
> +#define MBX_INDEX_DDR_WARM_BUDGET 55 /* DDR Warm Power Budget */
> +#define MBX_INDEX_DDR_HOT_BUDGET 56 /* DDR Hot Power Budget */
> +#define MBX_INDEX_PKG_PSYS_PWR_LIM3 57 /* Package/Psys Power Limit3 */
> +#define MBX_INDEX_PKG_PSYS_PWR_LIM1 58 /* Package/Psys Power Limit1 */
> +#define MBX_INDEX_PKG_PSYS_PWR_LIM2 59 /* Package/Psys Power Limit2 */
> +#define MBX_INDEX_PKG_PSYS_PWR_LIM4 60 /* Package/Psys Power Limit4 */
> +#define MBX_INDEX_PERF_LIMIT_REASON 65 /* Performance Limit Reasons */
> +
> +/* WrPkgConfig Index */
> +#define MBX_INDEX_DIMM_AMBIENT 19
> +#define MBX_INDEX_DIMM_TEMP 24
> +
> +/* Device Specific Completion Code (CC) Definition */
> +#define DEV_PECI_CC_SUCCESS 0x40
> +#define DEV_PECI_CC_TIMEOUT 0x80
> +#define DEV_PECI_CC_OUT_OF_RESOURCE 0x81
> +#define DEV_PECI_CC_UNAVAIL_RESOURCE 0x82
> +#define DEV_PECI_CC_INVALID_REQ 0x90
> +
> +/* Completion Code mask to check retry needs */
> +#define DEV_PECI_CC_RETRY_CHECK_MASK 0xf0
> +#define DEV_PECI_CC_NEED_RETRY 0x80
> +
> +/* Skylake EDS says to retry for 250ms */
> +#define DEV_PECI_RETRY_TIME_MS 250
> +#define DEV_PECI_RETRY_INTERVAL_USEC 10000
> +#define DEV_PECI_RETRY_BIT 0x01
> +
> +#define GET_TEMP_WR_LEN 1
> +#define GET_TEMP_RD_LEN 2
> +#define GET_TEMP_PECI_CMD 0x01
> +
> +#define GET_DIB_WR_LEN 1
> +#define GET_DIB_RD_LEN 8
> +#define GET_DIB_PECI_CMD 0xf7
> +
> +#define RDPKGCFG_WRITE_LEN 5
> +#define RDPKGCFG_READ_LEN_BASE 1
> +#define RDPKGCFG_PECI_CMD 0xa1
> +
> +#define WRPKGCFG_WRITE_LEN_BASE 6
> +#define WRPKGCFG_READ_LEN 1
> +#define WRPKGCFG_PECI_CMD 0xa5
> +
> +#define RDIAMSR_WRITE_LEN 5
> +#define RDIAMSR_READ_LEN 9
> +#define RDIAMSR_PECI_CMD 0xb1
> +
> +#define WRIAMSR_PECI_CMD 0xb5
> +
> +#define RDPCICFG_WRITE_LEN 6
> +#define RDPCICFG_READ_LEN 5
> +#define RDPCICFG_PECI_CMD 0x61
> +
> +#define WRPCICFG_PECI_CMD 0x65
> +
> +#define RDPCICFGLOCAL_WRITE_LEN 5
> +#define RDPCICFGLOCAL_READ_LEN_BASE 1
> +#define RDPCICFGLOCAL_PECI_CMD 0xe1
> +
> +#define WRPCICFGLOCAL_WRITE_LEN_BASE 6
> +#define WRPCICFGLOCAL_READ_LEN 1
> +#define WRPCICFGLOCAL_PECI_CMD 0xe5
Alignment?
> +
> +#define PECI_BUFFER_SIZE 32
Why don't all of these have PECI_ at the front of them?
> +/**
> + * enum peci_cmd - PECI client commands
> + * @PECI_CMD_XFER: raw PECI transfer
> + * @PECI_CMD_PING: ping, a required message for all PECI devices
> + * @PECI_CMD_GET_DIB: get DIB (Device Info Byte)
> + * @PECI_CMD_GET_TEMP: get maximum die temperature
> + * @PECI_CMD_RD_PKG_CFG: read access to the PCS (Package Configuration Space)
> + * @PECI_CMD_WR_PKG_CFG: write access to the PCS (Package Configuration Space)
> + * @PECI_CMD_RD_IA_MSR: read access to MSRs (Model Specific Registers)
> + * @PECI_CMD_WR_IA_MSR: write access to MSRs (Model Specific Registers)
> + * @PECI_CMD_RD_PCI_CFG: sideband read access to the PCI configuration space
> + * maintained in downstream devices external to the processor
> + * @PECI_CMD_WR_PCI_CFG: sideband write access to the PCI configuration space
> + * maintained in downstream devices external to the processor
> + * @PECI_CMD_RD_PCI_CFG_LOCAL: sideband read access to the PCI configuration
> + * space that resides within the processor
> + * @PECI_CMD_WR_PCI_CFG_LOCAL: sideband write access to the PCI configuration
> + * space that resides within the processor
> + *
> + * Available commands depend on client's PECI revision.
> + */
> +enum peci_cmd {
> + PECI_CMD_XFER = 0,
> + PECI_CMD_PING,
> + PECI_CMD_GET_DIB,
> + PECI_CMD_GET_TEMP,
> + PECI_CMD_RD_PKG_CFG,
> + PECI_CMD_WR_PKG_CFG,
> + PECI_CMD_RD_IA_MSR,
> + PECI_CMD_WR_IA_MSR,
> + PECI_CMD_RD_PCI_CFG,
> + PECI_CMD_WR_PCI_CFG,
> + PECI_CMD_RD_PCI_CFG_LOCAL,
> + PECI_CMD_WR_PCI_CFG_LOCAL,
> + PECI_CMD_MAX
> +};
> +
> +/**
> + * struct peci_xfer_msg - raw PECI transfer command
> + * @addr; address of the client
> + * @tx_len: number of data to be written in bytes
> + * @rx_len: number of data to be read in bytes
> + * @tx_buf: data to be written, or NULL
> + * @rx_buf: data to be read, or NULL
> + *
> + * raw PECI transfer
> + */
> +struct peci_xfer_msg {
> + __u8 addr;
> + __u8 tx_len;
> + __u8 rx_len;
> + __u8 tx_buf[PECI_BUFFER_SIZE];
> + __u8 rx_buf[PECI_BUFFER_SIZE];
> +} __attribute__((__packed__));
Go kick the hardware engineer who did not align things on a 32bit
boundry. They should have known better :(
> +
> +/**
> + * struct peci_ping_msg - ping command
> + * @addr: address of the client
> + *
> + * Ping() is a required message for all PECI devices. This message is used to
> + * enumerate devices or determine if a device has been removed, been
> + * powered-off, etc.
> + */
> +struct peci_ping_msg {
> + __u8 addr;
> +} __attribute__((__packed__));
> +
> +/**
> + * struct peci_get_dib_msg - GetDIB command
> + * @addr: address of the client
> + * @dib: DIB data to be read
> + *
> + * The processor PECI client implementation of GetDIB() includes an 8-byte
> + * response and provides information regarding client revision number and the
> + * number of supported domains. All processor PECI clients support the GetDIB()
> + * command.
> + */
> +struct peci_get_dib_msg {
> + __u8 addr;
> + __u64 dib;
> +} __attribute__((__packed__));
Ick, really? That's horrible, again, go kick them hard!
> +/**
> + * struct peci_get_temp_msg - GetTemp command
> + * @addr: address of the client
> + * @temp_raw: raw temperature data to be read
> + *
> + * The GetTemp() command is used to retrieve the maximum die temperature from a
> + * target PECI address. The temperature is used by the external thermal
> + * management system to regulate the temperature on the die. The data is
> + * returned as a negative value representing the number of degrees centigrade
> + * below the maximum processor junction temperature.
> + */
> +struct peci_get_temp_msg {
> + __u8 addr;
> + __s16 temp_raw;
> +} __attribute__((__packed__));
{kick kick kick}
> +
> +/**
> + * struct peci_rd_pkg_cfg_msg - RdPkgConfig command
> + * @addr: address of the client
> + * @index: encoding index for the requested service
> + * @param: specific data being requested
> + * @rx_len: number of data to be read in bytes
> + * @pkg_config: package config data to be read
> + *
> + * The RdPkgConfig() command provides read access to the Package Configuration
> + * Space (PCS) within the processor, including various power and thermal
> + * management functions. Typical PCS read services supported by the processor
> + * may include access to temperature data, energy status, run time information,
> + * DIMM temperatures and so on.
> + */
> +struct peci_rd_pkg_cfg_msg {
> + __u8 addr;
> + __u8 index;
> + __u16 param;
> + __u8 rx_len;
> + __u8 pkg_config[4];
> +} __attribute__((__packed__));
Ok, that's better, someone finally wisened up.
> +
> +/**
> + * struct peci_wr_pkg_cfg_msg - WrPkgConfig command
> + * @addr: address of the client
> + * @index: encoding index for the requested service
> + * @param: specific data being requested
> + * @tx_len: number of data to be written in bytes
> + * @value: package config data to be written
> + *
> + * The WrPkgConfig() command provides write access to the Package Configuration
> + * Space (PCS) within the processor, including various power and thermal
> + * management functions. Typical PCS write services supported by the processor
> + * may include power limiting, thermal averaging constant programming and so on.
> + */
> +struct peci_wr_pkg_cfg_msg {
> + __u8 addr;
> + __u8 index;
> + __u16 param;
> + __u8 tx_len;
> + __u32 value;
> +} __attribute__((__packed__));
No they didn't, ick, no one cares about speed here I guess. Oh well :(
> +
> +/**
> + * struct peci_rd_ia_msr_msg - RdIAMSR command
> + * @addr: address of the client
> + * @thread_id: ID of the specific logical processor
> + * @address: address of MSR to read from
> + * @value: data to be read
> + *
> + * The RdIAMSR() PECI command provides read access to Model Specific Registers
> + * (MSRs) defined in the processor's Intel Architecture (IA).
> + */
> +struct peci_rd_ia_msr_msg {
> + __u8 addr;
> + __u8 thread_id;
> + __u16 address;
> + __u64 value;
> +} __attribute__((__packed__));
They got lucky!
oh well,
greg k-h
More information about the openbmc
mailing list