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

From: Michalis Pappas
Date: Thu Jul 03 2014 - 13:27:25 EST

Next message: Suman Anna: "Re: [PATCHv5 03/15] hwspinlock/core: maintain a list of registered hwspinlock banks"
Previous message: Bandan Das: "Re: [PATCH] KVM: nVMX: Fix IRQs inject to L2 which belong to L1 since race"
In reply to: Michalis Pappas: "Re: [PATCH] staging: gdm72xx: conditionally compile debug code"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On 07/01/2014 07:08 PM, Ben Chan wrote:
>
>
>
> On Tue, Jul 1, 2014 at 9:40 AM, Michalis Pappas <mpappas@xxxxxxxxxxx
> <mailto:mpappas@xxxxxxxxxxx>> wrote:
>
> On 07/01/2014 04:30 PM, Ben Chan wrote:
> >
> >
> >
> > On Tue, Jul 1, 2014 at 6:00 AM, Michalis Pappas
> <mpappas@xxxxxxxxxxx <mailto:mpappas@xxxxxxxxxxx>
> > <mailto:mpappas@xxxxxxxxxxx <mailto:mpappas@xxxxxxxxxxx>>> wrote:
> >
> > Signed-off-by: Michalis Pappas <mpappas@xxxxxxxxxxx
> <mailto:mpappas@xxxxxxxxxxx>
> > <mailto:mpappas@xxxxxxxxxxx <mailto:mpappas@xxxxxxxxxxx>>>
> > ---
> > 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?

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Next message: Suman Anna: "Re: [PATCHv5 03/15] hwspinlock/core: maintain a list of registered hwspinlock banks"
Previous message: Bandan Das: "Re: [PATCH] KVM: nVMX: Fix IRQs inject to L2 which belong to L1 since race"
In reply to: Michalis Pappas: "Re: [PATCH] staging: gdm72xx: conditionally compile debug code"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]