Re: [Intel-wired-lan] [PATCH 2/4] ice: use kzalloc() to allocate staging buffer for reading from GNSS

[Przemek Kitszel <przemyslaw.kitszel@intel.com>]

On 7/1/26 15:57, Mike Rapoport (Microsoft) wrote:
> ice_gnss_read() uses get_zeroed_page() to  allocate a staging buffer for
> reading GNSS module data via I2C bus.
> 
> This buffer can be allocated with kmalloc() as there's nothing special
> about it to go directly to the page allocator.
> 
> kmalloc() provides a better API that does not require ugly casts and
> kfree() does not need to know the size of the freed object.
> 
> Performance difference between kmalloc() and __get_free_pages() is not
> measurable as both allocators take an object/page from a per-CPU list for
> fast path allocations.
> 
> For the slow path the performance is anyway determined by the amount of
> reclaim involved rather than by what allocator is used.
> 
> Replace use of get_zeroed_page() with kzalloc() and free_page() with
> kfree().
> 
> Link: https://lore.kernel.org/all/635405e4-9423-4a25-a6e7-e03c8ea0bcbe@redhat.com
> Signed-off-by: Mike Rapoport (Microsoft) <rppt@kernel.org>
> ---
>   drivers/net/ethernet/intel/ice/ice_gnss.c | 5 +++--
>   1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/ice/ice_gnss.c b/drivers/net/ethernet/intel/ice/ice_gnss.c
> index 8fd954f1ebd6..7d21c3417b0b 100644
> --- a/drivers/net/ethernet/intel/ice/ice_gnss.c
> +++ b/drivers/net/ethernet/intel/ice/ice_gnss.c
> @@ -2,6 +2,7 @@
>   /* Copyright (C) 2021-2022, Intel Corporation. */
>   
>   #include "ice.h"
> +#include <linux/slab.h>
>   #include "ice_lib.h"
>   
>   /**
> @@ -124,7 +125,7 @@ static void ice_gnss_read(struct kthread_work *work)
>   
>   	data_len = min_t(typeof(data_len), data_len, PAGE_SIZE);
>   
> -	buf = (char *)get_zeroed_page(GFP_KERNEL);
> +	buf = kzalloc(PAGE_SIZE, GFP_KERNEL);

nit:
from the code it is clear that we read at most a page, and @data_len
stores the actual amount needed

comment:
I don't know why we limit to a page, it's outside of the scope of this
series, but likely you have removed the limit (which will go into the
loop - single AQ call is likely limited by a PAGE too).

Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>

>   	if (!buf) {
>   		err = -ENOMEM;
>   		goto requeue;
> @@ -151,7 +152,7 @@ static void ice_gnss_read(struct kthread_work *work)
>   			 count, i);
>   	delay = ICE_GNSS_TIMER_DELAY_TIME;
>   free_buf:
> -	free_page((unsigned long)buf);
> +	kfree(buf);
>   requeue:
>   	kthread_queue_delayed_work(gnss->kworker, &gnss->read_work, delay);
>   	if (err)
>