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;
Sender: Guenter Roeck <groeck7@gmail.com>
Date:   Thu, 1 Sep 2022 07:44:34 -0700
From:   Guenter Roeck <linux@roeck-us.net>
To:     "Farber, Eliav" <farbere@amazon.com>
Cc:     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,
        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
Subject: Re: [PATCH v3 02/19] hwmon: (mr75203) fix VM sensor allocation when
 "intel, vm-map" not defined
Message-ID: <20220901144434.GB3477025@roeck-us.net>
References: <20220830192212.28570-1-farbere@amazon.com>
 <20220830192212.28570-3-farbere@amazon.com>
 <cddebb5a-3b83-e89d-db00-9a59ddbd6741@roeck-us.net>
 <84a68eff-be64-71ce-1533-1e228d3da2a4@amazon.com>
 <71d6d57c-2165-5fe3-515d-9395022921e2@roeck-us.net>
 <2f5c5828-87b9-f3d2-e3d3-0200adbe830c@amazon.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <2f5c5828-87b9-f3d2-e3d3-0200adbe830c@amazon.com>
Precedence: bulk

On Thu, Sep 01, 2022 at 11:39:58AM +0300, Farber, Eliav wrote:
> On 8/31/2022 2:48 PM, Guenter Roeck wrote:
> > On 8/30/22 22:49, Farber, Eliav wrote:
> > > On 8/31/2022 8:36 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;
> > > > 
> > > > So all vm_idx values from 0x00 to 0xfe would be acceptable ?
> > > > Does the chip really have that many registers (0x200 + 0x40 +
> > > > 0x200 * 0xfe) ?
> > > > Is that documented somewhere ?
> > > According to the code vm_num is limited to 32 because the mask is
> > > only 5 bits:
> > > 
> > > #define VM_NUM_MSK    GENMASK(20, 16)
> > > #define VM_NUM_SFT    16
> > > vm_num = (val & VM_NUM_MSK) >> VM_NUM_SFT;
> > > 
> > > In practice according to the data sheet I have:
> > > 0 <= VM instances <= 8
> > > 
> > Sorry, my bad. I misread the patch and thought the first part of
> > the if statement was removed.
> > 
> > Anyway, what is the difference between specifying an vm_idx value of
> > 0xff and not specifying anything ? Or, in other words, taking the dt
> > example, the difference between
> >        intel,vm-map = [03 01 04 ff ff];
> > and
> >        intel,vm-map = [03 01 04];
> 
> The actual number of VMs is read from a HW register:
>     ret = regmap_read(pvt->c_map, PVT_IP_CONFIG, &val);
>     ...
>     vm_num = (val & VM_NUM_MSK) >> VM_NUM_SFT;
> 
> Also, using:
>     ret = device_property_read_u8_array(dev, "intel,vm-map", vm_idx,
>                         vm_num);
> in the driver will fail if vm_num > sizeof array in device-tree.
> 
> So, if for example vm_num = 5, but you will want to map only 3 of them
> you most set property to be:
>     intel,vm-map = [03 01 04 ff ff];
> otherwise if you set:
>     intel,vm-map = [03 01 04];
> it will assume the property doesn't, and will continue the flow in code
> as if it doesn’t exist (which is not what the user wanted, and before my
> fix also has a bug).

There should be some error handling to catch this case (ie if the number
of entries does not match the expected count), or if a value in the array
is larger or equal to vm_num. Today the latter is silently handled as end
of entries (similar to 0xff), but that should result in an error.
This would avoid situations like
	intel,vm-map = [01 02 03 04 05];
ie where the person writing the devicetree file accidentally entered
index values starting with 1 instead of 0. A mismatch between vm_num
and the number of entries in the array is silently handled as if there
was no property at all, which is at the very least misleading and
most definitely unexpected and should also result in an error.

Also, what happens if the devicetree content is something like the
following ? Would that be valid ?
	intel,vm-map = [00 01 01 01 01 01];

Thanks,
Guenter