diff mbox

[1/3] mac802154: don't warn on unsupported frames

Message ID 1469207896-26481-1-git-send-email-arozansk@redhat.com (mailing list archive)
State Superseded
Headers show

Commit Message

Aristeu Rozanski July 22, 2016, 5:18 p.m. UTC
Just because we don't support certain types of frames yet doesn't mean
we have to flood the message log with warnings about "invalid" frames.

Signed-off-by: Aristeu Rozanski <arozansk@redhat.com>
---
 net/mac802154/rx.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

Comments

Alexander Aring July 23, 2016, 12:46 p.m. UTC | #1
Hi,

On 07/22/2016 07:18 PM, Aristeu Rozanski wrote:
> Just because we don't support certain types of frames yet doesn't mean
> we have to flood the message log with warnings about "invalid" frames.
> 

agree, but this patch introduce a different stats handling right now. :-)

> Signed-off-by: Aristeu Rozanski <arozansk@redhat.com>
> ---
>  net/mac802154/rx.c | 14 +++++++++++---
>  1 file changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/net/mac802154/rx.c b/net/mac802154/rx.c
> index 446e130..d388bf2 100644
> --- a/net/mac802154/rx.c
> +++ b/net/mac802154/rx.c
> @@ -82,6 +82,7 @@ ieee802154_subif_frame(struct ieee802154_sub_if_data *sdata,
>  		break;
>  	default:
>  		pr_debug("invalid dest mode\n");
> +		sdata->dev->stats.rx_frame_errors++;
>  		goto fail;
>  	}
>  
> @@ -97,15 +98,22 @@ ieee802154_subif_frame(struct ieee802154_sub_if_data *sdata,
>  		goto fail;
>  	}
>  
> -	sdata->dev->stats.rx_packets++;
> -	sdata->dev->stats.rx_bytes += skb->len;
> -
>  	switch (mac_cb(skb)->type) {
> +	case IEEE802154_FC_TYPE_BEACON:
> +	case IEEE802154_FC_TYPE_ACK:
> +	case IEEE802154_FC_TYPE_MAC_CMD:
> +		sdata->dev->stats.rx_dropped++;
> +		goto fail;
> +
>  	case IEEE802154_FC_TYPE_DATA:
> +		sdata->dev->stats.rx_bytes += skb->len;
> +		sdata->dev->stats.rx_packets++;

This should be moved into ieee802154_deliver_skb, right before calling
netif_rx(...);

The reason is because we have for "stats.rx_bytes" and
"stats.rx_packets" a very clean definition what it means. It means, the
frames hit a state where it is ready to put it into the packet-layer,
this is what netif_rx is doing.

It's okay for me to add support for rx_frame_errors and also rx_dropped
stuff. But with a very big warning that the meaning of such stats will
be touched again later if we found some strategie where we we define
what each stats attribute means. Also there exists subsystems which
simple ignore these stats, because at userspace side you need always to
lookup what the definition means and when we run such stats counters and
when not and some subsystems don't care about that.

- Alex
--
To unsubscribe from this list: send the line "unsubscribe linux-wpan" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Aristeu Rozanski July 25, 2016, 1:15 p.m. UTC | #2
Hi Alexander,
On Sat, Jul 23, 2016 at 02:46:28PM +0200, Alexander Aring wrote:
> agree, but this patch introduce a different stats handling right now. :-)

sorry, should have split both parts in different patches.

> This should be moved into ieee802154_deliver_skb, right before calling
> netif_rx(...);
> 
> The reason is because we have for "stats.rx_bytes" and
> "stats.rx_packets" a very clean definition what it means. It means, the
> frames hit a state where it is ready to put it into the packet-layer,
> this is what netif_rx is doing.
> 
> It's okay for me to add support for rx_frame_errors and also rx_dropped
> stuff. But with a very big warning that the meaning of such stats will
> be touched again later if we found some strategie where we we define
> what each stats attribute means. Also there exists subsystems which
> simple ignore these stats, because at userspace side you need always to
> lookup what the definition means and when we run such stats counters and
> when not and some subsystems don't care about that.

Fair enough, I'll redo the patch just removing the warning.
Thanks!
diff mbox

Patch

diff --git a/net/mac802154/rx.c b/net/mac802154/rx.c
index 446e130..d388bf2 100644
--- a/net/mac802154/rx.c
+++ b/net/mac802154/rx.c
@@ -82,6 +82,7 @@  ieee802154_subif_frame(struct ieee802154_sub_if_data *sdata,
 		break;
 	default:
 		pr_debug("invalid dest mode\n");
+		sdata->dev->stats.rx_frame_errors++;
 		goto fail;
 	}
 
@@ -97,15 +98,22 @@  ieee802154_subif_frame(struct ieee802154_sub_if_data *sdata,
 		goto fail;
 	}
 
-	sdata->dev->stats.rx_packets++;
-	sdata->dev->stats.rx_bytes += skb->len;
-
 	switch (mac_cb(skb)->type) {
+	case IEEE802154_FC_TYPE_BEACON:
+	case IEEE802154_FC_TYPE_ACK:
+	case IEEE802154_FC_TYPE_MAC_CMD:
+		sdata->dev->stats.rx_dropped++;
+		goto fail;
+
 	case IEEE802154_FC_TYPE_DATA:
+		sdata->dev->stats.rx_bytes += skb->len;
+		sdata->dev->stats.rx_packets++;
 		return ieee802154_deliver_skb(skb);
+
 	default:
 		pr_warn("ieee802154: bad frame received (type = %d)\n",
 			mac_cb(skb)->type);
+		sdata->dev->stats.rx_frame_errors++;
 		goto fail;
 	}