Remove array_size() calls and replace vmalloc() with vmalloc_array() to
simplify the code.

Signed-off-by: Qianfeng Rong <rongqianfeng@vivo.com>
---
 drivers/net/wireless/marvell/mwifiex/cfg80211.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/wireless/marvell/mwifiex/cfg80211.c b/drivers/net/wireless/marvell/mwifiex/cfg80211.c
index 3498743d5ec0..fb4183ff02a9 100644
--- a/drivers/net/wireless/marvell/mwifiex/cfg80211.c
+++ b/drivers/net/wireless/marvell/mwifiex/cfg80211.c
@@ -4673,8 +4673,8 @@ int mwifiex_init_channel_scan_gap(struct mwifiex_adapter *adapter)
 	 * additional active scan request for hidden SSIDs on passive channels.
 	 */
 	adapter->num_in_chan_stats = 2 * (n_channels_bg + n_channels_a);
-	adapter->chan_stats = vmalloc(array_size(sizeof(*adapter->chan_stats),
-						 adapter->num_in_chan_stats));
+	adapter->chan_stats = vmalloc_array(adapter->num_in_chan_stats,
+					    sizeof(*adapter->chan_stats));
 
 	if (!adapter->chan_stats)
 		return -ENOMEM;
-- 
2.34.1

On Tue, Aug 12, 2025 at 09:32:18PM +0800, Qianfeng Rong wrote:
> Remove array_size() calls and replace vmalloc() with vmalloc_array() to
> simplify the code.
> 
> Signed-off-by: Qianfeng Rong <rongqianfeng@vivo.com>
> ---
>  drivers/net/wireless/marvell/mwifiex/cfg80211.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/wireless/marvell/mwifiex/cfg80211.c b/drivers/net/wireless/marvell/mwifiex/cfg80211.c
> index 3498743d5ec0..fb4183ff02a9 100644
> --- a/drivers/net/wireless/marvell/mwifiex/cfg80211.c
> +++ b/drivers/net/wireless/marvell/mwifiex/cfg80211.c
> @@ -4673,8 +4673,8 @@ int mwifiex_init_channel_scan_gap(struct mwifiex_adapter *adapter)
>  	 * additional active scan request for hidden SSIDs on passive channels.
>  	 */
>  	adapter->num_in_chan_stats = 2 * (n_channels_bg + n_channels_a);
> -	adapter->chan_stats = vmalloc(array_size(sizeof(*adapter->chan_stats),
> -						 adapter->num_in_chan_stats));
> +	adapter->chan_stats = vmalloc_array(adapter->num_in_chan_stats,
> +					    sizeof(*adapter->chan_stats));

n_channels_bg is 14
n_channels_a is either 0 or 31 depending on if we're using BAND_A.
sizeof(*adapter->chan_stats) is 10.

So we're either allocating 280 or 900 bytes, which is quite small.  We
should just use kmalloc_array() instead of vmalloc_array().

regards,
dan carpenter

On 8/12/2025 6:48 AM, Dan Carpenter wrote:
> On Tue, Aug 12, 2025 at 09:32:18PM +0800, Qianfeng Rong wrote:
>> Remove array_size() calls and replace vmalloc() with vmalloc_array() to
>> simplify the code.
>>
>> Signed-off-by: Qianfeng Rong <rongqianfeng@vivo.com>
>> ---
>>  drivers/net/wireless/marvell/mwifiex/cfg80211.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/wireless/marvell/mwifiex/cfg80211.c b/drivers/net/wireless/marvell/mwifiex/cfg80211.c
>> index 3498743d5ec0..fb4183ff02a9 100644
>> --- a/drivers/net/wireless/marvell/mwifiex/cfg80211.c
>> +++ b/drivers/net/wireless/marvell/mwifiex/cfg80211.c
>> @@ -4673,8 +4673,8 @@ int mwifiex_init_channel_scan_gap(struct mwifiex_adapter *adapter)
>>  	 * additional active scan request for hidden SSIDs on passive channels.
>>  	 */
>>  	adapter->num_in_chan_stats = 2 * (n_channels_bg + n_channels_a);
>> -	adapter->chan_stats = vmalloc(array_size(sizeof(*adapter->chan_stats),
>> -						 adapter->num_in_chan_stats));
>> +	adapter->chan_stats = vmalloc_array(adapter->num_in_chan_stats,
>> +					    sizeof(*adapter->chan_stats));
> 
> n_channels_bg is 14
> n_channels_a is either 0 or 31 depending on if we're using BAND_A.
> sizeof(*adapter->chan_stats) is 10.
> 
> So we're either allocating 280 or 900 bytes, which is quite small.  We
> should just use kmalloc_array() instead of vmalloc_array().

Should transition from v*() to k*() be separate from transition from *malloc()
to *malloc_array()?

/jeff

On Wed, Aug 13, 2025 at 11:35:45AM -0700, Jeff Johnson wrote:
> On 8/12/2025 6:48 AM, Dan Carpenter wrote:
> > On Tue, Aug 12, 2025 at 09:32:18PM +0800, Qianfeng Rong wrote:
> >> Remove array_size() calls and replace vmalloc() with vmalloc_array() to
> >> simplify the code.
> >>
> >> Signed-off-by: Qianfeng Rong <rongqianfeng@vivo.com>
> >> ---
> >>  drivers/net/wireless/marvell/mwifiex/cfg80211.c | 4 ++--
> >>  1 file changed, 2 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/net/wireless/marvell/mwifiex/cfg80211.c b/drivers/net/wireless/marvell/mwifiex/cfg80211.c
> >> index 3498743d5ec0..fb4183ff02a9 100644
> >> --- a/drivers/net/wireless/marvell/mwifiex/cfg80211.c
> >> +++ b/drivers/net/wireless/marvell/mwifiex/cfg80211.c
> >> @@ -4673,8 +4673,8 @@ int mwifiex_init_channel_scan_gap(struct mwifiex_adapter *adapter)
> >>  	 * additional active scan request for hidden SSIDs on passive channels.
> >>  	 */
> >>  	adapter->num_in_chan_stats = 2 * (n_channels_bg + n_channels_a);
> >> -	adapter->chan_stats = vmalloc(array_size(sizeof(*adapter->chan_stats),
> >> -						 adapter->num_in_chan_stats));
> >> +	adapter->chan_stats = vmalloc_array(adapter->num_in_chan_stats,
> >> +					    sizeof(*adapter->chan_stats));
> > 
> > n_channels_bg is 14
> > n_channels_a is either 0 or 31 depending on if we're using BAND_A.
> > sizeof(*adapter->chan_stats) is 10.
> > 
> > So we're either allocating 280 or 900 bytes, which is quite small.  We
> > should just use kmalloc_array() instead of vmalloc_array().
> 
> Should transition from v*() to k*() be separate from transition from *malloc()
> to *malloc_array()?

It doesn't make sense to split this up.  The right thing is kcalloc().

regards,
dan carpenter

在 2025/8/12 21:48, Dan Carpenter 写道:
> On Tue, Aug 12, 2025 at 09:32:18PM +0800, Qianfeng Rong wrote:
>> Remove array_size() calls and replace vmalloc() with vmalloc_array() to
>> simplify the code.
>>
>> Signed-off-by: Qianfeng Rong <rongqianfeng@vivo.com>
>> ---
>>   drivers/net/wireless/marvell/mwifiex/cfg80211.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/wireless/marvell/mwifiex/cfg80211.c b/drivers/net/wireless/marvell/mwifiex/cfg80211.c
>> index 3498743d5ec0..fb4183ff02a9 100644
>> --- a/drivers/net/wireless/marvell/mwifiex/cfg80211.c
>> +++ b/drivers/net/wireless/marvell/mwifiex/cfg80211.c
>> @@ -4673,8 +4673,8 @@ int mwifiex_init_channel_scan_gap(struct mwifiex_adapter *adapter)
>>   	 * additional active scan request for hidden SSIDs on passive channels.
>>   	 */
>>   	adapter->num_in_chan_stats = 2 * (n_channels_bg + n_channels_a);
>> -	adapter->chan_stats = vmalloc(array_size(sizeof(*adapter->chan_stats),
>> -						 adapter->num_in_chan_stats));
>> +	adapter->chan_stats = vmalloc_array(adapter->num_in_chan_stats,
>> +					    sizeof(*adapter->chan_stats));
> n_channels_bg is 14
> n_channels_a is either 0 or 31 depending on if we're using BAND_A.
> sizeof(*adapter->chan_stats) is 10.
>
> So we're either allocating 280 or 900 bytes, which is quite small.  We
> should just use kmalloc_array() instead of vmalloc_array().
Switching to kmalloc_array() requires changing vfree() to kfree(),
but I couldn't locate the memory release code.  This modification
likely requires deep familiarity with the codebase.  Also, for
variable-sized allocations, kvmalloc_array() is preferable, but
requires kvfree() for proper memory release.

Best regards,
Qianfeng
>
> regards,
> dan carpenter
>

On Tue, Aug 12, 2025 at 10:13:43PM +0800, Qianfeng Rong wrote:
> 
> 在 2025/8/12 21:48, Dan Carpenter 写道:
> > On Tue, Aug 12, 2025 at 09:32:18PM +0800, Qianfeng Rong wrote:
> > > Remove array_size() calls and replace vmalloc() with vmalloc_array() to
> > > simplify the code.
> > > 
> > > Signed-off-by: Qianfeng Rong <rongqianfeng@vivo.com>
> > > ---
> > >   drivers/net/wireless/marvell/mwifiex/cfg80211.c | 4 ++--
> > >   1 file changed, 2 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/net/wireless/marvell/mwifiex/cfg80211.c b/drivers/net/wireless/marvell/mwifiex/cfg80211.c
> > > index 3498743d5ec0..fb4183ff02a9 100644
> > > --- a/drivers/net/wireless/marvell/mwifiex/cfg80211.c
> > > +++ b/drivers/net/wireless/marvell/mwifiex/cfg80211.c
> > > @@ -4673,8 +4673,8 @@ int mwifiex_init_channel_scan_gap(struct mwifiex_adapter *adapter)
> > >   	 * additional active scan request for hidden SSIDs on passive channels.
> > >   	 */
> > >   	adapter->num_in_chan_stats = 2 * (n_channels_bg + n_channels_a);
> > > -	adapter->chan_stats = vmalloc(array_size(sizeof(*adapter->chan_stats),
> > > -						 adapter->num_in_chan_stats));
> > > +	adapter->chan_stats = vmalloc_array(adapter->num_in_chan_stats,
> > > +					    sizeof(*adapter->chan_stats));
> > n_channels_bg is 14
> > n_channels_a is either 0 or 31 depending on if we're using BAND_A.
> > sizeof(*adapter->chan_stats) is 10.
> > 
> > So we're either allocating 280 or 900 bytes, which is quite small.  We
> > should just use kmalloc_array() instead of vmalloc_array().
> Switching to kmalloc_array() requires changing vfree() to kfree(),
> but I couldn't locate the memory release code.

It's not hard to locate with a bit of looking.  ;)

>  This modification
> likely requires deep familiarity with the codebase.  Also, for
> variable-sized allocations, kvmalloc_array() is preferable, but
> requires kvfree() for proper memory release.

Don't use kvmalloc_array().  The rules are:
1) small amounts of memory: kmalloc()
2) possibly large ammounts of memory up to 2GB: kvmalloc()
3) definitely large ammounts of memory or larger than 2GB: vmalloc()

There are also places where the memory needs to be contiguous and for
those situations kvmalloc() and vmalloc() can't be used.

Here we're allocating less than a PAGE so the appropriate thing is
kmalloc_array().

I'm looking at this some more and these statistics are updated in
mwifiex_update_chan_statistics() and not necessarily all of them
are updated.  But they're used in mwifiex_cfg80211_dump_survey()
when potentially the memory is still uninitialized.

I really think we should zero this memory, so lets allocate it
with kcalloc().

regards,
dan carpenter

在 2025/8/12 22:31, Dan Carpenter 写道:
> On Tue, Aug 12, 2025 at 10:13:43PM +0800, Qianfeng Rong wrote:
>> 在 2025/8/12 21:48, Dan Carpenter 写道:
>>> On Tue, Aug 12, 2025 at 09:32:18PM +0800, Qianfeng Rong wrote:
>>>> Remove array_size() calls and replace vmalloc() with vmalloc_array() to
>>>> simplify the code.
>>>>
>>>> Signed-off-by: Qianfeng Rong <rongqianfeng@vivo.com>
>>>> ---
>>>>    drivers/net/wireless/marvell/mwifiex/cfg80211.c | 4 ++--
>>>>    1 file changed, 2 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/net/wireless/marvell/mwifiex/cfg80211.c b/drivers/net/wireless/marvell/mwifiex/cfg80211.c
>>>> index 3498743d5ec0..fb4183ff02a9 100644
>>>> --- a/drivers/net/wireless/marvell/mwifiex/cfg80211.c
>>>> +++ b/drivers/net/wireless/marvell/mwifiex/cfg80211.c
>>>> @@ -4673,8 +4673,8 @@ int mwifiex_init_channel_scan_gap(struct mwifiex_adapter *adapter)
>>>>    	 * additional active scan request for hidden SSIDs on passive channels.
>>>>    	 */
>>>>    	adapter->num_in_chan_stats = 2 * (n_channels_bg + n_channels_a);
>>>> -	adapter->chan_stats = vmalloc(array_size(sizeof(*adapter->chan_stats),
>>>> -						 adapter->num_in_chan_stats));
>>>> +	adapter->chan_stats = vmalloc_array(adapter->num_in_chan_stats,
>>>> +					    sizeof(*adapter->chan_stats));
>>> n_channels_bg is 14
>>> n_channels_a is either 0 or 31 depending on if we're using BAND_A.
>>> sizeof(*adapter->chan_stats) is 10.
>>>
>>> So we're either allocating 280 or 900 bytes, which is quite small.  We
>>> should just use kmalloc_array() instead of vmalloc_array().
>> Switching to kmalloc_array() requires changing vfree() to kfree(),
>> but I couldn't locate the memory release code.
> It's not hard to locate with a bit of looking.  ;)
Yes, I did find it. ;)
>>    This modification
>> likely requires deep familiarity with the codebase.  Also, for
>> variable-sized allocations, kvmalloc_array() is preferable, but
>> requires kvfree() for proper memory release.
> Don't use kvmalloc_array().  The rules are:
> 1) small amounts of memory: kmalloc()
> 2) possibly large ammounts of memory up to 2GB: kvmalloc()
> 3) definitely large ammounts of memory or larger than 2GB: vmalloc()
Allocations under 8 pages should use kmalloc() for efficiency.  For
allocations exceeding 8 pages, kvmalloc() or vmalloc() is preferable
(of course, non-contiguous physical memory can be used) since
kernel memory management defines PAGE_ALLOC_COSTLY_ORDER = 3
(indicating 8 pages), beyond which contiguous physical memory
allocation becomes unreliable and costly.
>
> There are also places where the memory needs to be contiguous and for
> those situations kvmalloc() and vmalloc() can't be used.
>
> Here we're allocating less than a PAGE so the appropriate thing is
> kmalloc_array().
>
> I'm looking at this some more and these statistics are updated in
> mwifiex_update_chan_statistics() and not necessarily all of them
> are updated.  But they're used in mwifiex_cfg80211_dump_survey()
> when potentially the memory is still uninitialized.
>
> I really think we should zero this memory, so lets allocate it
> with kcalloc().
I agree with you, I will try to do this in the next version.
>
> regards,
> dan carpenter
>

On Wed, Aug 13, 2025 at 02:52:51PM +0800, Qianfeng Rong wrote:
> > 
> > I really think we should zero this memory, so lets allocate it
> > with kcalloc().
> I agree with you, I will try to do this in the next version.

Thanks.  When you resend it make sure to add a Fixes tag and CC stable.
Forgetting to zero the memory is a sort of security issue.

regards,
dan carpenter