| 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: <CACSj6VW2+8TBh971CERjxPzhyO5JeP0rbkrw0ZjfE4bkhMfo4Q@mail.gmail.com>
Date: Wed, 7 Apr 2021 01:25:02 +0300
From: Konstantin Aladyshev <aladyshev22@...il.com>
To: Guenter Roeck <linux@...ck-us.net>
Cc: Kun Yi <kunyi@...gle.com>, Jean Delvare <jdelvare@...e.com>,
linux-hwmon@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] hwmon: (sbtsi) Don't read sensor more than once if it
doesn't respond
What I'm trying to say, shouldn't the call
"i2c_smbus_read_byte_data(data->client, SBTSI_REG_CONFIG)" be
surrounded with the "mutex_lock/mutex_unlock" like it is done for the
others "i2c_smbus_read_byte_data" calls?
Something like:
```
mutex_lock(&data->lock);
err = i2c_smbus_read_byte_data(data->client, SBTSI_REG_CONFIG);
if (err < 0)
return err;
mutex_unlock(&data->lock);
```
Because it is not surrounded with the mutex lock/unlock in the original driver.
Best regards,
Konstantin Aladyshev
On Wed, Apr 7, 2021 at 12:34 AM Guenter Roeck <linux@...ck-us.net> wrote:
>
> On 4/6/21 2:09 PM, Konstantin Aladyshev wrote:
> > Thanks for the answer!
> > Sorry for the confusion, by the "CPU is off" I meant "CPU is present,
> > but currently it is in the powered off state".
> > Therefore it is not possible to put these checks only in a probe
> > function. And I don't know either if it is a good idea to cache
> > config/min/max values.
> >
> > I use this driver on an OpenBMC system, which uses other software
> > rather than lm-sensors utility. I guess that is why my priorities are
> > shifted.
> >
> > By the way, I've noticed that the mutex check is absent in a
> > SBTSI_REG_CONFIG read call both in the original driver version and in
> > my patch, is this an error?
> >
>
> What do you mean with "mutex check" ?
>
> Thanks,
> Guenter
>
>
> > Best regards,
> > Konstantin Aladyshev
> >
> >
> > On Tue, Apr 6, 2021 at 11:09 PM Guenter Roeck <linux@...ck-us.net> wrote:
> >>
> >> On 4/6/21 12:20 PM, Konstantin Aladyshev wrote:
> >>> Thanks for the comment.
> >>> Yes, you are correct, this patch adds an extra 'i2c_smbus_read_byte_data' call for the temp_max/temp_min reads.
> >>> I guess I did that intentionally because I just wanted to keep the restructured code concise. After all I thought, 'temp_input' generally is read more often than 'temp_max/temp_min'.
> >>> As I understand now, it seems like it is not acceptable. Therefore could you point me in the right direction about what I should do?
> >>> Should I just stick with the original driver version and simply add two more i2c call checks for the first operations for min/max?
> >>>
> >>
> >> Correct, it is not acceptable. A normal use case for hwmon devices is to use the "sensors"
> >> command which _will_ read all attributes. i2c reads are expensive, and unnecessary read
> >> operations should be avoided.
> >>
> >> There are several ways to solve the problem. Checking return values after each
> >> read is the simple option. There are other possibilities, such as reading the limits
> >> and the read order only once during probe, but I don't know enough about the
> >> hardware to suggest a more sophisticated solution. For example, I don't know
> >> what "CPU is off" means. Offline ? Not present ? If it means "not present",
> >> or if the status is permanent, the condition should be handled in the is_visible
> >> function (or the driver should not be instantiated in the first place).
> >> Otherwise, the code should possibly return -ENODATA instead of -ETIMEDOUT
> >> on error. But, again, I can not really suggest a better solution since
> >> I don't know enough (nothing, actually) about the hardware (and the public
> >> part of the SBTSI specification doesn't say anything about expected behavior
> >> for offline CPUs or CPU cores).
> >>
> >> What I did find, though, is that the driver does not implement temperature
> >> offset support, and it that doesn't support reporting alerts. I'd have assumed
> >> this to be more important than optimizing error handling, but that is just
> >> my personal opinion.
> >>
> >> Thanks,
> >> Guenter
> >>
> >>> Best regards,
> >>> Konstantin Aladyshev
> >>>
> >>>
> >>> On Tue, Apr 6, 2021 at 9:42 PM Guenter Roeck <linux@...ck-us.net <mailto:linux@...ck-us.net>> wrote:
> >>>
> >>> On 4/6/21 11:16 AM, Konstantin Aladyshev wrote:
> >>> > SBTSI sensors don't work when the CPU is off.
> >>> > In this case every 'i2c_smbus_read_byte_data' function would fail
> >>> > by a timeout.
> >>> > Currently temp1_max/temp1_min file reads can cause two such timeouts
> >>> > for every read.
> >>> > Restructure code so there will be no more than one timeout for every
> >>> > read operation.
> >>> >
> >>> > Signed-off-by: Konstantin Aladyshev <aladyshev22@...il.com <mailto:aladyshev22@...il.com>>
> >>> > ---
> >>> > Changes in v2:
> >>> > - Fix typo in a commit message
> >>> > - Don't swap temp_int/temp_dec checks at the end of the 'sbtsi_read' function
> >>> >
> >>>
> >>> This doesn't explain the reason for the extra read operation for
> >>> limits. Preventing a second read in error cases is not an argument
> >>> for adding an extra read for non-error cases.
> >>>
> >>> Guenter
> >>>
> >>> > drivers/hwmon/sbtsi_temp.c | 55 +++++++++++++++++++-------------------
> >>> > 1 file changed, 27 insertions(+), 28 deletions(-)
> >>> >
> >>> > diff --git a/drivers/hwmon/sbtsi_temp.c b/drivers/hwmon/sbtsi_temp.c
> >>> > index e35357c48b8e..4ae48635bb31 100644
> >>> > --- a/drivers/hwmon/sbtsi_temp.c
> >>> > +++ b/drivers/hwmon/sbtsi_temp.c
> >>> > @@ -74,48 +74,47 @@ static int sbtsi_read(struct device *dev, enum hwmon_sensor_types type,
> >>> > u32 attr, int channel, long *val)
> >>> > {
> >>> > struct sbtsi_data *data = dev_get_drvdata(dev);
> >>> > + u8 temp_int_reg, temp_dec_reg;
> >>> > s32 temp_int, temp_dec;
> >>> > int err;
> >>> >
> >>> > switch (attr) {
> >>> > case hwmon_temp_input:
> >>> > - /*
> >>> > - * ReadOrder bit specifies the reading order of integer and
> >>> > - * decimal part of CPU temp for atomic reads. If bit == 0,
> >>> > - * reading integer part triggers latching of the decimal part,
> >>> > - * so integer part should be read first. If bit == 1, read
> >>> > - * order should be reversed.
> >>> > - */
> >>> > - err = i2c_smbus_read_byte_data(data->client, SBTSI_REG_CONFIG);
> >>> > - if (err < 0)
> >>> > - return err;
> >>> > -
> >>> > - mutex_lock(&data->lock);
> >>> > - if (err & BIT(SBTSI_CONFIG_READ_ORDER_SHIFT)) {
> >>> > - temp_dec = i2c_smbus_read_byte_data(data->client, SBTSI_REG_TEMP_DEC);
> >>> > - temp_int = i2c_smbus_read_byte_data(data->client, SBTSI_REG_TEMP_INT);
> >>> > - } else {
> >>> > - temp_int = i2c_smbus_read_byte_data(data->client, SBTSI_REG_TEMP_INT);
> >>> > - temp_dec = i2c_smbus_read_byte_data(data->client, SBTSI_REG_TEMP_DEC);
> >>> > - }
> >>> > - mutex_unlock(&data->lock);
> >>> > + temp_int_reg = SBTSI_REG_TEMP_INT;
> >>> > + temp_dec_reg = SBTSI_REG_TEMP_DEC;
> >>> > break;
> >>> > case hwmon_temp_max:
> >>> > - mutex_lock(&data->lock);
> >>> > - temp_int = i2c_smbus_read_byte_data(data->client, SBTSI_REG_TEMP_HIGH_INT);
> >>> > - temp_dec = i2c_smbus_read_byte_data(data->client, SBTSI_REG_TEMP_HIGH_DEC);
> >>> > - mutex_unlock(&data->lock);
> >>> > + temp_int_reg = SBTSI_REG_TEMP_HIGH_INT;
> >>> > + temp_dec_reg = SBTSI_REG_TEMP_HIGH_DEC;
> >>> > break;
> >>> > case hwmon_temp_min:
> >>> > - mutex_lock(&data->lock);
> >>> > - temp_int = i2c_smbus_read_byte_data(data->client, SBTSI_REG_TEMP_LOW_INT);
> >>> > - temp_dec = i2c_smbus_read_byte_data(data->client, SBTSI_REG_TEMP_LOW_DEC);
> >>> > - mutex_unlock(&data->lock);
> >>> > + temp_int_reg = SBTSI_REG_TEMP_LOW_INT;
> >>> > + temp_dec_reg = SBTSI_REG_TEMP_LOW_DEC;
> >>> > break;
> >>> > default:
> >>> > return -EINVAL;
> >>> > }
> >>> >
> >>> > + /*
> >>> > + * ReadOrder bit specifies the reading order of integer and
> >>> > + * decimal part of CPU temp for atomic reads. If bit == 0,
> >>> > + * reading integer part triggers latching of the decimal part,
> >>> > + * so integer part should be read first. If bit == 1, read
> >>> > + * order should be reversed.
> >>> > + */
> >>> > + err = i2c_smbus_read_byte_data(data->client, SBTSI_REG_CONFIG);
> >>> > + if (err < 0)
> >>> > + return err;
> >>> > +
> >>> > + mutex_lock(&data->lock);
> >>> > + if (err & BIT(SBTSI_CONFIG_READ_ORDER_SHIFT)) {
> >>> > + temp_dec = i2c_smbus_read_byte_data(data->client, temp_dec_reg);
> >>> > + temp_int = i2c_smbus_read_byte_data(data->client, temp_int_reg);
> >>> > + } else {
> >>> > + temp_int = i2c_smbus_read_byte_data(data->client, temp_int_reg);
> >>> > + temp_dec = i2c_smbus_read_byte_data(data->client, temp_dec_reg);
> >>> > + }
> >>> > + mutex_unlock(&data->lock);
> >>> >
> >>> > if (temp_int < 0)
> >>> > return temp_int;
> >>> >
> >>>
> >>
>
Powered by blists - more mailing lists