On Sat, Nov 12, 2022 at 04:37:50PM +0800, Zhang Rui wrote:
> 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.
>
Good point. I am not sure if it matters at that point if the code
uses the old or the new CPU, but I guess it is safer.
Thanks for the clarification,
Guenter
> > > +
> > > + /*
> > > + * 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
>
