Re: [PATCH v3 02/19] hwmon: (mr75203) fix VM sensor allocation when "intel, vm-map" not defined
From: Farber, Eliav
Date: Thu Sep 01 2022 - 14:37:53 EST
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)
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