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 <[email protected]> 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 [email protected] http://driverdev.linuxdriverproject.org/mailman/listinfo/devel
