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? 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; > > > > > >
