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

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

[Felix Schlepper]

Making sizeof operator type-independent.

Reported by checkpatch:

CHECK: Prefer kmalloc(sizeof(*txb)...) over
        kmalloc(sizeof(struct rtllib_txb)...)

---
Note: First patch, trying to follow kernelnewbies tutorial.

Signed-off-by: Felix Schlepper <f3sch.git@outlook.com>
---
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);
 	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]);
+	kfree(txb);
+
+	return NULL;
 }

 static int rtllib_classify(struct sk_buff *skb, u8 bIsAmsdu)
--
2.36.1

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@outlook.com>

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