[PATCH v2 0/1] ieee802154: cc2520: Check CRC & add promiscuous

[PATCH v2 0/1] ieee802154: cc2520: Check CRC & add promiscuous

[Brad Campbell]

I've updated the patch to provide monitors a method for determining if
the incoming packet had a valid CRC or not. This required adding promiscuous
mode, which is no longer the default.

Also, am I correct in noting that configuring a node interface to be in
promiscuous mode with something like:

ioctl(sockfd, SIOCGIFFLAGS, &ifopts);
ifopts.ifr_flags |= IFF_PROMISC;
ioctl(sockfd, SIOCSIFFLAGS, &ifopts);

is not currently supported? That's how I was testing at first and it
didn't seem to work.... Also, if a user does set an interface to
promiscuous using an ioctl, it seems like they would still want CRC
checking, which is not what would currently happen.


Changes since v1:
- Add support for promiscuous mode in cc2520
- Make frame filtering default
- Append CRC to incoming packets for monitors
- Actually calculate LQI

Brad Campbell (1):
  ieee802154: cc2520: Check CRC & add promiscuous

 drivers/net/ieee802154/cc2520.c | 128 ++++++++++++++++++++++++++++++++++------
 1 file changed, 111 insertions(+), 17 deletions(-)

--
2.6.3

[PATCH v2 1/1] ieee802154: cc2520: Check CRC & add promiscuous

[Brad Campbell]

This patch adds checking the "CRC_OK" bit at the end of packets coming
from the CC2520 radio. It also adds support for putting the radio in
promiscuous mode (in which packets are not dropped if the CRC fails).
In order to provide monitors information about CRC pass/fail, the CRC is
recreated in the driver and appended to the packet before being passed
to higher layers. If the CRC failed, a dummy CRC is appended.

NOTE: This is an API breaking change for the CC2520 radio. The radio now
defaults to frame filtering (checking that the destination and PANID in
the incoming packet matches the local node). This matches the other
15.4 radios and is what a user would expect to be the default.

Other changes:

1. Adds LQI calculation
2. Makes #defines for relevant bit fields in CC2520 registers

Signed-off-by: Brad Campbell <bradjc5@gmail.com>
---
 drivers/net/ieee802154/cc2520.c | 128 ++++++++++++++++++++++++++++++++++------
 1 file changed, 111 insertions(+), 17 deletions(-)

diff --git a/drivers/net/ieee802154/cc2520.c b/drivers/net/ieee802154/cc2520.c
index e65b605..3d66717 100644
--- a/drivers/net/ieee802154/cc2520.c
+++ b/drivers/net/ieee802154/cc2520.c
@@ -21,6 +21,8 @@
 #include <linux/skbuff.h>
 #include <linux/of_gpio.h>
 #include <linux/ieee802154.h>
+#include <linux/crc-ccitt.h>
+#include <asm/unaligned.h>
 
 #include <net/mac802154.h>
 #include <net/cfg802154.h>
@@ -189,6 +191,18 @@
 #define	CC2520_RXFIFOCNT		0x3E
 #define	CC2520_TXFIFOCNT		0x3F
 
+/* CC2520_FRMFILT0 */
+#define FRMFILT0_FRAME_FILTER_EN	BIT(0)
+#define FRMFILT0_PAN_COORDINATOR	BIT(1)
+
+/* CC2520_FRMCTRL0 */
+#define FRMCTRL0_AUTOACK		BIT(5)
+#define FRMCTRL0_AUTOCRC		BIT(6)
+
+/* CC2520_FRMCTRL1 */
+#define FRMCTRL1_SET_RXENMASK_ON_TX	BIT(0)
+#define FRMCTRL1_IGNORE_TX_UNDERF	BIT(1)
+
 /* Driver private information */
 struct cc2520_private {
 	struct spi_device *spi;		/* SPI device structure */
@@ -201,6 +215,7 @@ struct cc2520_private {
 	struct work_struct fifop_irqwork;/* Workqueue for FIFOP */
 	spinlock_t lock;		/* Lock for is_tx*/
 	struct completion tx_complete;	/* Work completion for Tx */
+	bool promiscuous;               /* Flag for promiscuous mode */
 };
 
 /* Generic Functions */
@@ -414,7 +429,7 @@ cc2520_write_txfifo(struct cc2520_private *priv, u8 *data, u8 len)
 }
 
 static int
-cc2520_read_rxfifo(struct cc2520_private *priv, u8 *data, u8 len, u8 *lqi)
+cc2520_read_rxfifo(struct cc2520_private *priv, u8 *data, u8 len)
 {
 	int status;
 	struct spi_message msg;
@@ -516,24 +531,67 @@ err_tx:
 static int cc2520_rx(struct cc2520_private *priv)
 {
 	u8 len = 0, lqi = 0, bytes = 1;
+	bool crc_ok;
+	u16 crc;
 	struct sk_buff *skb;
 
-	cc2520_read_rxfifo(priv, &len, bytes, &lqi);
+	/* Read single length byte from the radio. */
+	cc2520_read_rxfifo(priv, &len, bytes);
 
-	if (len < 2 || len > IEEE802154_MTU)
-		return -EINVAL;
+	if (!ieee802154_is_valid_psdu_len(len)) {
+		/* Corrupted frame received, clear frame buffer by
+		 * reading entire buffer.
+		 */
+		dev_dbg(&priv->spi->dev, "corrupted frame received\n");
+		len = IEEE802154_MTU;
+	}
 
 	skb = dev_alloc_skb(len);
 	if (!skb)
 		return -ENOMEM;
 
-	if (cc2520_read_rxfifo(priv, skb_put(skb, len), len, &lqi)) {
+	if (cc2520_read_rxfifo(priv, skb_put(skb, len), len)) {
 		dev_dbg(&priv->spi->dev, "frame reception failed\n");
 		kfree_skb(skb);
 		return -EINVAL;
 	}
 
-	skb_trim(skb, skb->len - 2);
+	/* Check if the CRC is valid and calculate LQI. With AUTOCRC set,
+	 * the most significant bit of the last byte returned from the CC2520
+	 * is CRC_OK flag. See section 20.3.4 of the datasheet. To calculate
+	 * LQI, the lower 7 bits of the last byte (the correlation value
+	 * provided by the radio) must be scaled to the range 0-255. According
+	 * to section 20.6, the correlation value ranges from 50-110. Ideally
+	 * this would be calibrated per hardware design, but we use roughly the
+	 * datasheet values to get close enough while avoiding floating point.
+	 */
+	crc_ok = skb->data[len - 1] & BIT(7);
+	lqi = skb->data[len - 1] & 0x7f;
+	if (lqi < 50)
+		lqi = 50;
+	else if (lqi > 113)
+		lqi = 113;
+	lqi = (lqi - 50) * 4;
+
+	/* If we failed CRC and we are not in promiscuous mode, drop the
+	 * packet in the driver layer.
+	 */
+	if (!crc_ok && !priv->promiscuous) {
+		dev_dbg(&priv->spi->dev, "CRC check failed\n");
+		kfree_skb(skb);
+		return -EINVAL;
+	}
+
+	/* We pass the packet up to the mac layer if the CRC is valid or
+	 * unconditionally in promiscuous mode. We do need to append the
+	 * original CRC bytes for the upper layer, and we calculate them
+	 * here so that we can ensure that we append a bad CRC if the check
+	 * failed.
+	 */
+	crc = crc_ccitt(0, skb->data, skb->len - 2);
+	if (!crc_ok)
+		crc = ~crc;
+	put_unaligned_le16(crc, skb->data + len - 2);
 
 	ieee802154_rx_irqsafe(priv->hw, skb, lqi);
 
@@ -619,14 +677,19 @@ cc2520_filter(struct ieee802154_hw *hw,
 	}
 
 	if (changed & IEEE802154_AFILT_PANC_CHANGED) {
+		u8 frmfilt0;
+
 		dev_vdbg(&priv->spi->dev,
 			 "cc2520_filter called for panc change\n");
+
+		cc2520_read_register(priv, CC2520_FRMFILT0, &frmfilt0);
+
 		if (filt->pan_coord)
-			ret = cc2520_write_register(priv, CC2520_FRMFILT0,
-						    0x02);
+			frmfilt0 |= FRMFILT0_PAN_COORDINATOR;
 		else
-			ret = cc2520_write_register(priv, CC2520_FRMFILT0,
-						    0x00);
+			frmfilt0 &= ~FRMFILT0_PAN_COORDINATOR;
+
+		ret = cc2520_write_register(priv, CC2520_FRMFILT0, frmfilt0);
 	}
 
 	return ret;
@@ -723,6 +786,30 @@ cc2520_set_txpower(struct ieee802154_hw *hw, s32 mbm)
 	return cc2520_cc2591_set_tx_power(priv, mbm);
 }
 
+static int
+cc2520_set_promiscuous_mode(struct ieee802154_hw *hw, bool on)
+{
+	struct cc2520_private *priv = hw->priv;
+	u8 frmfilt0;
+
+	dev_dbg(&priv->spi->dev, "%s : mode %d\n", __func__, on);
+
+	priv->promiscuous = on;
+
+	cc2520_read_register(priv, CC2520_FRMFILT0, &frmfilt0);
+
+	if (on) {
+		/* Disable automatic ACK and frame filtering. */
+		cc2520_write_register(priv, CC2520_FRMCTRL0, FRMCTRL0_AUTOCRC);
+		frmfilt0 &= ~FRMFILT0_FRAME_FILTER_EN;
+	} else {
+		cc2520_write_register(priv, CC2520_FRMCTRL0, FRMCTRL0_AUTOACK |
+							     FRMCTRL0_AUTOCRC);
+		frmfilt0 |= FRMFILT0_FRAME_FILTER_EN;
+	}
+	return cc2520_write_register(priv, CC2520_FRMFILT0, frmfilt0);
+}
+
 static const struct ieee802154_ops cc2520_ops = {
 	.owner = THIS_MODULE,
 	.start = cc2520_start,
@@ -732,6 +819,7 @@ static const struct ieee802154_ops cc2520_ops = {
 	.set_channel = cc2520_set_channel,
 	.set_hw_addr_filt = cc2520_filter,
 	.set_txpower = cc2520_set_txpower,
+	.set_promiscuous_mode = cc2520_set_promiscuous_mode,
 };
 
 static int cc2520_register(struct cc2520_private *priv)
@@ -749,7 +837,8 @@ static int cc2520_register(struct cc2520_private *priv)
 
 	/* We do support only 2.4 Ghz */
 	priv->hw->phy->supported.channels[0] = 0x7FFF800;
-	priv->hw->flags = IEEE802154_HW_OMIT_CKSUM | IEEE802154_HW_AFILT;
+	priv->hw->flags = IEEE802154_HW_TX_OMIT_CKSUM | IEEE802154_HW_AFILT |
+			  IEEE802154_HW_PROMISCUOUS;
 
 	priv->hw->phy->flags = WPAN_PHY_FLAG_TXPOWER;
 
@@ -919,6 +1008,11 @@ static int cc2520_hw_init(struct cc2520_private *priv)
 	}
 
 	/* Registers default value: section 28.1 in Datasheet */
+
+	/* Set the CCA threshold to -50 dBm. This seems to have been copied
+	 * from the TinyOS CC2520 driver and is much higher than the -84 dBm
+	 * threshold suggested in the datasheet.
+	 */
 	ret = cc2520_write_register(priv, CC2520_CCACTRL0, 0x1A);
 	if (ret)
 		goto err_ret;
@@ -955,15 +1049,15 @@ static int cc2520_hw_init(struct cc2520_private *priv)
 	if (ret)
 		goto err_ret;
 
-	ret = cc2520_write_register(priv, CC2520_FRMCTRL0, 0x60);
-	if (ret)
-		goto err_ret;
-
-	ret = cc2520_write_register(priv, CC2520_FRMCTRL1, 0x03);
+	/* Configure registers correctly for this driver. */
+	ret = cc2520_write_register(priv, CC2520_FRMCTRL0, FRMCTRL0_AUTOACK |
+							   FRMCTRL0_AUTOCRC);
 	if (ret)
 		goto err_ret;
 
-	ret = cc2520_write_register(priv, CC2520_FRMFILT0, 0x00);
+	ret = cc2520_write_register(priv, CC2520_FRMCTRL1,
+				    FRMCTRL1_SET_RXENMASK_ON_TX |
+				    FRMCTRL1_IGNORE_TX_UNDERF);
 	if (ret)
 		goto err_ret;
 
-- 
2.6.3

Re: [PATCH v2 1/1] ieee802154: cc2520: Check CRC & add promiscuous

[Alexander Aring]

On Wed, Dec 23, 2015 at 03:17:58AM -0500, Brad Campbell wrote:
> This patch adds checking the "CRC_OK" bit at the end of packets coming
> from the CC2520 radio. It also adds support for putting the radio in
> promiscuous mode (in which packets are not dropped if the CRC fails).
> In order to provide monitors information about CRC pass/fail, the CRC is
> recreated in the driver and appended to the packet before being passed
> to higher layers. If the CRC failed, a dummy CRC is appended.
> 

Why append a dummy CRC? There is a CRC and leave it there.

There could be one reason to do this:

There is no CRC appended when CRC failed.

Is this true? I don't know how the CC2520 is working and why it should
do this.



> NOTE: This is an API breaking change for the CC2520 radio. The radio now
> defaults to frame filtering (checking that the destination and PANID in
> the incoming packet matches the local node). This matches the other
> 15.4 radios and is what a user would expect to be the default.
> 

Was the frame filtering stuff disabled before? Then this is no API
change, it's an improvement for CC2520.

It would be an API change if the frame filtering of CC2520 is not
802.15.4 compliant.

Maybe it breaks API now but then this is a TODO/BUG in mac802154.

> Other changes:
> 
> 1. Adds LQI calculation
> 2. Makes #defines for relevant bit fields in CC2520 registers
> 
> Signed-off-by: Brad Campbell <bradjc5@gmail.com>
> ---
>  drivers/net/ieee802154/cc2520.c | 128 ++++++++++++++++++++++++++++++++++------
>  1 file changed, 111 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/net/ieee802154/cc2520.c b/drivers/net/ieee802154/cc2520.c
> index e65b605..3d66717 100644
> --- a/drivers/net/ieee802154/cc2520.c
> +++ b/drivers/net/ieee802154/cc2520.c
> @@ -21,6 +21,8 @@
>  #include <linux/skbuff.h>
>  #include <linux/of_gpio.h>
>  #include <linux/ieee802154.h>
> +#include <linux/crc-ccitt.h>
> +#include <asm/unaligned.h>
>  
>  #include <net/mac802154.h>
>  #include <net/cfg802154.h>
> @@ -189,6 +191,18 @@
>  #define	CC2520_RXFIFOCNT		0x3E
>  #define	CC2520_TXFIFOCNT		0x3F
>  
> +/* CC2520_FRMFILT0 */
> +#define FRMFILT0_FRAME_FILTER_EN	BIT(0)
> +#define FRMFILT0_PAN_COORDINATOR	BIT(1)
> +
> +/* CC2520_FRMCTRL0 */
> +#define FRMCTRL0_AUTOACK		BIT(5)
> +#define FRMCTRL0_AUTOCRC		BIT(6)
> +
> +/* CC2520_FRMCTRL1 */
> +#define FRMCTRL1_SET_RXENMASK_ON_TX	BIT(0)
> +#define FRMCTRL1_IGNORE_TX_UNDERF	BIT(1)
> +
>  /* Driver private information */
>  struct cc2520_private {
>  	struct spi_device *spi;		/* SPI device structure */
> @@ -201,6 +215,7 @@ struct cc2520_private {
>  	struct work_struct fifop_irqwork;/* Workqueue for FIFOP */
>  	spinlock_t lock;		/* Lock for is_tx*/
>  	struct completion tx_complete;	/* Work completion for Tx */
> +	bool promiscuous;               /* Flag for promiscuous mode */
>  };
>  
>  /* Generic Functions */
> @@ -414,7 +429,7 @@ cc2520_write_txfifo(struct cc2520_private *priv, u8 *data, u8 len)
>  }
>  
>  static int
> -cc2520_read_rxfifo(struct cc2520_private *priv, u8 *data, u8 len, u8 *lqi)
> +cc2520_read_rxfifo(struct cc2520_private *priv, u8 *data, u8 len)
>  {
>  	int status;
>  	struct spi_message msg;
> @@ -516,24 +531,67 @@ err_tx:
>  static int cc2520_rx(struct cc2520_private *priv)
>  {
>  	u8 len = 0, lqi = 0, bytes = 1;
> +	bool crc_ok;
> +	u16 crc;
>  	struct sk_buff *skb;
>  
> -	cc2520_read_rxfifo(priv, &len, bytes, &lqi);
> +	/* Read single length byte from the radio. */
> +	cc2520_read_rxfifo(priv, &len, bytes);
>  
> -	if (len < 2 || len > IEEE802154_MTU)
> -		return -EINVAL;
> +	if (!ieee802154_is_valid_psdu_len(len)) {
> +		/* Corrupted frame received, clear frame buffer by
> +		 * reading entire buffer.
> +		 */
> +		dev_dbg(&priv->spi->dev, "corrupted frame received\n");
> +		len = IEEE802154_MTU;
> +	}
>  
>  	skb = dev_alloc_skb(len);
>  	if (!skb)
>  		return -ENOMEM;
>  
> -	if (cc2520_read_rxfifo(priv, skb_put(skb, len), len, &lqi)) {
> +	if (cc2520_read_rxfifo(priv, skb_put(skb, len), len)) {
>  		dev_dbg(&priv->spi->dev, "frame reception failed\n");
>  		kfree_skb(skb);
>  		return -EINVAL;
>  	}
>  
> -	skb_trim(skb, skb->len - 2);

This code told me before it was there: we cut off the CRC for delivery.

> +	/* Check if the CRC is valid and calculate LQI. With AUTOCRC set,
> +	 * the most significant bit of the last byte returned from the CC2520
> +	 * is CRC_OK flag. See section 20.3.4 of the datasheet. To calculate
> +	 * LQI, the lower 7 bits of the last byte (the correlation value
> +	 * provided by the radio) must be scaled to the range 0-255. According
> +	 * to section 20.6, the correlation value ranges from 50-110. Ideally
> +	 * this would be calibrated per hardware design, but we use roughly the
> +	 * datasheet values to get close enough while avoiding floating point.
> +	 */
> +	crc_ok = skb->data[len - 1] & BIT(7);
> +	lqi = skb->data[len - 1] & 0x7f;
> +	if (lqi < 50)
> +		lqi = 50;
> +	else if (lqi > 113)
> +		lqi = 113;
> +	lqi = (lqi - 50) * 4;
> +
> +	/* If we failed CRC and we are not in promiscuous mode, drop the
> +	 * packet in the driver layer.
> +	 */
> +	if (!crc_ok && !priv->promiscuous) {
> +		dev_dbg(&priv->spi->dev, "CRC check failed\n");
> +		kfree_skb(skb);
> +		return -EINVAL;
> +	}
> +
> +	/* We pass the packet up to the mac layer if the CRC is valid or
> +	 * unconditionally in promiscuous mode. We do need to append the
> +	 * original CRC bytes for the upper layer, and we calculate them
> +	 * here so that we can ensure that we append a bad CRC if the check
> +	 * failed.
> +	 */
> +	crc = crc_ccitt(0, skb->data, skb->len - 2);
> +	if (!crc_ok)
> +		crc = ~crc;
> +	put_unaligned_le16(crc, skb->data + len - 2);
>  

Now you append one again, but in an area which is not prepared to put
some data there, because there is no "skb_put" again. I don't understand
this part, is there no CRC comming from cc2520 transceiver, but then I
don't understand the "skb_trim(skb, skb->len - 2);" which was before
there and I think it was the CRC.

I think it's fine to remove this part, did you test it with monitor
interface and node interface?

>  	ieee802154_rx_irqsafe(priv->hw, skb, lqi);
>  
> @@ -619,14 +677,19 @@ cc2520_filter(struct ieee802154_hw *hw,
>  	}
>  
>  	if (changed & IEEE802154_AFILT_PANC_CHANGED) {
> +		u8 frmfilt0;
> +
>  		dev_vdbg(&priv->spi->dev,
>  			 "cc2520_filter called for panc change\n");
> +
> +		cc2520_read_register(priv, CC2520_FRMFILT0, &frmfilt0);
> +
>  		if (filt->pan_coord)
> -			ret = cc2520_write_register(priv, CC2520_FRMFILT0,
> -						    0x02);
> +			frmfilt0 |= FRMFILT0_PAN_COORDINATOR;
>  		else
> -			ret = cc2520_write_register(priv, CC2520_FRMFILT0,
> -						    0x00);
> +			frmfilt0 &= ~FRMFILT0_PAN_COORDINATOR;
> +
> +		ret = cc2520_write_register(priv, CC2520_FRMFILT0, frmfilt0);
>  	}
>  
>  	return ret;

You need to be sure here that this function does not enable again some
filtering stuff bit's which was set by "set_promiscuous_mode". Is this
the case then we need to do more handling here (maybe with
priv->promiscuous).

This functionality should really only set the address filtering stuff
_not_ enable it (if possible). Is this the case, then the function
should be fine.

> @@ -723,6 +786,30 @@ cc2520_set_txpower(struct ieee802154_hw *hw, s32 mbm)
>  	return cc2520_cc2591_set_tx_power(priv, mbm);
>  }
>  
> +static int
> +cc2520_set_promiscuous_mode(struct ieee802154_hw *hw, bool on)
> +{
> +	struct cc2520_private *priv = hw->priv;
> +	u8 frmfilt0;
> +
> +	dev_dbg(&priv->spi->dev, "%s : mode %d\n", __func__, on);
> +
> +	priv->promiscuous = on;
> +
> +	cc2520_read_register(priv, CC2520_FRMFILT0, &frmfilt0);
> +
> +	if (on) {
> +		/* Disable automatic ACK and frame filtering. */
> +		cc2520_write_register(priv, CC2520_FRMCTRL0, FRMCTRL0_AUTOCRC);
> +		frmfilt0 &= ~FRMFILT0_FRAME_FILTER_EN;
The frmfilt0 was readout before doing:
"cc2520_write_register(priv, CC2520_FRMCTRL0, FRMCTRL0_AUTOCRC);"

Are you sure that this will not change again the setting og
FRMCTRL0_AUTOCRC when set it again below.

Simple do all manipulations (update bits) in "frmfilt0", then write it
below.

> +	} else {
> +		cc2520_write_register(priv, CC2520_FRMCTRL0, FRMCTRL0_AUTOACK |
> +							     FRMCTRL0_AUTOCRC);
> +		frmfilt0 |= FRMFILT0_FRAME_FILTER_EN;
> +	}

Maybe the FRMCTRL0_AUTOCRC will unset here it's the same register.

> +	return cc2520_write_register(priv, CC2520_FRMFILT0, frmfilt0);
> +}
> +
>  static const struct ieee802154_ops cc2520_ops = {
>  	.owner = THIS_MODULE,
>  	.start = cc2520_start,
> @@ -732,6 +819,7 @@ static const struct ieee802154_ops cc2520_ops = {
>  	.set_channel = cc2520_set_channel,
>  	.set_hw_addr_filt = cc2520_filter,
>  	.set_txpower = cc2520_set_txpower,
> +	.set_promiscuous_mode = cc2520_set_promiscuous_mode,
>  };
>  
>  static int cc2520_register(struct cc2520_private *priv)
> @@ -749,7 +837,8 @@ static int cc2520_register(struct cc2520_private *priv)
>  
>  	/* We do support only 2.4 Ghz */
>  	priv->hw->phy->supported.channels[0] = 0x7FFF800;
> -	priv->hw->flags = IEEE802154_HW_OMIT_CKSUM | IEEE802154_HW_AFILT;
> +	priv->hw->flags = IEEE802154_HW_TX_OMIT_CKSUM | IEEE802154_HW_AFILT |
> +			  IEEE802154_HW_PROMISCUOUS;
>  
>  	priv->hw->phy->flags = WPAN_PHY_FLAG_TXPOWER;
>  
> @@ -919,6 +1008,11 @@ static int cc2520_hw_init(struct cc2520_private *priv)
>  	}
>  
>  	/* Registers default value: section 28.1 in Datasheet */
> +
> +	/* Set the CCA threshold to -50 dBm. This seems to have been copied
> +	 * from the TinyOS CC2520 driver and is much higher than the -84 dBm
> +	 * threshold suggested in the datasheet.
> +	 */

I think the CCA isn't working here inside the CC2520, because the CC2520
do _not_ CSMA handling on transceiver side. There need to be a handling
(maybe with hrtimers) which adds CSMA handling and doing CCA scans
before transmit, but the CC2520 driver doesn't has such functionality to
support CSMA-CA.

See my comments on this at [0].

>  	ret = cc2520_write_register(priv, CC2520_CCACTRL0, 0x1A);
>  	if (ret)
>  		goto err_ret;
> @@ -955,15 +1049,15 @@ static int cc2520_hw_init(struct cc2520_private *priv)
>  	if (ret)
>  		goto err_ret;
>  
> -	ret = cc2520_write_register(priv, CC2520_FRMCTRL0, 0x60);
> -	if (ret)
> -		goto err_ret;
> -
> -	ret = cc2520_write_register(priv, CC2520_FRMCTRL1, 0x03);
> +	/* Configure registers correctly for this driver. */
> +	ret = cc2520_write_register(priv, CC2520_FRMCTRL0, FRMCTRL0_AUTOACK |
> +							   FRMCTRL0_AUTOCRC);
>  	if (ret)
>  		goto err_ret;
>  
> -	ret = cc2520_write_register(priv, CC2520_FRMFILT0, 0x00);
> +	ret = cc2520_write_register(priv, CC2520_FRMCTRL1,
> +				    FRMCTRL1_SET_RXENMASK_ON_TX |
> +				    FRMCTRL1_IGNORE_TX_UNDERF);

I think:
You don't need to set this. Before calling "start" callback we will call
"set_promiscuous_mode" with on/off depends on monitor/node type.

same for enable the FRMCTRL0_AUTOACK. What you should enable here is
FRMCTRL0_AUTOCRC (I suppose for transmit).

- Alex

[0] http://www.spinics.net/lists/linux-wpan/msg03198.html

Re: [PATCH v2 1/1] ieee802154: cc2520: Check CRC & add promiscuous

[Brad Campbell]

On Wed, Dec 23, 2015 at 4:55 AM, Alexander Aring <alex.aring@gmail.com> wrote:
> On Wed, Dec 23, 2015 at 03:17:58AM -0500, Brad Campbell wrote:
>> This patch adds checking the "CRC_OK" bit at the end of packets coming
>> from the CC2520 radio. It also adds support for putting the radio in
>> promiscuous mode (in which packets are not dropped if the CRC fails).
>> In order to provide monitors information about CRC pass/fail, the CRC is
>> recreated in the driver and appended to the packet before being passed
>> to higher layers. If the CRC failed, a dummy CRC is appended.
>>
>
> Why append a dummy CRC? There is a CRC and leave it there.
>
> There could be one reason to do this:
>
> There is no CRC appended when CRC failed.
>
> Is this true? I don't know how the CC2520 is working and why it should
> do this.
>

When the AUTOCRC bit is set, upon packet reception the CC2520 does
not return the two FCS bytes, instead it replaces them with 1 byte
of RSSI, 1 bit of CRC_OK and 7 bits of correlation value. So, in order to
comply with the ieee802154_rx() interface, the driver could do one of:

1. Turn off AUTOCRC, which would require linux to calculate the CRC for
TX packets, but the CC2520 would give us the CRC bytes which would
then be passed up.

2. Leave AUTOCRC on and calculate the CRC in software at the driver
level so that if the CRC failed a bad CRC could get appended. (This
is what the patch currently does).

3. Leave AUTOCRC on, set IEEE802154_HW_RX_OMIT_CKSUM and
let ieee802154_rx() calculate the CRC. This is bad because the monitor
layer will always see a correct CRC, even if it did in fact fail. (This is
how the driver works currently).

I think the best thing to do is to actually change the ieee802154_rx()
interface to allow 15.4 drivers to pass in a boolean if the CRC passed
and get rid of IEEE802154_HW_RX_DROP_BAD_CKSUM. Then
ieee802154_rx() can recreate CRCs and/or drop packets as needed,
and the 15.4 drivers wouldn't need to do "if (promiscuous_mode)"
checks. Drivers would be able to pass in the actual received CRC
values instead of this flag if appropriate, of course.


>
>
>> NOTE: This is an API breaking change for the CC2520 radio. The radio now
>> defaults to frame filtering (checking that the destination and PANID in
>> the incoming packet matches the local node). This matches the other
>> 15.4 radios and is what a user would expect to be the default.
>>
>
> Was the frame filtering stuff disabled before? Then this is no API
> change, it's an improvement for CC2520.
>
> It would be an API change if the frame filtering of CC2520 is not
> 802.15.4 compliant.
>
> Maybe it breaks API now but then this is a TODO/BUG in mac802154.
>

Currently, the CC2520 driver does no frame filtering. The patch makes
it 802.15.4 compliant.


>> Other changes:
>>
>> 1. Adds LQI calculation
>> 2. Makes #defines for relevant bit fields in CC2520 registers
>>
>> Signed-off-by: Brad Campbell <bradjc5@gmail.com>
>> ---
>>  drivers/net/ieee802154/cc2520.c | 128 ++++++++++++++++++++++++++++++++++------
>>  1 file changed, 111 insertions(+), 17 deletions(-)
>>
>> diff --git a/drivers/net/ieee802154/cc2520.c b/drivers/net/ieee802154/cc2520.c
>> index e65b605..3d66717 100644
>> --- a/drivers/net/ieee802154/cc2520.c
>> +++ b/drivers/net/ieee802154/cc2520.c
>> @@ -21,6 +21,8 @@
>>  #include <linux/skbuff.h>
>>  #include <linux/of_gpio.h>
>>  #include <linux/ieee802154.h>
>> +#include <linux/crc-ccitt.h>
>> +#include <asm/unaligned.h>
>>
>>  #include <net/mac802154.h>
>>  #include <net/cfg802154.h>
>> @@ -189,6 +191,18 @@
>>  #define      CC2520_RXFIFOCNT                0x3E
>>  #define      CC2520_TXFIFOCNT                0x3F
>>
>> +/* CC2520_FRMFILT0 */
>> +#define FRMFILT0_FRAME_FILTER_EN     BIT(0)
>> +#define FRMFILT0_PAN_COORDINATOR     BIT(1)
>> +
>> +/* CC2520_FRMCTRL0 */
>> +#define FRMCTRL0_AUTOACK             BIT(5)
>> +#define FRMCTRL0_AUTOCRC             BIT(6)
>> +
>> +/* CC2520_FRMCTRL1 */
>> +#define FRMCTRL1_SET_RXENMASK_ON_TX  BIT(0)
>> +#define FRMCTRL1_IGNORE_TX_UNDERF    BIT(1)
>> +
>>  /* Driver private information */
>>  struct cc2520_private {
>>       struct spi_device *spi;         /* SPI device structure */
>> @@ -201,6 +215,7 @@ struct cc2520_private {
>>       struct work_struct fifop_irqwork;/* Workqueue for FIFOP */
>>       spinlock_t lock;                /* Lock for is_tx*/
>>       struct completion tx_complete;  /* Work completion for Tx */
>> +     bool promiscuous;               /* Flag for promiscuous mode */
>>  };
>>
>>  /* Generic Functions */
>> @@ -414,7 +429,7 @@ cc2520_write_txfifo(struct cc2520_private *priv, u8 *data, u8 len)
>>  }
>>
>>  static int
>> -cc2520_read_rxfifo(struct cc2520_private *priv, u8 *data, u8 len, u8 *lqi)
>> +cc2520_read_rxfifo(struct cc2520_private *priv, u8 *data, u8 len)
>>  {
>>       int status;
>>       struct spi_message msg;
>> @@ -516,24 +531,67 @@ err_tx:
>>  static int cc2520_rx(struct cc2520_private *priv)
>>  {
>>       u8 len = 0, lqi = 0, bytes = 1;
>> +     bool crc_ok;
>> +     u16 crc;
>>       struct sk_buff *skb;
>>
>> -     cc2520_read_rxfifo(priv, &len, bytes, &lqi);
>> +     /* Read single length byte from the radio. */
>> +     cc2520_read_rxfifo(priv, &len, bytes);
>>
>> -     if (len < 2 || len > IEEE802154_MTU)
>> -             return -EINVAL;
>> +     if (!ieee802154_is_valid_psdu_len(len)) {
>> +             /* Corrupted frame received, clear frame buffer by
>> +              * reading entire buffer.
>> +              */
>> +             dev_dbg(&priv->spi->dev, "corrupted frame received\n");
>> +             len = IEEE802154_MTU;
>> +     }
>>
>>       skb = dev_alloc_skb(len);
>>       if (!skb)
>>               return -ENOMEM;
>>
>> -     if (cc2520_read_rxfifo(priv, skb_put(skb, len), len, &lqi)) {
>> +     if (cc2520_read_rxfifo(priv, skb_put(skb, len), len)) {
>>               dev_dbg(&priv->spi->dev, "frame reception failed\n");
>>               kfree_skb(skb);
>>               return -EINVAL;
>>       }
>>
>> -     skb_trim(skb, skb->len - 2);
>
> This code told me before it was there: we cut off the CRC for delivery.
>

The CC2520 does append two bytes, but not the CRC.

>> +     /* Check if the CRC is valid and calculate LQI. With AUTOCRC set,
>> +      * the most significant bit of the last byte returned from the CC2520
>> +      * is CRC_OK flag. See section 20.3.4 of the datasheet. To calculate
>> +      * LQI, the lower 7 bits of the last byte (the correlation value
>> +      * provided by the radio) must be scaled to the range 0-255. According
>> +      * to section 20.6, the correlation value ranges from 50-110. Ideally
>> +      * this would be calibrated per hardware design, but we use roughly the
>> +      * datasheet values to get close enough while avoiding floating point.
>> +      */
>> +     crc_ok = skb->data[len - 1] & BIT(7);
>> +     lqi = skb->data[len - 1] & 0x7f;
>> +     if (lqi < 50)
>> +             lqi = 50;
>> +     else if (lqi > 113)
>> +             lqi = 113;
>> +     lqi = (lqi - 50) * 4;
>> +
>> +     /* If we failed CRC and we are not in promiscuous mode, drop the
>> +      * packet in the driver layer.
>> +      */
>> +     if (!crc_ok && !priv->promiscuous) {
>> +             dev_dbg(&priv->spi->dev, "CRC check failed\n");
>> +             kfree_skb(skb);
>> +             return -EINVAL;
>> +     }
>> +
>> +     /* We pass the packet up to the mac layer if the CRC is valid or
>> +      * unconditionally in promiscuous mode. We do need to append the
>> +      * original CRC bytes for the upper layer, and we calculate them
>> +      * here so that we can ensure that we append a bad CRC if the check
>> +      * failed.
>> +      */
>> +     crc = crc_ccitt(0, skb->data, skb->len - 2);
>> +     if (!crc_ok)
>> +             crc = ~crc;
>> +     put_unaligned_le16(crc, skb->data + len - 2);
>>
>
> Now you append one again, but in an area which is not prepared to put
> some data there, because there is no "skb_put" again. I don't understand
> this part, is there no CRC comming from cc2520 transceiver, but then I
> don't understand the "skb_trim(skb, skb->len - 2);" which was before
> there and I think it was the CRC.
>
> I think it's fine to remove this part, did you test it with monitor
> interface and node interface?
>

That should be just replacing the two existing bytes (RSSI/CRCOK/LQI)
with an actual CRC.

I tested this with wireshark and verified that the CRC that is being
appended passes wireshark's check (and of course fails if the received
CRC was wrong).

>>       ieee802154_rx_irqsafe(priv->hw, skb, lqi);
>>
>> @@ -619,14 +677,19 @@ cc2520_filter(struct ieee802154_hw *hw,
>>       }
>>
>>       if (changed & IEEE802154_AFILT_PANC_CHANGED) {
>> +             u8 frmfilt0;
>> +
>>               dev_vdbg(&priv->spi->dev,
>>                        "cc2520_filter called for panc change\n");
>> +
>> +             cc2520_read_register(priv, CC2520_FRMFILT0, &frmfilt0);
>> +
>>               if (filt->pan_coord)
>> -                     ret = cc2520_write_register(priv, CC2520_FRMFILT0,
>> -                                                 0x02);
>> +                     frmfilt0 |= FRMFILT0_PAN_COORDINATOR;
>>               else
>> -                     ret = cc2520_write_register(priv, CC2520_FRMFILT0,
>> -                                                 0x00);
>> +                     frmfilt0 &= ~FRMFILT0_PAN_COORDINATOR;
>> +
>> +             ret = cc2520_write_register(priv, CC2520_FRMFILT0, frmfilt0);
>>       }
>>
>>       return ret;
>
> You need to be sure here that this function does not enable again some
> filtering stuff bit's which was set by "set_promiscuous_mode". Is this
> the case then we need to do more handling here (maybe with
> priv->promiscuous).
>
> This functionality should really only set the address filtering stuff
> _not_ enable it (if possible). Is this the case, then the function
> should be fine.
>

Yes this function will not change the state of filtering, just update the filter
parameters. It does this by reading the current value from the CC2520
and then only setting or clearing the PAN_COORDINATOR bit.

>> @@ -723,6 +786,30 @@ cc2520_set_txpower(struct ieee802154_hw *hw, s32 mbm)
>>       return cc2520_cc2591_set_tx_power(priv, mbm);
>>  }
>>
>> +static int
>> +cc2520_set_promiscuous_mode(struct ieee802154_hw *hw, bool on)
>> +{
>> +     struct cc2520_private *priv = hw->priv;
>> +     u8 frmfilt0;
>> +
>> +     dev_dbg(&priv->spi->dev, "%s : mode %d\n", __func__, on);
>> +
>> +     priv->promiscuous = on;
>> +
>> +     cc2520_read_register(priv, CC2520_FRMFILT0, &frmfilt0);
>> +
>> +     if (on) {
>> +             /* Disable automatic ACK and frame filtering. */
>> +             cc2520_write_register(priv, CC2520_FRMCTRL0, FRMCTRL0_AUTOCRC);
>> +             frmfilt0 &= ~FRMFILT0_FRAME_FILTER_EN;
> The frmfilt0 was readout before doing:
> "cc2520_write_register(priv, CC2520_FRMCTRL0, FRMCTRL0_AUTOCRC);"
>
> Are you sure that this will not change again the setting og
> FRMCTRL0_AUTOCRC when set it again below.
>
> Simple do all manipulations (update bits) in "frmfilt0", then write it
> below.
>
>> +     } else {
>> +             cc2520_write_register(priv, CC2520_FRMCTRL0, FRMCTRL0_AUTOACK |
>> +                                                          FRMCTRL0_AUTOCRC);
>> +             frmfilt0 |= FRMFILT0_FRAME_FILTER_EN;
>> +     }
>
> Maybe the FRMCTRL0_AUTOCRC will unset here it's the same register.
>

The only time FRMCTRL0 is changed is in this function and to enable/disable
automatic ACKs. I think it's fine to just set it directly as it should
never change
otherwise. The FRMCTRL0 register, however, does get set in other places, so
I read its current value, set/clear the filter bit, and then write it back.

>> +     return cc2520_write_register(priv, CC2520_FRMFILT0, frmfilt0);
>> +}
>> +
>>  static const struct ieee802154_ops cc2520_ops = {
>>       .owner = THIS_MODULE,
>>       .start = cc2520_start,
>> @@ -732,6 +819,7 @@ static const struct ieee802154_ops cc2520_ops = {
>>       .set_channel = cc2520_set_channel,
>>       .set_hw_addr_filt = cc2520_filter,
>>       .set_txpower = cc2520_set_txpower,
>> +     .set_promiscuous_mode = cc2520_set_promiscuous_mode,
>>  };
>>
>>  static int cc2520_register(struct cc2520_private *priv)
>> @@ -749,7 +837,8 @@ static int cc2520_register(struct cc2520_private *priv)
>>
>>       /* We do support only 2.4 Ghz */
>>       priv->hw->phy->supported.channels[0] = 0x7FFF800;
>> -     priv->hw->flags = IEEE802154_HW_OMIT_CKSUM | IEEE802154_HW_AFILT;
>> +     priv->hw->flags = IEEE802154_HW_TX_OMIT_CKSUM | IEEE802154_HW_AFILT |
>> +                       IEEE802154_HW_PROMISCUOUS;
>>
>>       priv->hw->phy->flags = WPAN_PHY_FLAG_TXPOWER;
>>
>> @@ -919,6 +1008,11 @@ static int cc2520_hw_init(struct cc2520_private *priv)
>>       }
>>
>>       /* Registers default value: section 28.1 in Datasheet */
>> +
>> +     /* Set the CCA threshold to -50 dBm. This seems to have been copied
>> +      * from the TinyOS CC2520 driver and is much higher than the -84 dBm
>> +      * threshold suggested in the datasheet.
>> +      */
>
> I think the CCA isn't working here inside the CC2520, because the CC2520
> do _not_ CSMA handling on transceiver side. There need to be a handling
> (maybe with hrtimers) which adds CSMA handling and doing CCA scans
> before transmit, but the CC2520 driver doesn't has such functionality to
> support CSMA-CA.
>
> See my comments on this at [0].
>
>>       ret = cc2520_write_register(priv, CC2520_CCACTRL0, 0x1A);
>>       if (ret)
>>               goto err_ret;
>> @@ -955,15 +1049,15 @@ static int cc2520_hw_init(struct cc2520_private *priv)
>>       if (ret)
>>               goto err_ret;
>>
>> -     ret = cc2520_write_register(priv, CC2520_FRMCTRL0, 0x60);
>> -     if (ret)
>> -             goto err_ret;
>> -
>> -     ret = cc2520_write_register(priv, CC2520_FRMCTRL1, 0x03);
>> +     /* Configure registers correctly for this driver. */
>> +     ret = cc2520_write_register(priv, CC2520_FRMCTRL0, FRMCTRL0_AUTOACK |
>> +                                                        FRMCTRL0_AUTOCRC);
>>       if (ret)
>>               goto err_ret;
>>
>> -     ret = cc2520_write_register(priv, CC2520_FRMFILT0, 0x00);
>> +     ret = cc2520_write_register(priv, CC2520_FRMCTRL1,
>> +                                 FRMCTRL1_SET_RXENMASK_ON_TX |
>> +                                 FRMCTRL1_IGNORE_TX_UNDERF);
>
> I think:
> You don't need to set this. Before calling "start" callback we will call
> "set_promiscuous_mode" with on/off depends on monitor/node type.
>
> same for enable the FRMCTRL0_AUTOACK. What you should enable here is
> FRMCTRL0_AUTOCRC (I suppose for transmit).
>

Ah OK I will get rid of setting FRMCTRL0.


> - Alex
>
> [0] http://www.spinics.net/lists/linux-wpan/msg03198.html


- Brad

Re: [PATCH v2 1/1] ieee802154: cc2520: Check CRC & add promiscuous

[Alexander Aring]

On Wed, Dec 23, 2015 at 04:27:12PM -0500, Brad Campbell wrote:
> On Wed, Dec 23, 2015 at 4:55 AM, Alexander Aring <alex.aring@gmail.com> wrote:
> > On Wed, Dec 23, 2015 at 03:17:58AM -0500, Brad Campbell wrote:
> >> This patch adds checking the "CRC_OK" bit at the end of packets coming
> >> from the CC2520 radio. It also adds support for putting the radio in
> >> promiscuous mode (in which packets are not dropped if the CRC fails).
> >> In order to provide monitors information about CRC pass/fail, the CRC is
> >> recreated in the driver and appended to the packet before being passed
> >> to higher layers. If the CRC failed, a dummy CRC is appended.
> >>
> >
> > Why append a dummy CRC? There is a CRC and leave it there.
> >
> > There could be one reason to do this:
> >
> > There is no CRC appended when CRC failed.
> >
> > Is this true? I don't know how the CC2520 is working and why it should
> > do this.
> >
> 
> When the AUTOCRC bit is set, upon packet reception the CC2520 does
> not return the two FCS bytes, instead it replaces them with 1 byte
> of RSSI, 1 bit of CRC_OK and 7 bits of correlation value. So, in order to
> comply with the ieee802154_rx() interface, the driver could do one of:
> 
> 1. Turn off AUTOCRC, which would require linux to calculate the CRC for
> TX packets, but the CC2520 would give us the CRC bytes which would
> then be passed up.
> 
> 2. Leave AUTOCRC on and calculate the CRC in software at the driver
> level so that if the CRC failed a bad CRC could get appended. (This
> is what the patch currently does).
> 
> 3. Leave AUTOCRC on, set IEEE802154_HW_RX_OMIT_CKSUM and
> let ieee802154_rx() calculate the CRC. This is bad because the monitor
> layer will always see a correct CRC, even if it did in fact fail. (This is
> how the driver works currently).
> 

Okay, then you have one of these transceiver which doesn't offer the CRC
for upper layers. I would not turn AUTOCRC off for tx handling then.

btw: this is exacly the TODO in IEEE802154_HW_RX_OMIT_CKSUM, there
should be some way that userspace programms can see "the frame is
without CRC", it makes no sense to give that to userspace. But we
doesn't support that right now. (I know wireless monitors can to that
somehow) :-)

Then wireshark will disable frames without CRC. The own caculated is for
workaround this handling that you don't see stupid things in wireshark.

> I think the best thing to do is to actually change the ieee802154_rx()
> interface to allow 15.4 drivers to pass in a boolean if the CRC passed
> and get rid of IEEE802154_HW_RX_DROP_BAD_CKSUM. Then
> ieee802154_rx() can recreate CRCs and/or drop packets as needed,
> and the 15.4 drivers wouldn't need to do "if (promiscuous_mode)"
> checks. Drivers would be able to pass in the actual received CRC
> values instead of this flag if appropriate, of course.
> 

mmhhh, what about such handling:

if going into promiscuous mode (turn off all filtering), then additional
you change the AUTOCRC = 0 (if promiscuous_mode) and = 1 (if not
promiscuous_mode):

Then on TX:

if (promiscuous_mode)
	/* append own calculated CRC, rarely case */
else
	/* nothing */

set the  IEEE802154_HW_TX_OMIT_CKSUM

Then on RX:

if (promiscuous_mode)
	/* do nothing, do 0xff lqi's if there are not available */
else
	/* grab RSSI, LQI, check crc bit(because transceiver don't do
	 * that). Don't remove the last two bytes, it will dropped anyway
	 * because non-promiscuous interfaces will drop the crc at first
	 * step of receiving. Random data of 2 bytes is okay, I think
	 * we will never change that and if so then the CC2520 needs other
	 * workarounds to deal with mac802154 */

don't set IEEE802154_HW_RX_OMIT_CKSUM.

This will set the CRC for monitor receiving and do some crc filtering which
is in your case transceiver specific and should be handled in-driver.


A note about monitor transmit:

There was some discussion about it, it's currently possible but if it
really makes sense is another question. This is very unlikely _except_
you want put some weird data into the network which comes from a node
which doesn't exist. But this _may_ be a use-case for monitors.
Also I didn't test xmit with monitors, put maybe you want try that with
AF_PACKET and RAW sockets on wpan interfaces. :-)

> 
> >
> >
> >> NOTE: This is an API breaking change for the CC2520 radio. The radio now
> >> defaults to frame filtering (checking that the destination and PANID in
> >> the incoming packet matches the local node). This matches the other
> >> 15.4 radios and is what a user would expect to be the default.
> >>
> >
> > Was the frame filtering stuff disabled before? Then this is no API
> > change, it's an improvement for CC2520.
> >
> > It would be an API change if the frame filtering of CC2520 is not
> > 802.15.4 compliant.
> >
> > Maybe it breaks API now but then this is a TODO/BUG in mac802154.
> >
> 
> Currently, the CC2520 driver does no frame filtering. The patch makes
> it 802.15.4 compliant.
> 

ok.

> >> Other changes:
> >>
> >> 1. Adds LQI calculation
> >> 2. Makes #defines for relevant bit fields in CC2520 registers
> >>
> >> Signed-off-by: Brad Campbell <bradjc5@gmail.com>
> >> ---
> >>  drivers/net/ieee802154/cc2520.c | 128 ++++++++++++++++++++++++++++++++++------
> >>  1 file changed, 111 insertions(+), 17 deletions(-)
> >>
> >> diff --git a/drivers/net/ieee802154/cc2520.c b/drivers/net/ieee802154/cc2520.c
> >> index e65b605..3d66717 100644
> >> --- a/drivers/net/ieee802154/cc2520.c
> >> +++ b/drivers/net/ieee802154/cc2520.c
> >> @@ -21,6 +21,8 @@
> >>  #include <linux/skbuff.h>
> >>  #include <linux/of_gpio.h>
> >>  #include <linux/ieee802154.h>
> >> +#include <linux/crc-ccitt.h>
> >> +#include <asm/unaligned.h>
> >>
> >>  #include <net/mac802154.h>
> >>  #include <net/cfg802154.h>
> >> @@ -189,6 +191,18 @@
> >>  #define      CC2520_RXFIFOCNT                0x3E
> >>  #define      CC2520_TXFIFOCNT                0x3F
> >>
> >> +/* CC2520_FRMFILT0 */
> >> +#define FRMFILT0_FRAME_FILTER_EN     BIT(0)
> >> +#define FRMFILT0_PAN_COORDINATOR     BIT(1)
> >> +
> >> +/* CC2520_FRMCTRL0 */
> >> +#define FRMCTRL0_AUTOACK             BIT(5)
> >> +#define FRMCTRL0_AUTOCRC             BIT(6)
> >> +
> >> +/* CC2520_FRMCTRL1 */
> >> +#define FRMCTRL1_SET_RXENMASK_ON_TX  BIT(0)
> >> +#define FRMCTRL1_IGNORE_TX_UNDERF    BIT(1)
> >> +
> >>  /* Driver private information */
> >>  struct cc2520_private {
> >>       struct spi_device *spi;         /* SPI device structure */
> >> @@ -201,6 +215,7 @@ struct cc2520_private {
> >>       struct work_struct fifop_irqwork;/* Workqueue for FIFOP */
> >>       spinlock_t lock;                /* Lock for is_tx*/
> >>       struct completion tx_complete;  /* Work completion for Tx */
> >> +     bool promiscuous;               /* Flag for promiscuous mode */
> >>  };
> >>
> >>  /* Generic Functions */
> >> @@ -414,7 +429,7 @@ cc2520_write_txfifo(struct cc2520_private *priv, u8 *data, u8 len)
> >>  }
> >>
> >>  static int
> >> -cc2520_read_rxfifo(struct cc2520_private *priv, u8 *data, u8 len, u8 *lqi)
> >> +cc2520_read_rxfifo(struct cc2520_private *priv, u8 *data, u8 len)
> >>  {
> >>       int status;
> >>       struct spi_message msg;
> >> @@ -516,24 +531,67 @@ err_tx:
> >>  static int cc2520_rx(struct cc2520_private *priv)
> >>  {
> >>       u8 len = 0, lqi = 0, bytes = 1;
> >> +     bool crc_ok;
> >> +     u16 crc;
> >>       struct sk_buff *skb;
> >>
> >> -     cc2520_read_rxfifo(priv, &len, bytes, &lqi);
> >> +     /* Read single length byte from the radio. */
> >> +     cc2520_read_rxfifo(priv, &len, bytes);
> >>
> >> -     if (len < 2 || len > IEEE802154_MTU)
> >> -             return -EINVAL;
> >> +     if (!ieee802154_is_valid_psdu_len(len)) {
> >> +             /* Corrupted frame received, clear frame buffer by
> >> +              * reading entire buffer.
> >> +              */
> >> +             dev_dbg(&priv->spi->dev, "corrupted frame received\n");
> >> +             len = IEEE802154_MTU;
> >> +     }
> >>
> >>       skb = dev_alloc_skb(len);
> >>       if (!skb)
> >>               return -ENOMEM;
> >>
> >> -     if (cc2520_read_rxfifo(priv, skb_put(skb, len), len, &lqi)) {
> >> +     if (cc2520_read_rxfifo(priv, skb_put(skb, len), len)) {
> >>               dev_dbg(&priv->spi->dev, "frame reception failed\n");
> >>               kfree_skb(skb);
> >>               return -EINVAL;
> >>       }
> >>
> >> -     skb_trim(skb, skb->len - 2);
> >
> > This code told me before it was there: we cut off the CRC for delivery.
> >
> 
> The CC2520 does append two bytes, but not the CRC.
> 

but when AUTOCRC = 0, then a CRC is appended but no LQI/RSSI/CRC_OK(BIT7)?

- Alex

Re: [PATCH v2 0/1] ieee802154: cc2520: Check CRC & add promiscuous

[Alexander Aring]

On Wed, Dec 23, 2015 at 03:17:57AM -0500, Brad Campbell wrote:
> I've updated the patch to provide monitors a method for determining if
> the incoming packet had a valid CRC or not. This required adding promiscuous
> mode, which is no longer the default.
> 
> Also, am I correct in noting that configuring a node interface to be in
> promiscuous mode with something like:
> 
> ioctl(sockfd, SIOCGIFFLAGS, &ifopts);
> ifopts.ifr_flags |= IFF_PROMISC;
> ioctl(sockfd, SIOCSIFFLAGS, &ifopts);
> 
> is not currently supported? That's how I was testing at first and it
> didn't seem to work.... Also, if a user does set an interface to
> promiscuous using an ioctl, it seems like they would still want CRC
> checking, which is not what would currently happen.
> 

These flags does nothing, like in wireless we need to create a special
interface type for starting in promiscuous mode. Try [0].

The receiving path and the reason that monitors has no addresses is why
we handle it in that way.

All "NODE" types need to be parsed again on 802.15.4 conform header and
doing address filtering/etc stuff again. This is different on "MONITOR"
types, we parse nothing and this goes directly into userspace, the PHR
header (the len field) is the only thing which we should filter and
mostly in-driver because after frame[len] there is much moslty some
data (lqi/rssi/etc).


When you sniff on normal "NODE" type, then you will sniff with transceiver
filtering stuff enabled (address filtering/frame/etc.) what the
transceiver does AND what mac802154 does.

- Alex

[0] http://wpan.cakelab.org/#_sniffing

Re: [PATCH v2 0/1] ieee802154: cc2520: Check CRC & add promiscuous

[Brad Campbell]

On Wed, Dec 23, 2015 at 4:22 AM, Alexander Aring <alex.aring@gmail.com> wrote:
> On Wed, Dec 23, 2015 at 03:17:57AM -0500, Brad Campbell wrote:
>> I've updated the patch to provide monitors a method for determining if
>> the incoming packet had a valid CRC or not. This required adding promiscuous
>> mode, which is no longer the default.
>>
>> Also, am I correct in noting that configuring a node interface to be in
>> promiscuous mode with something like:
>>
>> ioctl(sockfd, SIOCGIFFLAGS, &ifopts);
>> ifopts.ifr_flags |= IFF_PROMISC;
>> ioctl(sockfd, SIOCSIFFLAGS, &ifopts);
>>
>> is not currently supported? That's how I was testing at first and it
>> didn't seem to work.... Also, if a user does set an interface to
>> promiscuous using an ioctl, it seems like they would still want CRC
>> checking, which is not what would currently happen.
>>
>
> These flags does nothing, like in wireless we need to create a special
> interface type for starting in promiscuous mode. Try [0].
>
> The receiving path and the reason that monitors has no addresses is why
> we handle it in that way.
>
> All "NODE" types need to be parsed again on 802.15.4 conform header and
> doing address filtering/etc stuff again. This is different on "MONITOR"
> types, we parse nothing and this goes directly into userspace, the PHR
> header (the len field) is the only thing which we should filter and
> mostly in-driver because after frame[len] there is much moslty some
> data (lqi/rssi/etc).
>
>
> When you sniff on normal "NODE" type, then you will sniff with transceiver
> filtering stuff enabled (address filtering/frame/etc.) what the
> transceiver does AND what mac802154 does.
>

Is it possible to create two interfaces on a phy, one node and one monitor?
Then the driver would be in promiscuous mode and not checking CRC, and
the node interface would get packets with failed CRC with no way to check
that they were wrong.

> - Alex
>
> [0] http://wpan.cakelab.org/#_sniffing

- Brad

Re: [PATCH v2 0/1] ieee802154: cc2520: Check CRC & add promiscuous

[Alexander Aring]

On Wed, Dec 23, 2015 at 04:33:24PM -0500, Brad Campbell wrote:
> On Wed, Dec 23, 2015 at 4:22 AM, Alexander Aring <alex.aring@gmail.com> wrote:
> > On Wed, Dec 23, 2015 at 03:17:57AM -0500, Brad Campbell wrote:
> >> I've updated the patch to provide monitors a method for determining if
> >> the incoming packet had a valid CRC or not. This required adding promiscuous
> >> mode, which is no longer the default.
> >>
> >> Also, am I correct in noting that configuring a node interface to be in
> >> promiscuous mode with something like:
> >>
> >> ioctl(sockfd, SIOCGIFFLAGS, &ifopts);
> >> ifopts.ifr_flags |= IFF_PROMISC;
> >> ioctl(sockfd, SIOCSIFFLAGS, &ifopts);
> >>
> >> is not currently supported? That's how I was testing at first and it
> >> didn't seem to work.... Also, if a user does set an interface to
> >> promiscuous using an ioctl, it seems like they would still want CRC
> >> checking, which is not what would currently happen.
> >>
> >
> > These flags does nothing, like in wireless we need to create a special
> > interface type for starting in promiscuous mode. Try [0].
> >
> > The receiving path and the reason that monitors has no addresses is why
> > we handle it in that way.
> >
> > All "NODE" types need to be parsed again on 802.15.4 conform header and
> > doing address filtering/etc stuff again. This is different on "MONITOR"
> > types, we parse nothing and this goes directly into userspace, the PHR
> > header (the len field) is the only thing which we should filter and
> > mostly in-driver because after frame[len] there is much moslty some
> > data (lqi/rssi/etc).
> >
> >
> > When you sniff on normal "NODE" type, then you will sniff with transceiver
> > filtering stuff enabled (address filtering/frame/etc.) what the
> > transceiver does AND what mac802154 does.
> >
> 
> Is it possible to create two interfaces on a phy, one node and one monitor?

This depends on many things if you have address filtering enabled then you can't
and we currently doesn't support such handling.

> Then the driver would be in promiscuous mode and not checking CRC, and
> the node interface would get packets with failed CRC with no way to check
> that they were wrong.
> 

Currently we don't support "two interfaces on a phy" and I don't know
exactly a use-case for that. BUT if you have a transceiver which has
"phy" functionality only e.g. promiscuous mode. Then this might be
possible and mac802154 will do all stuff of filtering for you.

This has also some disadvantages because the workload of the transceiver
will be increased (no filtering on transceiver side anymore).

Note: you can still not operate on two different channels. Except you
doing some channel-hoping mechanism, which may requires real time
handling then.

- Alex