On Sat, Mar 30, 2024 at 12:24:01PM +0100, Maximilian Luz wrote:
> From: Ivor Wanders <ivor@xxxxxxxxxxxx>
>
> The thermal subsystem of the Surface Aggregator Module allows us to
> query the names of the respective thermal sensors. Forward those to
> userspace.
>
> Signed-off-by: Ivor Wanders <ivor@xxxxxxxxxxxx>
> Co-developed-by: Maximilian Luz <luzmaximilian@xxxxxxxxx>
> Signed-off-by: Maximilian Luz <luzmaximilian@xxxxxxxxx>
> Reviewed-by: Hans de Goede <hdegoede@xxxxxxxxxx>
> ---
> drivers/hwmon/surface_temp.c | 112 +++++++++++++++++++++++++++++------
> 1 file changed, 95 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/hwmon/surface_temp.c b/drivers/hwmon/surface_temp.c
> index 48c3e826713f6..7a2e1f638336c 100644
> --- a/drivers/hwmon/surface_temp.c
> +++ b/drivers/hwmon/surface_temp.c
> @@ -17,6 +17,27 @@
>
> /* -- SAM interface. -------------------------------------------------------- */
>
> +/*
> + * Available sensors are indicated by a 16-bit bitfield, where a 1 marks the
> + * presence of a sensor. So we have at most 16 possible sensors/channels.
> + */
> +#define SSAM_TMP_SENSOR_MAX_COUNT 16
#define<space>DEFINITION<tab>value
> +
> +/*
> + * All names observed so far are 6 characters long, but there's only
> + * zeros after the name, so perhaps they can be longer. This number reflects
> + * the maximum zero-padded space observed in the returned buffer.
> + */
> +#define SSAM_TMP_SENSOR_NAME_LENGTH 18
> +
> +struct ssam_tmp_get_name_rsp {
> + __le16 unknown1;
> + char unknown2;
> + char name[SSAM_TMP_SENSOR_NAME_LENGTH];
> +} __packed;
> +
> +static_assert(sizeof(struct ssam_tmp_get_name_rsp) == 21);
> +
> SSAM_DEFINE_SYNC_REQUEST_CL_R(__ssam_tmp_get_available_sensors, __le16, {
> .target_category = SSAM_SSH_TC_TMP,
> .command_id = 0x04,
> @@ -27,6 +48,11 @@ SSAM_DEFINE_SYNC_REQUEST_MD_R(__ssam_tmp_get_temperature, __le16, {
> .command_id = 0x01,
> });
>
> +SSAM_DEFINE_SYNC_REQUEST_MD_R(__ssam_tmp_get_name, struct ssam_tmp_get_name_rsp, {
> + .target_category = SSAM_SSH_TC_TMP,
> + .command_id = 0x0e,
> +});
> +
> static int ssam_tmp_get_available_sensors(struct ssam_device *sdev, s16 *sensors)
> {
> __le16 sensors_le;
> @@ -54,12 +80,37 @@ static int ssam_tmp_get_temperature(struct ssam_device *sdev, u8 iid, long *temp
> return 0;
> }
>
> +static int ssam_tmp_get_name(struct ssam_device *sdev, u8 iid, char *buf, size_t buf_len)
> +{
> + struct ssam_tmp_get_name_rsp name_rsp;
> + int status;
> +
> + status = __ssam_tmp_get_name(sdev->ctrl, sdev->uid.target, iid, &name_rsp);
> + if (status)
> + return status;
> +
> + /*
> + * This should not fail unless the name in the returned struct is not
> + * null-terminated or someone changed something in the struct
> + * definitions above, since our buffer and struct have the same
> + * capacity by design. So if this fails blow this up with a warning.
> + * Since the more likely cause is that the returned string isn't
> + * null-terminated, we might have received garbage (as opposed to just
> + * an incomplete string), so also fail the function.
> + */
> + status = strscpy(buf, name_rsp.name, buf_len);
> + WARN_ON(status < 0);
Not acceptable. From include/asm-generic/bug.h:
* Do not use these macros when checking for invalid external inputs
* (e.g. invalid system call arguments, or invalid data coming from
* network/devices), and on transient conditions like ENOMEM or EAGAIN.
* These macros should be used for recoverable kernel issues only.
> +
> + return status < 0 ? status : 0;
> +}
> +
>
> /* -- Driver.---------------------------------------------------------------- */
>
> struct ssam_temp {
> struct ssam_device *sdev;
> s16 sensors;
> + char names[SSAM_TMP_SENSOR_MAX_COUNT][SSAM_TMP_SENSOR_NAME_LENGTH];
> };
>
> static umode_t ssam_temp_hwmon_is_visible(const void *data,
> @@ -83,33 +134,47 @@ static int ssam_temp_hwmon_read(struct device *dev,
> return ssam_tmp_get_temperature(ssam_temp->sdev, channel + 1, value);
> }
>
> +static int ssam_temp_hwmon_read_string(struct device *dev,
> + enum hwmon_sensor_types type,
> + u32 attr, int channel, const char **str)
> +{
> + const struct ssam_temp *ssam_temp = dev_get_drvdata(dev);
> +
> + *str = ssam_temp->names[channel];
> + return 0;
> +}
> +
> static const struct hwmon_channel_info * const ssam_temp_hwmon_info[] = {
> HWMON_CHANNEL_INFO(chip,
> HWMON_C_REGISTER_TZ),
> - /* We have at most 16 thermal sensor channels. */
> + /*
> + * We have at most SSAM_TMP_SENSOR_MAX_COUNT = 16 thermal sensor
> + * channels.
> + */
Pointless comment. Already explained above, and perfect example showing
why it has no value separating this and the previous patch.
> HWMON_CHANNEL_INFO(temp,
> - HWMON_T_INPUT,
> - HWMON_T_INPUT,
> - HWMON_T_INPUT,
> - HWMON_T_INPUT,
> - HWMON_T_INPUT,
> - HWMON_T_INPUT,
> - HWMON_T_INPUT,
> - HWMON_T_INPUT,
> - HWMON_T_INPUT,
> - HWMON_T_INPUT,
> - HWMON_T_INPUT,
> - HWMON_T_INPUT,
> - HWMON_T_INPUT,
> - HWMON_T_INPUT,
> - HWMON_T_INPUT,
> - HWMON_T_INPUT),
> + 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_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),
Another example. Why have me review the previous patch
just to change the code here ?
[ ... ]
> + /* Retrieve the name for each available sensor. */
> + for (channel = 0; channel < SSAM_TMP_SENSOR_MAX_COUNT; channel++) {
> + if (!(sensors & BIT(channel)))
> + continue;
> +
> + status = ssam_tmp_get_name(sdev, channel + 1,
> + ssam_temp->names[channel],
> + SSAM_TMP_SENSOR_NAME_LENGTH);
> + if (status)
> + return status;
Your call to fail probe in this case just because it can not find
a sensor name. I personally find that quite aggressive.
Guenter
