Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Subject: Re: [PATCH v3 02/19] hwmon: (mr75203) fix VM sensor allocation when "intel,
 vm-map" not defined
Message-ID: <644bd790-ce9d-822b-50a1-ad4e94ec0958@amazon.com>
Date:   Wed, 31 Aug 2022 07:36:20 +0300
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.2.0
Content-Language: en-US
To:     Guenter Roeck <linux@roeck-us.net>, <jdelvare@suse.com>,
        <robh+dt@kernel.org>, <p.zabel@pengutronix.de>,
        <rtanwar@maxlinear.com>, <linux-hwmon@vger.kernel.org>,
        <devicetree@vger.kernel.org>, <linux-kernel@vger.kernel.org>
CC:     <talel@amazon.com>, <hhhawa@amazon.com>, <jonnyc@amazon.com>,
        <hanochu@amazon.com>, <ronenk@amazon.com>, <itamark@amazon.com>,
        <shellykz@amazon.com>, <shorer@amazon.com>, <amitlavi@amazon.com>,
        <almogbs@amazon.com>, <dkl@amazon.com>,
        <andriy.shevchenko@intel.com>, "Farber, Eliav" <farbere@amazon.com>
References: <20220830192212.28570-1-farbere@amazon.com>
 <20220830192212.28570-3-farbere@amazon.com>
 <bd510d56-b161-ff17-8a09-ed4a8a66ca57@roeck-us.net>
From:   "Farber, Eliav" <farbere@amazon.com>
In-Reply-To: <bd510d56-b161-ff17-8a09-ed4a8a66ca57@roeck-us.net>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk

On 8/31/2022 5:39 AM, Guenter Roeck wrote:
> On 8/30/22 12:21, Eliav Farber wrote:
>> Bug fix - in case "intel,vm-map" is missing in device-tree ,'num' is set
>> to 0, and no voltage channel infos are allocated.
>>
>> Signed-off-by: Eliav Farber <farbere@amazon.com>
>> ---
>>   drivers/hwmon/mr75203.c | 28 ++++++++++++----------------
>>   1 file changed, 12 insertions(+), 16 deletions(-)
>>
>> diff --git a/drivers/hwmon/mr75203.c b/drivers/hwmon/mr75203.c
>> index 046523d47c29..0e29877a1a9c 100644
>> --- a/drivers/hwmon/mr75203.c
>> +++ b/drivers/hwmon/mr75203.c
>> @@ -580,8 +580,6 @@ static int mr75203_probe(struct platform_device 
>> *pdev)
>>       }
>>
>>       if (vm_num) {
>> -             u32 num = vm_num;
>> -
>>               ret = pvt_get_regmap(pdev, "vm", pvt);
>>               if (ret)
>>                       return ret;
>> @@ -594,30 +592,28 @@ static int mr75203_probe(struct platform_device 
>> *pdev)
>>               ret = device_property_read_u8_array(dev, "intel,vm-map",
>> pvt->vm_idx, vm_num);
>>               if (ret) {
>> -                     num = 0;
>> +                     /*
>> +                      * Incase intel,vm-map property is not defined, we
>> +                      * assume incremental channel numbers.
>> +                      */
>> +                     for (i = 0; i < vm_num; i++)
>> +                             pvt->vm_idx[i] = i;
>>               } else {
>>                       for (i = 0; i < vm_num; i++)
>>                               if (pvt->vm_idx[i] >= vm_num ||
>> -                                 pvt->vm_idx[i] == 0xff) {
>> -                                     num = i;
>> +                                 pvt->vm_idx[i] == 0xff)
>>                                       break;
>> -                             }
>> -             }
>>
>> -             /*
>> -              * Incase intel,vm-map property is not defined, we assume
>> -              * incremental channel numbers.
>> -              */
>> -             for (i = num; i < vm_num; i++)
>> -                     pvt->vm_idx[i] = i;
>> +                     vm_num = i;
>> +             }
>>
>
> So this is actually a functional change: In the old code, unspecified 
> channel
> numbers (num ... vm_num) were filled with incremental channel numbers.
> This is no longer the case.
>
The only place that uses pvt->vm_idx[] besides setting it in the probe
function is pvr_read_in().
It is quite clear from pvr_read_in() that unspecified channel numbers
(num ... vm_num) are not accessed, therefore there is also no need to
set them.

     if (channel >= pvt->v_num)
         return -EINVAL;

     vm_idx = pvt->vm_idx[channel];

Therefore I removed the setting of unspecified channels, and only if
“intel,vm-map” property is not defined, I set the specified channels
in incremental order.

>> -             in_config = devm_kcalloc(dev, num + 1,
>> +             in_config = devm_kcalloc(dev, vm_num + 1,
>>                                        sizeof(*in_config), GFP_KERNEL);
>
> The relevant difference (and maybe bug in the existing code ?) seems 
> to be
> this change. Have you considered leaving everything else in place and 
> only
> changing this code as well as the num -> vm_num changes below ?

Yes, using vm_num instead of num (which will be 0 if “intel,vm-map”
property is not defined) is the actual fix.
Both changes seemed to me to be coupled but if you think it will be
more clear to split this patch to two, first that removes unnecessary
setting of unspecified channels, and second that changes num to vm_num
for in_config, then sure I will do it.

--
Thanks, Eliav