Re: [PATCH v2] Staging: rtl8192e: make sizeof type-independent

[Dan Carpenter]

On Tue, Jun 21, 2022 at 10:52:55PM +0200, Felix Schlepper wrote:
> Making sizeof operator type-independent.
> 
> Reported by checkpatch:
> 
> CHECK: Prefer kmalloc(sizeof(*txb)...) over
>         kmalloc(sizeof(struct rtllib_txb)...)
> 

Hi Felix,

The subject and commit message are out of date now.  But you're doing
too many things in a single patch.  It needs to be split up.

> ---
> Note: First patch, trying to follow kernelnewbies tutorial.
> 
> Signed-off-by: Felix Schlepper <f3sch.git@xxxxxxxxxxx>

The Signed-off-by needs to be before the --- cut off line.

> ---
> Changes in v2:
>  - replaced kmalloc with kzalloc against memory initialization defects,
>    thus also removing a memset
>  - made error handling more consistent
> 
> ---
>  drivers/staging/rtl8192e/rtllib_tx.c | 24 +++++++++++-------------
>  1 file changed, 11 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/staging/rtl8192e/rtllib_tx.c b/drivers/staging/rtl8192e/rtllib_tx.c
> index 37715afb0210..bcccde91fa0b 100644
> --- a/drivers/staging/rtl8192e/rtllib_tx.c
> +++ b/drivers/staging/rtl8192e/rtllib_tx.c
> @@ -205,30 +205,28 @@ static struct rtllib_txb *rtllib_alloc_txb(int nr_frags, int txb_size,
>  	struct rtllib_txb *txb;
>  	int i;
> 
> -	txb = kmalloc(sizeof(struct rtllib_txb) + (sizeof(u8 *) * nr_frags),
> -		      gfp_mask);
> +	txb = kzalloc(struct_size(txb, fragments, nr_frags), gfp_mask);

Patch 1/3: use struct_size() is one patch because it's safer
Patch 2/3: use kzalloc() and delete memset

>  	if (!txb)
>  		return NULL;
> 
> -	memset(txb, 0, sizeof(struct rtllib_txb));
>  	txb->nr_frags = nr_frags;
>  	txb->frag_size = cpu_to_le16(txb_size);
> 
>  	for (i = 0; i < nr_frags; i++) {
>  		txb->fragments[i] = dev_alloc_skb(txb_size);
> -		if (unlikely(!txb->fragments[i])) {
> -			i--;
> -			break;
> -		}
> +		if (!txb->fragments[i])
> +			goto err_free;
>  		memset(txb->fragments[i]->cb, 0, sizeof(txb->fragments[i]->cb));
>  	}
> -	if (unlikely(i != nr_frags)) {
> -		while (i >= 0)
> -			dev_kfree_skb_any(txb->fragments[i--]);
> -		kfree(txb);
> -		return NULL;
> -	}
> +
>  	return txb;
> +
> +err_free:
> +	while (i > 0)
> +		dev_kfree_skb_any(txb->fragments[--i]);

Patch 3/3: clean up error handling

But can you either write this loop as:

	while (--i >= 0)
		dev_kfree_skb_any(txb->fragments[i]);

Or:

	while (i-- > 0)
		dev_kfree_skb_any(txb->fragments[i]);

Either one is fine but put the -- in the condition.

regards,
dan carpenter