Re: [PATCH v3 02/19] hwmon: (mr75203) fix VM sensor allocation when "intel, vm-map" not defined

Guenter Roeck <linux@xxxxxxxxxxxx> · Thu, 1 Sep 2022 12:25:16 -0700

On 9/1/22 11:36, Farber, Eliav wrote:
On 9/1/2022 8:11 PM, Guenter Roeck wrote:
CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe.

On 9/1/22 08:24, Farber, Eliav wrote:
On 9/1/2022 5:44 PM, Guenter Roeck wrote:
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@xxxxxxxxxx>
> > > > ---
> > > >   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.

I assume it is possible to tell according to the return value, if property
doesn’t exist at all, or if it does exists and size of array in
device-tree is smaller than vm_num.
In [PATCH v3 17/19] Andy wrote that “code shouldn't be a YAML validator.
Drop this and make sure you have correct DT schema” so I’m a bit confused
if code should validate “intel,bm-map” or if it is the user responsibility.
As this property was not added by me, I prefer not to fix it as part of
this series of patches.

You are changing the driver all over the place with 19 patches, including
this code, but you don't want to add code that validates the devicetree
data ? That seems odd.

OK. I have added patch #20 to validate that same VM index doesn't appear
more than once in intel,vm-map.

u32 vm_mask = 0;

for (i = 0; i < vm_num; i++) {
     if (vm_idx[i] >= vm_num || vm_idx[i] == 0xff)

I think "vm_idx[i] >= vm_num && vm_idx[i] != 0xff)
should also be invalid, ie.

	if (vm_idx[i] == 0xff)
		break;
	if (vm_idx[i] >= vm_num)
		return -EINVAL;

Thanks,
Guenter

         break;

     if (vm_mask & BIT(vm_idx[i])) {
         dev_err(dev, "Same VM appears more than once in intel,vm-map\n",
             vm_idx[i]);
         return EINVAL;
     }

     vm_mask |= BIT(vm_idx[i]);
}

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];

If device-tree content would be:
     intel,vm-map = [00 01 01 01 01 01];
and assuming 16 channels for each VM, the hwmon sub-system will expose 90
sysfs to read voltage values.
In practice 16 – 31, 32 – 47, 48 – 63, 64 – 89 will all report the same
input signals for VM1.

Does that make any sense, and is there a valid reason to have a mapping
table like the one in this example ?

I can't find any sense in having such a mapping.
Anyway the new patch will not allow it to happen.

--
Regards, Eliav