Re: [PATCH v3 09/10] drivers/hwmon: Add PECI hwmon client drivers

[Jae Hyun Yoo]

On 4/12/2018 10:37 AM, Guenter Roeck wrote:
> On Thu, Apr 12, 2018 at 10:09:51AM -0700, Jae Hyun Yoo wrote:
> [ ... ]
> > > > > > > > +static int find_core_index(struct peci_cputemp *priv, int channel)
> > > > > > > > +{
> > > > > > > > +ÂÂÂ int core_channel = channel - DEFAULT_CHANNEL_NUMS;
> > > > > > > > +ÂÂÂ int idx, found = 0;
> > > > > > > > +
> > > > > > > > +ÂÂÂ for (idx = 0; idx < priv->gen_info->core_max; idx++) {
> > > > > > > > +ÂÂÂÂÂÂÂ if (priv->core_mask & BIT(idx)) {
> > > > > > > > +ÂÂÂÂÂÂÂÂÂÂÂ if (core_channel == found)
> > > > > > > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ break;
> > > > > > > > +
> > > > > > > > +ÂÂÂÂÂÂÂÂÂÂÂ found++;
> > > > > > > > +ÂÂÂÂÂÂÂ }
> > > > > > > > +ÂÂÂ }
> > > > > > > > +
> > > > > > > > +ÂÂÂ return idx;
> > > > > > >
> > > > > > > What if nothing is found ?
> > > > > >
> > > > > > Core temperature group will be registered only when it detects at
> > > > > > least one core checked by check_resolved_cores(), so
> > > > > > find_core_index() can be called only when priv->core_mask has a
> > > > > > non-zero value. The 'nothing is found' case will not happen.
> > > > > That doesn't guarantee a match. If what you are saying is correct
> > > > > there should always be
> > > > > a well defined match of channel -> idx, and the search should be
> > > > > unnecessary.
> > > >
> > > > There could be some disabled cores in the resolved core mask bit
> > > > sequence also it should remove indexing gap in channel numbering so it
> > > > is the reason why this search function is needed. Well defined match of
> > > > channel -> idx would not be always satisfied.
> > > Are you saying that each call to the function, with the same parameters,
> > > can return a different result ?
> >
> > No, the result will be consistent. After reading the priv->core_mask once in
> > check_resolved_cores(), the value will not be changed. I'm saying about this
> > case, for example if core number 2 is unresolved in total 4 cores, then the
> > idx order will be '0, 1, 3' but channel order will be '5, 6, 7' without
> > making any indexing gap.
>
> And you yet you claim that this is not well defined ? Or are you concerned
> about the amount of memory consumed by providing an array for the mapping ?
>
> Note that an indexing gap is acceptable and, in many cases, preferred.

If the indexing gap is acceptable, the index search function isn't 
needed anymore. I'll fix all relating code to make that use direct 
mapping of channel -> idx then. Thanks!

> [ ... ]
>
> > > > > > > > +
> > > > > > > > +ÂÂÂ dev_dbg(dev, "%s: sensor '%s'\n", dev_name(hwmon_dev),
> > > > > > > > priv->name);
> > > > > > > > +
> > > > >
> > > > > Why does this message display the device name twice ?
> > > >
> > > > For an example, dev_name(hwmon_dev) shows 'hwmon5' and priv->name shows
> > > > 'peci-cputemp0'.
> > > And dev_dbg() shows another device name. So you'll have something like
> > >
> > > peci-cputemp0: hwmon5: sensor 'peci-cputemp0'
> >
> > Practically it shows like
> >
> > peci-cputemp 0-30:00: hwmon10: sensor 'peci_cputemp.cpu0'
> >
> > where 0-30:00 is assigned by peci core.
>
> And what message would you see for cpu1 ?

It shows like

peci-cputemp 0-31:00: hwmon10: sensor 'peci_cputemp.cpu1'