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
