Re: [PATCH v2] hwmon: (sbtsi) Don't read sensor more than once if it doesn't respond

From: Guenter Roeck
Date: Tue Apr 06 2021 - 19:28:52 EST

Next message: patchwork-bot+netdevbpf: "Re: [PATCH net-next v4 0/4] usbnet: speed reporting for devices without MDIO"
Previous message: Mathieu Poirier: "Re: [PATCH 1/3] remoteproc: pru: Fixup interrupt-parent logic for fw events"
In reply to: Konstantin Aladyshev: "Re: [PATCH v2] hwmon: (sbtsi) Don't read sensor more than once if it doesn't respond"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On 4/6/21 3:25 PM, Konstantin Aladyshev wrote:
> 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.
>

Why would that be necessary ? My understanding is that the returned value
is read-only and won't change. FWIW, I don't even understand why it is
read more than once to start with.

On a side note, the above code would leave the mutex locked on error.

Guenter

> Best regards,
> Konstantin Aladyshev
>
>
> On Wed, Apr 7, 2021 at 12:34 AM Guenter Roeck <linux@xxxxxxxxxxxx> 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@xxxxxxxxxxxx> 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@xxxxxxxxxxxx <mailto:linux@xxxxxxxxxxxx>> 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@xxxxxxxxx <mailto:aladyshev22@xxxxxxxxx>>
>>>>> > ---
>>>>> > 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;
>>>>> >
>>>>>
>>>>
>>

Next message: patchwork-bot+netdevbpf: "Re: [PATCH net-next v4 0/4] usbnet: speed reporting for devices without MDIO"
Previous message: Mathieu Poirier: "Re: [PATCH 1/3] remoteproc: pru: Fixup interrupt-parent logic for fw events"
In reply to: Konstantin Aladyshev: "Re: [PATCH v2] hwmon: (sbtsi) Don't read sensor more than once if it doesn't respond"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]