linux-kernel - Re: [PATCH v4 2/2] Add hardware monitoring driver for Moortec MR75203 PVT controller

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] [day] [month] [year] [list]

Message-ID: <CAHp75VcHk0qZ_pFAvZLC4+8=r7E=cqe3J+enB3UWD9X1KQQ4Ng@mail.gmail.com>
Date:   Mon, 5 Oct 2020 12:06:48 +0300
From:   Andy Shevchenko <andy.shevchenko@...il.com>
To:     "Tanwar, Rahul" <rahul.tanwar@...ux.intel.com>
Cc:     Andy Shevchenko <andriy.shevchenko@...el.com>,
        Jean Delvare <jdelvare@...e.com>,
        Guenter Roeck <linux@...ck-us.net>,
        Philipp Zabel <p.zabel@...gutronix.de>,
        linux-hwmon@...r.kernel.org, Rob Herring <robh+dt@...nel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        devicetree <devicetree@...r.kernel.org>, songjun.Wu@...el.com,
        cheol.yong.kim@...el.com, qi-ming.wu@...el.com,
        rtanwar@...linear.com
Subject: Re: [PATCH v4 2/2] Add hardware monitoring driver for Moortec MR75203
 PVT controller

On Mon, Oct 5, 2020 at 11:53 AM Tanwar, Rahul
<rahul.tanwar@...ux.intel.com> wrote:
> On 3/10/2020 2:11 am, Andy Shevchenko wrote:
> > On Fri, Oct 02, 2020 at 03:04:27PM +0800, Rahul Tanwar wrote:

...

> >> +            pvt_temp.config = temp_config;
> >> +
> >> +            pvt_info[index++] = &pvt_temp;
> >> +    }
> >> +
> >> +    if (pd_num) {
> >> +            ret = pvt_get_regmap(pdev, "pd", pvt);
> >> +            if (ret)
> >> +                    return ret;
> >> +    }
> >> +
> >> +    if (vm_num) {
> >> +            u32 num = vm_num;
> >> +
> >> +            ret = pvt_get_regmap(pdev, "vm", pvt);
> >> +            if (ret)
> >> +                    return ret;
> >> +
> >> +            pvt->vm_idx = devm_kcalloc(dev, vm_num, sizeof(*pvt->vm_idx),
> >> +                                       GFP_KERNEL);
> >> +            if (!pvt->vm_idx)
> >> +                    return -ENOMEM;
> >> +            for (i = 0; i < vm_num; i++)
> >> +                    pvt->vm_idx[i] = i;
> > What the point if you are replace them below in one case?
> >
> >> +            ret = device_property_read_u8_array(dev, "intel,vm-map",
> >> +                                                pvt->vm_idx, vm_num);
> >> +            if (!ret)
> > Misses {} and because of above
> >
> >       if (ret) {
> >               for () ...
> >       } else {
> >               for () ...
> >       }
> >
> >> +                    for (i = 0; i < vm_num; i++)
> >> +                            if (pvt->vm_idx[i] >= vm_num ||
> >> +                                pvt->vm_idx[i] == 0xff) {
> >> +                                    num = i;
> >> +                                    break;
> >> +                            }
> > Or looking in this, perhaps move the incremental for-loop here and start it
> > with num which is 0.
>
> Not able to understand what exactly you are suggesting here. Presently
> it is like below:
> 1. Init vm_idx array with incremental values.
> 2. Read array from device property.
> 3. If success, figure out the last valid value and assign to num.
>
> Can you please elaborate and explain more clearly? Thanks.

device_property_read_u8_array() effectively (partially) rewrites the
vm_idx array.
The code above is inefficient and not clear.
My understanding based on the above is that half of the code may be dropped.

So, clearer variant looks like this to me:

  ret = device_property_read_u8_array(dev, "intel,vm-map", pvt->vm_idx, vm_num);
  if (ret) {
    num = 0;
  } else {
    for (i = 0; i < vm_num; i++) {
      if (pvt->vm_idx[i] >= vm_num || pvt->vm_idx[i] == 0xff)
        break;
    }
    num = i;
  }
  for (i = num; i < vm_num; i++)
    pvt->vm_idx[i] = i;

And all these require a good comment to describe why you are doing the
trailing loop.

-- 
With Best Regards,
Andy Shevchenko