On Sun, Apr 11, 2021 at 06:42:25PM +0200, W_Armin@xxxxxx wrote:
> From: Armin Wolf <W_Armin@xxxxxx>
>
> An error in sch5627_update_device() could cause sch5627_read()
> to fail even if the error did not affect the target sensor type.
> Split sch5627_update_device() to prevent that.
>
> Tested on a Fujitsu Esprimo P720.
>
> Signed-off-by: Armin Wolf <W_Armin@xxxxxx>
Applied.
Thanks,
Guenter
> ---
> drivers/hwmon/sch5627.c | 102 +++++++++++++++++++++++++++-------------
> 1 file changed, 69 insertions(+), 33 deletions(-)
>
> --
> 2.20.1
>
> diff --git a/drivers/hwmon/sch5627.c b/drivers/hwmon/sch5627.c
> index 023376082ee6..9735f3a81cbb 100644
> --- a/drivers/hwmon/sch5627.c
> +++ b/drivers/hwmon/sch5627.c
> @@ -73,66 +73,96 @@ struct sch5627_data {
>
> struct mutex update_lock;
> unsigned long last_battery; /* In jiffies */
> - char valid; /* !=0 if following fields are valid */
> - unsigned long last_updated; /* In jiffies */
> + char temp_valid; /* !=0 if following fields are valid */
> + char fan_valid;
> + char in_valid;
> + unsigned long temp_last_updated; /* In jiffies */
> + unsigned long fan_last_updated;
> + unsigned long in_last_updated;
> u16 temp[SCH5627_NO_TEMPS];
> u16 fan[SCH5627_NO_FANS];
> u16 in[SCH5627_NO_IN];
> };
>
> -static struct sch5627_data *sch5627_update_device(struct device *dev)
> +static int sch5627_update_temp(struct sch5627_data *data)
> {
> - struct sch5627_data *data = dev_get_drvdata(dev);
> - struct sch5627_data *ret = data;
> + int ret = 0;
> int i, val;
>
> mutex_lock(&data->update_lock);
>
> - /* Trigger a Vbat voltage measurement every 5 minutes */
> - if (time_after(jiffies, data->last_battery + 300 * HZ)) {
> - sch56xx_write_virtual_reg(data->addr, SCH5627_REG_CTRL,
> - data->control | 0x10);
> - data->last_battery = jiffies;
> - }
> -
> /* Cache the values for 1 second */
> - if (time_after(jiffies, data->last_updated + HZ) || !data->valid) {
> + if (time_after(jiffies, data->temp_last_updated + HZ) || !data->temp_valid) {
> for (i = 0; i < SCH5627_NO_TEMPS; i++) {
> - val = sch56xx_read_virtual_reg12(data->addr,
> - SCH5627_REG_TEMP_MSB[i],
> - SCH5627_REG_TEMP_LSN[i],
> - SCH5627_REG_TEMP_HIGH_NIBBLE[i]);
> + val = sch56xx_read_virtual_reg12(data->addr, SCH5627_REG_TEMP_MSB[i],
> + SCH5627_REG_TEMP_LSN[i],
> + SCH5627_REG_TEMP_HIGH_NIBBLE[i]);
> if (unlikely(val < 0)) {
> - ret = ERR_PTR(val);
> + ret = val;
> goto abort;
> }
> data->temp[i] = val;
> }
> + data->temp_last_updated = jiffies;
> + data->temp_valid = 1;
> + }
> +abort:
> + mutex_unlock(&data->update_lock);
> + return ret;
> +}
> +
> +static int sch5627_update_fan(struct sch5627_data *data)
> +{
> + int ret = 0;
> + int i, val;
>
> + mutex_lock(&data->update_lock);
> +
> + /* Cache the values for 1 second */
> + if (time_after(jiffies, data->fan_last_updated + HZ) || !data->fan_valid) {
> for (i = 0; i < SCH5627_NO_FANS; i++) {
> - val = sch56xx_read_virtual_reg16(data->addr,
> - SCH5627_REG_FAN[i]);
> + val = sch56xx_read_virtual_reg16(data->addr, SCH5627_REG_FAN[i]);
> if (unlikely(val < 0)) {
> - ret = ERR_PTR(val);
> + ret = val;
> goto abort;
> }
> data->fan[i] = val;
> }
> + data->fan_last_updated = jiffies;
> + data->fan_valid = 1;
> + }
> +abort:
> + mutex_unlock(&data->update_lock);
> + return ret;
> +}
> +
> +static int sch5627_update_in(struct sch5627_data *data)
> +{
> + int ret = 0;
> + int i, val;
> +
> + mutex_lock(&data->update_lock);
>
> + /* Trigger a Vbat voltage measurement every 5 minutes */
> + if (time_after(jiffies, data->last_battery + 300 * HZ)) {
> + sch56xx_write_virtual_reg(data->addr, SCH5627_REG_CTRL, data->control | 0x10);
> + data->last_battery = jiffies;
> + }
> +
> + /* Cache the values for 1 second */
> + if (time_after(jiffies, data->in_last_updated + HZ) || !data->in_valid) {
> for (i = 0; i < SCH5627_NO_IN; i++) {
> - val = sch56xx_read_virtual_reg12(data->addr,
> - SCH5627_REG_IN_MSB[i],
> - SCH5627_REG_IN_LSN[i],
> - SCH5627_REG_IN_HIGH_NIBBLE[i]);
> + val = sch56xx_read_virtual_reg12(data->addr, SCH5627_REG_IN_MSB[i],
> + SCH5627_REG_IN_LSN[i],
> + SCH5627_REG_IN_HIGH_NIBBLE[i]);
> if (unlikely(val < 0)) {
> - ret = ERR_PTR(val);
> + ret = val;
> goto abort;
> }
> data->in[i] = val;
> }
> -
> - data->last_updated = jiffies;
> - data->valid = 1;
> + data->in_last_updated = jiffies;
> + data->in_valid = 1;
> }
> abort:
> mutex_unlock(&data->update_lock);
> @@ -200,14 +230,14 @@ static umode_t sch5627_is_visible(const void *drvdata, enum hwmon_sensor_types t
> static int sch5627_read(struct device *dev, enum hwmon_sensor_types type, u32 attr, int channel,
> long *val)
> {
> - struct sch5627_data *data = sch5627_update_device(dev);
> + struct sch5627_data *data = dev_get_drvdata(dev);
> int ret;
>
> - if (IS_ERR(data))
> - return PTR_ERR(data);
> -
> switch (type) {
> case hwmon_temp:
> + ret = sch5627_update_temp(data);
> + if (ret < 0)
> + return ret;
> switch (attr) {
> case hwmon_temp_input:
> *val = reg_to_temp(data->temp[channel]);
> @@ -226,6 +256,9 @@ static int sch5627_read(struct device *dev, enum hwmon_sensor_types type, u32 at
> }
> break;
> case hwmon_fan:
> + ret = sch5627_update_fan(data);
> + if (ret < 0)
> + return ret;
> switch (attr) {
> case hwmon_fan_input:
> ret = reg_to_rpm(data->fan[channel]);
> @@ -247,6 +280,9 @@ static int sch5627_read(struct device *dev, enum hwmon_sensor_types type, u32 at
> }
> break;
> case hwmon_in:
> + ret = sch5627_update_in(data);
> + if (ret < 0)
> + return ret;
> switch (attr) {
> case hwmon_in_input:
> *val = DIV_ROUND_CLOSEST(data->in[channel] * SCH5627_REG_IN_FACTOR[channel],
