Re: [PATCH v3 1/3] staging: rtl8723au: use list_for_each_entry*()

[Jes Sorensen]

Geliang Tang <geliangtang@xxxxxxx> writes:
> Use list_for_each_entry*() instead of list_for_each*() to simplify
> the code.
>
> Signed-off-by: Geliang Tang <geliangtang@xxxxxxx>
> ---
> Changes in v3:
>  - split it into three patches.
> Changes in v2:
>  - drop the coding style fixing in v1.
> ---
>  drivers/staging/rtl8723au/core/rtw_ap.c           | 55 ++++++++-----------
>  drivers/staging/rtl8723au/core/rtw_mlme.c         | 26 ++++-----
>  drivers/staging/rtl8723au/core/rtw_mlme_ext.c     | 10 ++--
>  drivers/staging/rtl8723au/core/rtw_recv.c         | 22 ++++----
>  drivers/staging/rtl8723au/core/rtw_sta_mgt.c      | 25 ++++-----
>  drivers/staging/rtl8723au/core/rtw_xmit.c         | 64 ++++++++++-------------
>  drivers/staging/rtl8723au/os_dep/ioctl_cfg80211.c | 14 +++--
>  drivers/staging/rtl8723au/os_dep/usb_ops_linux.c  |  9 ++--
>  8 files changed, 96 insertions(+), 129 deletions(-)

This one is mostly OK, however when you do multi patch sets, always
include a cover letter describing the overall changes of the set.

A few nit picks:

> diff --git a/drivers/staging/rtl8723au/core/rtw_recv.c b/drivers/staging/rtl8723au/core/rtw_recv.c
> index 404b618..18b7d03 100644
> --- a/drivers/staging/rtl8723au/core/rtw_recv.c
> +++ b/drivers/staging/rtl8723au/core/rtw_recv.c
> @@ -88,13 +88,13 @@ int _rtw_init_recv_priv23a(struct recv_priv *precvpriv,
>  void _rtw_free_recv_priv23a (struct recv_priv *precvpriv)
>  {
>  	struct rtw_adapter *padapter = precvpriv->adapter;
> -	struct recv_frame *precvframe;
> -	struct list_head *plist, *ptmp;
> +	struct recv_frame *precvframe, *ptmp;
>  
>  	rtw_free_uc_swdec_pending_queue23a(padapter);
>  
> -	list_for_each_safe(plist, ptmp, &precvpriv->free_recv_queue.queue) {
> -		precvframe = container_of(plist, struct recv_frame, list);
> +	list_for_each_entry_safe(precvframe, ptmp,
> +				 &precvpriv->free_recv_queue.queue,
> +				 list) {
>  		list_del_init(&precvframe->list);
>  		kfree(precvframe);
>  	}

Too aggressive line breaking, the 'list' fits within 80 characters on
the line above.

> @@ -195,16 +195,15 @@ using spinlock to protect
>  
>  static void rtw_free_recvframe23a_queue(struct rtw_queue *pframequeue)
>  {
> -	struct recv_frame *hdr;
> -	struct list_head *plist, *phead, *ptmp;
> +	struct recv_frame *hdr, *ptmp;
> +	struct list_head *phead;
>  
>  	spin_lock(&pframequeue->lock);
>  
>  	phead = get_list_head(pframequeue);
>  	plist = phead->next;
>  
> -	list_for_each_safe(plist, ptmp, phead) {
> -		hdr = container_of(plist, struct recv_frame, list);
> +	list_for_each_entry_safe(hdr, ptmp, phead, list) {
>  		rtw_free_recvframe23a(hdr);
>  	}
>  

You could remove the brackets here, since you are fixing that specific
line. I am fine with this as is, some of the checkpatch police force
might bite over it.

On overall this patch is a lot better than the first version.

All set and done, I am thinking of removing this driver once Kalle pulls
in my currently posted set of changes for rtl8xxxu, plus the next one I
have lined up. I seems to me rtl8xxxu can replace rtl8723au at this
point.

I will probably mark rtl8723au deprecated after 4.6 comes out, and
remove the driver around 4.7.

Cheers,
Jes