Re: [PATCH net-next v5 1/3] ieee802154: cc2520: adds driver for TI CC2520 radio

On Thu, Jun 19, 2014 at 05:34:30PM +0530, Varka Bhadram wrote:

Hi Alex,

Thanks for the comments

On 06/19/2014 04:14 PM, Alexander Aring wrote:

quoted

Hi Varka,

why do you add new features while you trying to get the first version
mainline?

Yes, address filtering is needed for AACK support. I did not notice that
your chip supports AACK, please change your hw flags then to:

flags = IEEE802154_HW_OMIT_CKSUM | IEEE802154_HW_AACK;

This h/w address filtering feature is required for me to get CC2520 Hardware ACK, which
enable TinyOS nodes to communicate with the linux node.

I want to get this feature also in mainline for the first version. So that it will be full fledged driver.

quoted

On Thu, Jun 19, 2014 at 02:08:48PM +0530, Varka Bhadram wrote:

quoted

This patch adds the driver support for the cc2520 radio.

Driver support:
- Tx and Rx of IEEE-802.15.4 packets.
- Energy Detection on channel.
- Setting the Channel for the radio. [b/w 11 - 26 channels]
- Start and Stop the radio
- h/w address filtering.

[...]

quoted

quoted

+static int
+cc2520_read_rxfifo(struct cc2520_private *priv, u8 *data, u8 len, u8 *lqi)
+{
+	int status;
+	struct spi_message msg;
+
+	struct spi_transfer xfer_head = {
+		.len = 0,
+		.tx_buf = priv->buf,
+		.rx_buf = priv->buf,
+	};
+	struct spi_transfer xfer_buf = {
+		.len = len,
+		.rx_buf = data,
+	};
+
+	spi_message_init(&msg);
+	spi_message_add_tail(&xfer_head, &msg);
+	spi_message_add_tail(&xfer_buf, &msg);
+
+	mutex_lock(&priv->buffer_mutex);
+	priv->buf[xfer_head.len++] = CC2520_CMD_RXBUF;
+
+	dev_vdbg(&priv->spi->dev, "read rxfifo buf[0] = %02x\n", priv->buf[0]);
+	dev_vdbg(&priv->spi->dev, "buf[1] = %02x\n", priv->buf[1]);
+
+	status = spi_sync(priv->spi, &msg);
+	dev_vdbg(&priv->spi->dev, "status = %d\n", status);
+	if (msg.status)
+		status = msg.status;
+	dev_vdbg(&priv->spi->dev, "status = %d\n", status);
+	dev_vdbg(&priv->spi->dev,
+		 "return status buf[0] = %02x\n", priv->buf[0]);
+	dev_vdbg(&priv->spi->dev, "length buf[1] = %02x\n", priv->buf[1]);
+
+	*lqi = data[priv->buf[1] - 1] & 0x7f;

This is a little bit critical... but I know others driver doesn't check
also on this.

I will check how other drivers using lqi field , which is actually passing to higher layers.

The lqi value going to trash in the higher layers. But I need this value
for my future work with RPL.

quoted

After the cc2520_read_rxfifo you check if (len < 2 ...) but here you
already use the len. Maybe but the length constraints check in this function.

quoted

+
+	mutex_unlock(&priv->buffer_mutex);
+
+	return status;

[...]

quoted

quoted

+static int cc2520_rx(struct cc2520_private *priv)
+{
+	u8 len = 0, lqi = 0, bytes = 1;
+	struct sk_buff *skb;
+
+	cc2520_read_rxfifo(priv, &len, bytes, &lqi);

Okay, you get here the length for your pdu. But then you can check
afterwards on:

if (len < 2) instead of doing this in the second rxfifo call. And please
do a:

if (len < 2 || len > IEEE802154_MTU)

I will make this change in v6

quoted

The reason is, I don't know if your chip does filter something like
that. The at86rf230 don't filter pdu above the MTU size and we have no
generic mac802154 layer function right now to check on this. I like to
improve that in the near future...

When you do this check you can save the kfree_skb in this branch.

quoted

+
+	skb = alloc_skb(len, GFP_KERNEL);
+	if (!skb)
+		return -ENOMEM;
+
+	cc2520_read_rxfifo(priv, skb_put(skb, len), len, &lqi);
+	if (len < 2) {
+		kfree_skb(skb);
+		return -EINVAL;
+	}
+
+	skb_trim(skb, skb->len - 2);
+
+	ieee802154_rx_irqsafe(priv->dev, skb, lqi);
+
+	dev_vdbg(&priv->spi->dev, "RXFIFO: %x %x\n", len, lqi);
+
+	return 0;
+}
+
+static int
+cc2520_ed(struct ieee802154_dev *dev, u8 *level)
+{
+	struct cc2520_private *priv = dev->priv;
+	u8 status = 0xff;
+	u8 rssi;
+	int ret;
+
+	ret = cc2520_read_register(priv , CC2520_RSSISTAT, &status);
+	if (ret)
+		return ret;
+
+	if (status != RSSI_VALID) {
+		ret = -EINVAL;
+		return ret;

return -EINVAL;

Ok. I will do it in v6

quoted

quoted

+	}
+
+	ret = cc2520_read_register(priv , CC2520_RSSI, &rssi);
+	if (ret)
+		return ret;
+
+	/* level = RSSI(rssi) - OFFSET [dBm] : offset is 76dBm*/
+	*level = rssi - RSSI_OFFSET;
+
+	return ret;
+}
+

[...]

quoted

quoted

+static int
+cc2520_filter(struct ieee802154_dev *dev,
+	      struct ieee802154_hw_addr_filt *filt, unsigned long changed)
+{
+	struct cc2520_private *priv = dev->priv;
+
+	if (changed & IEEE802515_AFILT_PANID_CHANGED) {
+		u8 panid[2];
+		panid[0] = filt->pan_id & 0xff;
+		panid[1] = filt->pan_id >> 8;

const u16 panid = le16_to_cpu(filt->pan_id);

Ok.

quoted

quoted

+		cc2520_write_ram(priv, CC2520RAM_PANID, sizeof(panid), panid);
+	}
+
+	if (changed & IEEE802515_AFILT_IEEEADDR_CHANGED)
+		cc2520_write_ram(priv, CC2520RAM_IEEEADDR,
+				sizeof(filt->ieee_addr), (u8 *)&filt->ieee_addr);
+
+	return 0;
+
+}

What's about to handle IEEE802515_AFILT_PANC_CHANGED and
IEEE802515_AFILT_SADDR_CHANGED? These are fully ignored, your chip
should have such functions.

CC2520 supports IEEE802515_AFILT_SADDR_CHANGED functionality , but i didn't find any info about
Pan co-coordinator changed register details. If that is possible i will do it in v6

mhh, there exist FFD and RFD 802.15.4 chips. FFD means full
function devices, which have coordinator support. The RFD are
reduced function devices, which have not coordinator support. But I
think your chip should support to run as coordinator. If your chip is a
RFD... then it's a little bit more complex. Because this callback is
only for FFD device and you can not simple return -EOPNOTSUPP in this
case. Maybe you changed coordinator behaviour and set short address/long
address/pan id. Then it would return -EOPNOTSUPP but setting of short
address/long address is supported.

quoted

If you don't support them you need to return -EOPNOTSUPP; but this would
be weird because you have a IEEE 802.15.4 complaint chip. :-)

Ok.

Better forget this..., see above. :-)

- Alex