| 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: <b85a7486-578f-42c9-9ec2-b5befa75d893@bootlin.com>
Date: Wed, 6 Nov 2024 14:46:58 +0100
From: Thomas Richard <thomas.richard@...tlin.com>
To: Guenter Roeck <linux@...ck-us.net>, Jean Delvare <jdelvare@...e.com>,
Lee Jones <lee@...nel.org>
Cc: linux-kernel@...r.kernel.org, linux-hwmon@...r.kernel.org,
thomas.petazzoni@...tlin.com, blake.vermeer@...sight.com
Subject: Re: [PATCH 1/2] hwmon: Add Congatec Board Controller monitoring
driver
Hello Guenter,
Thanks for the review !!
>
> For the first loop, this essentially repeats the cgbc_hwmon_cmd() from
> above.
> Is that how it works, i.e., that index == 0 returns both the number of
> sensors
> in the first byte of return data plus the data for the first sensor ?
Yes it is.
This is based on the Congatec implementation.
First time I read the their code, I had the same reaction than you.
But it works.
Please note that the driver provided by Congatec is the only source of
information I have (there is no technical documentation which describes
the internal behavior of this Board Controller).
I'll skip the cgbc_hwmon_cmd() for the first loop and add a comment to
not forget that it is not an error.
>
>> + if (ret)
>> + return ret;
>> +
>> + type = FIELD_GET(CGBC_HWMON_TYPE_MASK, data[1]);
>> + id = FIELD_GET(CGBC_HWMON_ID_MASK, data[1]) - 1;
>> +
>> + if (type == CGBC_HWMON_TYPE_TEMP && id <
>> ARRAY_SIZE(cgbc_hwmon_labels_temp)) {
>> + sensor->type = hwmon_temp;
>> + sensor->label = cgbc_hwmon_labels_temp[id];
>> + } else if (type == CGBC_HWMON_TYPE_IN && id <
>> ARRAY_SIZE(cgbc_hwmon_labels_in)) {
>> + /*
>> + * The Board Controller doesn't do differences between
>> current and voltage
>
> doesn't differentiate
>
>> + * sensors
>> + */
>
> This doesn't really explain what is happening. Please add something like
> "Get the sensor type from cgbc_hwmon_labels_in[id].label instead".
>
>> + sensor->type = cgbc_hwmon_labels_in[id].type;
>> + sensor->label = cgbc_hwmon_labels_in[id].label;
>> + } else if (type == CGBC_HWMON_TYPE_FAN && id <
>> ARRAY_SIZE(cgbc_hwmon_labels_fan)) {
>> + sensor->type = hwmon_fan;
>> + sensor->label = cgbc_hwmon_labels_fan[id];
>> + } else {
>> + dev_warn(dev, "Board Controller returned an unknown
>> sensor (type=%d, id=%d), ignore it",
>> + type, id);
>> + continue;
>> + }
>> +
>> + sensor->active = FIELD_GET(CGBC_HWMON_ACTIVE_BIT, data[1]);
>> + sensor->index = i;
>> + sensor++;
>> + hwmon->nb_sensors++;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static struct cgbc_hwmon_sensor *cgbc_hwmon_find_sensor(struct
>> cgbc_hwmon_data *hwmon,
>> + enum hwmon_sensor_types type, int channel)
>> +{
>> + struct cgbc_hwmon_sensor *sensor = NULL;
>> + int i, cnt = 0;
>> +
>> + for (i = 0; i < hwmon->nb_sensors; i++) {
>> + if (hwmon->sensors[i].type == type && cnt++ == channel) {
>
> Isn't that a bit fragile ? It assumes that the nth reported sensor of a
> given type
> reflects a specific named sensor. If that is indeed the case, why bother
> with
> all the code in cgbc_hwmon_probe_sensors() ? The index to sensor
> association
> should be well defined, and the sensor type plus the channel index
> should always
> be a constant.
I'm not sure to get your comment.
I cannot assume that the sensor list returned by the Board Controller
will be the same for all boards.
I know the MFD driver only supports one board, but I think it's better
if we can have a generic hwmon driver.
If I add some debug in cgbc_hwmon_probe_sensors() I can dump the sensor
list returned by the Board Controller:
cgbc_hwmon_probe_sensors: index=0 type=1 id=5 label=Chipset Temperature
cgbc_hwmon_probe_sensors: index=1 type=7 id=0 label=CPU Fan
cgbc_hwmon_probe_sensors: index=4 type=1 id=3 label=Board Temperature
cgbc_hwmon_probe_sensors: index=5 type=2 id=1 label=DC Runtime Voltage
It is the type and the id which give the name of the sensor.
I don't see how to do it in a different way if I cannot assume that the
list above is not the same for all boards.
If I assume that the list returned by the Board Controller is always the
same for a board (which I not even sure, if for example a fan is
plugged), I could have a static list for each different boards. But the
driver will not be generic.
If I miss something, please let me know.
>
>> + sensor = &hwmon->sensors[i];
>> + break;
>> + }
>> + }
>> +
>> + return sensor;
>> +}
>> +
>> +static int cgbc_hwmon_read(struct device *dev, enum
>> hwmon_sensor_types type, u32 attr, int channel,
>> + long *val)
>> +{
>> + struct cgbc_hwmon_data *hwmon = dev_get_drvdata(dev);
>> + struct cgbc_hwmon_sensor *sensor = cgbc_hwmon_find_sensor(hwmon,
>> type, channel);
>> + struct cgbc_device_data *cgbc = hwmon->cgbc;
>> + u8 data[CGBC_HWMON_CMD_SENSOR_DATA_SIZE];
>> + int ret;
>> +
>> + if (!sensor)
>> + return -ENODEV;
>
> How would this ever happen ? Unless I am missing something, that means
> there is a bug somewhere in the code. "No such device" is definitely
> wrong here (and elsewhere). If you don't trust your code and think
> this can happen, at least return -ENODATA.
I can remove this return -ENODEV (and also the one in read_string()).
The read() and read_string() callbacks can only be called if the sensor
is created in sysfs.
And the hwmon core creates the sysfs entry only if is_visible() does not
return an error.
The function cgbc_hwmon_find_sensor() is called in is_visible() and the
returned pointer is checked.
So if read() or read_string() is called, we know that is_visible()
didn't return an error, so cgbc_hwmon_find_sensor() didn't return an error.
So this "return -ENODEV" (in read() and read_string()) is dead code.
>
>> +
>> + ret = cgbc_hwmon_cmd(cgbc, sensor->index, &data[0]);
>> + if (ret)
>> + return ret;
>> +
>> + *val = FIELD_PREP(CGBC_HWMON_DATA_HIGH, data[3]) |
>> FIELD_PREP(CGBC_HWMON_DATA_LOW, data[2]);
>> +
>
> That is a pretty complex way of writing
> *val = (data[3] << 8) | data[2];
> I'd say that is close to obfuscation, but that is your call.
I agree, it's easier to read.
>
>> + /* For the Board Controller 1lsb = 0.1 degree centigrade */
>
> All other units are as expected (mV, mA, rpm) ?
Yes they are, I will mention it.
>
>> + if (sensor->type == hwmon_temp)
>> + *val *= 100;
>> +
>> + return 0;
>> +}
>> +
>> +static umode_t cgbc_hwmon_is_visible(const void *_data, enum
>> hwmon_sensor_types type, u32 attr,
>> + int channel)
>> +{
>> + struct cgbc_hwmon_data *data = (struct cgbc_hwmon_data *)_data;
>> + struct cgbc_hwmon_sensor *sensor;
>> +
>> + sensor = cgbc_hwmon_find_sensor(data, type, channel);
>> + if (!sensor)
>> + return 0;
>> +
>> + return sensor->active ? 0444 : 0;
>> +}
>> +
>> +static int cgbc_hwmon_read_string(struct device *dev, enum
>> hwmon_sensor_types type, u32 attr,
>> + int channel, const char **str)
>> +{
>> + struct cgbc_hwmon_data *hwmon = dev_get_drvdata(dev);
>> + struct cgbc_hwmon_sensor *sensor = cgbc_hwmon_find_sensor(hwmon,
>> type, channel);
>> +
>> + if (!sensor)
>> + return -ENODEV;
>> +
>> + *str = sensor->label;
>> +
>> + return 0;
>> +}
>> +
>> +static const struct hwmon_channel_info * const cgbc_hwmon_info[] = {
>> + HWMON_CHANNEL_INFO(temp,
>> + HWMON_T_INPUT | HWMON_T_LABEL, HWMON_T_INPUT |
>> HWMON_T_LABEL,
>> + HWMON_T_INPUT | HWMON_T_LABEL, HWMON_T_INPUT |
>> HWMON_T_LABEL,
>> + HWMON_T_INPUT | HWMON_T_LABEL, HWMON_T_INPUT |
>> HWMON_T_LABEL,
>> + HWMON_T_INPUT | HWMON_T_LABEL, HWMON_T_INPUT |
>> HWMON_T_LABEL,
>> + HWMON_T_INPUT | HWMON_T_LABEL, HWMON_T_INPUT |
>> HWMON_T_LABEL),
>> + HWMON_CHANNEL_INFO(in,
>> + HWMON_I_INPUT | HWMON_I_LABEL, HWMON_I_INPUT |
>> HWMON_I_LABEL,
>> + HWMON_I_INPUT | HWMON_I_LABEL, HWMON_I_INPUT |
>> HWMON_I_LABEL,
>> + HWMON_I_INPUT | HWMON_I_LABEL, HWMON_I_INPUT |
>> HWMON_I_LABEL,
>> + HWMON_I_INPUT | HWMON_I_LABEL, HWMON_I_INPUT |
>> HWMON_I_LABEL,
>> + HWMON_I_INPUT | HWMON_I_LABEL, HWMON_I_INPUT |
>> HWMON_I_LABEL,
>> + HWMON_I_INPUT | HWMON_I_LABEL, HWMON_I_INPUT |
>> HWMON_I_LABEL,
>> + HWMON_I_INPUT | HWMON_I_LABEL, HWMON_I_INPUT |
>> HWMON_I_LABEL),
>> + HWMON_CHANNEL_INFO(curr,
>> + HWMON_C_INPUT | HWMON_C_LABEL, HWMON_C_INPUT |
>> HWMON_C_LABEL,
>> + HWMON_C_INPUT | HWMON_C_LABEL),
>> + HWMON_CHANNEL_INFO(fan,
>> + HWMON_F_INPUT | HWMON_F_LABEL, HWMON_F_INPUT |
>> HWMON_F_LABEL,
>> + HWMON_F_INPUT | HWMON_F_LABEL, HWMON_F_INPUT |
>> HWMON_F_LABEL,
>> + HWMON_F_INPUT | HWMON_F_LABEL, HWMON_F_INPUT |
>> HWMON_F_LABEL),
>> + NULL
>> +};
>> +
>> +static const struct hwmon_ops cgbc_hwmon_ops = {
>> + .is_visible = cgbc_hwmon_is_visible,
>> + .read = cgbc_hwmon_read,
>> + .read_string = cgbc_hwmon_read_string,
>> +};
>> +
>> +static const struct hwmon_chip_info cgbc_chip_info = {
>> + .ops = &cgbc_hwmon_ops,
>> + .info = cgbc_hwmon_info,
>> +};
>> +
>> +static int cgbc_hwmon_probe(struct platform_device *pdev)
>> +{
>> + struct cgbc_device_data *cgbc = dev_get_drvdata(pdev->dev.parent);
>> + struct device *dev = &pdev->dev;
>> + struct cgbc_hwmon_data *data;
>> + struct device *hwmon_dev;
>> + int ret;
>> +
>> + data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
>> + if (!data)
>> + return -ENOMEM;
>> +
>> + data->cgbc = cgbc;
>> +
>> + platform_set_drvdata(pdev, data);
>
> What is this used for ? There are no suspend/resume functions,
> so I don't see the use case.
It's useless indeed.
Regards,
Thomas
Powered by blists - more mailing lists