lkml.org 
[lkml]   [2022]   [Aug]   [31]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [PATCH v3 02/19] hwmon: (mr75203) fix VM sensor allocation when "intel, vm-map" not defined
    Date
    From
    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

    \
     
     \ /
      Last update: 2022-08-31 06:37    [W:4.121 / U:0.008 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site