Re: [PATCH 3/3] hwmon (coretemp): Add support for dynamic ttarget

[Guenter Roeck <linux@roeck-us.net>]

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
>