Fix voltage allocation and reading to support all channels in all VMs.
Prior to this change allocation and reading were done only for the first
channel in each VM.
This change counts the total number of channels for allocation, and takes
into account the channel offset when reading the sample data register.

Fixes: 9d823351a337 ("hwmon: Add hardware monitoring driver for Moortec MR75203 PVT controller")
Signed-off-by: Eliav Farber <farbere@amazon.com>
---
V4 -> V3:
- Keep lines sorted according to length.

V3 -> V2:
- Remove configuration of ip-polling register to a separate commit.
- Explain the fix.

 drivers/hwmon/mr75203.c | 29 +++++++++++++++++------------
 1 file changed, 17 insertions(+), 12 deletions(-)

diff --git a/drivers/hwmon/mr75203.c b/drivers/hwmon/mr75203.c
index 78dc471e843c..69f38c05b02d 100644
--- a/drivers/hwmon/mr75203.c
+++ b/drivers/hwmon/mr75203.c
@@ -68,8 +68,9 @@
 
 /* VM Individual Macro Register */
 #define VM_COM_REG_SIZE	0x200
-#define VM_SDIF_DONE(n)	(VM_COM_REG_SIZE + 0x34 + 0x200 * (n))
-#define VM_SDIF_DATA(n)	(VM_COM_REG_SIZE + 0x40 + 0x200 * (n))
+#define VM_SDIF_DONE(vm)	(VM_COM_REG_SIZE + 0x34 + 0x200 * (vm))
+#define VM_SDIF_DATA(vm, ch)	\
+	(VM_COM_REG_SIZE + 0x40 + 0x200 * (vm) + 0x4 * (ch))
 
 /* SDA Slave Register */
 #define IP_CTRL			0x00
@@ -115,6 +116,7 @@ struct pvt_device {
 	u32			t_num;
 	u32			p_num;
 	u32			v_num;
+	u32			c_num;
 	u32			ip_freq;
 	u8			*vm_idx;
 };
@@ -178,14 +180,15 @@ static int pvt_read_in(struct device *dev, u32 attr, int channel, long *val)
 {
 	struct pvt_device *pvt = dev_get_drvdata(dev);
 	struct regmap *v_map = pvt->v_map;
+	u8 vm_idx, ch_idx;
 	u32 n, stat;
-	u8 vm_idx;
 	int ret;
 
-	if (channel >= pvt->v_num)
+	if (channel >= pvt->v_num * pvt->c_num)
 		return -EINVAL;
 
-	vm_idx = pvt->vm_idx[channel];
+	vm_idx = pvt->vm_idx[channel / pvt->c_num];
+	ch_idx = channel % pvt->c_num;
 
 	switch (attr) {
 	case hwmon_in_input:
@@ -196,7 +199,7 @@ static int pvt_read_in(struct device *dev, u32 attr, int channel, long *val)
 		if (ret)
 			return ret;
 
-		ret = regmap_read(v_map, VM_SDIF_DATA(vm_idx), &n);
+		ret = regmap_read(v_map, VM_SDIF_DATA(vm_idx, ch_idx), &n);
 		if (ret < 0)
 			return ret;
 
@@ -499,8 +502,8 @@ static int pvt_reset_control_deassert(struct device *dev, struct pvt_device *pvt
 
 static int mr75203_probe(struct platform_device *pdev)
 {
+	u32 ts_num, vm_num, pd_num, ch_num, val, index, i;
 	const struct hwmon_channel_info **pvt_info;
-	u32 ts_num, vm_num, pd_num, val, index, i;
 	struct device *dev = &pdev->dev;
 	u32 *temp_config, *in_config;
 	struct device *hwmon_dev;
@@ -541,9 +544,11 @@ static int mr75203_probe(struct platform_device *pdev)
 	ts_num = (val & TS_NUM_MSK) >> TS_NUM_SFT;
 	pd_num = (val & PD_NUM_MSK) >> PD_NUM_SFT;
 	vm_num = (val & VM_NUM_MSK) >> VM_NUM_SFT;
+	ch_num = (val & CH_NUM_MSK) >> CH_NUM_SFT;
 	pvt->t_num = ts_num;
 	pvt->p_num = pd_num;
 	pvt->v_num = vm_num;
+	pvt->c_num = ch_num;
 	val = 0;
 	if (ts_num)
 		val++;
@@ -580,7 +585,7 @@ static int mr75203_probe(struct platform_device *pdev)
 	}
 
 	if (vm_num) {
-		u32 num = vm_num;
+		u32 total_ch;
 
 		ret = pvt_get_regmap(pdev, "vm", pvt);
 		if (ret)
@@ -604,20 +609,20 @@ static int mr75203_probe(struct platform_device *pdev)
 			for (i = 0; i < vm_num; i++)
 				if (pvt->vm_idx[i] >= vm_num ||
 				    pvt->vm_idx[i] == 0xff) {
-					num = i;
 					pvt->v_num = i;
 					vm_num = i;
 					break;
 				}
 		}
 
-		in_config = devm_kcalloc(dev, num + 1,
+		total_ch = ch_num * vm_num;
+		in_config = devm_kcalloc(dev, total_ch + 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, total_ch);
+		in_config[total_ch] = 0;
 		pvt_in.config = in_config;
 
 		pvt_info[index++] = &pvt_in;
-- 
2.37.1

On Tue, Sep 06, 2022 at 08:33:41AM +0000, Eliav Farber wrote:
> Fix voltage allocation and reading to support all channels in all VMs.
> Prior to this change allocation and reading were done only for the first
> channel in each VM.
> This change counts the total number of channels for allocation, and takes
> into account the channel offset when reading the sample data register.

...

> +		total_ch = ch_num * vm_num;
> +		in_config = devm_kcalloc(dev, total_ch + 1,
>  					 sizeof(*in_config), GFP_KERNEL);

Strictly speaking this should be `size_add(size_mul(...) ...)` construction
from overflow.h.

		total_ch = size_mul(ch_num, vm_num);
		in_config = devm_kcalloc(dev, size_add(total_ch, 1),
					 sizeof(*in_config), GFP_KERNEL);

Alternatively before doing all these, add a check

		if (array3_size(ch_num, vm_num, sizeof(*in_config)) < SIZE_MAX - sizeof(*in_config))
			return -EOVERFLOW;

But this is a bit monstrous. Seems like the above looks and feels better.

Also for backporting purposes perhaps it's fine to do without using those macro
helpers.

>  		if (!in_config)
>  			return -ENOMEM;


-- 
With Best Regards,
Andy Shevchenko

On 9/6/2022 5:10 PM, Andy Shevchenko wrote:
> On Tue, Sep 06, 2022 at 08:33:41AM +0000, Eliav Farber wrote:
>> Fix voltage allocation and reading to support all channels in all VMs.
>> Prior to this change allocation and reading were done only for the first
>> channel in each VM.
>> This change counts the total number of channels for allocation, and 
>> takes
>> into account the channel offset when reading the sample data register.
>
> ...
>
>> +             total_ch = ch_num * vm_num;
>> +             in_config = devm_kcalloc(dev, total_ch + 1,
>>                                        sizeof(*in_config), GFP_KERNEL);
>
> Strictly speaking this should be `size_add(size_mul(...) ...)` 
> construction
> from overflow.h.
>
>                total_ch = size_mul(ch_num, vm_num);
>                in_config = devm_kcalloc(dev, size_add(total_ch, 1),
>                                         sizeof(*in_config), GFP_KERNEL);
>
> Alternatively before doing all these, add a check
>
>                if (array3_size(ch_num, vm_num, sizeof(*in_config)) < 
> SIZE_MAX - sizeof(*in_config))
>                        return -EOVERFLOW;
>
> But this is a bit monstrous. Seems like the above looks and feels better.
>
> Also for backporting purposes perhaps it's fine to do without using 
> those macro
> helpers. 
According to the driver code total_ch is a u32 variable while vm_num
and ch_num are both limited to a value of 31:

#define VM_NUM_MSK GENMASK(20, 16)
#define VM_NUM_SFT 16
#define CH_NUM_MSK GENMASK(31, 24)
#define CH_NUM_SFT 24

In addition the PVT Controller Series 3+ Specification mentions that
the actual maximum values are even smaller – 8 for vm_num and 16 for
ch_num.
Therefore we are very far from a scenario of an overflow.
Do you still think overflow protection in necessary?

--
Thanks, Eliav

On Wed, Sep 07, 2022 at 08:15:36AM +0300, Farber, Eliav wrote:
> On 9/6/2022 5:10 PM, Andy Shevchenko wrote:
> > On Tue, Sep 06, 2022 at 08:33:41AM +0000, Eliav Farber wrote:

...

> > > +             total_ch = ch_num * vm_num;
> > > +             in_config = devm_kcalloc(dev, total_ch + 1,
> > >                                        sizeof(*in_config), GFP_KERNEL);
> > 
> > Strictly speaking this should be `size_add(size_mul(...) ...)`
> > construction
> > from overflow.h.
> > 
> >                total_ch = size_mul(ch_num, vm_num);
> >                in_config = devm_kcalloc(dev, size_add(total_ch, 1),
> >                                         sizeof(*in_config), GFP_KERNEL);
> > 
> > Alternatively before doing all these, add a check
> > 
> >                if (array3_size(ch_num, vm_num, sizeof(*in_config)) <
> > SIZE_MAX - sizeof(*in_config))
> >                        return -EOVERFLOW;
> > 
> > But this is a bit monstrous. Seems like the above looks and feels better.
> > 
> > Also for backporting purposes perhaps it's fine to do without using
> > those macro
> > helpers.
> According to the driver code total_ch is a u32 variable while vm_num
> and ch_num are both limited to a value of 31:
> 
> #define VM_NUM_MSK GENMASK(20, 16)
> #define VM_NUM_SFT 16
> #define CH_NUM_MSK GENMASK(31, 24)
> #define CH_NUM_SFT 24
> 
> In addition the PVT Controller Series 3+ Specification mentions that
> the actual maximum values are even smaller – 8 for vm_num and 16 for
> ch_num.
> Therefore we are very far from a scenario of an overflow.
> Do you still think overflow protection in necessary?

Like I said "Strictly..." Means it's up to you, but allocations are
usually be protected against the overflows.

-- 
With Best Regards,
Andy Shevchenko