| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-Id: <71d7fbc0-263e-e6e7-5c88-95a2288ddd79@linux.vnet.ibm.com>
Date: Fri, 10 Feb 2017 15:02:08 -0600
From: Eddie James <eajames@...ux.vnet.ibm.com>
To: Joel Stanley <joel@....id.au>
Cc: Guenter Roeck <linux@...ck-us.net>, devicetree@...r.kernel.org,
linux-kernel@...r.kernel.org, linux-hwmon@...r.kernel.org,
linux-doc@...r.kernel.org, jdelvare@...e.com, corbet@....net,
Mark Rutland <mark.rutland@....com>,
Rob Herring <robh+dt@...nel.org>,
Wolfram Sang <wsa@...-dreams.de>,
Andrew Jeffery <andrew@...id.au>,
Benjamin Herrenschmidt <benh@...nel.crashing.org>,
"Edward A. James" <eajames@...ibm.com>
Subject: Re: [PATCH linux v7 1/6] hwmon: Add core On-Chip Controller support
for POWER CPUs
On 02/09/2017 11:31 PM, Joel Stanley wrote:
> On Wed, Feb 8, 2017 at 9:40 AM, <eajames@...ux.vnet.ibm.com> wrote:
>
>> diff --git a/Documentation/hwmon/occ b/Documentation/hwmon/occ
>> new file mode 100644
>> index 0000000..79d1642
>> --- /dev/null
>> +++ b/Documentation/hwmon/occ
> The kernel is using reStructuredText these days. You should consider
> following suit:
>
> https://www.kernel.org/doc/html/latest/doc-guide/sphinx.html#writing-documentation
>
>
>> @@ -0,0 +1,40 @@
>> +Kernel driver occ
>> +=================
>> +
>> +Supported chips:
>> + * ASPEED AST2400
>> + * ASPEED AST2500
> Not really - this will run on any chip that's connected to a P8 or P9.
>
>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 5f10c28..193a13b 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -9128,6 +9128,13 @@ T: git git://linuxtv.org/media_tree.git
>> S: Maintained
>> F: drivers/media/i2c/ov7670.c
>>
>> +ON-CHIP CONTROLLER HWMON DRIVER
> Mention P8 and P9?
>
>> +M: Eddie James <eajames@...ibm.com>
>> +L: linux-hwmon@...r.kernel.org
> Have you subscribed to this list? Would you prefer the mail to come to
> the openbmc list?
>
>> +S: Maintained
>> +F: Documentation/hwmon/occ
>> +F: drivers/hwmon/occ/
>> +
>> ONENAND FLASH DRIVER
>> M: Kyungmin Park <kyungmin.park@...sung.com>
>> L: linux-mtd@...ts.infradead.org
>> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
>> index 190d270..e80ca81 100644
>> --- a/drivers/hwmon/Kconfig
>> +++ b/drivers/hwmon/Kconfig
>> @@ -1240,6 +1240,8 @@ config SENSORS_NSA320
>> This driver can also be built as a module. If so, the module
>> will be called nsa320-hwmon.
>>
>> +source drivers/hwmon/occ/Kconfig
>> +
>> config SENSORS_PCF8591
>> tristate "Philips PCF8591 ADC/DAC"
>> depends on I2C
>> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
>> index d2cb7e8..c7ec5d4 100644
>> --- a/drivers/hwmon/Makefile
>> +++ b/drivers/hwmon/Makefile
>> @@ -169,6 +169,7 @@ obj-$(CONFIG_SENSORS_WM831X) += wm831x-hwmon.o
>> obj-$(CONFIG_SENSORS_WM8350) += wm8350-hwmon.o
>> obj-$(CONFIG_SENSORS_XGENE) += xgene-hwmon.o
>>
>> +obj-$(CONFIG_SENSORS_PPC_OCC) += occ/
>> obj-$(CONFIG_PMBUS) += pmbus/
>>
>> ccflags-$(CONFIG_HWMON_DEBUG_CHIP) := -DDEBUG
>
>> diff --git a/drivers/hwmon/occ/occ.c b/drivers/hwmon/occ/occ.c
>> new file mode 100644
>> index 0000000..af077f2
>> --- /dev/null
>> +++ b/drivers/hwmon/occ/occ.c
>> + sensors = &resp->data.blocks[b].sensors;
>> + if (!sensors) {
>> + /* first poll response */
>> + sensors = driver->ops.alloc_sensor(dev, sensor_type,
>> + block->num_sensors);
>> + if (!sensors)
>> + return -ENOMEM;
>> +
>> + resp->data.blocks[b].sensors = sensors;
>> + resp->data.sensor_block_id[sensor_type] = b;
>> + resp->data.blocks[b].header = *block;
>> + } else if (block->sensor_length !=
>> + resp->data.blocks[b].header.sensor_length) {
>> + dev_warn(dev,
>> + "different sensor length than first poll\n");
> The driver has changed behaviour from your previous version; now it
> discards data if the sensors change.
>
> Under what circumstances does the sensor data change?
Yes. The sensor data shouldn't change, as far as I know. I think
something would be wrong if it did.
>
>> + continue;
>> + }
>> +
>> + for (s = 0; s < block->num_sensors &&
>> + s < resp->data.blocks[b].header.num_sensors; s++) {
>> + if (offset + block->sensor_length > num_bytes) {
>> + dev_warn(dev, "exceeded data length\n");
>> + return 0;
>> + }
>> +
>> + driver->ops.parse_sensor(data, sensors, sensor_type,
>> + offset, s);
>> + offset += block->sensor_length;
>> + }
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static u8 occ_send_cmd(struct occ *driver, u8 seq, u8 type, u16 length,
>> + const u8 *data, u8 *resp)
>> +{
>> + u32 cmd1, cmd2 = 0;
>> + u16 checksum = 0;
>> + bool retry = false;
>> + int i, rc, tries = 0;
>> +
>> + cmd1 = (seq << 24) | (type << 16) | length;
>> + memcpy(&cmd2, data, length);
>> + cmd2 <<= ((4 - length) * 8);
>> +
>> + /* checksum: sum of every bytes of cmd1, cmd2 */
>> + for (i = 0; i < 4; i++) {
>> + checksum += (cmd1 >> (i * 8)) & 0xFF;
>> + checksum += (cmd2 >> (i * 8)) & 0xFF;
>> + }
>> +
>> + cmd2 |= checksum << ((2 - length) * 8);
>> +
>> + /* Init OCB */
> You should mention what OCB means in your documentation.
>
>> + rc = driver->bus_ops.putscom(driver->bus, OCB_STATUS_CONTROL_OR,
>> + OCB_OR_INIT0, OCB_OR_INIT1);
>> + if (rc)
>> + goto err;
>> +
>> +int occ_set_user_powercap(struct occ *occ, u16 cap)
>> +{
>> + u8 resp[8];
>> +
>> + cap = cpu_to_be16(cap);
>> +
>> + return occ_send_cmd(occ, 0, OCC_SET_USER_POWR_CAP, 2, (const u8 *)&cap,
>> + resp);
>> +}
>> +EXPORT_SYMBOL(occ_set_user_powercap);
>> +
>> +struct occ *occ_start(struct device *dev, void *bus,
> From what I can tell this doesn't start anything. Call it occ_init()
> or something.
>
>> + struct occ_bus_ops *bus_ops, const struct occ_ops *ops,
>> + const struct occ_config *config)
> Create a structure with all of these details in it. Some of them don't
> need to be broken out into their own, for instance:
>
> struct occ *occ_start(struct device *dev, const struct occ_init_context *init)
> {
>
> driver->dev = dev;
> driver->bus = init->bus;
> driver->bus_read = init->bus_read;
> driver->bus_write = init->bus_write;
> driver->config = init->config;
> driver->cmd_addr = init->cmd_addr;
>
> etc.
>
>
>
>> +{
>> + struct occ *driver = devm_kzalloc(dev, sizeof(struct occ), GFP_KERNEL);
>> +
>> + if (!driver)
>> + return ERR_PTR(-ENOMEM);
>> +
>> + driver->dev = dev;
>> + driver->bus = bus;
>> + driver->bus_ops = *bus_ops;
>> + driver->ops = *ops;
>> + driver->config = *config;
>> + driver->raw_data = devm_kzalloc(dev, OCC_DATA_MAX, GFP_KERNEL);
>> + if (!driver->raw_data)
>> + return ERR_PTR(-ENOMEM);
>> +
>> + mutex_init(&driver->update_lock);
>> +
>> + return driver;
>> +}
>> +EXPORT_SYMBOL(occ_start);
>> +
>> +MODULE_AUTHOR("Eddie James <eajames@...ibm.com>");
>> +MODULE_DESCRIPTION("OCC hwmon core driver");
>> +MODULE_LICENSE("GPL");
>> diff --git a/drivers/hwmon/occ/scom.h b/drivers/hwmon/occ/scom.h
>> new file mode 100644
>> index 0000000..b0691f3
>> --- /dev/null
>> +++ b/drivers/hwmon/occ/scom.h
>> @@ -0,0 +1,47 @@
>> +/*
>> + * scom.h - hwmon OCC driver
>> + *
>> + * This file contains data structures for scom operations to the OCC
> Are these really SCOM operations?
>
> I think they're better described read and write callbacks, as the
> operation is may be talking i2c or FSI or in the future some other
> kind of access.
>
> They do take scom addresses as parameters, so I can see the argument
> for calling them getscom/putscom.
These are scom operations. The only way to talk to the OCC is with a
scom operation as I understand it. The transport mechanism can change
(i2c or fsi/sbe), but it's still a "scom."
>
>
>> + *
>> + * Copyright 2016 IBM Corp.
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License as published by
>> + * the Free Software Foundation; either version 2 of the License, or
>> + * (at your option) any later version.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>> + * GNU General Public License for more details.
>> + */
>> +
>> +#ifndef __SCOM_H__
>> +#define __SCOM_H__
>> +
>> +/*
>> + * occ_bus_ops - represent the low-level transfer methods to communicate with
>> + * the OCC.
>> + *
>> + * getscom - OCC scom read
>> + * @bus: handle to slave device
>> + * @address: address
>> + * @data: where to store data read from slave; buffer size must be at least
>> + * eight bytes.
> Are there situations where it's more than 8 bytes?
>
> Would it be safer to add a length argument so the read call so we
> don't put more data in the buffer than the caller expects?
I guess the documentation isn't that clear, but it's always an 8 byte
operation. I said "at least" 8 bytes because the calling code calls this
function repeatedly with one large buffer at different offsets. I'll add
a comment that it's always 8 bytes.
>
>
>> + *
>> + * Returns 0 on success or a negative errno on error
>> + *
>> + * putscom - OCC scom write
>> + * @bus: handle to slave device
>> + * @address: address
>> + * @data0: first data word to write
>> + * @data1: second data word to write
>> + *
>> + * Returns 0 on success or a negative errno on error
>> + */
>> +struct occ_bus_ops {
>> + int (*getscom)(void *bus, u32 address, u64 *data);
>> + int (*putscom)(void *bus, u32 address, u32 data0, u32 data1);
>> +};
>> +
>> +#endif /* __SCOM_H__ */
Powered by blists - more mailing lists