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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 12 Apr 2018 12:51:10 -0700
From:   Jae Hyun Yoo <jae.hyun.yoo@...ux.intel.com>
To:     Guenter Roeck <linux@...ck-us.net>
Cc:     Alan Cox <alan@...ux.intel.com>, Andrew Jeffery <andrew@...id.au>,
        Andrew Lunn <andrew@...n.ch>,
        Andy Shevchenko <andriy.shevchenko@...el.com>,
        Arnd Bergmann <arnd@...db.de>,
        Benjamin Herrenschmidt <benh@...nel.crashing.org>,
        Fengguang Wu <fengguang.wu@...el.com>,
        Greg KH <gregkh@...uxfoundation.org>,
        Haiyue Wang <haiyue.wang@...ux.intel.com>,
        James Feist <james.feist@...ux.intel.com>,
        Jason M Biils <jason.m.bills@...ux.intel.com>,
        Jean Delvare <jdelvare@...e.com>,
        Joel Stanley <joel@....id.au>,
        Julia Cartwright <juliac@....teric.us>,
        Miguel Ojeda <miguel.ojeda.sandonis@...il.com>,
        Milton Miller II <miltonm@...ibm.com>,
        Pavel Machek <pavel@....cz>,
        Randy Dunlap <rdunlap@...radead.org>,
        Stef van Os <stef.van.os@...drive-technologies.com>,
        Sumeet R Pawnikar <sumeet.r.pawnikar@...el.com>,
        Vernon Mauery <vernon.mauery@...ux.intel.com>,
        linux-kernel@...r.kernel.org, linux-doc@...r.kernel.org,
        devicetree@...r.kernel.org, linux-hwmon@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org, openbmc@...ts.ozlabs.org
Subject: Re: [PATCH v3 09/10] drivers/hwmon: Add PECI hwmon client drivers

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'