| 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: <070d5b8b-74cf-aa79-7faa-e6917a00243c@roeck-us.net>
Date: Tue, 11 Jul 2017 20:20:01 -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/11/2017 06:20 PM, Andrew Jeffery wrote:
> On Tue, 2017-07-11 at 06:31 -0700, Guenter Roeck wrote:
>> 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 ?
>
> I considered this up front however I wasn't sure where the PMBus sensor
> classes were modelled from. The PMBus spec generally doesn't discuss
The classes are modeled from my brain, so we can do whatever we want with them.
> PMW beyond the concept of fans, and given PSC_FAN already existed I had
> a vague feeling that introducing PSC_PWM might not be the way to go. It
> also means that PSC_FAN is implicitly RPM in some circumstances, or
> both RPM and PWM in others, and wasn't previously contrasted against
> PWM as PWM-specific configuration wasn't an option.
>
> However if it's reasonable it should lead to a more straight forward
> patch. I'll rework and resend if it falls out nicely.
>
Please do.
Thanks,
Guenter
> Thanks,
>
> Andrew
>
>>
>>>>> 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