Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 22 Jun 2022 08:54:00 +0300
From:   Dan Carpenter <dan.carpenter@oracle.com>
To:     Felix Schlepper <f3sch.git@outlook.com>
Cc:     gregkh@linuxfoundation.org, linux-staging@lists.linux.dev,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2] Staging: rtl8192e: make sizeof type-independent
Message-ID: <20220622055359.GM16517@kadam>
References: <AM9P190MB1299AC4FCE60C59C35034DCEA5B39@AM9P190MB1299.EURP190.PROD.OUTLOOK.COM>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <AM9P190MB1299AC4FCE60C59C35034DCEA5B39@AM9P190MB1299.EURP190.PROD.OUTLOOK.COM>
User-Agent: Mutt/1.9.4 (2018-02-28)
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?QlehjJHk4WokzbPKpz3QD7WBH9KM8wECk9YVfPKQ1gxQzkPnSk5LuH4zzXqh?=
 =?us-ascii?Q?wMsaBxOYvt+J2Wv2liNy73es8yeXeFobLgn08HE61Zir0Lr/q7A68eFogy+Y?=
 =?us-ascii?Q?SqxKTem2sP50W5Lcczn6u3tgFa1/DTWzSHN0zsZfJB1ZQrEmQJFcRO9zUE5/?=
 =?us-ascii?Q?nVMVSdNaqvJc+002H4sARwUAoEuEr1o83VDD0Hn1MP0SVIZO5IevmLYNg9mw?=
 =?us-ascii?Q?IW5PzxqOaDHJqDX6OkhnRIbTfdrXiL3ifIf1WnE9t4az4iTZC7Dfgcxovag9?=
 =?us-ascii?Q?enrbS08zR4XdsbTuhgMye3NHc/QObZ+LVfjUaAcdiQpsPw5MwZifXXOplYKx?=
 =?us-ascii?Q?5dMciCOhYB/q5wMsvSvp39GFzF/+E6NQNAqhevE3kaC4zMEssd/TywDot9kE?=
 =?us-ascii?Q?cc9v4QjErfc7+psMNPaNzHMNpkPiu+q2OPFIaodiOmt5I6Nah73RrlZCwmko?=
 =?us-ascii?Q?NHsW1cRuYhTgA7CQ/F6dHHk6bjX1+3eonaxJ5TTlYYYcqO5vtT/yCpxedauX?=
 =?us-ascii?Q?AdkrpAgEf2pj05zai07gXWU0y7gQyz2V/1gvpTshsNJdNWLhrQQZ6HtKgQjy?=
 =?us-ascii?Q?oTCR+0oo4Riq9rVWNROss1U45XhpRuUk0WHbEIzwRFmmMx0S9MHmm+9OfggF?=
 =?us-ascii?Q?jm8hFCtYAkPHbFRFZEl5n0rUm/SSQM9rrlU8Oy7RRQdbeUpKnDExfWaXQLdI?=
 =?us-ascii?Q?yGMD1/ZsjasKKrA2QEWtMNya/3kJUNKJmOscOTDGrEKfgZkaikv0Y1m50TV+?=
 =?us-ascii?Q?hejiS9p9Vv2i1qtVeB4xOBaMynfMveQi5S+rcltrgRtKeugAdX0R9YfNMFqB?=
 =?us-ascii?Q?x+q16PmxqiM3T0aOOInxKcGn41aIS2D8NdH3N8cEAYbDi8O1BR3EuEYJHdWN?=
 =?us-ascii?Q?KMG6pAlp2L0KqSr9VgglelF5NVJzyvockcYdIidSLf9HRxMXdrouVd3b9iLv?=
 =?us-ascii?Q?x2TN2rDWsWVVYKJA2KQDjEp0maC18WyVC1q51Ew2nKKHPSspBhSQqSW1vJio?=
 =?us-ascii?Q?O2INWwTZ4ef7MAEvs5ARi3NCjl1MRx6ozDAoFhiuvb6Yl7qxDkDVuCnsazD4?=
 =?us-ascii?Q?+N1CLk2HNEfXAQzXgmx05YM99PNuXFF81tltEi+r1Yyd3tmfsIqIpPBzNEXK?=
 =?us-ascii?Q?EfxHGfA1Mp/khuJr/eEZt2n09QDqltehoZfGMmG5YJ70EgiKdbQhFbZ2PCGy?=
 =?us-ascii?Q?4r7aycYLPfDJTuvDwFgOzjzLdrs04eoUZ0BPUwGab6zQYLMBHjReP5eQibnC?=
 =?us-ascii?Q?k7BgM7y1EmFVpRC85uSYE+3pPezu3LwpxEKCAjaCP2yp6HtsFGIFtYzq8/MU?=
 =?us-ascii?Q?F+gGdu4bimzdVtoD5s6puPpvL8bPy1JhwyEcuDAYrdJBnGl+ybC+lfTWRaYB?=
 =?us-ascii?Q?V+yPRjIvlWOciw+nSmv3QAYA7jKe5mid9UJuQy9znrudMGPP32FAJ/OXxZ61?=
 =?us-ascii?Q?AY9ixUQP5ZhFP2JKTa7vSZACLZIFgscgQgAPKgsrtgakUc6Vt7P6EP3E5Wkg?=
 =?us-ascii?Q?FWoSs4rMV3SyGOUWlaoJ7YPHjiZz9vpNAjWTr924ZN5jsh+vUmMRboYaUhCz?=
 =?us-ascii?Q?jyJyI24ryvMsbUfzEttNIWamdn9mpJLY/8RAaNmsQo7PmndbvdC7YaZvQ5rB?=
 =?us-ascii?Q?DhgjpYPpL0GoOsXMc+ldycmM7BZHvEnVlTdWq6LkhWN+Uq1ULyXyp2BO3NDb?=
 =?us-ascii?Q?LcgPFBnpDZ63r4grY47TIx0glrqk7XkkYIblWSJ25UTfZ0Dj/vaV4xHOE+O0?=
 =?us-ascii?Q?5PhI2P87kDZFnvHj/yNNwDHo76sh12I=3D?=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 6cd01dcc-cca9-4e50-c3a5-08da5413a40b
X-MS-Exchange-CrossTenant-AuthSource: MWHPR1001MB2365.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 22 Jun 2022 05:54:16.3598
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 4e2c6054-71cb-48f1-bd6c-3a9705aca71b
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: 7Y75iNdz03ZzsrCg5+2yHIFDRSrJ7gNUTiBZkCBoS3YL6uAj+OvmyH8BKu2OG5h2/iMClR0vxelxksFdvtwXzNEVYLs3qKbupH28zYu139Q=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CH2PR10MB3863
X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:6.0.517,18.0.883
 definitions=2022-06-21_11:2022-06-21,2022-06-21 signatures=0
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 adultscore=0 bulkscore=0 phishscore=0
 mlxlogscore=820 mlxscore=0 suspectscore=0 spamscore=0 malwarescore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2204290000
 definitions=main-2206220029
X-Proofpoint-GUID: Qx6g0HPsdp292cudn6OAJxHfoyj2qEJw
X-Proofpoint-ORIG-GUID: Qx6g0HPsdp292cudn6OAJxHfoyj2qEJw
X-Spam-Status: No, score=-2.8 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_LOW,
        RCVD_IN_MSPIKE_H2,SPF_HELO_NONE,SPF_NONE,T_SCC_BODY_TEXT_LINE
        autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

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