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

[Alexander Aring <alex.aring@gmail.com>]

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:
> >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.
> 
> >On Thu, Jun 19, 2014 at 02:08:48PM +0530, Varka Bhadram wrote:
> >>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.
> >>
> [...]
> 
> >>+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.

> >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.
> >
> >>+
> >>+	mutex_unlock(&priv->buffer_mutex);
> >>+
> >>+	return status;
> 
> [...]
> 
> >>+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
> 
> >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.
> >
> >>+
> >>+	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
> 
> >>+	}
> >>+
> >>+	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;
> >>+}
> >>+
> [...]
> >>+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.
> 
> >>+		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.

> >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