From: Guenter Roeck <linux@roeck-us.net>
To: Frank Crawford <frank@crawford.emu.id.au>
Cc: Jean Delvare <jdelvare@suse.com>, linux-hwmon@vger.kernel.org
Subject: Re: [PATCH v1 2/4] hwmon: (it87) Add calls to smbus_enable/smbus_disable as required.
Date: Wed, 12 Apr 2023 11:44:58 -0700 [thread overview]
Message-ID: <8c8408ff-ab22-4a6a-b9d1-64bab64d6f05@roeck-us.net> (raw)
In-Reply-To: <20230409034718.1938695-3-frank@crawford.emu.id.au>
On Sun, Apr 09, 2023 at 01:47:16PM +1000, Frank Crawford wrote:
> Disable/re-enable access through SMBus for chip registers when they are
> are being read or written.
>
> For simple cases this is done at the same time as when a mutex is set,
> however, within loops or during initialisation it is done separately.
>
> Signed-off-by: Frank Crawford <frank@crawford.emu.id.au>
This gives me:
ERROR: code indent should use tabs where possible
#629: FILE: drivers/hwmon/it87.c:3268:
+^I ^I^I it87_read_value(data, IT87_REG_CHIPID) != 0x90) {$
WARNING: please, no space before tabs
#629: FILE: drivers/hwmon/it87.c:3268:
+^I ^I^I it87_read_value(data, IT87_REG_CHIPID) != 0x90) {$
> ---
> drivers/hwmon/it87.c | 181 ++++++++++++++++++++++++++++++++-----------
> 1 file changed, 135 insertions(+), 46 deletions(-)
>
> diff --git a/drivers/hwmon/it87.c b/drivers/hwmon/it87.c
> index b74dd861f0fe..1ca3247efb9e 100644
> --- a/drivers/hwmon/it87.c
> +++ b/drivers/hwmon/it87.c
> @@ -746,6 +746,7 @@ static int smbus_enable(struct it87_data *data)
>
> /*
> * Must be called with data->update_lock held, except during initialization.
> + * Must be called with SMBus accesses disabled.
> * We ignore the IT87 BUSY flag at this moment - it could lead to deadlocks,
> * would slow down the IT87 access and should not be necessary.
> */
> @@ -757,6 +758,7 @@ static int it87_read_value(struct it87_data *data, u8 reg)
>
> /*
> * Must be called with data->update_lock held, except during initialization.
> + * Must be called with SMBus accesses disabled.
> * We ignore the IT87 BUSY flag at this moment - it could lead to deadlocks,
> * would slow down the IT87 access and should not be necessary.
> */
> @@ -816,15 +818,39 @@ static void it87_update_pwm_ctrl(struct it87_data *data, int nr)
> }
> }
>
> +static int it87_lock(struct it87_data *data)
> +{
> + int err;
> +
> + mutex_lock(&data->update_lock);
> + err = smbus_disable(data);
> + if (err)
> + mutex_unlock(&data->update_lock);
> + return err;
> +}
> +
> +static void it87_unlock(struct it87_data *data)
> +{
> + smbus_enable(data);
> + mutex_unlock(&data->update_lock);
> +}
> +
> static struct it87_data *it87_update_device(struct device *dev)
> {
> struct it87_data *data = dev_get_drvdata(dev);
> + struct it87_data *ret = data;
> + int err;
> int i;
>
> mutex_lock(&data->update_lock);
>
> if (time_after(jiffies, data->last_updated + HZ + HZ / 2) ||
> - !data->valid) {
> + !data->valid) {
> + err = smbus_disable(data);
> + if (err) {
> + ret = ERR_PTR(err);
> + goto unlock;
> + }
> if (update_vbat) {
> /*
> * Cleared after each update, so reenable. Value
> @@ -927,11 +953,11 @@ static struct it87_data *it87_update_device(struct device *dev)
> }
> data->last_updated = jiffies;
> data->valid = true;
> + smbus_enable(data);
> }
> -
> +unlock:
> mutex_unlock(&data->update_lock);
> -
> - return data;
> + return ret;
> }
>
> static ssize_t show_in(struct device *dev, struct device_attribute *attr,
> @@ -953,17 +979,21 @@ static ssize_t set_in(struct device *dev, struct device_attribute *attr,
> int index = sattr->index;
> int nr = sattr->nr;
> unsigned long val;
> + int err;
>
> if (kstrtoul(buf, 10, &val) < 0)
> return -EINVAL;
>
> - mutex_lock(&data->update_lock);
> + err = it87_lock(data);
> + if (err)
> + return err;
> +
> data->in[nr][index] = in_to_reg(data, nr, val);
> it87_write_value(data,
> index == 1 ? IT87_REG_VIN_MIN(nr)
> : IT87_REG_VIN_MAX(nr),
> data->in[nr][index]);
> - mutex_unlock(&data->update_lock);
> + it87_unlock(data);
> return count;
> }
>
> @@ -1042,11 +1072,14 @@ static ssize_t set_temp(struct device *dev, struct device_attribute *attr,
> struct it87_data *data = dev_get_drvdata(dev);
> long val;
> u8 reg, regval;
> + int err;
>
> if (kstrtol(buf, 10, &val) < 0)
> return -EINVAL;
>
> - mutex_lock(&data->update_lock);
> + err = it87_lock(data);
> + if (err)
> + return err;
>
> switch (index) {
> default:
> @@ -1069,7 +1102,7 @@ static ssize_t set_temp(struct device *dev, struct device_attribute *attr,
>
> data->temp[nr][index] = TEMP_TO_REG(val);
> it87_write_value(data, reg, data->temp[nr][index]);
> - mutex_unlock(&data->update_lock);
> + it87_unlock(data);
> return count;
> }
>
> @@ -1126,10 +1159,15 @@ static ssize_t set_temp_type(struct device *dev, struct device_attribute *attr,
> struct it87_data *data = dev_get_drvdata(dev);
> long val;
> u8 reg, extra;
> + int err;
>
> if (kstrtol(buf, 10, &val) < 0)
> return -EINVAL;
>
> + err = it87_lock(data);
> + if (err)
> + return err;
> +
> reg = it87_read_value(data, IT87_REG_TEMP_ENABLE);
> reg &= ~(1 << nr);
> reg &= ~(8 << nr);
> @@ -1152,17 +1190,19 @@ static ssize_t set_temp_type(struct device *dev, struct device_attribute *attr,
> reg |= (nr + 1) << 6;
> else if (has_temp_old_peci(data, nr) && val == 6)
> extra |= 0x80;
> - else if (val != 0)
> - return -EINVAL;
> + else if (val != 0) {
> + count = -EINVAL;
> + goto unlock;
> + }
>
> - mutex_lock(&data->update_lock);
> data->sensor = reg;
> data->extra = extra;
> it87_write_value(data, IT87_REG_TEMP_ENABLE, data->sensor);
> if (has_temp_old_peci(data, nr))
> it87_write_value(data, IT87_REG_TEMP_EXTRA, data->extra);
> data->valid = false; /* Force cache refresh */
> - mutex_unlock(&data->update_lock);
> +unlock:
> + it87_unlock(data);
> return count;
> }
>
> @@ -1263,12 +1303,15 @@ static ssize_t set_fan(struct device *dev, struct device_attribute *attr,
>
> struct it87_data *data = dev_get_drvdata(dev);
> long val;
> + int err;
> u8 reg;
>
> if (kstrtol(buf, 10, &val) < 0)
> return -EINVAL;
>
> - mutex_lock(&data->update_lock);
> + err = it87_lock(data);
> + if (err)
> + return err;
>
> if (has_16bit_fans(data)) {
> data->fan[nr][index] = FAN16_TO_REG(val);
> @@ -1295,7 +1338,7 @@ static ssize_t set_fan(struct device *dev, struct device_attribute *attr,
> data->fan[nr][index]);
> }
>
> - mutex_unlock(&data->update_lock);
> + it87_unlock(data);
> return count;
> }
>
> @@ -1306,13 +1349,16 @@ static ssize_t set_fan_div(struct device *dev, struct device_attribute *attr,
> struct it87_data *data = dev_get_drvdata(dev);
> int nr = sensor_attr->index;
> unsigned long val;
> - int min;
> + int min, err;
> u8 old;
>
> if (kstrtoul(buf, 10, &val) < 0)
> return -EINVAL;
>
> - mutex_lock(&data->update_lock);
> + err = it87_lock(data);
> + if (err)
> + return err;
> +
> old = it87_read_value(data, IT87_REG_FAN_DIV);
>
> /* Save fan min limit */
> @@ -1340,7 +1386,7 @@ static ssize_t set_fan_div(struct device *dev, struct device_attribute *attr,
> data->fan[nr][1] = FAN_TO_REG(min, DIV_FROM_REG(data->fan_div[nr]));
> it87_write_value(data, IT87_REG_FAN_MIN[nr], data->fan[nr][1]);
>
> - mutex_unlock(&data->update_lock);
> + it87_unlock(data);
> return count;
> }
>
> @@ -1381,6 +1427,7 @@ static ssize_t set_pwm_enable(struct device *dev, struct device_attribute *attr,
> struct it87_data *data = dev_get_drvdata(dev);
> int nr = sensor_attr->index;
> long val;
> + int err;
>
> if (kstrtol(buf, 10, &val) < 0 || val < 0 || val > 2)
> return -EINVAL;
> @@ -1391,7 +1438,9 @@ static ssize_t set_pwm_enable(struct device *dev, struct device_attribute *attr,
> return -EINVAL;
> }
>
> - mutex_lock(&data->update_lock);
> + err = it87_lock(data);
> + if (err)
> + return err;
>
> if (val == 0) {
> if (nr < 3 && data->type != it8603) {
> @@ -1442,7 +1491,7 @@ static ssize_t set_pwm_enable(struct device *dev, struct device_attribute *attr,
> }
> }
>
> - mutex_unlock(&data->update_lock);
> + it87_unlock(data);
> return count;
> }
>
> @@ -1453,11 +1502,15 @@ static ssize_t set_pwm(struct device *dev, struct device_attribute *attr,
> struct it87_data *data = dev_get_drvdata(dev);
> int nr = sensor_attr->index;
> long val;
> + int err;
>
> if (kstrtol(buf, 10, &val) < 0 || val < 0 || val > 255)
> return -EINVAL;
>
> - mutex_lock(&data->update_lock);
> + err = it87_lock(data);
> + if (err)
> + return err;
> +
> it87_update_pwm_ctrl(data, nr);
> if (has_newer_autopwm(data)) {
> /*
> @@ -1465,8 +1518,8 @@ static ssize_t set_pwm(struct device *dev, struct device_attribute *attr,
> * is read-only so we can't write the value.
> */
> if (data->pwm_ctrl[nr] & 0x80) {
> - mutex_unlock(&data->update_lock);
> - return -EBUSY;
> + count = -EBUSY;
> + goto unlock;
> }
> data->pwm_duty[nr] = pwm_to_reg(data, val);
> it87_write_value(data, IT87_REG_PWM_DUTY[nr],
> @@ -1483,7 +1536,8 @@ static ssize_t set_pwm(struct device *dev, struct device_attribute *attr,
> data->pwm_ctrl[nr]);
> }
> }
> - mutex_unlock(&data->update_lock);
> +unlock:
> + it87_unlock(data);
> return count;
> }
>
> @@ -1494,6 +1548,7 @@ static ssize_t set_pwm_freq(struct device *dev, struct device_attribute *attr,
> struct it87_data *data = dev_get_drvdata(dev);
> int nr = sensor_attr->index;
> unsigned long val;
> + int err;
> int i;
>
> if (kstrtoul(buf, 10, &val) < 0)
> @@ -1508,7 +1563,10 @@ static ssize_t set_pwm_freq(struct device *dev, struct device_attribute *attr,
> break;
> }
>
> - mutex_lock(&data->update_lock);
> + err = it87_lock(data);
> + if (err)
> + return err;
> +
> if (nr == 0) {
> data->fan_ctl = it87_read_value(data, IT87_REG_FAN_CTL) & 0x8f;
> data->fan_ctl |= i << 4;
> @@ -1518,7 +1576,7 @@ static ssize_t set_pwm_freq(struct device *dev, struct device_attribute *attr,
> data->extra |= i << 4;
> it87_write_value(data, IT87_REG_TEMP_EXTRA, data->extra);
> }
> - mutex_unlock(&data->update_lock);
> + it87_unlock(data);
>
> return count;
> }
> @@ -1548,6 +1606,7 @@ static ssize_t set_pwm_temp_map(struct device *dev,
> struct it87_data *data = dev_get_drvdata(dev);
> int nr = sensor_attr->index;
> long val;
> + int err;
> u8 reg;
>
> if (kstrtol(buf, 10, &val) < 0)
> @@ -1570,7 +1629,10 @@ static ssize_t set_pwm_temp_map(struct device *dev,
> return -EINVAL;
> }
>
> - mutex_lock(&data->update_lock);
> + err = it87_lock(data);
> + if (err)
> + return err;
> +
> it87_update_pwm_ctrl(data, nr);
> data->pwm_temp_map[nr] = reg;
> /*
> @@ -1582,7 +1644,7 @@ static ssize_t set_pwm_temp_map(struct device *dev,
> data->pwm_temp_map[nr];
> it87_write_value(data, IT87_REG_PWM[nr], data->pwm_ctrl[nr]);
> }
> - mutex_unlock(&data->update_lock);
> + it87_unlock(data);
> return count;
> }
>
> @@ -1609,18 +1671,22 @@ static ssize_t set_auto_pwm(struct device *dev, struct device_attribute *attr,
> int point = sensor_attr->index;
> int regaddr;
> long val;
> + int err;
>
> if (kstrtol(buf, 10, &val) < 0 || val < 0 || val > 255)
> return -EINVAL;
>
> - mutex_lock(&data->update_lock);
> + err = it87_lock(data);
> + if (err)
> + return err;
> +
> data->auto_pwm[nr][point] = pwm_to_reg(data, val);
> if (has_newer_autopwm(data))
> regaddr = IT87_REG_AUTO_TEMP(nr, 3);
> else
> regaddr = IT87_REG_AUTO_PWM(nr, point);
> it87_write_value(data, regaddr, data->auto_pwm[nr][point]);
> - mutex_unlock(&data->update_lock);
> + it87_unlock(data);
> return count;
> }
>
> @@ -1642,15 +1708,19 @@ static ssize_t set_auto_pwm_slope(struct device *dev,
> struct sensor_device_attribute *sensor_attr = to_sensor_dev_attr(attr);
> int nr = sensor_attr->index;
> unsigned long val;
> + int err;
>
> if (kstrtoul(buf, 10, &val) < 0 || val > 127)
> return -EINVAL;
>
> - mutex_lock(&data->update_lock);
> + err = it87_lock(data);
> + if (err)
> + return err;
> +
> data->auto_pwm[nr][1] = (data->auto_pwm[nr][1] & 0x80) | val;
> it87_write_value(data, IT87_REG_AUTO_TEMP(nr, 4),
> data->auto_pwm[nr][1]);
> - mutex_unlock(&data->update_lock);
> + it87_unlock(data);
> return count;
> }
>
> @@ -1682,11 +1752,15 @@ static ssize_t set_auto_temp(struct device *dev, struct device_attribute *attr,
> int point = sensor_attr->index;
> long val;
> int reg;
> + int err;
>
> if (kstrtol(buf, 10, &val) < 0 || val < -128000 || val > 127000)
> return -EINVAL;
>
> - mutex_lock(&data->update_lock);
> + err = it87_lock(data);
> + if (err)
> + return err;
> +
> if (has_newer_autopwm(data) && !point) {
> reg = data->auto_temp[nr][1] - TEMP_TO_REG(val);
> reg = clamp_val(reg, 0, 0x1f) | (data->auto_temp[nr][0] & 0xe0);
> @@ -1699,7 +1773,7 @@ static ssize_t set_auto_temp(struct device *dev, struct device_attribute *attr,
> point--;
> it87_write_value(data, IT87_REG_AUTO_TEMP(nr, point), reg);
> }
> - mutex_unlock(&data->update_lock);
> + it87_unlock(data);
> return count;
> }
>
> @@ -1902,13 +1976,16 @@ static ssize_t clear_intrusion(struct device *dev,
> size_t count)
> {
> struct it87_data *data = dev_get_drvdata(dev);
> - int config;
> + int err, config;
> long val;
>
> if (kstrtol(buf, 10, &val) < 0 || val != 0)
> return -EINVAL;
>
> - mutex_lock(&data->update_lock);
> + err = it87_lock(data);
> + if (err)
> + return err;
> +
> config = it87_read_value(data, IT87_REG_CONFIG);
> if (config < 0) {
> count = config;
> @@ -1918,8 +1995,7 @@ static ssize_t clear_intrusion(struct device *dev,
> /* Invalidate cache to force re-read */
> data->valid = false;
> }
> - mutex_unlock(&data->update_lock);
> -
> + it87_unlock(data);
> return count;
> }
>
> @@ -1958,18 +2034,22 @@ static ssize_t set_beep(struct device *dev, struct device_attribute *attr,
> int bitnr = to_sensor_dev_attr(attr)->index;
> struct it87_data *data = dev_get_drvdata(dev);
> long val;
> + int err;
>
> if (kstrtol(buf, 10, &val) < 0 || (val != 0 && val != 1))
> return -EINVAL;
>
> - mutex_lock(&data->update_lock);
> + err = it87_lock(data);
> + if (err)
> + return err;
> +
> data->beeps = it87_read_value(data, IT87_REG_BEEP_ENABLE);
> if (val)
> data->beeps |= BIT(bitnr);
> else
> data->beeps &= ~BIT(bitnr);
> it87_write_value(data, IT87_REG_BEEP_ENABLE, data->beeps);
> - mutex_unlock(&data->update_lock);
> + it87_unlock(data);
> return count;
> }
>
> @@ -3129,6 +3209,7 @@ static int it87_probe(struct platform_device *pdev)
> struct it87_sio_data *sio_data = dev_get_platdata(dev);
> int enable_pwm_interface;
> struct device *hwmon_dev;
> + int err;
>
> res = platform_get_resource(pdev, IORESOURCE_IO, 0);
> if (!devm_request_region(&pdev->dev, res->start, IT87_EC_EXTENT,
> @@ -3174,15 +3255,21 @@ static int it87_probe(struct platform_device *pdev)
> break;
> }
>
> - /* Now, we do the remaining detection. */
> - if ((it87_read_value(data, IT87_REG_CONFIG) & 0x80) ||
> - it87_read_value(data, IT87_REG_CHIPID) != 0x90)
> - return -ENODEV;
> -
> platform_set_drvdata(pdev, data);
>
> mutex_init(&data->update_lock);
>
> + err = smbus_disable(data);
> + if (err)
> + return err;
> +
> + /* Now, we do the remaining detection. */
> + if ((it87_read_value(data, IT87_REG_CONFIG) & 0x80) ||
> + it87_read_value(data, IT87_REG_CHIPID) != 0x90) {
> + smbus_enable(data);
> + return -ENODEV;
> + }
> +
> /* Check PWM configuration */
> enable_pwm_interface = it87_check_pwm(dev);
> if (!enable_pwm_interface)
> @@ -3243,6 +3330,8 @@ static int it87_probe(struct platform_device *pdev)
> /* Initialize the IT87 chip */
> it87_init_device(pdev);
>
> + smbus_enable(data);
> +
> if (!sio_data->skip_vid) {
> data->has_vid = true;
> data->vrm = vid_which_vrm();
> @@ -3310,7 +3399,7 @@ static int it87_resume(struct device *dev)
>
> it87_resume_sio(pdev);
>
> - mutex_lock(&data->update_lock);
> + it87_lock(data);
Not checking for error here means that the lock was not acquired if
it87_lock() failed, yet it87_unlock() is called below. I don't really
know how to best handle this situation, but calling unlock if the lock
was not acquired is definitely wrong.
Guenter
>
> it87_check_pwm(dev);
> it87_check_limit_regs(data);
> @@ -3323,7 +3412,7 @@ static int it87_resume(struct device *dev)
> /* force update */
> data->valid = false;
>
> - mutex_unlock(&data->update_lock);
> + it87_unlock(data);
>
> it87_update_device(dev);
>
next prev parent reply other threads:[~2023-04-12 18:45 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-04-09 3:47 [PATCH v1 0/4] hwmon: (it87) Add access control for SMBus Frank Crawford
2023-04-09 3:47 ` [PATCH v1 1/4] hwmon: (it87) Disable SMBus access for environmental controller registers Frank Crawford
2023-04-09 3:47 ` [PATCH v1 2/4] hwmon: (it87) Add calls to smbus_enable/smbus_disable as required Frank Crawford
2023-04-12 18:44 ` Guenter Roeck [this message]
2023-04-09 3:47 ` [PATCH v1 3/4] hwmon: (it87) Test for error in it87_update_device Frank Crawford
2023-04-12 18:46 ` Guenter Roeck
2023-04-09 3:47 ` [PATCH v1 4/4] hwmon: (it87) Disable/enable SMBus access for IT8622E chipset Frank Crawford
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=8c8408ff-ab22-4a6a-b9d1-64bab64d6f05@roeck-us.net \
--to=linux@roeck-us.net \
--cc=frank@crawford.emu.id.au \
--cc=jdelvare@suse.com \
--cc=linux-hwmon@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).