Re: [PATCH] staging: gdm72xx: conditionally compile debug code

[Michalis Pappas <mpappas@fastmail.fm>]

On 07/01/2014 07:08 PM, Ben Chan wrote:
> 
> 
> 
> On Tue, Jul 1, 2014 at 9:40 AM, Michalis Pappas <mpappas@fastmail.fm
> <mailto:mpappas@fastmail.fm>> wrote:
> 
>     On 07/01/2014 04:30 PM, Ben Chan wrote:
>     >
>     >
>     >
>     > On Tue, Jul 1, 2014 at 6:00 AM, Michalis Pappas
>     <mpappas@fastmail.fm <mailto:mpappas@fastmail.fm>
>     > <mailto:mpappas@fastmail.fm <mailto:mpappas@fastmail.fm>>> wrote:
>     >
>     >     Signed-off-by: Michalis Pappas <mpappas@fastmail.fm
>     <mailto:mpappas@fastmail.fm>
>     >     <mailto:mpappas@fastmail.fm <mailto:mpappas@fastmail.fm>>>
>     >     ---
>     >      drivers/staging/gdm72xx/gdm_qos.c   | 2 ++
>     >      drivers/staging/gdm72xx/gdm_sdio.c  | 7 +++++++
>     >      drivers/staging/gdm72xx/gdm_usb.c   | 7 +++++++
>     >      drivers/staging/gdm72xx/gdm_wimax.c | 6 ++++++
>     >      drivers/staging/gdm72xx/gdm_wimax.h | 2 ++
>     >      5 files changed, 24 insertions(+)
>     >
>     >     diff --git a/drivers/staging/gdm72xx/gdm_qos.c
>     >     b/drivers/staging/gdm72xx/gdm_qos.c
>     >     index b08c8e1..7900981 100644
>     >     --- a/drivers/staging/gdm72xx/gdm_qos.c
>     >     +++ b/drivers/staging/gdm72xx/gdm_qos.c
>     >     @@ -88,7 +88,9 @@ static void free_qos_entry_list(struct list_head
>     >     *free_list)
>     >                     total_free++;
>     >             }
>     >
>     >     +       #if defined(GDM72xx_DEBUG)
>     >             pr_debug("%s: total_free_cnt=%d\n", __func__, total_free);
>     >     +       #endif
>     >      }
>     >
>     >      void gdm_qos_init(void *nic_ptr)
>     >     diff --git a/drivers/staging/gdm72xx/gdm_sdio.c
>     >     b/drivers/staging/gdm72xx/gdm_sdio.c
>     >     index 9d2de6f..914fd75 100644
>     >     --- a/drivers/staging/gdm72xx/gdm_sdio.c
>     >     +++ b/drivers/staging/gdm72xx/gdm_sdio.c
>     >     @@ -280,9 +280,11 @@ static void send_sdu(struct sdio_func *func,
>     >     struct tx_cxt *tx)
>     >
>     >             spin_unlock_irqrestore(&tx->lock, flags);
>     >
>     >     +       #if defined(GDM72xx_DEBUG)
>     >             print_hex_dump_debug("sdio_send: ", DUMP_PREFIX_NONE,
>     16, 1,
>     >                                  tx->sdu_buf + TYPE_A_HEADER_SIZE,
>     >                                  aggr_len - TYPE_A_HEADER_SIZE,
>     false);
>     >     +       #endif
>     >
>     >             for (pos = TYPE_A_HEADER_SIZE; pos < aggr_len; pos +=
>     >     TX_CHUNK_SIZE) {
>     >                     len = aggr_len - pos;
>     >     @@ -317,9 +319,12 @@ static void send_hci(struct sdio_func *func,
>     >     struct tx_cxt *tx,
>     >      {
>     >             unsigned long flags;
>     >
>     >     +       #if defined(GDM72xx_DEBUG)
>     >             print_hex_dump_debug("sdio_send: ", DUMP_PREFIX_NONE,
>     16, 1,
>     >                                  t->buf + TYPE_A_HEADER_SIZE,
>     >                                  t->len - TYPE_A_HEADER_SIZE, false);
>     >     +       #endif
>     >     +
>     >             send_sdio_pkt(func, t->buf, t->len);
>     >
>     >             spin_lock_irqsave(&tx->lock, flags);
>     >     @@ -551,8 +556,10 @@ static void gdm_sdio_irq(struct sdio_func
>     *func)
>     >             }
>     >
>     >      end_io:
>     >     +       #if defined(GDM72xx_DEBUG)
>     >             print_hex_dump_debug("sdio_receive: ",
>     DUMP_PREFIX_NONE, 16, 1,
>     >                                  rx->rx_buf, len, false);
>     >     +       #endif
>     >             len = control_sdu_tx_flow(sdev, rx->rx_buf, len);
>     >
>     >             spin_lock_irqsave(&rx->lock, flags);
>     >     diff --git a/drivers/staging/gdm72xx/gdm_usb.c
>     >     b/drivers/staging/gdm72xx/gdm_usb.c
>     >     index 971976c..bfd347a 100644
>     >     --- a/drivers/staging/gdm72xx/gdm_usb.c
>     >     +++ b/drivers/staging/gdm72xx/gdm_usb.c
>     >     @@ -348,8 +348,11 @@ static int gdm_usb_send(void *priv_dev, void
>     >     *data, int len,
>     >             usb_fill_bulk_urb(t->urb, usbdev, usb_sndbulkpipe(usbdev,
>     >     1), t->buf,
>     >                               len + padding,
>     gdm_usb_send_complete, t);
>     >
>     >     +       #if defined(GDM72xx_DEBUG)
>     >             print_hex_dump_debug("usb_send: ", DUMP_PREFIX_NONE,
>     16, 1,
>     >     t->buf,
>     >                                  len + padding, false);
>     >     +       #endif
>     >     +
>     >      #ifdef CONFIG_WIMAX_GDM72XX_USB_PM
>     >             if (usbdev->state & USB_STATE_SUSPENDED) {
>     >                     list_add_tail(&t->p_list, &tx->pending_list);
>     >     @@ -427,8 +430,12 @@ static void gdm_usb_rcv_complete(struct
>     urb *urb)
>     >
>     >             if (!urb->status) {
>     >                     cmd_evt = (r->buf[0] << 8) | (r->buf[1]);
>     >     +
>     >     +               #if defined(GDM72xx_DEBUG)
>     >                     print_hex_dump_debug("usb_receive: ",
>     >     DUMP_PREFIX_NONE, 16, 1,
>     >                                          r->buf,
>     urb->actual_length, false);
>     >     +               #endif
>     >     +
>     >                     if (cmd_evt == WIMAX_SDU_TX_FLOW) {
>     >                             if (r->buf[4] == 0) {
>     >                                     dev_dbg(&dev->dev, "WIMAX ==> STOP
>     >     SDU TX\n");
>     >     diff --git a/drivers/staging/gdm72xx/gdm_wimax.c
>     >     b/drivers/staging/gdm72xx/gdm_wimax.c
>     >     index c2e6bfe..63a760b 100644
>     >     --- a/drivers/staging/gdm72xx/gdm_wimax.c
>     >     +++ b/drivers/staging/gdm72xx/gdm_wimax.c
>     >     @@ -61,6 +61,7 @@ static u8 gdm_wimax_macaddr[6] = {0x00, 0x0a,
>     >     0x3b, 0xf0, 0x01, 0x30};
>     >      static void gdm_wimax_ind_fsm_update(struct net_device *dev,
>     struct
>     >     fsm_s *fsm);
>     >      static void gdm_wimax_ind_if_updown(struct net_device *dev,
>     int if_up);
>     >
>     >     +#if defined(GDM72xx_DEBUG)
>     >      static const char *get_protocol_name(u16 protocol)
>     >      {
>     >             static char buf[32];
>     >     @@ -162,6 +163,7 @@ static void dump_eth_packet(struct net_device
>     >     *dev, const char *title,
>     >
>     >             print_hex_dump_debug("", DUMP_PREFIX_NONE, 16, 1,
>     data, len,
>     >     false);
>     >      }
>     >     +#endif
>     >
>     >      static inline int gdm_wimax_header(struct sk_buff **pskb)
>     >      {
>     >     @@ -369,7 +371,9 @@ static int gdm_wimax_tx(struct sk_buff *skb,
>     >     struct net_device *dev)
>     >      {
>     >             int ret = 0;
>     >
>     >     +       #if defined(GDM72xx_DEBUG)
>     >             dump_eth_packet(dev, "TX", skb->data, skb->len);
>     >     +       #endif
>     >
>     >             ret = gdm_wimax_header(&skb);
>     >             if (ret < 0) {
>     >     @@ -698,7 +702,9 @@ static void gdm_wimax_netif_rx(struct
>     net_device
>     >     *dev, char *buf, int len)
>     >             struct sk_buff *skb;
>     >             int ret;
>     >
>     >     +       #if defined(GDM72xx_DEBUG)
>     >             dump_eth_packet(dev, "RX", buf, len);
>     >     +       #endif
>     >
>     >             skb = dev_alloc_skb(len + 2);
>     >             if (!skb) {
>     >     diff --git a/drivers/staging/gdm72xx/gdm_wimax.h
>     >     b/drivers/staging/gdm72xx/gdm_wimax.h
>     >     index 7e2c888..4670729 100644
>     >     --- a/drivers/staging/gdm72xx/gdm_wimax.h
>     >     +++ b/drivers/staging/gdm72xx/gdm_wimax.h
>     >     @@ -23,6 +23,8 @@
>     >
>     >      #define DRIVER_VERSION         "3.2.3"
>     >
>     >     +/* #define GDM72xx_DEBUG       1 */
>     >     +
>     >      #define H2L(x)         __cpu_to_le16(x)
>     >      #define L2H(x)         __le16_to_cpu(x)
>     >      #define DH2L(x)                __cpu_to_le32(x)
>     >     --
>     >     1.8.4
>     >
>     >
>     > Hi Michalis,
>     >
>     > Would be it better to control this symbol Kconfig? Also, it should be
>     > all caps like GDM72XX_DEBUG
>     >
>     > Thanks,
>     > Ben
>     >
> 
>     Yes we could add a new option in Kconfig in consistence with other
>     drivers. There are also some debug messages displayed through dev_dbg()
>     and netdev_dbg(), that are controlled through CONFIG_DYNAMIC_DEBUG. Any
>     idea whether these should be also enclosed by GDM72XX_DEBUG, or left to
>     be handled separately by CONFIG_DYNAMIC_DEBUG only?
> 
>     The capitals was a slip, thanks for spotting this. I'll submit an
>     updated patch to fix this too.
> 
> 
> Actually, dev_dbg, netdev_dbg and print_hex_dump_debug are already
> conditioned upon CONFIG_DYNAMIC_DEBUG, so I don't think we should
> introduce another config option. With some rearrangement of the code, it
> may not be necessary to add these guards.
>  
> 

Ok, got it. With respect to rearrangements in code I assume you're
referring to dump_eth_packet() right? What do you recommend?