Hi, Guenter,
Thanks for reviewing the patches.
I will address the comments in next version. And also some comments
below.
On Fri, 2022-11-11 at 13:34 -0800, Guenter Roeck wrote:
> >
> > /* Keep track of how many zone pointers we allocated in init() */
> > static int max_zones __read_mostly;
> > @@ -150,8 +150,17 @@ static ssize_t show_ttarget(struct device
> > *dev,
> > {
> > struct sensor_device_attribute *attr =
> > to_sensor_dev_attr(devattr);
> > struct platform_data *pdata = dev_get_drvdata(dev);
> > + struct temp_data *tdata = pdata->core_data[attr->index];
> > + int ttarget;
> > +
> > + mutex_lock(&tdata->update_lock);
>
> Is that mutex really necessary ? I don't immediately see the need
> for it.
I just followed the same pattern as show_crit_alarm().
I checked the history and it was introduced by commit 723f573433b2
("hwmon: (coretemp) Fixup target cpu for package when cpu is
offlined"), to make sure the msr is not running on an offlined cpu.
> > +
> > + /*
> > + * ttarget is valid only if tjmax can be retrieved from
> > + * MSR_IA32_TEMPERATURE_TARGET
> > + */
> > + if (tdata->tjmax)
> > + return -ENODEV;
> > +
> > + if (c->x86_model <= 0xe || c->x86_model == 0x1c)
> > + return -ENODEV;
> > +
>
> Does it really make sense to re-check this each time the target
> temperature
> is read ?
You're right. We can keep this as it was, when deciding whether to
create this sysfs attr or not. Then the check in get_ttarget() can be
removed.
thanks,
rui
