Re: [PATCH v3 4/4] rtl8192u: fix printk calls in r8192U_core.c

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Sun, May 26, 2013 at 10:42:44PM +0300, Xenia Ragiadakou wrote:
> This patch replaces calls to printk with their
> corresponding calls to pr_<loglevel>(fmt, ...)
> and netdev_<loglevel>(dev, fmt, ...), when it
> is applicable, to make the log messages more
> informative.
> 
> Also, it fixes some small typos and whitespaces
> found in log messages.
> 

This patch is nice.

Acked-by: Dan Carpenter <dan.carpenter@xxxxxxxxxx>

You asked me for other ideas for patches so I have included some
thoughts inline.

> @@ -421,7 +421,7 @@ u16 read_nic_word_E(struct net_device *dev, int indx)
>  				       indx|0xfe00, 0, &data, 2, HZ / 2);
>  
>  	if (status < 0)
> -		printk("read_nic_word TimeOut! status:%d\n", status);
> +		netdev_err(dev, "read_nic_word TimeOut! status: %d\n", status);
>  
>  	return data;


Your patch doesn't introduce this, but if usb_control_msg() fails
then data is uninitialized data.  This funciton should take a u16
pointer and set the parameter.  It should return 0 on success and
status on failure.

>  }
> @@ -446,7 +446,7 @@ u32 read_nic_dword(struct net_device *dev, int indx)
>  	 */
         ^^
This comment is dead code.  Delete.


>  
>  	if (status < 0)
> -		printk("read_nic_dword TimeOut! status:%d\n", status);
> +		netdev_err(dev, "read_nic_dword TimeOut! status:%d\n", status);
>  
>  	return data;

Gar...  Returning uninitialized data again.  Same solution.

>  }
> @@ -999,7 +999,7 @@ void rtl8192_rtx_disable(struct net_device *dev)
>  	}
>  
>  	if (skb_queue_len(&priv->skb_queue)) {
> -		printk(KERN_WARNING "skb_queue not empty\n");
> +		netdev_warn(dev, "skb_queue not empty\n");

Can this actually happen?  If so then we should fix that bug, and
remove the printk.

>  	}
>  
>  	skb_queue_purge(&priv->skb_queue);
> @@ -1080,7 +1080,7 @@ static void rtl8192_rx_isr(struct urb *urb)
>  	skb = dev_alloc_skb(RX_URB_SIZE);
>  	if (unlikely(!skb)) {
>  		usb_free_urb(urb);
> -		printk("%s():can,t alloc skb\n", __func__);
> +		netdev_err(dev, "%s(): can't alloc skb\n", __func__);

Allocation failures already print enough information.  This printk()
can be deleted.

>  		/* TODO check rx queue length and refill *somewhere* */
>  		return;
>  	}
> @@ -1099,7 +1099,7 @@ static void rtl8192_rx_isr(struct urb *urb)
>  	skb_queue_tail(&priv->rx_queue, skb);
>  	err = usb_submit_urb(urb, GFP_ATOMIC);
>  	if (err && err != EPERM)
> -		printk("can not submit rxurb, err is %x,URB status is %x\n", err, urb->status);
> +		netdev_err(dev, "can not submit rxurb, err is %x, URB status is %x\n", err, urb->status);
>  }
>  
>  u32
> @@ -1257,7 +1257,7 @@ struct sk_buff *DrvAggr_Aggregation(struct net_device *dev, struct ieee80211_drv
>  	tcb_desc->drv_agg_enable = 1;
>  	tcb_desc->pkt_size = skb->len;
>  	tcb_desc->DrvAggrNum = pSendList->nr_drv_agg_frames;
> -	printk("DrvAggNum = %d\n", tcb_desc->DrvAggrNum);
> +	netdev_dbg(dev, "DrvAggNum = %d\n", tcb_desc->DrvAggrNum);
>  //	RT_DEBUG_DATA(COMP_SEND, skb->cb, sizeof(skb->cb));
>  //	printk("========>skb->data ======> \n");
>  //	RT_DEBUG_DATA(COMP_SEND, skb->data, skb->len);
> @@ -1951,7 +1951,7 @@ short rtl8192_tx(struct net_device *dev, struct sk_buff *skb)
>  	 * !!! For debug purpose
>  	 */
>  	if (pend > MAX_TX_URB){
> -		printk("To discard skb packet!\n");
> +		netdev_dbg(dev, "To discard skb packet!\n");
>  		dev_kfree_skb_any(skb);
>  		return -1;
>  	}
> @@ -2191,7 +2191,7 @@ short rtl8192_usb_initendpoints(struct net_device *dev)
>  		return -ENOMEM;
>  	}
>  
> -	printk("End of initendpoints\n");
> +	netdev_dbg(dev, "End of initendpoints\n");

The kernel has ftrace which does function tracing.  This can be
deleted.

>  	return 0;
>  
>  }
> @@ -3074,7 +3074,7 @@ short rtl8192_get_channel_map(struct net_device *dev)
>  {
>  	struct r8192_priv *priv = ieee80211_priv(dev);
>  	if (priv->ChannelPlan > COUNTRY_CODE_GLOBAL_DOMAIN){
> -		printk("rtl8180_init:Error channel plan! Set to default.\n");
> +		netdev_err(dev, "rtl8180_init: Error channel plan! Set to default.\n");
>  		priv->ChannelPlan = 0;
>  	}
>  	RT_TRACE(COMP_INIT, "Channel plan is %d\n", priv->ChannelPlan);
> @@ -3886,7 +3886,7 @@ RESET_START:
>  		if (ieee->state == IEEE80211_LINKED)
>  		{
>  			down(&ieee->wx_sem);
> -			printk("ieee->state is IEEE80211_LINKED\n");
> +			netdev_dbg(dev, "ieee->state is IEEE80211_LINKED\n");
>  			ieee80211_stop_send_beacons(priv->ieee80211);
>  			del_timer_sync(&ieee->associate_timer);
>  			cancel_delayed_work(&ieee->associate_retry_wq);
> @@ -3895,7 +3895,7 @@ RESET_START:
>  			up(&ieee->wx_sem);
>  		}
>  		else{
> -			printk("ieee->state is NOT LINKED\n");
> +			netdev_dbg(dev, "ieee->state is NOT LINKED\n");
>  			ieee80211_softmac_stop_protocol(priv->ieee80211);			}
>  		up(&priv->wx_sem);
>  		RT_TRACE(COMP_RESET, "%s():<==========down process is finished\n", __func__);
> @@ -4054,7 +4054,7 @@ extern	void	rtl819x_watchdog_wqcallback(struct work_struct *work)
>  				if (rfState == eRfOff)
>  					RT_TRACE(COMP_ERR, "========>%s()\n", __func__);
>  				#endif
                                ^^^^^^

All these ifdefs annoy me.  This one is improperly indented and it
is useless dead code.  Delete.

> -				printk("===>%s(): AP is power off,connect another one\n", __func__);
> +				netdev_dbg(dev, "===>%s(): AP is power off, connect another one\n", __func__);
>  			//	Dot11d_Reset(dev);
>  				priv->ieee80211->state = IEEE80211_ASSOCIATING;
>  				notify_wx_assoc_event(priv->ieee80211);
> @@ -5514,7 +5514,7 @@ void rtl8192_rx_nomal(struct sk_buff *skb)
>  #endif
>  	} else {
>  		priv->stats.rxurberr++;
> -		printk("actual_length:%d\n", skb->len);
> +		netdev_dbg(dev, "actual_length: %d\n", skb->len);
>  		dev_kfree_skb_any(skb);
>  	}
>  
> @@ -5815,38 +5815,36 @@ static int __init rtl8192_usb_module_init(void)
>  #ifdef CONFIG_IEEE80211_DEBUG

This ifdef should be hidden in a .h file like this:

#ifdef CONFIG_IEEE80211_DEBUG
int ieee80211_debug_init(void);
#else
int ieee80211_debug_init(void){ return 0; }
#endif

That way we can just do:

	ret = ieee80211_debug_init()
	if (ret)
		return ret;

No #ifdef in the .c code.  Of course, right now the function is not
declared in a .h file so that would need to be fixed as well.

regards,
dan carpenter

_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/devel




[Index of Archives]     [Linux Driver Backports]     [DMA Engine]     [Linux GPIO]     [Linux SPI]     [Video for Linux]     [Linux USB Devel]     [Linux Coverity]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]
  Powered by Linux