| 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: <a7448db8-6df1-38ee-a66d-420d8e429f4e@roeck-us.net>
Date: Tue, 11 Jul 2017 06:31:29 -0700
From: Guenter Roeck <linux@...ck-us.net>
To: Andrew Jeffery <andrew@...id.au>, linux-hwmon@...r.kernel.org
Cc: jdelvare@...e.com, linux-kernel@...r.kernel.org, joel@....id.au,
openbmc@...ts.ozlabs.org, msbarth@...ux.vnet.ibm.com,
mspinler@...ux.vnet.ibm.com
Subject: Re: [RFC PATCH 3/4] pmbus: Allow dynamic fan coefficient values
On 07/10/2017 06:56 AM, Andrew Jeffery wrote:
> Some PMBus chips, such as the MAX31785, use different coefficients for
> FAN_COMMAND_[1-4] depending on whether the fan is in PWM (percent duty)
> or RPM mode. Add a callback to allow the driver to provide the
> applicable coefficients to avoid imposing on devices that don't have
> this requirement.
>
Why not just introduce another class, such as PSC_PWM ?
> Signed-off-by: Andrew Jeffery <andrew@...id.au>
> ---
> drivers/hwmon/pmbus/pmbus.h | 18 +++++--
> drivers/hwmon/pmbus/pmbus_core.c | 112 ++++++++++++++++++++++++++++++++-------
> 2 files changed, 108 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/hwmon/pmbus/pmbus.h b/drivers/hwmon/pmbus/pmbus.h
> index 927eabc1b273..338ecc8b25a4 100644
> --- a/drivers/hwmon/pmbus/pmbus.h
> +++ b/drivers/hwmon/pmbus/pmbus.h
> @@ -345,6 +345,12 @@ enum pmbus_sensor_classes {
> enum pmbus_data_format { linear = 0, direct, vid };
> enum vrm_version { vr11 = 0, vr12 };
>
> +struct pmbus_coeffs {
> + int m; /* mantissa for direct data format */
> + int b; /* offset */
> + int R; /* exponent */
> +};
> +
> struct pmbus_driver_info {
> int pages; /* Total number of pages */
> enum pmbus_data_format format[PSC_NUM_CLASSES];
> @@ -353,9 +359,7 @@ struct pmbus_driver_info {
> * Support one set of coefficients for each sensor type
> * Used for chips providing data in direct mode.
> */
> - int m[PSC_NUM_CLASSES]; /* mantissa for direct data format */
> - int b[PSC_NUM_CLASSES]; /* offset */
> - int R[PSC_NUM_CLASSES]; /* exponent */
> + struct pmbus_coeffs coeffs[PSC_NUM_CLASSES];
>
> u32 func[PMBUS_PAGES]; /* Functionality, per page */
> /*
> @@ -382,6 +386,14 @@ struct pmbus_driver_info {
> int (*identify)(struct i2c_client *client,
> struct pmbus_driver_info *info);
>
> + /*
> + * If a fan's coefficents change over time (e.g. between RPM and PWM
> + * mode), then the driver can provide a function for retrieving the
> + * currently applicable coefficients.
> + */
> + const struct pmbus_coeffs *(*get_fan_coeffs)(
> + const struct pmbus_driver_info *info, int index,
> + enum pmbus_fan_mode mode, int command);
> /* Allow the driver to interpret the fan command value */
> int (*get_pwm_mode)(int id, u8 fan_config, u16 fan_command);
> int (*set_pwm_mode)(int id, long mode, u8 *fan_config,
> diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
> index 3b0a55bbbd2c..4ff6a1fd5cce 100644
> --- a/drivers/hwmon/pmbus/pmbus_core.c
> +++ b/drivers/hwmon/pmbus/pmbus_core.c
> @@ -58,10 +58,11 @@
> struct pmbus_sensor {
> struct pmbus_sensor *next;
> char name[PMBUS_NAME_SIZE]; /* sysfs sensor name */
> - struct device_attribute attribute;
> + struct sensor_device_attribute attribute;
> u8 page; /* page number */
> u16 reg; /* register */
> enum pmbus_sensor_classes class; /* sensor class */
> + const struct pmbus_coeffs *coeffs;
> bool update; /* runtime sensor update needed */
> int data; /* Sensor data.
> Negative if there was a read error */
> @@ -89,6 +90,7 @@ struct pmbus_fan_ctrl {
> u8 id;
> u8 config;
> u16 command;
> + const struct pmbus_coeffs *coeffs;
> };
> #define to_pmbus_fan_ctrl_attr(_attr) \
> container_of(_attr, struct pmbus_fan_ctrl_attr, attribute)
> @@ -511,9 +513,15 @@ static long pmbus_reg2data_direct(struct pmbus_data *data,
> long val = (s16) sensor->data;
> long m, b, R;
>
> - m = data->info->m[sensor->class];
> - b = data->info->b[sensor->class];
> - R = data->info->R[sensor->class];
> + if (sensor->coeffs) {
> + m = sensor->coeffs->m;
> + b = sensor->coeffs->b;
> + R = sensor->coeffs->R;
> + } else {
> + m = data->info->coeffs[sensor->class].m;
> + b = data->info->coeffs[sensor->class].b;
> + R = data->info->coeffs[sensor->class].R;
> + }
>
> if (m == 0)
> return 0;
> @@ -663,9 +671,15 @@ static u16 pmbus_data2reg_direct(struct pmbus_data *data,
> {
> long m, b, R;
>
> - m = data->info->m[sensor->class];
> - b = data->info->b[sensor->class];
> - R = data->info->R[sensor->class];
> + if (sensor->coeffs) {
> + m = sensor->coeffs->m;
> + b = sensor->coeffs->b;
> + R = sensor->coeffs->R;
> + } else {
> + m = data->info->coeffs[sensor->class].m;
> + b = data->info->coeffs[sensor->class].b;
> + R = data->info->coeffs[sensor->class].R;
> + }
>
> /* Power is in uW. Adjust R and b. */
> if (sensor->class == PSC_POWER) {
> @@ -796,7 +810,9 @@ static ssize_t pmbus_show_sensor(struct device *dev,
> struct device_attribute *devattr, char *buf)
> {
> struct pmbus_data *data = pmbus_update_device(dev);
> - struct pmbus_sensor *sensor = to_pmbus_sensor(devattr);
> + struct pmbus_sensor *sensor;
> +
> + sensor = to_pmbus_sensor(to_sensor_dev_attr(devattr));
>
> if (sensor->data < 0)
> return sensor->data;
> @@ -810,12 +826,14 @@ static ssize_t pmbus_set_sensor(struct device *dev,
> {
> struct i2c_client *client = to_i2c_client(dev->parent);
> struct pmbus_data *data = i2c_get_clientdata(client);
> - struct pmbus_sensor *sensor = to_pmbus_sensor(devattr);
> + struct pmbus_sensor *sensor;
> ssize_t rv = count;
> long val = 0;
> int ret;
> u16 regval;
>
> + sensor = to_pmbus_sensor(to_sensor_dev_attr(devattr));
> +
> if (kstrtol(buf, 10, &val) < 0)
> return -EINVAL;
>
> @@ -856,6 +874,7 @@ static ssize_t pmbus_show_fan_command(struct device *dev,
> }
>
> sensor.class = PSC_FAN;
> + sensor.coeffs = fan->coeffs;
> if (mode == percent)
> sensor.data = fan->command * 255 / 100;
> else
> @@ -882,6 +901,29 @@ static ssize_t pmbus_show_pwm(struct device *dev,
> buf);
> }
>
> +static int pmbus_update_fan_coeffs(struct pmbus_data *data,
> + struct pmbus_fan_ctrl *fan,
> + enum pmbus_fan_mode mode)
> +{
> + const struct pmbus_driver_info *info = data->info;
> + struct pmbus_sensor *curr = data->sensors;
> +
> + fan->coeffs = info->get_fan_coeffs(info, fan->index, mode,
> + PMBUS_FAN_COMMAND_1 + fan->id);
> +
> + while (curr) {
> + if (curr->class == PSC_FAN &&
> + curr->attribute.index == fan->index) {
> + curr->coeffs = info->get_fan_coeffs(info, fan->index,
> + mode, curr->reg);
> + }
> +
> + curr = curr->next;
> + }
> +
> + return 0;
> +}
> +
> static ssize_t pmbus_set_fan_command(struct device *dev,
> enum pmbus_fan_mode mode,
> struct pmbus_fan_ctrl *fan,
> @@ -889,6 +931,7 @@ static ssize_t pmbus_set_fan_command(struct device *dev,
> {
> struct i2c_client *client = to_i2c_client(dev->parent);
> struct pmbus_data *data = i2c_get_clientdata(client);
> + const struct pmbus_driver_info *info = data->info;
> int config_addr, command_addr;
> struct pmbus_sensor sensor;
> ssize_t rv;
> @@ -899,7 +942,13 @@ static ssize_t pmbus_set_fan_command(struct device *dev,
>
> mutex_lock(&data->update_lock);
>
> + if (info->format[PSC_FAN] == direct && info->get_fan_coeffs) {
> + pmbus_update_fan_coeffs(data, fan, mode);
> + sensor.coeffs = fan->coeffs;
> + }
> +
> sensor.class = PSC_FAN;
> + sensor.attribute.index = fan->index;
>
> val = pmbus_data2reg(data, &sensor, val);
>
> @@ -968,6 +1017,8 @@ static ssize_t pmbus_show_pwm_enable(struct device *dev,
> struct pmbus_sensor sensor = {
> .class = PSC_FAN,
> .data = fan->command,
> + .attribute.index = fan->index,
> + .coeffs = fan->coeffs,
> };
> long command;
>
> @@ -992,6 +1043,7 @@ static ssize_t pmbus_set_pwm_enable(struct device *dev,
> struct pmbus_fan_ctrl *fan = pwm_enable_to_pmbus_fan_ctrl(da);
> struct i2c_client *client = to_i2c_client(dev->parent);
> struct pmbus_data *data = i2c_get_clientdata(client);
> + const struct pmbus_driver_info *info = data->info;
> int config_addr, command_addr;
> struct pmbus_sensor sensor;
> ssize_t rv = count;
> @@ -1003,15 +1055,21 @@ static ssize_t pmbus_set_pwm_enable(struct device *dev,
> mutex_lock(&data->update_lock);
>
> sensor.class = PSC_FAN;
> + sensor.attribute.index = fan->index;
> +
> + if (info->format[PSC_FAN] == direct && info->get_fan_coeffs) {
> + pmbus_update_fan_coeffs(data, fan, percent);
> + sensor.coeffs = fan->coeffs;
> + }
>
> config_addr = (fan->id < 2) ? PMBUS_FAN_CONFIG_12 : PMBUS_FAN_CONFIG_34;
> command_addr = config_addr + 1 + (fan->id & 1);
>
> - if (data->info->set_pwm_mode) {
> + if (info->set_pwm_mode) {
> u8 config = PB_FAN_CONFIG_PUT(fan->id, fan->config);
> u16 command = fan->command;
>
> - rv = data->info->set_pwm_mode(fan->id, mode, &config, &command);
> + rv = info->set_pwm_mode(fan->id, mode, &config, &command);
> if (rv < 0)
> goto done;
>
> @@ -1138,7 +1196,7 @@ static struct pmbus_sensor *pmbus_add_sensor(struct pmbus_data *data,
> sensor = devm_kzalloc(data->dev, sizeof(*sensor), GFP_KERNEL);
> if (!sensor)
> return NULL;
> - a = &sensor->attribute;
> + a = &sensor->attribute.dev_attr;
>
> snprintf(sensor->name, sizeof(sensor->name), "%s%d_%s",
> name, seq, type);
> @@ -1146,9 +1204,9 @@ static struct pmbus_sensor *pmbus_add_sensor(struct pmbus_data *data,
> sensor->reg = reg;
> sensor->class = class;
> sensor->update = update;
> - pmbus_dev_attr_init(a, sensor->name,
> + pmbus_attr_init(&sensor->attribute, sensor->name,
> readonly ? S_IRUGO : S_IRUGO | S_IWUSR,
> - pmbus_show_sensor, pmbus_set_sensor);
> + pmbus_show_sensor, pmbus_set_sensor, seq);
>
> if (pmbus_add_attribute(data, &a->attr))
> return NULL;
> @@ -1886,7 +1944,7 @@ static const u32 pmbus_fan_status_flags[] = {
> /* Fans */
> static int pmbus_add_fan_ctrl(struct i2c_client *client,
> struct pmbus_data *data, int index, int page, int id,
> - u8 config)
> + u8 config, const struct pmbus_coeffs *coeffs)
> {
> struct pmbus_fan_ctrl *fan;
> int rv;
> @@ -1898,6 +1956,7 @@ static int pmbus_add_fan_ctrl(struct i2c_client *client,
> fan->index = index;
> fan->page = page;
> fan->id = id;
> + fan->coeffs = coeffs;
> fan->config = config;
>
> rv = _pmbus_read_word_data(client, page,
> @@ -1921,6 +1980,8 @@ static int pmbus_add_fan_attributes(struct i2c_client *client,
> struct pmbus_data *data)
> {
> const struct pmbus_driver_info *info = data->info;
> + const struct pmbus_coeffs *coeffs = NULL;
> + enum pmbus_fan_mode mode;
> int index = 1;
> int page;
> int ret;
> @@ -1929,6 +1990,7 @@ static int pmbus_add_fan_attributes(struct i2c_client *client,
> int f;
>
> for (f = 0; f < ARRAY_SIZE(pmbus_fan_registers); f++) {
> + struct pmbus_sensor *sensor;
> int regval;
>
> if (!(info->func[page] & pmbus_fan_flags[f]))
> @@ -1949,13 +2011,25 @@ static int pmbus_add_fan_attributes(struct i2c_client *client,
> (!(regval & (PB_FAN_1_INSTALLED >> ((f & 1) * 4)))))
> continue;
>
> - if (pmbus_add_sensor(data, "fan", "input", index,
> - page, pmbus_fan_registers[f],
> - PSC_FAN, true, true) == NULL)
> + sensor = pmbus_add_sensor(data, "fan", "input", index,
> + page, pmbus_fan_registers[f],
> + PSC_FAN, true, true);
> + if (!sensor)
> return -ENOMEM;
>
> + /* Add coeffs here as we have access to the fan mode */
> + if (info->format[PSC_FAN] == direct &&
> + info->get_fan_coeffs) {
> + const u16 mask = PB_FAN_1_RPM >> ((f & 1) * 4);
> +
> + mode = (regval & mask) ? rpm : percent;
> + coeffs = info->get_fan_coeffs(info, index, mode,
> + pmbus_fan_registers[f]);
> + sensor->coeffs = coeffs;
> + }
> +
> ret = pmbus_add_fan_ctrl(client, data, index, page, f,
> - regval);
> + regval, coeffs);
> if (ret < 0)
> return ret;
>
>
Powered by blists - more mailing lists