From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752296AbdFNKst (ORCPT ); Wed, 14 Jun 2017 06:48:49 -0400 Received: from mail.linuxfoundation.org ([140.211.169.12]:41284 "EHLO mail.linuxfoundation.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752254AbdFNKsq (ORCPT ); Wed, 14 Jun 2017 06:48:46 -0400 Date: Wed, 14 Jun 2017 12:48:32 +0200 From: Greg Kroah-Hartman To: Joe Perches Cc: linux-kernel@vger.kernel.org, devel@driverdev.osuosl.org, Hans de Goede , Larry Finger Subject: Re: [PATCH 1/2] staging: rtl8723bs: Convert LIST_CONTAINOR to use kernel container_of Message-ID: <20170614104832.GA1320@kroah.com> References: <629c18e79e3528e03deabdd1996ceb857f34aa81.1497388055.git.joe@perches.com> <20170614101853.GA8107@kroah.com> <1497436701.18751.41.camel@perches.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1497436701.18751.41.camel@perches.com> User-Agent: Mutt/1.8.3 (2017-05-23) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, Jun 14, 2017 at 03:38:21AM -0700, Joe Perches wrote: > On Wed, 2017-06-14 at 12:18 +0200, Greg Kroah-Hartman wrote: > > On Tue, Jun 13, 2017 at 02:12:56PM -0700, Joe Perches wrote: > > > These are similar macros so use the normal kernel one. > > > > > > As well, there are odd games being played with casting a plist to > > > a union recv_frame by using LIST_CONTAINOR. Just use a direct cast > > > to union recv_frame instead. > [] > > > diff --git a/drivers/staging/rtl8723bs/core/rtw_recv.c b/drivers/staging/rtl8723bs/core/rtw_recv.c > [] > > > @@ -129,7 +129,7 @@ union recv_frame *_rtw_alloc_recvframe(struct __queue *pfree_recv_queue) > > > > > > plist = get_next(phead); > > > > > > - precvframe = LIST_CONTAINOR(plist, union recv_frame, u); > > > + precvframe = (union recv_frame *)plist; > > > > No, you are "assuming" that the list_head is going to stay the first > > object of this structure, and what if it isn't? > > > > Just use container_of, that way at least you get the type safeness of > > the call, and if something changes in the future, you don't instantly > > break the code everywhere without knowing it. > > It's a named union u and it's exactly at the beginning. > It's unlikely to change. > > The container_of macro doesn't work here as there it has > a BUILD_BUG_ON_MSG and there are pointer type mismatches > on those uses. Yeah, but random pointer casts like this are not good, I don't want to see that here, sorry. Fix up the pointer mismatches, if there are any, as that implies there are bugs here... thanks, greg k-h