| Message ID | 20220830192212.28570-3-farbere@amazon.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Variety of fixes and new features for mr75203 driver | expand |
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. > - 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 ? Thanks, Guenter > if (!in_config) > return -ENOMEM; > > - memset32(in_config, HWMON_I_INPUT, num); > - in_config[num] = 0; > + memset32(in_config, HWMON_I_INPUT, vm_num); > + in_config[vm_num] = 0; > pvt_in.config = in_config; > > pvt_info[index++] = &pvt_in;
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
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 ? Thanks, Guenter > - } > - } > > - /* > - * 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; > + } > > - in_config = devm_kcalloc(dev, num + 1, > + in_config = devm_kcalloc(dev, vm_num + 1, > sizeof(*in_config), GFP_KERNEL); > if (!in_config) > return -ENOMEM; > > - memset32(in_config, HWMON_I_INPUT, num); > - in_config[num] = 0; > + memset32(in_config, HWMON_I_INPUT, vm_num); > + in_config[vm_num] = 0; > pvt_in.config = in_config; > > pvt_info[index++] = &pvt_in;
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 -- Regards, Eliav
On 8/31/2022 8:49 AM, 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: > For 5 bit mask, maximum is of course 31 and not 32. > #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 > > -- > Regards, Eliav
On Tue, Aug 30, 2022 at 07:21:55PM +0000, 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. Care to provide a Fixes tag for all fixes in your series. Also don't forget to start series with fixes followed by cleanups and new features..
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]; Thanks, Guenter
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). -- Regards, Eliav
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
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@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. 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. > 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. -- Regards, Eliav
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@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. > > > 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. > >> 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 ? Thanks, Guenter
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@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. >> >> >> 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
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@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. >>> >>> >>> 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 >
On Thu, Sep 01, 2022 at 06:24:51PM +0300, 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: ... > > 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. I'm just a messenger of Rob's words. If I'm mistaken I would like Rob to correct me.
On 8/31/2022 12:38 PM, Andy Shevchenko wrote: > On Tue, Aug 30, 2022 at 07:21:55PM +0000, 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. > > Care to provide a Fixes tag for all fixes in your series. Also don't > forget to > start series with fixes followed by cleanups and new features.. For v4 I provided a Fixes tag where it was relevant. I also changed the order of some patches so that all fixes will be first. -- Thanks, Eliav
On Fri, Sep 02, 2022 at 03:08:41PM +0300, Farber, Eliav wrote: > On 8/31/2022 12:38 PM, Andy Shevchenko wrote: > > On Tue, Aug 30, 2022 at 07:21:55PM +0000, 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. > > > > Care to provide a Fixes tag for all fixes in your series. Also don't > > forget to > > start series with fixes followed by cleanups and new features.. > For v4 I provided a Fixes tag where it was relevant. Thanks! > I also changed the order of some patches so that all fixes will be first. Note, fixes should prepend the other patches in the series.
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; + } - in_config = devm_kcalloc(dev, num + 1, + in_config = devm_kcalloc(dev, vm_num + 1, sizeof(*in_config), GFP_KERNEL); if (!in_config) return -ENOMEM; - memset32(in_config, HWMON_I_INPUT, num); - in_config[num] = 0; + memset32(in_config, HWMON_I_INPUT, vm_num); + in_config[vm_num] = 0; pvt_in.config = in_config; pvt_info[index++] = &pvt_in;
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(-)