Remove code surrounded by otherwise unused #define LOOPBACK_TEST
Signed-off-by: Michalis Pappas <[email protected]>
---
drivers/staging/gdm72xx/gdm_wimax.c | 18 ------------------
1 file changed, 18 deletions(-)
diff --git a/drivers/staging/gdm72xx/gdm_wimax.c b/drivers/staging/gdm72xx/gdm_wimax.c
index 4148013..c2e6bfe 100644
--- a/drivers/staging/gdm72xx/gdm_wimax.c
+++ b/drivers/staging/gdm72xx/gdm_wimax.c
@@ -368,8 +368,6 @@ int gdm_wimax_send_tx(struct sk_buff *skb, struct net_device *dev)
static int gdm_wimax_tx(struct sk_buff *skb, struct net_device *dev)
{
int ret = 0;
- struct nic *nic = netdev_priv(dev);
- struct fsm_s *fsm = (struct fsm_s *)nic->sdk_data[SIOC_DATA_FSM].buf;
dump_eth_packet(dev, "TX", skb->data, skb->len);
@@ -379,17 +377,6 @@ static int gdm_wimax_tx(struct sk_buff *skb, struct net_device *dev)
return ret;
}
- #if !defined(LOOPBACK_TEST)
- if (!fsm) {
- netdev_err(dev, "ASSERTION ERROR: fsm is NULL!!\n");
- } else if (fsm->m_status != M_CONNECTED) {
- netdev_emerg(dev, "ASSERTION ERROR: Device is NOT ready. status=%d\n",
- fsm->m_status);
- kfree_skb(skb);
- return 0;
- }
- #endif
-
#if defined(CONFIG_WIMAX_GDM72XX_QOS)
ret = gdm_qos_send_hci_pkt(skb, dev);
#else
@@ -919,12 +906,7 @@ int register_wimax_device(struct phy_dev *phy_dev, struct device *pdev)
if (ret)
goto cleanup;
- #if defined(LOOPBACK_TEST)
- netif_start_queue(dev);
- netif_carrier_on(dev);
- #else
netif_carrier_off(dev);
- #endif
#ifdef CONFIG_WIMAX_GDM72XX_QOS
gdm_qos_init(nic);
--
1.8.4
Signed-off-by: Michalis Pappas <[email protected]>
---
drivers/staging/gdm72xx/gdm_wimax.c | 10 +++-------
drivers/staging/gdm72xx/hci.h | 6 ++++++
2 files changed, 9 insertions(+), 7 deletions(-)
diff --git a/drivers/staging/gdm72xx/gdm_wimax.c b/drivers/staging/gdm72xx/gdm_wimax.c
index 63a760b..1fc64a9 100644
--- a/drivers/staging/gdm72xx/gdm_wimax.c
+++ b/drivers/staging/gdm72xx/gdm_wimax.c
@@ -600,10 +600,6 @@ static void gdm_wimax_prepare_device(struct net_device *dev)
u16 len = 0;
u32 val = 0;
- #define BIT_MULTI_CS 0
- #define BIT_WIMAX 1
- #define BIT_QOS 2
- #define BIT_AGGREGATION 3
/* GetInformation mac address */
len = 0;
@@ -612,12 +608,12 @@ static void gdm_wimax_prepare_device(struct net_device *dev)
hci->length = H2B(len);
gdm_wimax_send(nic, hci, HCI_HEADER_SIZE+len);
- val = (1<<BIT_WIMAX) | (1<<BIT_MULTI_CS);
+ val = (1 << T_CAPABILITY_BIT_WIMAX) | (1 << T_CAPABILITY_BIT_MULTI_CS);
#if defined(CONFIG_WIMAX_GDM72XX_QOS)
- val |= (1<<BIT_QOS);
+ val |= (1 << T_CAPABILITY_BIT_QOS);
#endif
#if defined(CONFIG_WIMAX_GDM72XX_WIMAX2)
- val |= (1<<BIT_AGGREGATION);
+ val |= (1 << T_CAPABILITY_BIT_AGGREGATION);
#endif
/* Set capability */
diff --git a/drivers/staging/gdm72xx/hci.h b/drivers/staging/gdm72xx/hci.h
index 059ba00..0cdd1fc 100644
--- a/drivers/staging/gdm72xx/hci.h
+++ b/drivers/staging/gdm72xx/hci.h
@@ -198,6 +198,12 @@
#define T_FFTSIZE (0xda | (4 << 16))
#define T_DUPLEX_MODE (0xdb | (4 << 16))
+/* T_CAPABILITY */
+#define T_CAPABILITY_BIT_MULTI_CS 0
+#define T_CAPABILITY_BIT_WIMAX 1
+#define T_CAPABILITY_BIT_QOS 2
+#define T_CAPABILITY_BIT_AGGREGATION 3
+
struct hci_s {
unsigned short cmd_evt;
unsigned short length;
--
1.8.4
Signed-off-by: Michalis Pappas <[email protected]>
---
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
On Tue, Jul 1, 2014 at 6:00 AM, Michalis Pappas <[email protected]> wrote:
> Signed-off-by: Michalis Pappas <[email protected]>
> ---
> drivers/staging/gdm72xx/gdm_wimax.c | 10 +++-------
> drivers/staging/gdm72xx/hci.h | 6 ++++++
> 2 files changed, 9 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/staging/gdm72xx/gdm_wimax.c b/drivers/staging/gdm72xx/gdm_wimax.c
> index 63a760b..1fc64a9 100644
> --- a/drivers/staging/gdm72xx/gdm_wimax.c
> +++ b/drivers/staging/gdm72xx/gdm_wimax.c
> @@ -600,10 +600,6 @@ static void gdm_wimax_prepare_device(struct net_device *dev)
> u16 len = 0;
> u32 val = 0;
>
> - #define BIT_MULTI_CS 0
> - #define BIT_WIMAX 1
> - #define BIT_QOS 2
> - #define BIT_AGGREGATION 3
>
> /* GetInformation mac address */
> len = 0;
> @@ -612,12 +608,12 @@ static void gdm_wimax_prepare_device(struct net_device *dev)
> hci->length = H2B(len);
> gdm_wimax_send(nic, hci, HCI_HEADER_SIZE+len);
>
> - val = (1<<BIT_WIMAX) | (1<<BIT_MULTI_CS);
> + val = (1 << T_CAPABILITY_BIT_WIMAX) | (1 << T_CAPABILITY_BIT_MULTI_CS);
> #if defined(CONFIG_WIMAX_GDM72XX_QOS)
> - val |= (1<<BIT_QOS);
> + val |= (1 << T_CAPABILITY_BIT_QOS);
> #endif
> #if defined(CONFIG_WIMAX_GDM72XX_WIMAX2)
> - val |= (1<<BIT_AGGREGATION);
> + val |= (1 << T_CAPABILITY_BIT_AGGREGATION);
> #endif
[Ben] We can simply the code by defining these constants as bitmasks
instead, e.g.
T_CAPABILITY_MULTI_CS (1 << 0)
T_CAPABILITY_WIMAX (1 << 1)
T_CAPABILITY_QOS (1 << 2)
T_CAPABILITY_AGGREGATION (1 << 3)
> /* Set capability */
> diff --git a/drivers/staging/gdm72xx/hci.h b/drivers/staging/gdm72xx/hci.h
> index 059ba00..0cdd1fc 100644
> --- a/drivers/staging/gdm72xx/hci.h
> +++ b/drivers/staging/gdm72xx/hci.h
> @@ -198,6 +198,12 @@
> #define T_FFTSIZE (0xda | (4 << 16))
> #define T_DUPLEX_MODE (0xdb | (4 << 16))
>
> +/* T_CAPABILITY */
> +#define T_CAPABILITY_BIT_MULTI_CS 0
> +#define T_CAPABILITY_BIT_WIMAX 1
> +#define T_CAPABILITY_BIT_QOS 2
> +#define T_CAPABILITY_BIT_AGGREGATION 3
> +
> struct hci_s {
> unsigned short cmd_evt;
> unsigned short length;
> --
> 1.8.4
>
> _______________________________________________
> devel mailing list
> [email protected]
> http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
On 07/01/2014 04:37 PM, Ben Chan wrote:
> On Tue, Jul 1, 2014 at 6:00 AM, Michalis Pappas <[email protected]> wrote:
>> Signed-off-by: Michalis Pappas <[email protected]>
>> ---
>> drivers/staging/gdm72xx/gdm_wimax.c | 10 +++-------
>> drivers/staging/gdm72xx/hci.h | 6 ++++++
>> 2 files changed, 9 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/staging/gdm72xx/gdm_wimax.c b/drivers/staging/gdm72xx/gdm_wimax.c
>> index 63a760b..1fc64a9 100644
>> --- a/drivers/staging/gdm72xx/gdm_wimax.c
>> +++ b/drivers/staging/gdm72xx/gdm_wimax.c
>> @@ -600,10 +600,6 @@ static void gdm_wimax_prepare_device(struct net_device *dev)
>> u16 len = 0;
>> u32 val = 0;
>>
>> - #define BIT_MULTI_CS 0
>> - #define BIT_WIMAX 1
>> - #define BIT_QOS 2
>> - #define BIT_AGGREGATION 3
>>
>> /* GetInformation mac address */
>> len = 0;
>> @@ -612,12 +608,12 @@ static void gdm_wimax_prepare_device(struct net_device *dev)
>> hci->length = H2B(len);
>> gdm_wimax_send(nic, hci, HCI_HEADER_SIZE+len);
>>
>> - val = (1<<BIT_WIMAX) | (1<<BIT_MULTI_CS);
>> + val = (1 << T_CAPABILITY_BIT_WIMAX) | (1 << T_CAPABILITY_BIT_MULTI_CS);
>> #if defined(CONFIG_WIMAX_GDM72XX_QOS)
>> - val |= (1<<BIT_QOS);
>> + val |= (1 << T_CAPABILITY_BIT_QOS);
>> #endif
>> #if defined(CONFIG_WIMAX_GDM72XX_WIMAX2)
>> - val |= (1<<BIT_AGGREGATION);
>> + val |= (1 << T_CAPABILITY_BIT_AGGREGATION);
>> #endif
>
> [Ben] We can simply the code by defining these constants as bitmasks
> instead, e.g.
>
> T_CAPABILITY_MULTI_CS (1 << 0)
> T_CAPABILITY_WIMAX (1 << 1)
> T_CAPABILITY_QOS (1 << 2)
> T_CAPABILITY_AGGREGATION (1 << 3)
>
Agreed, I'll submit a new patch.
On 07/01/2014 04:30 PM, Ben Chan wrote:
>
>
>
> On Tue, Jul 1, 2014 at 6:00 AM, Michalis Pappas <[email protected]
> <mailto:[email protected]>> wrote:
>
> Signed-off-by: Michalis Pappas <[email protected]
> <mailto:[email protected]>>
> ---
> 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.
Signed-off-by: Michalis Pappas <[email protected]>
---
drivers/staging/gdm72xx/gdm_wimax.c | 11 ++++-------
drivers/staging/gdm72xx/hci.h | 6 ++++++
2 files changed, 10 insertions(+), 7 deletions(-)
diff --git a/drivers/staging/gdm72xx/gdm_wimax.c b/drivers/staging/gdm72xx/gdm_wimax.c
index 4148013..50b7bf0 100644
--- a/drivers/staging/gdm72xx/gdm_wimax.c
+++ b/drivers/staging/gdm72xx/gdm_wimax.c
@@ -609,10 +609,7 @@ static void gdm_wimax_prepare_device(struct net_device *dev)
u16 len = 0;
u32 val = 0;
- #define BIT_MULTI_CS 0
- #define BIT_WIMAX 1
- #define BIT_QOS 2
- #define BIT_AGGREGATION 3
+
/* GetInformation mac address */
len = 0;
@@ -621,12 +618,12 @@ static void gdm_wimax_prepare_device(struct net_device *dev)
hci->length = H2B(len);
gdm_wimax_send(nic, hci, HCI_HEADER_SIZE+len);
- val = (1<<BIT_WIMAX) | (1<<BIT_MULTI_CS);
+ val = (1 << T_CAPABILITY_WIMAX) | (1 << T_CAPABILITY_MULTI_CS);
#if defined(CONFIG_WIMAX_GDM72XX_QOS)
- val |= (1<<BIT_QOS);
+ val |= (1 << T_CAPABILITY_QOS);
#endif
#if defined(CONFIG_WIMAX_GDM72XX_WIMAX2)
- val |= (1<<BIT_AGGREGATION);
+ val |= (1 << T_CAPABILITY_AGGREGATION);
#endif
/* Set capability */
diff --git a/drivers/staging/gdm72xx/hci.h b/drivers/staging/gdm72xx/hci.h
index 059ba00..4dd253d 100644
--- a/drivers/staging/gdm72xx/hci.h
+++ b/drivers/staging/gdm72xx/hci.h
@@ -198,6 +198,12 @@
#define T_FFTSIZE (0xda | (4 << 16))
#define T_DUPLEX_MODE (0xdb | (4 << 16))
+/* T_CAPABILITY */
+#define T_CAPABILITY_MULTI_CS (1 << 0)
+#define T_CAPABILITY_WIMAX (1 << 1)
+#define T_CAPABILITY_QOS (1 << 2)
+#define T_CAPABILITY_AGGREGATION (1 << 3)
+
struct hci_s {
unsigned short cmd_evt;
unsigned short length;
--
1.8.4
On 07/01/2014 07:08 PM, Ben Chan wrote:
>
>
>
> On Tue, Jul 1, 2014 at 9:40 AM, Michalis Pappas <[email protected]
> <mailto:[email protected]>> wrote:
>
> On 07/01/2014 04:30 PM, Ben Chan wrote:
> >
> >
> >
> > On Tue, Jul 1, 2014 at 6:00 AM, Michalis Pappas
> <[email protected] <mailto:[email protected]>
> > <mailto:[email protected] <mailto:[email protected]>>> wrote:
> >
> > Signed-off-by: Michalis Pappas <[email protected]
> <mailto:[email protected]>
> > <mailto:[email protected] <mailto:[email protected]>>>
> > ---
> > 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?
On Wed, Jul 02, 2014 at 11:18:07AM +0100, Michalis Pappas wrote:
> Signed-off-by: Michalis Pappas <[email protected]>
> ---
> drivers/staging/gdm72xx/gdm_wimax.c | 11 ++++-------
> drivers/staging/gdm72xx/hci.h | 6 ++++++
> 2 files changed, 10 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/staging/gdm72xx/gdm_wimax.c b/drivers/staging/gdm72xx/gdm_wimax.c
> index 4148013..50b7bf0 100644
> --- a/drivers/staging/gdm72xx/gdm_wimax.c
> +++ b/drivers/staging/gdm72xx/gdm_wimax.c
> @@ -609,10 +609,7 @@ static void gdm_wimax_prepare_device(struct net_device *dev)
> u16 len = 0;
> u32 val = 0;
>
> - #define BIT_MULTI_CS 0
> - #define BIT_WIMAX 1
> - #define BIT_QOS 2
> - #define BIT_AGGREGATION 3
> +
>
> /* GetInformation mac address */
> len = 0;
> @@ -621,12 +618,12 @@ static void gdm_wimax_prepare_device(struct net_device *dev)
> hci->length = H2B(len);
> gdm_wimax_send(nic, hci, HCI_HEADER_SIZE+len);
>
> - val = (1<<BIT_WIMAX) | (1<<BIT_MULTI_CS);
> + val = (1 << T_CAPABILITY_WIMAX) | (1 << T_CAPABILITY_MULTI_CS);
Double shifting here... It should just be:
val = T_CAPABILITY_WIMAX | T_CAPABILITY_MULTI_CS;
This bug feels like something a static checker should find. Let me test
that.
regards,
dan carpenter
On 07/09/2014 11:26 AM, Dan Carpenter wrote:
>> /* GetInformation mac address */
>> len = 0;
>> @@ -621,12 +618,12 @@ static void gdm_wimax_prepare_device(struct net_device *dev)
>> hci->length = H2B(len);
>> gdm_wimax_send(nic, hci, HCI_HEADER_SIZE+len);
>>
>> - val = (1<<BIT_WIMAX) | (1<<BIT_MULTI_CS);
>> + val = (1 << T_CAPABILITY_WIMAX) | (1 << T_CAPABILITY_MULTI_CS);
>
> Double shifting here... It should just be:
>
> val = T_CAPABILITY_WIMAX | T_CAPABILITY_MULTI_CS;
>
Ooops! Thanks for spotting that, will submit a corrected patch.
M.
Signed-off-by: Michalis Pappas <[email protected]>
---
drivers/staging/gdm72xx/gdm_wimax.c | 11 +++--------
drivers/staging/gdm72xx/hci.h | 6 ++++++
2 files changed, 9 insertions(+), 8 deletions(-)
diff --git a/drivers/staging/gdm72xx/gdm_wimax.c b/drivers/staging/gdm72xx/gdm_wimax.c
index 4148013..5748d39 100644
--- a/drivers/staging/gdm72xx/gdm_wimax.c
+++ b/drivers/staging/gdm72xx/gdm_wimax.c
@@ -609,11 +609,6 @@ static void gdm_wimax_prepare_device(struct net_device *dev)
u16 len = 0;
u32 val = 0;
- #define BIT_MULTI_CS 0
- #define BIT_WIMAX 1
- #define BIT_QOS 2
- #define BIT_AGGREGATION 3
-
/* GetInformation mac address */
len = 0;
hci->cmd_evt = H2B(WIMAX_GET_INFO);
@@ -621,12 +616,12 @@ static void gdm_wimax_prepare_device(struct net_device *dev)
hci->length = H2B(len);
gdm_wimax_send(nic, hci, HCI_HEADER_SIZE+len);
- val = (1<<BIT_WIMAX) | (1<<BIT_MULTI_CS);
+ val = T_CAPABILITY_WIMAX | T_CAPABILITY_MULTI_CS;
#if defined(CONFIG_WIMAX_GDM72XX_QOS)
- val |= (1<<BIT_QOS);
+ val |= T_CAPABILITY_QOS;
#endif
#if defined(CONFIG_WIMAX_GDM72XX_WIMAX2)
- val |= (1<<BIT_AGGREGATION);
+ val |= T_CAPABILITY_AGGREGATION;
#endif
/* Set capability */
diff --git a/drivers/staging/gdm72xx/hci.h b/drivers/staging/gdm72xx/hci.h
index 059ba00..4dd253d 100644
--- a/drivers/staging/gdm72xx/hci.h
+++ b/drivers/staging/gdm72xx/hci.h
@@ -198,6 +198,12 @@
#define T_FFTSIZE (0xda | (4 << 16))
#define T_DUPLEX_MODE (0xdb | (4 << 16))
+/* T_CAPABILITY */
+#define T_CAPABILITY_MULTI_CS (1 << 0)
+#define T_CAPABILITY_WIMAX (1 << 1)
+#define T_CAPABILITY_QOS (1 << 2)
+#define T_CAPABILITY_AGGREGATION (1 << 3)
+
struct hci_s {
unsigned short cmd_evt;
unsigned short length;
--
1.8.4
On Tue, Jul 01, 2014 at 02:00:15PM +0100, Michalis Pappas wrote:
> Signed-off-by: Michalis Pappas <[email protected]>
> ---
> 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
Ick, no, never put #ifdef in .c code if you can help it. For stuff like
this, just rely on the dynamic debug core and use the pr_debug and
dev_dbg() calls, like the driver is doing, so all should be fine.
> 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
This should be moved to use dev_dbg(), along with the other calls to
this function in this file.
thanks,
greg k-h
On Wed, Jul 09, 2014 at 07:31:22PM +0100, Michalis Pappas wrote:
> Signed-off-by: Michalis Pappas <[email protected]>
> ---
> drivers/staging/gdm72xx/gdm_wimax.c | 11 +++--------
> drivers/staging/gdm72xx/hci.h | 6 ++++++
> 2 files changed, 9 insertions(+), 8 deletions(-)
This no longer applies cleanly either, can you refresh and resend?
thanks,
greg k-h
Signed-off-by: Michalis Pappas <[email protected]>
---
drivers/staging/gdm72xx/gdm_wimax.c | 11 +++--------
drivers/staging/gdm72xx/hci.h | 6 ++++++
2 files changed, 9 insertions(+), 8 deletions(-)
diff --git a/drivers/staging/gdm72xx/gdm_wimax.c b/drivers/staging/gdm72xx/gdm_wimax.c
index 0f71d41..1693cc0 100644
--- a/drivers/staging/gdm72xx/gdm_wimax.c
+++ b/drivers/staging/gdm72xx/gdm_wimax.c
@@ -591,11 +591,6 @@ static void gdm_wimax_prepare_device(struct net_device *dev)
u32 val = 0;
__be32 val_be32;
- #define BIT_MULTI_CS 0
- #define BIT_WIMAX 1
- #define BIT_QOS 2
- #define BIT_AGGREGATION 3
-
/* GetInformation mac address */
len = 0;
hci->cmd_evt = cpu_to_be16(WIMAX_GET_INFO);
@@ -603,12 +598,12 @@ static void gdm_wimax_prepare_device(struct net_device *dev)
hci->length = cpu_to_be16(len);
gdm_wimax_send(nic, hci, HCI_HEADER_SIZE+len);
- val = (1<<BIT_WIMAX) | (1<<BIT_MULTI_CS);
+ val = T_CAPABILITY_WIMAX | T_CAPABILITY_MULTI_CS;
#if defined(CONFIG_WIMAX_GDM72XX_QOS)
- val |= (1<<BIT_QOS);
+ val |= T_CAPABILITY_QOS;
#endif
#if defined(CONFIG_WIMAX_GDM72XX_WIMAX2)
- val |= (1<<BIT_AGGREGATION);
+ val |= T_CAPABILITY_AGGREGATION;
#endif
/* Set capability */
diff --git a/drivers/staging/gdm72xx/hci.h b/drivers/staging/gdm72xx/hci.h
index dd2931d..10a6bfa 100644
--- a/drivers/staging/gdm72xx/hci.h
+++ b/drivers/staging/gdm72xx/hci.h
@@ -198,6 +198,12 @@
#define T_FFTSIZE (0xda | (4 << 16))
#define T_DUPLEX_MODE (0xdb | (4 << 16))
+/* T_CAPABILITY */
+#define T_CAPABILITY_MULTI_CS (1 << 0)
+#define T_CAPABILITY_WIMAX (1 << 1)
+#define T_CAPABILITY_QOS (1 << 2)
+#define T_CAPABILITY_AGGREGATION (1 << 3)
+
struct hci_s {
__be16 cmd_evt;
__be16 length;
--
1.8.4
On 07/09/2014 07:51 PM, Greg KH wrote:
> On Tue, Jul 01, 2014 at 02:00:15PM +0100, Michalis Pappas wrote:
>> Signed-off-by: Michalis Pappas <[email protected]>
>> ---
>> 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
>
> Ick, no, never put #ifdef in .c code if you can help it. For stuff like
> this, just rely on the dynamic debug core and use the pr_debug and
> dev_dbg() calls, like the driver is doing, so all should be fine.
>
But how about those cases where debug code consists of more than a
simple call to pr_debug() / dev_dbg()?
For instance consider dump_eth_packet(), defined in gdm_wimax.c. This
function is called every time a packet is received or transmitted, and
calls other helper functions too (get_protocol_name(),
get_ip_protocol_name(), get_port_name()).
Doesn't all this debug logic provide an overhead to the tx / rx functions?
On Wed, Jul 09, 2014 at 08:52:07PM +0100, Michalis Pappas wrote:
> On 07/09/2014 07:51 PM, Greg KH wrote:
> > On Tue, Jul 01, 2014 at 02:00:15PM +0100, Michalis Pappas wrote:
> >> Signed-off-by: Michalis Pappas <[email protected]>
> >> ---
> >> 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
> >
> > Ick, no, never put #ifdef in .c code if you can help it. For stuff like
> > this, just rely on the dynamic debug core and use the pr_debug and
> > dev_dbg() calls, like the driver is doing, so all should be fine.
> >
>
> But how about those cases where debug code consists of more than a
> simple call to pr_debug() / dev_dbg()?
Then that code is wrong :)
> For instance consider dump_eth_packet(), defined in gdm_wimax.c. This
> function is called every time a packet is received or transmitted, and
> calls other helper functions too (get_protocol_name(),
> get_ip_protocol_name(), get_port_name()).
>
> Doesn't all this debug logic provide an overhead to the tx / rx functions?
Yes, so much so it doesn't make sense to even have that type of
function, right?
greg k-h
On 07/09/2014 07:51 PM, Greg KH wrote:
>> 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
>
> This should be moved to use dev_dbg(), along with the other calls to
> this function in this file.
>
But dev_dbg() gets eventually to be printk(), which cannot print the
buffer, so using print_hex_dump_debug() seems to be correct for this
case, no?
On Wed, Jul 16, 2014 at 09:40:18PM +0100, Michalis Pappas wrote:
> On 07/09/2014 07:51 PM, Greg KH wrote:
> >> 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
> >
> > This should be moved to use dev_dbg(), along with the other calls to
> > this function in this file.
> >
>
> But dev_dbg() gets eventually to be printk(), which cannot print the
> buffer, so using print_hex_dump_debug() seems to be correct for this
> case, no?
No, you don't want to print the message unless debugging is enabled, and
dev_dbg() uses the proper in-kernel dynamic debug infrastructure. There
should never be a separate config option for debugging a driver, that
ensures that no user will ever be able to help you out with it.
So delete the ifdef stuff, and the config option, and just use the
proper in-kernel infrastructure for this.
thanks,
greg k-h
On 07/16/2014 09:50 PM, Greg KH wrote:
> On Wed, Jul 16, 2014 at 09:40:18PM +0100, Michalis Pappas wrote:
>> On 07/09/2014 07:51 PM, Greg KH wrote:
>>>> 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
>>>
>>> This should be moved to use dev_dbg(), along with the other calls to
>>> this function in this file.
>>>
>>
>> But dev_dbg() gets eventually to be printk(), which cannot print the
>> buffer, so using print_hex_dump_debug() seems to be correct for this
>> case, no?
>
> No, you don't want to print the message unless debugging is enabled, and
> dev_dbg() uses the proper in-kernel dynamic debug infrastructure. There
> should never be a separate config option for debugging a driver, that
> ensures that no user will ever be able to help you out with it.
>
> So delete the ifdef stuff, and the config option, and just use the
> proper in-kernel infrastructure for this.
>
> thanks,
>
> greg k-h
>
Ok, I agree on the ifdef stuff. My question was regarding your
suggestion above to replace print_hex_debug() with dev_dbg()
On Wed, Jul 16, 2014 at 11:03:06PM +0100, Michalis Pappas wrote:
> On 07/16/2014 09:50 PM, Greg KH wrote:
> > On Wed, Jul 16, 2014 at 09:40:18PM +0100, Michalis Pappas wrote:
> >> On 07/09/2014 07:51 PM, Greg KH wrote:
> >>>> 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
> >>>
> >>> This should be moved to use dev_dbg(), along with the other calls to
> >>> this function in this file.
> >>>
> >>
> >> But dev_dbg() gets eventually to be printk(), which cannot print the
> >> buffer, so using print_hex_dump_debug() seems to be correct for this
> >> case, no?
> >
> > No, you don't want to print the message unless debugging is enabled, and
> > dev_dbg() uses the proper in-kernel dynamic debug infrastructure. There
> > should never be a separate config option for debugging a driver, that
> > ensures that no user will ever be able to help you out with it.
> >
> > So delete the ifdef stuff, and the config option, and just use the
> > proper in-kernel infrastructure for this.
> >
> > thanks,
> >
> > greg k-h
> >
>
> Ok, I agree on the ifdef stuff. My question was regarding your
> suggestion above to replace print_hex_debug() with dev_dbg()
You want the device name/id/label to show up as well, that is why you
should use the dev_dbg() version, print_hex_dump() does not take a
struct device *, so the user has no idea what device this data was
coming from.
thanks,
greg k-h
On Wed, 2014-07-16 at 15:10 -0700, Greg KH wrote:
> On Wed, Jul 16, 2014 at 11:03:06PM +0100, Michalis Pappas wrote:
[]
> > Ok, I agree on the ifdef stuff. My question was regarding your
> > suggestion above to replace print_hex_debug() with dev_dbg()
>
> You want the device name/id/label to show up as well, that is why you
> should use the dev_dbg() version, print_hex_dump() does not take a
> struct device *, so the user has no idea what device this data was
> coming from.
But Michalis could alway add something like:
dev_hex_dump()
and
dev_dbg_hex_dump()
On Wed, Jul 16, 2014 at 03:46:09PM -0700, Joe Perches wrote:
> On Wed, 2014-07-16 at 15:10 -0700, Greg KH wrote:
> > On Wed, Jul 16, 2014 at 11:03:06PM +0100, Michalis Pappas wrote:
> []
> > > Ok, I agree on the ifdef stuff. My question was regarding your
> > > suggestion above to replace print_hex_debug() with dev_dbg()
> >
> > You want the device name/id/label to show up as well, that is why you
> > should use the dev_dbg() version, print_hex_dump() does not take a
> > struct device *, so the user has no idea what device this data was
> > coming from.
>
> But Michalis could alway add something like:
> dev_hex_dump()
> and
> dev_dbg_hex_dump()
>
With the built-in "hex dump primitive" in printk(), why would you want
to do that? You shouldn't be putting more than 64 bytes in a single
printk message in a hex dump, if you want to do more, use debugfs.
thanks,
greg k-h
On Wed, 2014-07-16 at 15:57 -0700, Greg KH wrote:
> On Wed, Jul 16, 2014 at 03:46:09PM -0700, Joe Perches wrote:
> > On Wed, 2014-07-16 at 15:10 -0700, Greg KH wrote:
> > > On Wed, Jul 16, 2014 at 11:03:06PM +0100, Michalis Pappas wrote:
> > []
> > > > Ok, I agree on the ifdef stuff. My question was regarding your
> > > > suggestion above to replace print_hex_debug() with dev_dbg()
> > >
> > > You want the device name/id/label to show up as well, that is why you
> > > should use the dev_dbg() version, print_hex_dump() does not take a
> > > struct device *, so the user has no idea what device this data was
> > > coming from.
> >
> > But Michalis could alway add something like:
> > dev_hex_dump()
> > and
> > dev_dbg_hex_dump()
> >
>
> With the built-in "hex dump primitive" in printk(), why would you want
> to do that? You shouldn't be putting more than 64 bytes in a single
> printk message in a hex dump, if you want to do more, use debugfs.
%p*h doesn't also emit "ascii or dots" for one.
I agree %p*h should be preferred but people do
all sorts of strange things in debugging printks.