2014-06-20 04:48:27

by Bing Zhao

[permalink] [raw]
Subject: [PATCH v3] mwifiex: Use the proper interfaces

From: Thomas Gleixner <[email protected]>

Why is converting time formats so desired if there are proper
interfaces for this?

Signed-off-by: Thomas Gleixner <[email protected]>
Acked-by: Bing Zhao <[email protected]>
---
v3: don't use net_timedelta() (Johannes Berg)
v2: remove unused variables
v1: wireless: mwifiex: Use the proper interfaces

drivers/net/wireless/mwifiex/cfg80211.c | 4 +---
drivers/net/wireless/mwifiex/main.c | 4 +---
drivers/net/wireless/mwifiex/tdls.c | 8 ++------
drivers/net/wireless/mwifiex/uap_txrx.c | 4 +---
drivers/net/wireless/mwifiex/wmm.c | 9 +--------
5 files changed, 6 insertions(+), 23 deletions(-)

diff --git a/drivers/net/wireless/mwifiex/cfg80211.c b/drivers/net/wireless/mwifiex/cfg80211.c
index 6ec2ee3..1a72b2f 100644
--- a/drivers/net/wireless/mwifiex/cfg80211.c
+++ b/drivers/net/wireless/mwifiex/cfg80211.c
@@ -121,7 +121,6 @@ mwifiex_form_mgmt_frame(struct sk_buff *skb, const u8 *buf, size_t len)
u8 addr[ETH_ALEN] = {0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF};
u16 pkt_len;
u32 tx_control = 0, pkt_type = PKT_TYPE_MGMT;
- struct timeval tv;

pkt_len = len + ETH_ALEN;

@@ -143,8 +142,7 @@ mwifiex_form_mgmt_frame(struct sk_buff *skb, const u8 *buf, size_t len)
len - sizeof(struct ieee80211_hdr_3addr));

skb->priority = LOW_PRIO_TID;
- do_gettimeofday(&tv);
- skb->tstamp = timeval_to_ktime(tv);
+ __net_timestamp(skb);

return 0;
}
diff --git a/drivers/net/wireless/mwifiex/main.c b/drivers/net/wireless/mwifiex/main.c
index 1261d9a..657504c 100644
--- a/drivers/net/wireless/mwifiex/main.c
+++ b/drivers/net/wireless/mwifiex/main.c
@@ -609,7 +609,6 @@ mwifiex_hard_start_xmit(struct sk_buff *skb, struct net_device *dev)
struct mwifiex_private *priv = mwifiex_netdev_get_priv(dev);
struct sk_buff *new_skb;
struct mwifiex_txinfo *tx_info;
- struct timeval tv;

dev_dbg(priv->adapter->dev, "data: %lu BSS(%d-%d): Data <= kernel\n",
jiffies, priv->bss_type, priv->bss_num);
@@ -656,8 +655,7 @@ mwifiex_hard_start_xmit(struct sk_buff *skb, struct net_device *dev)
* firmware for aggregate delay calculation for stats and
* MSDU lifetime expiry.
*/
- do_gettimeofday(&tv);
- skb->tstamp = timeval_to_ktime(tv);
+ __net_timestamp(skb);

mwifiex_queue_tx_pkt(priv, skb);

diff --git a/drivers/net/wireless/mwifiex/tdls.c b/drivers/net/wireless/mwifiex/tdls.c
index e73034f..3efbcbe 100644
--- a/drivers/net/wireless/mwifiex/tdls.c
+++ b/drivers/net/wireless/mwifiex/tdls.c
@@ -530,7 +530,6 @@ int mwifiex_send_tdls_data_frame(struct mwifiex_private *priv, const u8 *peer,
{
struct sk_buff *skb;
struct mwifiex_txinfo *tx_info;
- struct timeval tv;
int ret;
u16 skb_len;

@@ -608,8 +607,7 @@ int mwifiex_send_tdls_data_frame(struct mwifiex_private *priv, const u8 *peer,
tx_info->bss_num = priv->bss_num;
tx_info->bss_type = priv->bss_type;

- do_gettimeofday(&tv);
- skb->tstamp = timeval_to_ktime(tv);
+ __net_timestamp(skb);
mwifiex_queue_tx_pkt(priv, skb);

return 0;
@@ -702,7 +700,6 @@ int mwifiex_send_tdls_action_frame(struct mwifiex_private *priv, const u8 *peer,
{
struct sk_buff *skb;
struct mwifiex_txinfo *tx_info;
- struct timeval tv;
u8 *pos;
u32 pkt_type, tx_control;
u16 pkt_len, skb_len;
@@ -767,8 +764,7 @@ int mwifiex_send_tdls_action_frame(struct mwifiex_private *priv, const u8 *peer,
pkt_len = skb->len - MWIFIEX_MGMT_FRAME_HEADER_SIZE - sizeof(pkt_len);
memcpy(skb->data + MWIFIEX_MGMT_FRAME_HEADER_SIZE, &pkt_len,
sizeof(pkt_len));
- do_gettimeofday(&tv);
- skb->tstamp = timeval_to_ktime(tv);
+ __net_timestamp(skb);
mwifiex_queue_tx_pkt(priv, skb);

return 0;
diff --git a/drivers/net/wireless/mwifiex/uap_txrx.c b/drivers/net/wireless/mwifiex/uap_txrx.c
index 57fa47d..ddfc3c6 100644
--- a/drivers/net/wireless/mwifiex/uap_txrx.c
+++ b/drivers/net/wireless/mwifiex/uap_txrx.c
@@ -96,7 +96,6 @@ static void mwifiex_uap_queue_bridged_pkt(struct mwifiex_private *priv,
struct sk_buff *new_skb;
struct mwifiex_txinfo *tx_info;
int hdr_chop;
- struct timeval tv;
struct ethhdr *p_ethhdr;

uap_rx_pd = (struct uap_rxpd *)(skb->data);
@@ -192,8 +191,7 @@ static void mwifiex_uap_queue_bridged_pkt(struct mwifiex_private *priv,
tx_info->pkt_len = skb->len;
}

- do_gettimeofday(&tv);
- skb->tstamp = timeval_to_ktime(tv);
+ __net_timestamp(skb);
mwifiex_wmm_add_buf_txqueue(priv, skb);
atomic_inc(&adapter->tx_pending);
atomic_inc(&adapter->pending_bridged_pkts);
diff --git a/drivers/net/wireless/mwifiex/wmm.c b/drivers/net/wireless/mwifiex/wmm.c
index 8cd123e..e8556b6 100644
--- a/drivers/net/wireless/mwifiex/wmm.c
+++ b/drivers/net/wireless/mwifiex/wmm.c
@@ -878,15 +878,8 @@ u8
mwifiex_wmm_compute_drv_pkt_delay(struct mwifiex_private *priv,
const struct sk_buff *skb)
{
+ u32 queue_delay = ktime_to_ms(ktime_sub(ktime_get(), skb->tstamp));
u8 ret_val;
- struct timeval out_tstamp, in_tstamp;
- u32 queue_delay;
-
- do_gettimeofday(&out_tstamp);
- in_tstamp = ktime_to_timeval(skb->tstamp);
-
- queue_delay = (out_tstamp.tv_sec - in_tstamp.tv_sec) * 1000;
- queue_delay += (out_tstamp.tv_usec - in_tstamp.tv_usec) / 1000;

/*
* Queue delay is passed as a uint8 in units of 2ms (ms shifted
--
1.8.2.3



2014-06-20 23:15:56

by Thomas Gleixner

[permalink] [raw]
Subject: Re: [PATCH v3] mwifiex: Use the proper interfaces

On Thu, 19 Jun 2014, Bing Zhao wrote:

> @@ -143,8 +142,7 @@ mwifiex_form_mgmt_frame(struct sk_buff *skb, const u8 *buf, size_t len)
> len - sizeof(struct ieee80211_hdr_3addr));
>
> skb->priority = LOW_PRIO_TID;
> - do_gettimeofday(&tv);
> - skb->tstamp = timeval_to_ktime(tv);
> + __net_timestamp(skb);

and __net_timestamp(skb) does

skb->tstamp = ktime_get_real();

...

> mwifiex_wmm_compute_drv_pkt_delay(struct mwifiex_private *priv,
> const struct sk_buff *skb)
> {
> + u32 queue_delay = ktime_to_ms(ktime_sub(ktime_get(), skb->tstamp));

So now you do:

delay = now(CLOCK_MONOTONIC) - tstamp(CLOCK_REALTIME);

Brilliant brainfart!

My original patch merily converted open coded crap to a consistent
one. And all of this was using the same time base: CLOCK_REALTIME.

Johannes rightfully started a discussion about CLOCK_REALTIME
vs. CLOCk_MONOTONIC suitability for certain use cases and then you
send a patch which subtracts different time bases and assign the
responsibility for that to Johannes:

> v3: don't use net_timedelta() (Johannes Berg)

Did you ever test that patch? Clearly not. Simply because on every
machine where now(CLOCK_MONOTONIC) != now(CLOCK_REALTIME) this falls
apart in bits and pieces. And that's the sane case, not the stupid
eval board without a RTC driver which defaults to 1970:00:00:00:00 and
happens to have CLOCK_MONOTONIC and CLOCK_REALTIME in sync.

So aside of not testing it proper, you clearly have no clue what you
are doing and then you have the chuzpe to claim that Johannes asked
you to do so.

Nice try.

Thanks,

tglx




2014-06-20 23:42:28

by Bing Zhao

[permalink] [raw]
Subject: RE: [PATCH v3] mwifiex: Use the proper interfaces

Hi Thomas,

> > @@ -143,8 +142,7 @@ mwifiex_form_mgmt_frame(struct sk_buff *skb, const u8 *buf, size_t len)
> > len - sizeof(struct ieee80211_hdr_3addr));
> >
> > skb->priority = LOW_PRIO_TID;
> > - do_gettimeofday(&tv);
> > - skb->tstamp = timeval_to_ktime(tv);
> > + __net_timestamp(skb);
>
> and __net_timestamp(skb) does
>
> skb->tstamp = ktime_get_real();

Ah, I didn't realize that __net_timestamp also does ktime_get_real.
Thanks for pointing it out!

>
> ...
>
> > mwifiex_wmm_compute_drv_pkt_delay(struct mwifiex_private *priv,
> > const struct sk_buff *skb)
> > {
> > + u32 queue_delay = ktime_to_ms(ktime_sub(ktime_get(), skb->tstamp));
>
> So now you do:
>
> delay = now(CLOCK_MONOTONIC) - tstamp(CLOCK_REALTIME);
>
> Brilliant brainfart!
>
> My original patch merily converted open coded crap to a consistent
> one. And all of this was using the same time base: CLOCK_REALTIME.
>
> Johannes rightfully started a discussion about CLOCK_REALTIME
> vs. CLOCk_MONOTONIC suitability for certain use cases and then you
> send a patch which subtracts different time bases and assign the
> responsibility for that to Johannes:
>
> > v3: don't use net_timedelta() (Johannes Berg)

I just wanted to thank Johannes for commenting.

>
> Did you ever test that patch? Clearly not. Simply because on every
> machine where now(CLOCK_MONOTONIC) != now(CLOCK_REALTIME) this falls
> apart in bits and pieces. And that's the sane case, not the stupid
> eval board without a RTC driver which defaults to 1970:00:00:00:00 and
> happens to have CLOCK_MONOTONIC and CLOCK_REALTIME in sync.
>
> So aside of not testing it proper, you clearly have no clue what you
> are doing and then you have the chuzpe to claim that Johannes asked
> you to do so.

I will revert this patch.

Thank you again for pointing out my error.

Regards,
Bing

>
> Nice try.
>
> Thanks,
>
> tglx
>
>