Re: [PATCH v6 1/3] ieee802154: Add CA8210 IEEE 802.15.4 device driver

From: Harry Morris <h.morris@cascoda.com>
To: Alexander Aring <aar@pengutronix.de>, harrymorris12@gmail.com
Cc: linux-wpan@vger.kernel.org, stefan@osg.samsung.com, marcel@holtmann.org
Subject: Re: [PATCH v6 1/3] ieee802154: Add CA8210 IEEE 802.15.4 device driver
Date: Thu, 15 Dec 2016 13:52:26 +0000	[thread overview]
Message-ID: <98cb5f50-2e9b-5414-88c0-d8323a547152@cascoda.com> (raw)
In-Reply-To: <0aa78be5-7af1-8837-c546-16588646bc32@pengutronix.de>

Hi Alex,

On 12/12/2016 14:27, Alexander Aring wrote:
> All these spi stuff is low-level spi functionality, why not simple use
> the spi helper functions. E.g. "spi_write_then_read" and I don't mean
> only just this code, I mean every code here.
>
> Low-level is _maybe_ needed if you using async api.
As far as I can see, I'm unable to use the helper functions because I 
need (effectively) manual control of the spi chip select. The way the 
ca8210 API works I don't know how many bytes I'm reading in total until 
I read the "command length" byte. So I need to read two bytes, keep the 
chip select asserted while I process the length and then read the rest 
of the data. This is the same for the tx path since the ca8210 operates 
in full duplex mode, so when transmitting I may also be receiving a 
command that is longer than what I am transmitting so there is a risk of 
me de-asserting the chip select too early using a helper. Maybe I'm not 
completely understanding how the helpers work though.

>> +	msdulen = data_ind[22]; /* msdu_length */
>> +	if (msdulen > 127) {
> use ieee802154_is_valid_psdu_len.
>
> Also this is psdu_len, in case of monitors -> monitors will filter then
> minimum ACK frame length only.
So the reason for this is that this *is* actually msdu length and not 
psdu length. What's being received is an MCPS-DATA.indication, so the 
chip has already performed the is_valid_psdu_len check in its MAC. 
Truthfully this check is probably completely unnecessary, I just added 
it in case something gets messed up over the SPI but I've never seen it 
happen. Maybe it should be removed altogether, I just thought it's a 
simple check to perform that could easily highlight a big problem.

>> +		dev_err(
>> +			&priv->spi->dev,
>> +			"received erroneously large msdu length!\n"
>> +		);
>> +		kfree_skb(skb);
>> +		return -EMSGSIZE;
>> +	}
>> +	dev_dbg(&priv->spi->dev, "skb buffer length = %d\n", msdulen);
>> +
>> +	/* Populate hdr *
> Doing header parsing below here will not work with monitors, monitors
> can also receive stuff which are smaller than 29 - I don't know what's
> already filtered on firmware side.
Again the hardware MAC will have already filtered out any frames that 
are not valid data frames, so the header of the data indication will 
always be a fixed 29 bytes.
>
> I see the driver doesn't implement any promiscuousmode functionality,
> you need to care about that if you will add support for that later.
When it comes to promiscuous mode, the psdu is just copied into the msdu 
of the data indication. So again there will be a fixed 29 byte header on 
the front, just the contents is meaningless (zeroed). Again this is 
because the specification doesn't say how promiscuous frames are to be 
passed out of the MAC.
>> +	hdr.sec.level = data_ind[29 + msdulen];
>> +	dev_dbg(&priv->spi->dev, "security level: %#03x\n", hdr.sec.level);
>> +	if (hdr.sec.level > 0) {
>> +		hdr.sec.key_id_mode = data_ind[30 + msdulen];
>> +		memcpy(&hdr.sec.extended_src, &data_ind[31 + msdulen], 8);
>> +		hdr.sec.key_id = data_ind[39 + msdulen];
>> +	}
>> +	hdr.source.mode = data_ind[0];
>> +	dev_dbg(&priv->spi->dev, "srcAddrMode: %#03x\n", hdr.source.mode);
>> +	hdr.source.pan_id = *(u16 *)&data_ind[1];
>> +	dev_dbg(&priv->spi->dev, "srcPanId: %#06x\n", hdr.source.pan_id);
>> +	memcpy(&hdr.source.extended_addr, &data_ind[3], 8);
>> +	hdr.dest.mode = data_ind[11];
>> +	dev_dbg(&priv->spi->dev, "dstAddrMode: %#03x\n", hdr.dest.mode);
>> +	hdr.dest.pan_id = *(u16 *)&data_ind[12];
>> +	dev_dbg(&priv->spi->dev, "dstPanId: %#06x\n", hdr.dest.pan_id);
>> +	memcpy(&hdr.dest.extended_addr, &data_ind[14], 8);
>> +
>> +	/* Fill in FC implicitly */
>> +	hdr.fc.type = 1; /* Data frame */
>> +	if (hdr.sec.level)
>> +		hdr.fc.security_enabled = 1;
>> +	else
>> +		hdr.fc.security_enabled = 0;
>> +	if (data_ind[1] != data_ind[12] || data_ind[2] != data_ind[13])
>> +		hdr.fc.intra_pan = 1;
>> +	else
>> +		hdr.fc.intra_pan = 0;
>> +	hdr.fc.dest_addr_mode = hdr.dest.mode;
>> +	hdr.fc.source_addr_mode = hdr.source.mode;
>> +
>> +	/* Add hdr to front of buffer */
>> +	hlen = ieee802154_hdr_push(skb, &hdr);
>> +
>> +	if (hlen < 0) {
>> +		dev_crit(&priv->spi->dev, "failed to push mac hdr onto skb!\n");
>> +		kfree_skb(skb);
>> +		return hlen;
>> +	}
>> +
>> +	skb_reset_mac_header(skb);
>> +	skb->mac_len = hlen;
>> +
>> +	/* Add <msdulen> bytes of space to the back of the buffer */
>> +	/* Copy msdu to skb */
>> +	memcpy(skb_put(skb, msdulen), &data_ind[29], msdulen);
>> +
>> +	ieee802154_rx_irqsafe(hw, skb, data_ind[23]/*LQI*/);
>> +	/* TODO: Set protocol & checksum? */
>> +	/* TODO: update statistics */
>> +	return 0;
>> +}
>> +
>> +/**
>> + * ca8210_net_rx() - Acts upon received SAP commands relevant to the network
>> + *                   driver
>> + * @hw:       ieee802154_hw that command was received by
>> + * @command:  Octet array of received command
>> + * @len:      length of the received command
>> + *
>> + * Called by the spi driver whenever a SAP command is received, this function
>> + * will ascertain whether the command is of interest to the network driver and
>> + * take necessary action.
>> + *
>> + * Return: 0 or linux error code
>> + */
>> +static int ca8210_net_rx(struct ieee802154_hw *hw, u8 *command, size_t len)
>> +{
>> +	struct ca8210_priv *priv = hw->priv;
>> +	unsigned long flags;
>> +	u8 status;
>> +
>> +	dev_dbg(&priv->spi->dev, "ca8210_net_rx(), CmdID = %d\n", command[0]);
>> +
>> +	if (command[0] == SPI_MCPS_DATA_INDICATION) {
>> +		/* Received data */
>> +		spin_lock_irqsave(&priv->lock, flags);
>> +		if (command[26] == priv->last_dsn) {
>> +			dev_dbg(
>> +				&priv->spi->dev,
>> +				"DSN %d resend received, ignoring...\n",
>> +				command[26]
>> +			);
>> +			spin_unlock_irqrestore(&priv->lock, flags);
>> +			return 0;
>> +		}
>> +		priv->last_dsn = command[26];
>> +		spin_unlock_irqrestore(&priv->lock, flags);
>> +		return ca8210_skb_rx(hw, len - 2, command + 2);
>> +	} else if (command[0] == SPI_MCPS_DATA_CONFIRM) {
>> +		status = command[3];
>> +		if (priv->async_tx_pending) {
>> +			return ca8210_async_xmit_complete(
>> +				hw,
>> +				command[2],
>> +				status
>> +			);
>> +		} else if (priv->sync_tx_pending) {
>> +			priv->sync_tx_pending = false;
>> +		}
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +/**
>> + * ca8210_skb_tx() - Transmits a given socket buffer using the ca8210
>> + * @skb:         Socket buffer to transmit
>> + * @msduhandle:  Data identifier to pass to the 802.15.4 MAC
>> + * @priv:        Pointer to private data section of target ca8210
>> + *
>> + * Return: 0 or linux error code
>> + */
>> +static int ca8210_skb_tx(
>> +	struct sk_buff      *skb,
>> +	u8                   msduhandle,
>> +	struct ca8210_priv  *priv
>> +)
>> +{
>> +	int status;
>> +	struct ieee802154_hdr header = { 0 };
>> +	struct secspec secspec;
>> +	unsigned int mac_len;
>> +
>> +	dev_dbg(&priv->spi->dev, "ca8210_skb_tx() called\n");
>> +
>> +	/* Get addressing info from skb - ieee802154 layer creates a full
>> +	 * packet
>> +	 */
>> +	mac_len = ieee802154_hdr_peek_addrs(skb, &header);
>> +
> you need at least check if error occurred. Currently we support at least
> 3 bytes FC + SEQ is there at driver level...
Sorry but I'm not sure I understand what you mean, do I need to check 
for errors in the skb I'm being told to transmit? Over-air error 
checking is performed by the ca8210 with the use of a frame counter so 
maybe this doesn't apply to this situation?

>> +/**
>> + * ca8210_xmit_sync() - Synchronously transmits a given socket buffer using the
>> + *                      ca8210
>> + * @hw:   ieee802154_hw of ca8210 to transmit from
>> + * @skb:  Socket buffer to transmit
>> + *
>> + * Transmits the buffer and does not return until a confirmation of the exchange
>> + * is received from the ca8210.
>> + *
>> + * Return: 0 or linux error code
>> + */
>> +static int ca8210_xmit_sync(struct ieee802154_hw *hw, struct sk_buff *skb)
>> +{
>> +	struct ca8210_priv *priv = hw->priv;
>> +	int status;
>> +
>> +	dev_dbg(&priv->spi->dev, "calling ca8210_xmit_sync()\n");
>> +
>> +	status = ca8210_skb_tx(skb, priv->nextmsduhandle++, priv);
>> +	if (status)
>> +		return status;
>> +
>> +	priv->sync_tx_pending = true;
>> +
>> +	while (priv->sync_tx_pending)
>> +		msleep(1);
>> +
>> +	return 0;
>> +}
>> +
>> +/**
>> + * ca8210_xmit_async() - Asynchronously transmits a given socket buffer using
>> + *                       the ca8210
>> + * @hw:   ieee802154_hw of ca8210 to transmit from
>> + * @skb:  Socket buffer to transmit
>> + *
>> + * Hands the transmission of the buffer off to a workqueue and returns
>> + * immediately.
>> + *
>> + * Return: 0 or linux error code
>> + */
>> +static int ca8210_xmit_async(struct ieee802154_hw *hw, struct sk_buff *skb)
>> +{
>> +	struct ca8210_priv *priv = hw->priv;
>> +
>> +	dev_dbg(&priv->spi->dev, "calling ca8210_xmit_async()\n");
>> +
>> +	priv->tx_skb = skb;
>> +	queue_work(priv->async_tx_workqueue, &priv->async_tx_work);
>> +
>> +	return 0;
>> +}
>> +
> Okay, both sync and async are here implemented for that the callbacks
> are never made for.
>
> The above sync function looks like an synced operation around a async
> operation, because you wait until it's finished.
>
> More bad, you use busy-waiting there, look for "wait_for_completion" in
> linux kernel.
>
> Nevertheless, the sync operation will not be used when async is defined,
> so sync operation is useless here.
>
> THe idea of async is that the Linux kernel doesn't do _anything_ until a
> tx complete irq reports "tx is done" and wake the tx queue again. This
> will not happen if you queue it to a workqueue again... which makes the
> whole stuff synced again.
>
> If we one day have MLME-OPS it needs a synced xmit functionality ->
> which will use xmit_async then - because we can and need transmit stuff
> synced. But NOT for dataplane -> dataframes, these frames should be
> synced completely undepended from linux kernel. The kernel doesn't wait
> until it's done -> hardware will report it when it's done.
>
> The issue will begin when we have synced xmit above async
> functionality... Then we have a workqueue above a workqueue which is bad.
Okay so I've probably misunderstood how the sync and async functions 
should be implemented, sorry about that. I am still a bit confused though.
Regarding async, you say I shouldn't use a workqueue, but the context is 
atomic right? So if I follow my transmission path directly it will call 
spi_sync which can sleep. Does this mean that I *have* to use 
spi_async() instead? That would be a shame since it will make 
transmission a lot more complicated because I have to perform multiple 
spi exchanges to read the correct length.

Would it maybe be better just to implement xmit_sync and not xmit_async? 
Is that acceptable since it's to be deprecated? Using 
wait_for_completion definitely seems like a good idea though, thanks for 
pointing that out.
The reason why it looks async is because of the use of the management 
entity primitives - the transmission path is sending an 
MCPS-DATA.request command and then waiting for an MCPS-DATA.confirm to 
come back from the chip, confirming successful transmission. Just 
sending the request to the chip does not guarantee that the frame was or 
will be transmitted.

Thanks for the feedback,

Regards,
Harry