Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1665208 > unrolled thread

[PATCH 1/2] staging: rtl8723bs: Convert LIST_CONTAINOR to use kernel container_of

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 1/2] staging: rtl8723bs: Convert LIST_CONTAINOR to use kernel container_of Joe Perches <joe@perches.com> - 2017-06-13 23:20 +0200
    Re: [PATCH 1/2] staging: rtl8723bs: Convert LIST_CONTAINOR to use  kernel container_of Greg Kroah-Hartman <gregkh@linuxfoundation.org> - 2017-06-14 12:30 +0200
      Re: [PATCH 1/2] staging: rtl8723bs: Convert LIST_CONTAINOR to use  kernel container_of Joe Perches <joe@perches.com> - 2017-06-14 12:40 +0200
        Re: [PATCH 1/2] staging: rtl8723bs: Convert LIST_CONTAINOR to use  kernel container_of Greg Kroah-Hartman <gregkh@linuxfoundation.org> - 2017-06-14 12:50 +0200
          Re: [PATCH 1/2] staging: rtl8723bs: Convert LIST_CONTAINOR to use  kernel container_of Joe Perches <joe@perches.com> - 2017-06-14 13:00 +0200

#1665208 — [PATCH 1/2] staging: rtl8723bs: Convert LIST_CONTAINOR to use kernel container_of

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.

Signed-off-by: Joe Perches <joe@perches.com>
---
 drivers/staging/rtl8723bs/core/rtw_recv.c               | 14 +++++++-------
 drivers/staging/rtl8723bs/core/rtw_sta_mgt.c            |  2 +-
 drivers/staging/rtl8723bs/include/osdep_service_linux.h |  2 +-
 3 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/staging/rtl8723bs/core/rtw_recv.c b/drivers/staging/rtl8723bs/core/rtw_recv.c
index 695a5c958c80..e1d6d0a4b115 100644
--- 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;
 
 		list_del_init(&precvframe->u.hdr.list);
 		padapter = precvframe->u.hdr.adapter;
@@ -243,7 +243,7 @@ void rtw_free_recvframe_queue(struct __queue *pframequeue,  struct __queue *pfre
 	plist = get_next(phead);
 
 	while (phead != plist) {
-		precvframe = LIST_CONTAINOR(plist, union recv_frame, u);
+		precvframe = (union recv_frame *)plist;
 
 		plist = get_next(plist);
 
@@ -1732,7 +1732,7 @@ static union recv_frame *recvframe_defrag(struct adapter *adapter,
 
 	phead = get_list_head(defrag_q);
 	plist = get_next(phead);
-	prframe = LIST_CONTAINOR(plist, union recv_frame, u);
+	prframe = (union recv_frame *)plist;
 	pfhdr = &prframe->u.hdr;
 	list_del_init(&(prframe->u.list));
 
@@ -1754,7 +1754,7 @@ static union recv_frame *recvframe_defrag(struct adapter *adapter,
 	data = get_recvframe_data(prframe);
 
 	while (phead != plist) {
-		pnextrframe = LIST_CONTAINOR(plist, union recv_frame, u);
+		pnextrframe = (union recv_frame *)plist;
 		pnfhdr = &pnextrframe->u.hdr;
 
 
@@ -2071,7 +2071,7 @@ int enqueue_reorder_recvframe(struct recv_reorder_ctrl *preorder_ctrl, union rec
 	plist = get_next(phead);
 
 	while (phead != plist) {
-		pnextrframe = LIST_CONTAINOR(plist, union recv_frame, u);
+		pnextrframe = (union recv_frame *)plist;
 		pnextattrib = &pnextrframe->u.hdr.attrib;
 
 		if (SN_LESS(pnextattrib->seq_num, pattrib->seq_num))
@@ -2146,7 +2146,7 @@ int recv_indicatepkts_in_order(struct adapter *padapter, struct recv_reorder_ctr
 			return true;
 		}
 
-		prframe = LIST_CONTAINOR(plist, union recv_frame, u);
+		prframe = (union recv_frame *)plist;
 		pattrib = &prframe->u.hdr.attrib;
 
 		#ifdef DBG_RX_SEQ
@@ -2162,7 +2162,7 @@ int recv_indicatepkts_in_order(struct adapter *padapter, struct recv_reorder_ctr
 	/*  Check if there is any packet need indicate. */
 	while (!list_empty(phead)) {
 
-		prframe = LIST_CONTAINOR(plist, union recv_frame, u);
+		prframe = (union recv_frame *)plist;
 		pattrib = &prframe->u.hdr.attrib;
 
 		if (!SN_LESS(preorder_ctrl->indicate_seq, pattrib->seq_num)) {
diff --git a/drivers/staging/rtl8723bs/core/rtw_sta_mgt.c b/drivers/staging/rtl8723bs/core/rtw_sta_mgt.c
index cb43ec90a648..c6ebb9d8cb3c 100644
--- a/drivers/staging/rtl8723bs/core/rtw_sta_mgt.c
+++ b/drivers/staging/rtl8723bs/core/rtw_sta_mgt.c
@@ -429,7 +429,7 @@ u32 rtw_free_stainfo(struct adapter *padapter, struct sta_info *psta)
 		plist = get_next(phead);
 
 		while (!list_empty(phead)) {
-			prframe = LIST_CONTAINOR(plist, union recv_frame, u);
+			prframe = (union recv_frame *)plist;
 
 			plist = get_next(plist);
 
diff --git a/drivers/staging/rtl8723bs/include/osdep_service_linux.h b/drivers/staging/rtl8723bs/include/osdep_service_linux.h
index 486e8184b0b2..c39b058d91ba 100644
--- a/drivers/staging/rtl8723bs/include/osdep_service_linux.h
+++ b/drivers/staging/rtl8723bs/include/osdep_service_linux.h
@@ -86,7 +86,7 @@ __inline static struct list_head	*get_list_head(struct __queue	*queue)
 
 
 #define LIST_CONTAINOR(ptr, type, member) \
-        ((type *)((char *)(ptr)-(__kernel_size_t)(&((type *)0)->member)))
+	container_of(ptr, type, member)
 
 #define RTW_TIMER_HDL_ARGS void *FunctionContext
 
-- 
2.10.0.rc2.1.g053435c

[toc] | [next] | [standalone]

#1665682 — Re: [PATCH 1/2] staging: rtl8723bs: Convert LIST_CONTAINOR to use kernel container_of

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.
> 
> Signed-off-by: Joe Perches <joe@perches.com>
> ---
>  drivers/staging/rtl8723bs/core/rtw_recv.c               | 14 +++++++-------
>  drivers/staging/rtl8723bs/core/rtw_sta_mgt.c            |  2 +-
>  drivers/staging/rtl8723bs/include/osdep_service_linux.h |  2 +-
>  3 files changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/staging/rtl8723bs/core/rtw_recv.c b/drivers/staging/rtl8723bs/core/rtw_recv.c
> index 695a5c958c80..e1d6d0a4b115 100644
> --- 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.

thanks,

greg k-h

[toc] | [prev] | [next] | [standalone]

#1665683 — Re: [PATCH 1/2] staging: rtl8723bs: Convert LIST_CONTAINOR to use kernel container_of

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.

[toc] | [prev] | [next] | [standalone]

#1665688 — Re: [PATCH 1/2] staging: rtl8723bs: Convert LIST_CONTAINOR to use kernel container_of

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

[toc] | [prev] | [next] | [standalone]

#1665709 — Re: [PATCH 1/2] staging: rtl8723bs: Convert LIST_CONTAINOR to use kernel container_of

On Wed, 2017-06-14 at 12:48 +0200, Greg Kroah-Hartman wrote:
> 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...

<shrug>.

No thanks.

That's what the current code is doing now.

Ignore the whole thing if you want.

Someone else might fix it eventually, but more
likely the current rtl8723bs driver will be
dropped and completely rewritten anyway.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web